Page MenuHomePhabricator

Regression: Oojs ui icons replace mw-ui-icons when Echo used
Closed, ResolvedPublic

Description

The close icon gets substituted by a different close icon from OOui when Echo is used.

There was a previous bug which has been fixed via workaround:
Click the echo notification icon when logged in in mobile mode
Click the main menu icon
The watchlist star is now black and looks out of place

Event Timeline

Jdlrobson created this task.Apr 6 2016, 2:16 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptApr 6 2016, 2:16 PM
Jdlrobson updated the task description. (Show Details)

Why don't we prefix all of our icon class names with mf-? We should also do so with other class and IDs IMO.

I personally think the mw-ui-icon prefixed should be dropped from oojs ui generated icons. All it causes us is problems and it bloats the css produced for those icons. By all means do this in the beta cluster to bubble up inconsistencies but this shouldn't happen so often in production (this is about the third example).

@matmarex thoughts?

I would be fine with that. I'm afraid Collaboration team still relies on it for Flow, though.

I would be fine with that. I'm afraid Collaboration team still relies on it for Flow, though.

I think we do. We should stop doing that, and also stop using hovericon. That'll allow us to remove a lot of customization from the generated CSS for the OOUI icon modules, and reduce the CSS size as well.

Restricted Application added a subscriber: TerraCodes. · View Herald TranscriptApr 19 2016, 10:01 PM
phuedx added a subscriber: phuedx.Apr 25 2016, 8:47 AM

Why don't we prefix all of our icon class names with mf-? We should also do so with other class and IDs IMO.

+1

I think this is a symptom of both OOJS UI and MobileFrontend using the mw-ui-icon prefix. Shouldn't both be using their own prefix?

Sounds like a plan. The original idea was to consolidate icons carefully but clearly this has failed. We are even loading icons twice at times when using visual editor :/

For caching reasons we may want to only port icons as we encounter issues. Unfortunately I don't think resource loader image module has support for backwards compatibility. It only creates one CSS file per icon.

I think this is a symptom of both OOJS UI and MobileFrontend using the mw-ui-icon prefix. Shouldn't both be using their own prefix?

I would love to drop this from the OOjs UI modules' definitions, as soon as Flow and anything else stops relying on it. OOjs UI itself uses the oo-ui-icon prefix; support for mw-ui-icon was added for users of MediaWiki UI, which is now vaguely deprecated.

For caching reasons we may want to only port icons as we encounter issues. Unfortunately I don't think resource loader image module has support for backwards compatibility. It only creates one CSS file per icon.

I'm not sure what you mean here?

I'm not sure what you mean here?

Consider an icon mw-ui-edit that you rename to mw-ui-mf-edit that's cached in the page html.

As far as I'm aware you cannot generate a CSS rule
.mw-ui-edit,mw-ui-mf-edit for backwards compatibility using the resource loader image module without adding the icon twice (bad idea... Although possibly gzip may help I wouldn't want to try :))

Ah. Actually, it can be done, by (ab)using the same feature that allows those problematic selectors like .oo-ui-icon-derp, .mw-ui-icon-derp for OOjs UI icons ;) You can set the selector option in module definition to a selector template, like .mw-ui-mf-{name}, .mw-ui-{name}. It is mentioned here: https://github.com/wikimedia/mediawiki/blob/master/includes/resourceloader/ResourceLoaderImageModule.php#L60 (I'm not sure if I can call this "documented", but hey, C+ for effort). There's a couple modules in MF that use selector already.

Ah. Actually, it can be done, by (ab)using the same feature that allows those problematic selectors like .oo-ui-icon-derp, .mw-ui-icon-derp for OOjs UI icons ;) You can set the selector option in module definition to a selector template, like .mw-ui-mf-{name}, .mw-ui-{name}. It is mentioned here: https://github.com/wikimedia/mediawiki/blob/master/includes/resourceloader/ResourceLoaderImageModule.php#L60 (I'm not sure if I can call this "documented", but hey, C+ for effort). There's a couple modules in MF that use selector already.

Ah that's true.. with a prefix it can be done. Yet if you wanted to change {name} to {anothername} and keep the prefix that would be trickier.

Jdlrobson renamed this task from Regression: Oojs ui icon applying to main menu watch star when echo/editor used to Regression: Oojs ui icons replace mw-ui-icons when Echo used.May 10 2016, 4:01 PM
Jdlrobson updated the task description. (Show Details)
Jdlrobson closed this task as Resolved.Jun 6 2017, 9:57 PM
Jdlrobson claimed this task.

No longer a problem the lesson being prefix where possible.