Page MenuHomePhabricator

Deprecate and remove external links CSS module from core
Closed, ResolvedPublic

Description

'mediawiki.skinning.content.externallinks' is an opinionated CSS styling module that should not be part of MediaWiki core, but skin-specific!
Current usage: https://codesearch.wmcloud.org/search/?q=content.externallinks&i=nope&files=&repos=

It is opinionated in comparison to other core HTML element styling modules, to provide out of the box styling for some protocols, file-types etc. Its former most important use cases, like styling HTTPS links and links that open in other software, most importantly PDF files, have lost their importance in current times.

It features a number of problematic issues:

  • These opinionated styles should be skin-specific
  • It's not well-maintained, which is a result of it not being skin-specific
  • It features some protocols, and some file types, but these haven't been updated
  • Icons are non-HiDPI-ready
  • Icons are largely outdated and don't follow any style guidelines
  • Icon distances are set in px via padding which make them not scale properly when user increases browser font size, hence makes it an accessibility issue

Proposal

  • Deprecate 'content.externallinks' module
  • Copy styles to Vector, MinervaNeue(?), Modern and possibly Example Skin not necessary. Not a recommended feature.
  • Remove module calls in other skins T269626
  • Remove module from core T269626

Event Timeline

"Opinionated"?

The reason why this is a module in core is that it isn't particularly skin-specific. The module is called from twenty different skins in gerrit alone (who knows how many others also use it out in the wild), and Vector, Modern, and MinervaNeue, and Example are the main skins that don't use it as they either just have their own styles for this, or don't use any.

And why should it be skin-specific? Why should twenty different skins be individually maintaining common content styles following fairly standardised colours? If anything the fact that this is already ill-maintained really only serves to demonstrate what a categorically bad idea it would be to shuffle off the maintenance to have to be done individually twenty times in twenty different places...

Whatever the specific issues are, please document them so they can be resolved in one place, once. Not twenty times in different places. Per the stated list:

  • It features some protocols, and some file types, but these haven't been updated

That some protocols/filetypes may be unlikely to be relevant anymore: a good point. More investigation into the UX implications and needs is probably in order to determine which should be removed/what is missing that would be useful now, indeed. Not sure PDF is a good example of one to get rid of even from a glance, though, as depending on the device it absolutely can result in some rather weird application behaviour, and may also be relevant in terms of potential filesizes for metered connections, or files that can't be effectively used on particular displays. (Like if I'm on a phone, I probably don't want to download a pdf unless I have a particular reason?)

  • Icons are non-HiDPI-ready

In... what way? They're svgs. Yes, there are some issues on non-integer scale factors, but there's kind of... no way to avoid that? Pixels are square and any graphics with straight lines will either line up to them, or... not.

  • Icons are largely outdated and don't follow any style guidelines

Sounds like you have something specific in mind here? Then let's get that fixed. Please file a task for that with some specifics of what needs to be followed, and where the outdatedness is a problem.

  • Icon distances are set in px via padding which make them not scale properly when user increases browser font size, hence makes it an accessibility issue

Sounds both useful and important. And like something that should probably be fixed here so all the skins using the module can benefit. Not only that, but resolving this would likewise also likely resolve issues with alignment across different font-sizes in the skins themselves, thus making the module more suited for reuse.

So just to clarify that we're on the same page, @Volker_E ...you are proposing that we reopen T65521 and reintroduce a lot of unnecessary code duplication because the current module implemention has some bugs. Why not to fix those bugs? (Full disclaimer: I collaborated with @Isarra, @matmarex and @lcawte on T65521 and I might've written a skin or two over the years.)

I'm not going to lie, I find this proposal just simply baffling. Since when was intentionally reintroducing a lot of unnecessary code duplication something we wanted to do?

Let me try to address your concerns and raise some of my own...

'mediawiki.skinning.content.externallinks' is an opinionated

[citation needed]

CSS styling module that should not be part of MediaWiki core, but skin-specific!

[citation badly needed]

Current usage: https://codesearch.wmcloud.org/search/?q=content.externallinks&i=nope&files=&repos=

Alternatively: "Currently used by many maintained skins, including two used on WMF production and one actively developed one hosted on GitHub." Not to mention all the non-WMF-deployed, hosted-on-gerrit skins which use this module.

Its former most important use cases, like styling HTTPS links and links that open in other software, most importantly PDF files, have lost their importance in current times.

[according to whom?]

These opinionated styles should be skin-specific

[citation still needed]

It's not well-maintained, which is a result of it not being skin-specific

What qualifies as "well-maintained" and why cannot a core RL module be "well-maintained"? If we're going to throw dangerous arguments like these around, they oughta be well defined to prevent any future confusion. Either there is a criteria for "well-maintained", or this is just a subjective opinion.

It features some protocols, and some file types, but these haven't been updated

This sounds like a valid enhancement request, but not exactly like a blocker of any kind to me. What protocols and file types should be added? Might it be useful to move that discussion to a separate task?

Icons are non-HiDPI-ready

I believe @Isarra addressed this in her reply above.

Icons are largely outdated and don't follow any style guidelines

What do you mean by "largely outdated"? The latter part is no doubt valid as the very concept of these icons predates any and all related style guides by quite many years. IIRC we didn't want to make too many radical changes in 2013.

Icon distances are set in px via padding which make them not scale properly when user increases browser font size, hence makes it an accessibility issue

This sounds like a bug we really need to fix, for Accessibility is important!

Proposal

  • Fix the Accessibility bug
  • Curate the list of supported protocols and file types, add new relevant ones to the list

This might not be exactly as easy as "let's throw a bunch of widely-used code out of the window just because it's possible to do that", but is sure is way more useful in the long run and as a nice bonus it makes third-parties lives just a notch easier, which has been something of paramount importance to me.

"Opinionated"?

The reason why this is a module in core is that it isn't particularly skin-specific. The module is called from twenty different skins in gerrit alone (who knows how many others also use it out in the wild), and Vector, Modern, and MinervaNeue, and Example are the main skins that don't use it as they either just have their own styles for this, or don't use any.

And why should it be skin-specific? Why should twenty different skins be individually maintaining common content styles following fairly standardised colours? If anything the fact that this is already ill-maintained really only serves to demonstrate what a categorically bad idea it would be to shuffle off the maintenance to have to be done individually twenty times in twenty different places...

Thanks both, @Isarra and @ashley for sharing your opinions.
It hasn't obviously been clear enough where this take of moving these styles to a skin comes from.
The icons for external links, file types and protocols are currently a core file, but are they rightful so?
Is this such an important functionality that improves usability (only strong argument for it being in core IMHO) that it needs to be supported across skins – with all the listed shortcomings listed? Before we carry on a feature the right question to ask is about necessity and usefulness of the feature.

From my POV I don't think it is useful. it's an opinionated style that doesn't help a huge number of users (anymore) if it's helping anyone at all. I'd bet that many users have lil clue why there is a miniscule icon with a red widget spinner like symbol at 16x16px at the end of the link and what it should represent, what it should signify to the user.


Specifically as the link is also taking you to a different website (external link anyone).
All those link styles seem not be a useful part of core, it's an additional aesthetical choice, better located on a per skin-base.

All those link styles seem not be a useful part of core, it's an additional aesthetical choice, better located on a per skin-base.

Per skin? Per twenty different skins all doing the same thing, using the exact same code? I'm sorry, but how many projects are you maintaining that are forced to duplicate the same code time and again? I can only guess that your own opinion is perhaps based on not having to deal with such things, as otherwise I cannot fathom why you seem to think it would be a good thing for us to have to deal with such direct copies even more than we already do across skins in general (menu parsing, basic mobile support, all manner of things that never even made it to core).

This is only even more baffling when your main arguments still seem to be that the styles/icons may need updating:

I'd bet that many users have lil clue why there is a miniscule icon with a red widget spinner like symbol at 16x16px at the end of the link and what it should represent, what it should signify to the user.

You raised some better points initially, but as is, this is just baseless supposition. Mind you, it need not be. If you want to do some research into what icons are or would be useful for what, that could indeed be helpful. Again, you have raised potential issues that there is likely cause to update, narrow down, and strengthen these styles across the board here, but lacking some specifics (have you talked to the users? Do you have testing data on how clicks are affected? Are there any numbers how many people are being confused/misled? Any data on how often the different styles would apply in general?) this simply isn't actionable in any useful manner.

I'm just going to close this, as the task really doesn't make a whole lot of sense. It's basically 'let's deprecate these styles and move them into <the only four skins that don't use the styles> because of <reasons that range from just confusing to very important, where we definitely want to make sure that if this is fixed, all the skins using it get these fixes>'.

That being said, do please file tasks for the issues raised. Even the ones I personally find baffling probably have something to them, and some of the others are both much clearer and easy to implement, and should we properly resolve the lot there are many, many third-party sites (and MonoBook users, if all you care about is Wikimedia-hosted projects) who would be in the position to greatly benefit from this. Hells, it might even be worth reintroducing to Vector at that point and reducing some code there.

All mediawiki.skinning prefixed modules other than .interface should be considered deprecated now. The goal is for all skins to have a single module defining their stylesheets, so I think the task is valid, but we need to work on defining the problem here.

If we are retaining mediawiki.skinning.content.externallinks it should be merged into ResourceLoaderSkinModule as an optional module if the purpose is sharing CSS with skins. The acceptance criteria " Deprecate 'content.externallinks' module" is definitely valid to that point.

I am open minded about the other bullet points and happy to act as a friendly mediator for all the concerns raised here.
Regardless moving this to ResourceLoaderSkinModule seems like the first logical step.

Change 642493 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/core@master] The mediawiki.skinning.content.externallinks RL is deprecated

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

See also related old discussion in T56604: Large number of CSS rules for external links (especially Krinkle's technical comments there, which might still be useful here?).
My 2cents: As I wrote there in 2013, I think some of the icons were very useful such as those for media files (to avoid nasty surprises like sudden loud sounds when I'm sitting in a library) and mailto: links (which often open a program or tab that I didn't want to have open).
I strongly agree that we used to have far too many, and some skins still do (e.g. Modern still has the https padlock), but I'd welcome the return of a few in Vector/everywhere.
Hope that helps, and that this is the correct location to comment. :-)

Change 642493 merged by jenkins-bot:
[mediawiki/core@master] The mediawiki.skinning.content.externallinks styles are moved into ResourceLoaderSkinModule

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

Change 645451 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/Timeless@master] Timeless should use skin feature content-links for external link styling

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

Change 645453 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/MonoBook@master] Monobook should use content-links feature instead of mediawiki.skinning.content.externallinks

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

Change 645451 merged by jenkins-bot:
[mediawiki/skins/Timeless@master] Timeless should use skin feature content-links for external link styling

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

This solution is good, but we do need to actually migrate all the skins. Now that more of them are actually done, though, we have some decent examples so that should be more feasible than some of these changes have been, in terms of figuring out what to do with them.

Also seriously, if folks haven't filed tasks for the specific other issues with the module/feature itself, this would be really helpful as it's still going to need those fixes, whatever form it takes.

Change 645453 merged by jenkins-bot:
[mediawiki/skins/MonoBook@master] Monobook should use content-links feature instead of mediawiki.skinning.content.externallinks

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

Jdlrobson claimed this task.
Jdlrobson updated the task description. (Show Details)

Next step is T269626.
Given the amount of skins using this module I don't think it makes sense to remove this right now.

Given the question marks over these styles, I don't recommend copying these to Vector, Minerva, Modern or Example which would lead to further adoptional. The module is optional and defaults to false for now.