Page MenuHomePhabricator

External links icons are missing on special pages and various secondary content when using ResourceLoaderSkinModule
Open, LowPublic

Assigned To
None
Authored By
Isarra
Apr 29 2021, 11:16 PM
Referenced Files
F34442296: a.png
May 5 2021, 7:05 PM
F34442293: b.png
May 5 2021, 7:05 PM
F34442272: a.png
May 5 2021, 7:05 PM
F34442274: b.png
May 5 2021, 7:05 PM

Description

The content-links feature should apply external links styles to all page content, including parsed wikitext message strings on special pages. It currently only applies the links to mw-parser-output, which is not used for parsed message strings.

Either these parsed messages should be wrapped in mw-parser-output too, or the styles should just be applied to the page block in general, or something.

Details

Related Changes in Gerrit:

Event Timeline

But seriously I think maybe we should just start wrapping all parser output in mw-parser-output? This would also allow for other non-content blocks of normally parsed wikitext that extensions may add, such as to the sidebar or footer, to maintain consistent UX with regular content, and consistent UX is, uh, important. Good. Optimal? Maybe.

The content-links feature should apply external links styles to all page content, including parsed wikitext message strings on special pages. It currently only applies the links to mw-parser-output, which is not used for parsed message strings.

The content-links feature although new only keeps the status-quo.

This has been the case since 2017 https://gerrit.wikimedia.org/r/c/mediawiki/core/+/390423/1/resources/src/mediawiki.skinning/content.externallinks.css
@matmarex @Esanders given you switched from .mw-body-content to mw-parser-output you may be interested in this discussion.

External links icons are missing on special pages and various secondary content when using content-links feature

Which special pages? Which secondary content? Can you give some examples?

PS. (these styles are also provided by the legacy feature so generalized the title of this ticket)

Jdlrobson renamed this task from External links icons are missing on special pages and various secondary content when using content-links feature to External links icons are missing on special pages and various secondary content when using ResourceLoaderSkinModule.Apr 30 2021, 12:28 AM

...anything run through some sort of getMessage( 'msg', [params] )->parse()? Lots of special pages begin with some sort of customisable blurb that does that. Other extensions do that various places as well, inside or outside of the content block. And parse() sends it through the parser, so it's basically just parser output without the css class.

This has been the case since 2017 https://gerrit.wikimedia.org/r/c/mediawiki/core/+/390423/1/resources/src/mediawiki.skinning/content.externallinks.css
@matmarex @Esanders given you switched from .mw-body-content to mw-parser-output you may be interested in this discussion.

I don't really remember this change, but it was probably for compatibility with something in VisualEditor. We display parser output in some unusual places (e.g. the edit notices popup, or the editing surfaces inside dialogs), and presumably we couldn't add the 'mw-body-content' class there for some reason, or possibly we were already using 'mw-parser-output' and it seemed easier to change the styles (which is required for TemplateStyles's styles to apply).

External links icons are missing on special pages and various secondary content when using content-links feature

Which special pages? Which secondary content? Can you give some examples?

Two examples where the intro text has a bunch of external links:

Funnily enough, both of these use class="plainlinks" to remove the external links icons (but you can still see that some styles are missing because the link colors are the wrong shade of blue).

I don't really remember this change, but it was probably for compatibility with something in VisualEditor. We display parser output in some unusual places (e.g. the edit notices popup, or the editing surfaces inside dialogs), and presumably we couldn't add the 'mw-body-content' class there for some reason, or possibly we were already using 'mw-parser-output' and it seemed easier to change the styles (which is required for TemplateStyles's styles to apply).

I looked at Ed's merged changes around November 2017, and found these three commits that look related: 390423 390417 390422 and one of them references T180214.

Okay understood.

T279388 seeks to make the mw-body-content class mandatory for all skins and to change the meaning to "wraps output page content" and making sure that the contents of OutputPage::getHtml are wrapped in that element. Thoughts on that welcome. Using a new class there is also an option.

There's also an argument to be made here that these are overly specific selectors and the .mw-parser-output is redundant, but I imagine it's this way to avoid external link icons in the side bar?

Right now not every skin adds mw-body-content but I would accept a patch that applies the rules to both in the mean time.

e.g.

.mw-parser-output a.external[ href^='ftp://' ],

becomes

.mw-parser-output a.external[ href^='ftp://' ],
.mw-body-content a.external[ href^='ftp://' ],

[…] Picking up where I left off in in T279388#697456, when I linked to T155863.

… so, In T155863 and change 341073 the external link styles were changed from targetting .mw-body (which was and is a skin layout construct) to targetting .mw-body-content (which was and still is for the time being, a semantic construct for generic content styles). This was motivated by needing to allow VisualEditor and OOUI to let the skin apply these generic content styles to its modal dialogs, and in its "edit notice" popovers etc. (It couldn't apply mw-body as that would turn random parts of the UI into a vector skin column). Shortly after this change, we found that for regular page views, it was not exactly a no-op as intended. While there was little to nothing between "mw-body-content" child and "mw-body" parent in most skins, Vector and others do place sitenotice and page indicators there. Hence mw-body-content was applied there. See also T119430 and various other resolved tasks from back then.

[…] It seems preferable to use .mw-body-content h2 in a skin like Vector, as opposed to targetting h2 and then have to fight/undo its border and margin everywhere that isn't generic content such as in the skin header, skin sidebar, WVUI library components, etc.

Right now not every skin adds mw-body-content but I would accept a patch that applies the rules to both in the mean time. e.g.

- .mw-parser-output a.external[ href^='ftp://' ],
+ .mw-parser-output a.external[ href^='ftp://' ],
+ .mw-body-content a.external[ href^='ftp://' ],

If we keep this status quo, which could work, then we probably can't also do the proposed re-purposing from T279388 in its current form. Not exactly, anyhow.

The status quo being, in my view, that skins are recommended to have mw-body-content somewhere in their layout applied to elements, or a common parent, that contains content-like children. And to use this class in any generic selectors of their own (unless they only use core skin module features). And that extensions would, once again, be encouraged to add this class to other areas they produce in which such content is displayed.

I agree that mw-parser-output seems redundant here. And it didn't use to be there. It was changed from mw-body-content to mw-parser-output in 2017 in what seems to have been a well-intended effort by @Esanders to avoid impact of misuse from this skin class. However that in turn has exposed this problem in various core areas such as core's edit page, edit notices, block messages, special pages, as well as in VisualEditor itself it seems generic styles now don't always apply correctly.

Example:Block message (ext link)Editnotice in VE (ext link, and h2 heading font)
Actual (regression)
a.png (1×2 px, 350 KB)
b.png (1×2 px, 315 KB)
Expected
b.png (1×2 px, 378 KB)
a.png (1×2 px, 372 KB)
(no parser output)(no mw-body-content)

I'm sure there's more to find, but these are the two I was able to find easiest.


Perhaps we should re-approach scratching the itch of T279388 from a different prespective. I feel like we've gotten quite far away from the problem that started this whole chain reaction and it seems to me at least that it's not worth the cost of rethinking so much, just for the benefit of.... not copying one line of fairly-generic CSS (clear a float). If we do want to invest in making it "easier" to get one's float cleared in skins that have some kind of primary "column", and if we want to actually eagerly abstract this in anticipation of one day other things falling in this bucket, but do so in a way that isn't incompatible with established principles, then perhaps an alternative would be to offer a tiny SkinModule feature that clears the float after .mw-body (a skin-elected layout element). This would be opt-in of course, but many skins already use this, and any others that want to benefit from this would simply add this class in their layout to the appropiate element, typically the parent of elements like sitenotice, indicators and outputpage html (which in turn contains parser output, editpage, or special page output etc.). Yes, that's not fully automatic and requires some small amount of domain knowledge. But, as Einstein is quoted as having once said: "Everything should be made as simple as possible, but not simpler.". I think this is a minimal, simple, relevant, and valuable decision to lay upon the skin author. To say what their "column" wrapper is – if – they want to not clear the float manually.

The main issue I'm seeing is that we're trying to merge logically incompatible classes, as well as mix generic classes with layout classes, and I just don't see that working out. Happy to see other strategies, though.

@Esanders, we could now revert the changes in https://gerrit.wikimedia.org/r/c/mediawiki/core/+/390423/1/resources/src/mediawiki.skinning/content.externallinks.css since mw-body-content does apply to all skins, however, we'd need to make sure VisualEditor consistently adds that class. Is this something your team would be interested in taking on?

Side note: We could also drop the parent class altogether but I imagine there are good reasons to scope it - I suspect we don't want external link icons showing up in sidebars for example?

Change 704978 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):

[mediawiki/core@master] Relax external link CSS selector

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

Change 704978 abandoned by Jdlrobson:

[mediawiki/core@master] Relax external link CSS selector

Reason:

It seems I don't fully understand the problem being solved here, so I defer to someone that does :)

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

...anything run through some sort of getMessage( 'msg', [params] )->parse()? Lots of special pages begin with some sort of customisable blurb that does that. Other extensions do that various places as well, inside or outside of the content block. And parse() sends it through the parser, so it's basically just parser output without the css class.

Kind of T183603: Clarify how content-like messages should be handled out of the TemplateStyles side too (cc @Tgr ) for cross-language-wiki purposes (taking a template from wiki A to different-language B) and for the same on multi-language wikis via Translate extension.

I'm pretty sure this can be solved now by adapting the rules to use .mw-body-content rather than .mw-parser-output. The mw-body-content class wraps SpecialPage content as well as parser output.