Page MenuHomePhabricator

Prefix all icons with `mf-` and fix the download icon glyph change glitch
Closed, ResolvedPublic3 Story Points

Description

Due to a namespace clash with OOUI the download icon changes unintentionally when you open the editor. We will fix this and prevent similar issues from happening again.

  • Visit any page on English Wikipedia with download button (Chrome)
  • Click edit
  • Close editor overlay
  • Watch the icon glyph change

Acceptance criteria

  • All icons will be prefixed with mf- (mobilefrontend) or minerva- (Minerva skin) from now on to limit the icon to the domain intended so that this doesn't happen again.

Done in rEMFR66cd470e97cb: Prefix all icons with `mf-` and rSMINc5d09c028898: Prefix icons in Minerva respectively.

  • We'll fix the download icon issue

Testing criteria

Details

Related Gerrit Patches:
mediawiki/core : masterDo not load apply mw-ui-icon selector by default
mediawiki/skins/MinervaNeue : masterFix download icon spinner
mediawiki/skins/MinervaNeue : masterPrefix icons in Minerva
mediawiki/extensions/MobileFrontend : masterAlways load user styles on mobile user pages
mediawiki/extensions/MobileFrontend : masterPrefix all icons with `mf-`
mediawiki/skins/MinervaNeue : specialpagesWIP:Prefix icons in Minerva
mediawiki/skins/MinervaNeue : masterAvoid OOUI/download icon nameclash

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptDec 6 2017, 12:26 AM

Change 395679 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/core@master] Do not load apply mw-ui-icon selector by default

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

Restricted Application added a project: UI-Standardization. · View Herald TranscriptDec 6 2017, 12:40 AM
ovasileva triaged this task as Normal priority.Dec 6 2017, 10:49 AM

Change 396100 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/MinervaNeue@master] Avoid OOUI/download icon nameclash

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

Jdlrobson updated the task description. (Show Details)Jan 2 2018, 5:55 PM
Jdlrobson set the point value for this task to 3.

We chatted in IRC and moving into sprint since it's a little light; there's a patch and task is simple.

https://gerrit.wikimedia.org/r/396100 fixes the bug. When it's merged we should move back to "Todo" to prefix the other icons. It has 2 +1s from Bartosz and Volker.

Change 396100 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Avoid OOUI/download icon nameclash

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

Jdlrobson renamed this task from Download icon changes after a click to edit icon to Prefix all icons with `mf-` and fix the download icon glyph change glitch.Jan 4 2018, 9:44 PM
Jdlrobson removed a project: Patch-For-Review.
Jdlrobson updated the task description. (Show Details)

@Jdlrobson What does the prefix ms- stand for?

Jdlrobson updated the task description. (Show Details)Jan 4 2018, 10:49 PM

Change 402409 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] WIP: Prefix icons with mf-

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

Change 403083 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/MinervaNeue@specialpages] WIP:Prefix icons in Minerva

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

Change 403085 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/MinervaNeue@master] Prefix icons in Minerva

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

Change 403083 abandoned by Jdlrobson:
WIP:Prefix icons in Minerva

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

This change was a little trickier than I hoped:
https://gerrit.wikimedia.org/r/402409
https://gerrit.wikimedia.org/r/403085

It touches a lot of icons which will require a lot of testing. Is it worth the change?

Jdlrobson moved this task from Triage to Current Sprint on the Proton board.Jan 9 2018, 7:07 PM
phuedx added a subscriber: phuedx.Jan 10 2018, 6:17 PM

It touches a lot of icons which will require a lot of testing. Is it worth the change?

Per our conversation in our Sprint Kickoff - Reading Web ritual:

  • Yeah. Let's keep going. We decided to change all of the icons now in order to save our future selves a little pain, since tasks of this class have popped up before.
  • You (@Jdlrobson) said that this now is more like a 5 than a 3 so re-estimating isn't required, since it's not a drastic change in complexity.

Note, with T177432 and https://gerrit.wikimedia.org/r/#/c/400091/ on the horizon (current goal is OOUI v0.26.0 to be released next month – once accepted there, I'll bring the icons to all projects) this issue should be obsolete.
Because then we all share a broadly agreed-on icon set backed by people across the Foundation.

@VolkerE I'm not sure I follow. That task seems to add the canonical SVGs but it's not clear how MediaWiki UI will use this... (selectors are all using OOUI specific). All mediawiki ui icons would need to be converted to OOUI to use them (or MediaWiki UI adjusted to be compatible with using oo-ui-icon-glyph classes (currently not possible as MediaWiki UI uses pseudo elements)

@Jdlrobson The two icons would be identical after above patch and follow-ups. Follow-up aka “I'll bring the icons to all projects“.

I'll try to test this again later today and follow up here.

Niedzielski reassigned this task from Jdlrobson to pmiazga.Jan 18 2018, 8:37 PM

I downloaded the following patchsets:

https://gerrit.wikimedia.org/r/#/c/402409/
https://gerrit.wikimedia.org/r/#/c/403085/

And determined them to function properly. Specifically:

  • Search drop down field icon, clear button, close button, and no results icon
  • Search page field icon, menu button, search button, and no pending notifications button
  • Editing close button and edit mode buttons
    and back button
  • Notifications filter, edit, settings, and option buttons (I didn't capture the loading spinner when tapping the notifications button but that worked fine too)
  • User icon
  • Refresh icon
  • Pending notifications and watched buttons, and last edited and do nothing right arrow icon
  • User and (happy-angry?) anon edited icons
  • Settings page looks ok
  • Change language, watch, and hide buttons
  • Talk page looks ok
  • Menu icons for logged in users (be careful not to bump the power button or you'll turn off Wikipedia)
    and anons
  • Watchlist tutorial image
  • Download button
  • Article page looks ok
  • File page looks ok

@pmiazga, over to you for some +2 action!

FYI Mobile Frontend patch and Minerva patch are +1ed. To get a +2 we need some manual testing as it touches all icons. If you have bandwidth and your are not Piotr any input would be appreciated!

Change 402409 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Prefix all icons with mf-

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

Minerva patch (now making Jenkins happy) should be merged by EOD Monday as it temporarily breaks the download icon, otherwise we should revert the MobileFrontend patch.

Change 405824 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Always load user styles on mobile user pages

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

Jdlrobson reassigned this task from pmiazga to ABorbaWMF.Jan 23 2018, 12:23 AM
Jdlrobson updated the task description. (Show Details)

Change 405824 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Always load user styles on mobile user pages

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

Change 403085 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Prefix icons in Minerva

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

This one is looking good to me. I am not seeing any changes to the icon after editing. Tried a handful of devices.

After dismissing edit overlay

Change 406618 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/MinervaNeue@master] Fix download icon spinner

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

Change 406618 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Fix download icon spinner

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

phuedx claimed this task.Jan 31 2018, 11:03 AM
phuedx added a subscriber: ABorbaWMF.
phuedx updated the task description. (Show Details)Feb 1 2018, 10:13 AM

This LGTM. Thanks to @Niedzielski and @ABorbaWMF for their testing in T182162#3911061 and T182162#3920559 💪

🎉🎉🎉

phuedx closed this task as Resolved.Feb 1 2018, 10:14 AM
Volker_E raised the priority of this task from Normal to Needs Triage.Feb 13 2019, 6:49 PM
Volker_E moved this task from Backlog to Done on the UI-Standardization-Kanban board.

Change 395679 abandoned by Jdlrobson:
Do not load apply mw-ui-icon selector by default

Reason:
I think it is but it's not important enough right now for me to put bandwidth into.

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