Page MenuHomePhabricator

Remove redundant/non-critical styling rules in Minerva
Closed, ResolvedPublic3 Story Points

Description

Various styles in the Minerva skin are redundant or are not critical. I'm creating a task for these as I've been having trouble getting time to focus on this within a sprint.

Testing steps

Please do a general QA of the mobile site on http://reading-web-staging.wmflabs.org/, in particular the important workflows such as editing (without JS and with JS). Please ensure there are no visual regressions and if so that they are acceptable.

In particular the following should be checked for regressions:

  • Search icon in mobile view and search icon inside Special:Search/SearchOverlay
  • Check the last modified bar for any UI regressions, in particular when a page has recently been edited it should go green
  • Check animations for lazy loaded images
  • Check headers of all overlays - there shouldn't be any UI regressions there
  • Check text truncation with long usernames in main menu and long last modified bar and in Special:Watchlist
  • Check textareas inside editor workflows (e.g. talk overlay, edit overlay, edit without JavaScript enabled)

Acceptance criteria

  • Do not ship 2 versions of the same icon

Estimated savings: 0.2kb
We currently ship the magnifying glass icon twice in the mobile skin. Let's not do that!

  • Do not send JavaScript-only styles as render blocking

Estimated savings: 0.2kb
Various classes are not used if JS is never loaded:

  • .truncated-text
  • .cloaked-element
  • .position-fixed
  • lazy loading related image css
  • last modified active color
  • overlay header styles

Patches

https://gerrit.wikimedia.org/r/444141
https://gerrit.wikimedia.org/r/444134
https://gerrit.wikimedia.org/r/444135
https://gerrit.wikimedia.org/r/444138
https://gerrit.wikimedia.org/r/444137

Textareas:
https://gerrit.wikimedia.org/r/447968
https://gerrit.wikimedia.org/r/444131

Post merge analysis

These changes resulted in a 7.724kb drop in CSS on the beta cluster:

Details

Related Gerrit Patches:
mediawiki/skins/MinervaNeue : wmf/1.32.0-wmf.16Correct search icon
mediawiki/skins/MinervaNeue : masterCorrect search icon
mediawiki/skins/MinervaNeue : masterLimit editor text area styles to where they are needed
mediawiki/extensions/MobileFrontend : masterTextarea padding will be defined by Minerva

Event Timeline

Jdlrobson created this task.Jul 6 2018, 8:53 PM
Jdlrobson moved this task from Incoming to Triaged but Future on the Readers-Web-Backlog board.
Jdlrobson triaged this task as High priority.Jul 10 2018, 9:35 PM
Niedzielski changed the edit policy from "Custom Policy" to "All Users".Jul 17 2018, 4:25 PM
phuedx updated the task description. (Show Details)Jul 17 2018, 4:26 PM
nray claimed this task.Jul 19 2018, 3:58 PM
nray moved this task from To Do to Doing on the Readers-Web-Kanbanana-Board-Old board.
nray updated the task description. (Show Details)Jul 23 2018, 10:31 PM
nray updated the task description. (Show Details)Jul 25 2018, 3:23 PM
nray updated the task description. (Show Details)Jul 25 2018, 3:26 PM
nray updated the task description. (Show Details)Jul 25 2018, 3:32 PM
nray updated the task description. (Show Details)Jul 25 2018, 8:02 PM
nray updated the task description. (Show Details)Jul 25 2018, 9:30 PM

Change 447968 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Textarea padding will be defined by Minerva

https://gerrit.wikimedia.org/r/447968

Jdlrobson updated the task description. (Show Details)Jul 26 2018, 2:40 AM
nray updated the task description. (Show Details)Jul 26 2018, 11:15 PM

Change 447968 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Textarea padding will be defined by Minerva

https://gerrit.wikimedia.org/r/447968

Change 444131 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Limit editor text area styles to where they are needed

https://gerrit.wikimedia.org/r/444131

nray added a comment.Jul 26 2018, 11:32 PM

@Jdlrobson Now that these have all been merged, what column should this card move to - Needs QA? Needs Design Review?

Jdlrobson reassigned this task from nray to alexhollender.Jul 27 2018, 1:54 AM
Jdlrobson updated the task description. (Show Details)
Jdlrobson added a subscriber: nray.
Jdlrobson added a subscriber: Ryasmeen.

Thanks @nray
Let's get both @alexhollender and then @Ryasmeen to have a look at this since there is a risk of UI regressions.

Jdlrobson moved this task from Inbox to Next up on the User-Jdlrobson board.Jul 31 2018, 1:58 AM
Jdlrobson moved this task from Next up to Doing on the User-Jdlrobson board.Aug 10 2018, 6:16 PM
Jdlrobson moved this task from Doing to Done on the User-Jdlrobson board.Aug 10 2018, 8:55 PM

@Jdlrobson I think this is too broad to productively QA. Would like to discuss how to approach these kinds of QA tasks (added it to our retro items). @Jdrewniak mentioned that it would be great to use tests (e.g. visual diffs) for this kind of thing.

Would it be possible for you to be more specific about what types of visual regressions may have occurred here?

Jdlrobson added a subscriber: โ€ข Nirzar.

@Nirzar pointed out this regression today which I suspect is related to these changes:

alexhollender removed alexhollender as the assignee of this task.Aug 14 2018, 4:59 PM
alexhollender added a subscriber: alexhollender.

Change 452825 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/MinervaNeue@master] Correct search icon

https://gerrit.wikimedia.org/r/452825

Jdlrobson updated the task description. (Show Details)Aug 14 2018, 8:59 PM

Change 452825 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Correct search icon

https://gerrit.wikimedia.org/r/452825

Jdlrobson assigned this task to Ryasmeen.EditedAug 14 2018, 10:28 PM
Jdlrobson added a project: Product-QA.

@alexhollender moving this straight to QA. Please review QA steps in parallel as you're just as likely (if not more likely) to catch subtle design regressions. changes are up on staging.

Change 453200 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/MinervaNeue@wmf/1.32.0-wmf.16] Correct search icon

https://gerrit.wikimedia.org/r/453200

Search icon in mobile view and search icon inside Special:Search/SearchOverlay

JS on

  • looks good, same as production

JS off

  • looks good, same as production (in both cases the search icon is rather large in Special:Search)

Check the last modified bar for any UI regressions, in particular when a page has recently been edited it should go green

  • not seeing the last modified bar on staging (tried both logged out and logged in, JS on and JS off)

Check animations for lazy loaded images

JS on

  • all good

JS off

  • not sure if this is relevant?

Check headers of all overlays - there shouldn't be any UI regressions there

JS on

  • checked editing overlay, and page issues overlay - looks good, same as production

JS off

  • unable to access either overlay (tried going directly to the URL)

Check text truncation with long usernames in main menu and long last modified bar and in Special:Watchlist

JS on

  • Special:Watchlist - which part of the page are you referring to? Not seeing truncation (but not seeing it in production either)

  • Main menu - all good (although we could probably increase right margin to 4ems)
  • Last modified bar - not appearing on Staging

Check textareas inside editor workflows (e.g. talk overlay, edit overlay, edit without JavaScript enabled)

JS on

  • Potential bug in the wikitext editor: when I tap into the text field of an empty section (not sure if empty sections actually exist in production? unable to repro) the cursor/focus is above the viewport:

  • when you tap into the Reply field in a talk page discussion the browser zooms way in, although this is the same as production:


JS off

  • unable to access editing flows

Change 453200 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@wmf/1.32.0-wmf.16] Correct search icon

https://gerrit.wikimedia.org/r/453200

Mentioned in SAL (#wikimedia-operations) [2018-08-16T23:34:39Z] <catrope@deploy1001> Synchronized php-1.32.0-wmf.16/skins/MinervaNeue/resources/skins.minerva.content.styles.images/magnifying-glass.svg: Correct MinervaNeue search icon (T199000) (duration: 00m 51s)

phuedx claimed this task.Aug 22 2018, 12:03 PM
phuedx added a subscriber: phuedx.

Claiming this per Readers Web's discussion about clearing down the Needs QA column during yesterday's standup.

Search icon in mobile view and search icon inside Special:Search/SearchOverlay

๐Ÿ‘

Check the last modified bar for any UI regressions, in particular when a page has recently been edited it should go green

๐Ÿ‘

Check headers of all overlays - there shouldn't be any UI regressions there

๐Ÿ‘ I'm fairly certain I tested all overlays (editing, categorising, discussing). If not, LMK!

Check text truncation with long usernames in main menu and long last modified bar and in Special:Watchlist

๐Ÿ‘

Check textareas inside editor workflows (e.g. talk overlay, edit overlay, edit without JavaScript enabled)

๐Ÿ‘Ž

It looks like the left and right padding has been increased. I'm comparing the above with the screenshot that @Niedzielski submitted while they were reviewing rSMINc7cb4ecee0e6: Limit editor text area styles to where they are needed.

Following on from my comment above, I've noticed a couple of differences in the treatments of the last modified bar with JavaScript enabled and disabled.

ScenarioJS disabledJS enabled
Not edited recently (stale?)
Fresh

Notes

  1. When JS is disabled, the clock or chevron icons aren't visible.
  2. When JS is disabled, the background doesn't turn green.

I'm going to leave this in Needs QA for now until we figure out if the horizontal padding of the wikitext editor textarea has indeed changed.

When JS is disabled we don't show the icons. This was the case before the patches and now. We can revisit this now as icons can be shipped as uris rather than data uris but out of scope for this patch.

The padding for all text areas is now consistent so this change was intended. Previously there were 3 competing versions of padding. Will leave this to @alexhollender to decide whether the padding value of the textarea or any surrounding elements needs to change further.

When JS is disabled we don't show the icons. This was the case before the patches and now. We can revisit this now as icons can be shipped as uris rather than data uris but out of scope for this patch.

๐Ÿ‘

  1. When JS is disabled, the background doesn't turn green.

@Jdlrobson is this expected?

@Jdlrobson is this expected?

Yes. We can't do relative modified messages in the HTML e.g. "Last edited 5 hours ago" as these would get cached and always say "Last edited 5 years ago." so this is expected. Without JS we just show the timestamp.

Thanks for following up, @Jdlrobson.


The only outstanding question for @alexhollender is whether the padding in the following screenshot is sane.

Izno moved this task from Backlog to Change CSS on the CSS board.Sep 7 2018, 4:44 PM

Sorry for the delay. Padding looks fine. Moving to signoff.

Restricted Application added a project: User-Ryasmeen. ยท View Herald TranscriptSep 10 2018, 5:32 PM
Volker_E awarded a token.
Volker_E removed a subscriber: gerritbot.