Page MenuHomePhabricator

Large number of CSS rules for external links
Closed, ResolvedPublic

Description

Assigning to Parser as I suspect this is what would need to be fixed up to support this.

This patch [1] demonstrates the large number of rules exist for external links:

The browser matches styles from right to left, so the rightmost selector is really important.

We shouldn't need to have to resort to CSS rules in the form div#content a.external[href ^="https://"] - a link with class external should also have a class describing what type of external link it is if it is necessary. These selectors are less efficient and are not supported by older browsers.

Ideally we should simply these rules to become something like:

.link-audio,
.link-video,
.link-document,
.link-irc,
.link-ftp,
.link-https,
.link-news,
.link-mailto {
padding-right: 13px;
background: transparent center right no-repeat;
}

with rules for each of them for their correct icon.

[1] https://gerrit.wikimedia.org/r/#/c/85920/2/skins/vector/externalLinks.less


Version: unspecified
Severity: normal
See Also:
https://bugzilla.wikimedia.org/show_bug.cgi?id=63725
https://bugzilla.wikimedia.org/show_bug.cgi?id=63521

Details

Reference
bz54604

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
bzimport added a subscriber: Unknown Object (MLST).

Honestly I'd recommend just killing these link icons outright...

(In reply to comment #1)

Honestly I'd recommend just killing these link icons outright...

This is a design question, I think. Adding the Bugzilla keyword accordingly.

(In reply to comment #1)

Honestly I'd recommend just killing these link icons outright...

Some of the other icons could /possibly/ be removed without screaming.

But the PDF icon is important. Users get frustrated when they click a PDF without warning.

(The only alternative is [[Template:PDFlink]] which makes article/citation code more complicated, so that's not good.)

(In reply to comment #1)

Honestly I'd recommend just killing these link icons outright...

One of the things to consider is when and where the links are actually used, and if special treatment is even sensible anymore.

Generally we'd probably only want special icons for things that are both fairly commonly used and where the browser reaction would be significantly different from general browsing - for things like PDFs, which are often used for sources, that can open up in a new window or rearrange the tab depending on browser, or mailto and IRC which often send the request out of the browser entirely and open a new window. Users do like warning when it comes to that sort of thing, indeed.

Whereas we probably don't want anything special for things like https, ftp, etc where the browser just opens it like normal, especially since for instance https is becoming more and more common as the default...

My gut says remove them all esp. the https links, as @Isarra mentioned the only one that seems to make any real sense is links that point to files, like PDFs, however internal (Commons) links have their own viewer. External PDFs seem like the only file that marking their links seems relevant in any way. Most modern browsers have built-in PDF renders so even the days of PDFs being treated special is soon becoming irrelevant.

My recommendation would be to remove all special formatting other than wikilinks vs external links.

If we could determine and mark wikilinks that are to other sister projects, I'd recommend they be treated as regular wikilinks as well.

(In reply to comment #5)

My gut says remove them all esp. the https links, as @Isarra mentioned the
only
one that seems to make any real sense is links that point to files, like
PDFs,
however internal (Commons) links have their own viewer. External PDFs seem
like the only file that marking their links seems relevant in any way. Most
modern browsers have built-in PDF renders so even the days of PDFs being
treated special is soon becoming irrelevant.

Isarra mentioned mailto and IRC links too, which I'd agree are instances where the icon is useful.

The audio/video icons are also good to have, per principle of least astonishment (eg, someone adds a link to a loud .wav file into an article's external links, and someone else in a quiet office clicks on it).

They also help editors to notice that these uncommon link types are present, and hence to check whether they're really suitable/wanted or not. (If I see an audio-file icon, I often click it to check for spam/vandalism).

My recommendation would be to remove all special formatting other than
wikilinks vs external links.

If we could determine and mark wikilinks that are to other sister projects,
I'd recommend they be treated as regular wikilinks as well.

Interwiki links (everything in https://meta.wikimedia.org/wiki/Interwiki_map )
are already styled with a slightly-lighter blue, via class "extiw" (external interwiki) located in https://svn.wikimedia.org/viewvc/mediawiki/trunk/phase3/skins/common/commonElements.css?view=markup and skins/monobook/main.css

Per Isarra and Quiddity, I think that the only ones that should be killed are the HTTPS and FTP ones. That CSS is not ridiculous, it's just a little uncool.

The CSS is pretty ridiculous. Let's not forget that CSS parses from the rightmost rule so something like a.external[href ^="news:"] can be a pretty expensive operation. These rules should be killed with fire in favour of specific classes like link-pdf if needed.

I don't care if we kill or keep rules I just want these rules to die and the CSS in general to be cleaned up.

(In reply to comment #8)

The CSS is pretty ridiculous. Let's not forget that CSS parses from the
rightmost rule so something like a.external[href ^="news:"] can be a pretty
expensive operation.

[citation needed]. I don't see why would it be particularly more expensive than any of the hundreds of other rules we have. Assuming that the hypothetical browser does indeed naively match from the right side always, [href^="news:"] will immediately not match (most elements don't even have a href attribute) and the ten or so of such rules should not be noticeable.

(In reply to comment #9)

(In reply to comment #8)

The CSS is pretty ridiculous. Let's not forget that CSS parses from the
rightmost rule so something like a.external[href ^="news:"] can be a pretty
expensive operation.

[citation needed]. I don't see why would it be particularly more expensive
than
any of the hundreds of other rules we have. Assuming that the hypothetical
browser does indeed naively match from the right side always, [href^="news:"]
will immediately not match (most elements don't even have a href attribute)
and
the ten or so of such rules should not be noticeable.

Afaik selector engines have quick lookup maps for IDs, classes and elements. In the case of ".mw-link-pdf" that is a very quick lookup.

In the case of "#bodyContent a.external[href=..]" it is a lookup for elements of 'a', then for each the classname and the attribute is matched, and then it recursively does a parent check until it finds #bodyContent or the document element.

In theory it could optimise if it had a quick lookup for elements having a certain attribute. But afaik they don't have that, and even if they did, I isn't more efficient per se, cause then the post-check would have to verify tag name and class name for each.

Either way, none of this in particular is bad, it's the whole package of:

  • A very strong selector (#bodyContent)
  • A parent selector
  • Both element and class name (should never be used together)
  • Additional attribute matching and string operations on that
  • Hardcodes details that (other) stylesheets should not have to know or hardcode, thus causing maintenance issues (e.g. the fact that it is an <a>, and is inside #bodyContent. The container can change, and links might be wrapped for presentational reasons)

Up sides of using a simple class:

  • Faster query (one could argue about how much, but it is certainly faster)
  • Support for older browsers (this is not a priority though, since all our grade A browsers support it, and it's a minor enhancement with graceful degradation)
  • Easier to extend from other components as there is no need to have any knowledge about implementation details (container element, tag name etc.)

(In reply to comment #9)

(In reply to comment #8)

The CSS is pretty ridiculous. Let's not forget that CSS parses from the
rightmost rule so something like a.external[href ^="news:"] can be a pretty
expensive operation.

[citation needed]. I don't see why would it be particularly more expensive

If you really feel the need for a citation check out:
http://stevesouders.com/efws/css-selectors/csscreate.php

That aside use of a single class .link-https rather than a combination of class .link-https, div#content a.external[href ^="https://"] would be preferable for a variety of reasons readability, maintainability, reduced file size... are you really telling me this is not a no-brainer?

(In reply to comment #11)

That aside use of a single class .link-https rather than a
combination of class .link-https, div#content a.external[href
^="https://"] would be preferable for a variety of reasons
readability, maintainability, reduced file size... are you really
telling me this is not a no-brainer?

Using the classes as you proposed does seem sane and mostly non-troublesome (apart from the classic caching issues). I am perfectly fine with doing that (as long as I'm not the one who will have to jerry-rig the Parser).

Killing the icons as suggested in comment 1 is a completely different thing and, apart from what I mentioned in comment 7, I will vehemently oppose doing it.

(In reply to comment #11)

If you really feel the need for a citation check out:
http://stevesouders.com/efws/css-selectors/csscreate.php

Either I am unable to figure it out, or it doesn't work. Reflow time is 0 for me no matter what I do. "page load time" is measuring the speed of my connection if anything.

(In reply to comment #12)

CSS rule efficiency table:
http://csswizardry.com/2011/09/writing-efficient-css-selectors/

This looks interesting; sorry, I don't have time to dig into it right now, but hopefully I will later. (Hopefully it's still relevant - it was apparently written in 2011, and two years is like a few decades at least ;) )

Great. I'm agnostic about killing the icons or not. My main goal is to start cleaning up and improving all the desktop CSS.

(In reply to comment #14)

Great. I'm agnostic about killing the icons or not. My main goal is to start
cleaning up and improving all the desktop CSS.

Sensible thing to do, indeed. Just bear in mind caching issues. >.<

See http://lists.wikimedia.org/pipermail/wikitech-l/2013-October/072642.html

Main highlights:

  • We shouldn't confuse English Wikipedia's overrides with code in core. However, there is room to clean that up, even if they want to keep a different icon from core (e.g. moving the Modern-specific code to Modern.css).
  • I like the idea of doing it in the parser, but it obviously requires more testing than a CSS change. To expand on what I said in the email, we could do something generic like 'mw-link-extension-pdf'. We would need to decide whether to do it for all extensions, or just the ones where core styles it.
  • We could consider using the Commons PDF icon in core, if it's freely licensed (have not investigated).

Change 123817 had a related patch set uploaded by Jdlrobson:
Vector: Simply style external links

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

During the typography experiment, these were removed, no one raised this as an
issue during the 5 months it was live, suggesting that explicit pdf icons etc
are not as useful as we like to believe.

How many of the (English) wikis in question did not have *local* icons? How do you know how many were actually affected by these changes?

I don't understand the question/problem. All wikis got the beta features experiment.

If those wiki's have local icons, that is fine, they won't be effected by this change to Vector, and will get a performance boost by having a simpler set of css selectors for these icons.

If they're not affected, they're not representative. It doesn't sound rational to mention them in the commit message.

Change 123817 merged by jenkins-bot:
Vector: Simply style external links

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

(In reply to Jon from comment #14)

Great. I'm agnostic about killing the icons or not. My main goal is to start
cleaning up and improving all the desktop CSS.

I thought we'd agreed to clean up the expensive CSS, and not to delete the icons (which is what https://gerrit.wikimedia.org/r/123817 does).

:/

From my understand the external/file format link icons are only relevant when you are actually leaving the site, or when it would cause your browsers to download a file rather than loading it in some kind of built in player, either on site or in-browsers. The original code for this I assume was created prior to mediawiki being able to natively handle many of these file formats.

That said I think most if not all of these are no longer needed and confuse many users. Overall I would urge us to move towards simplicity when it comes to links, designating only links that take you out of the WMF sites, and links that point to non-existent articles.

If knowing target file types is relevant to a small subset of users I've argue that this code should be gadgetized by someone who cares to retain this feature.

Apologies if I misunderstood and this had been the agreement.

That said, I am happy to deal with any after effects of this change. I'd hope such a drastic change would surface the icons that are important.

I think the removal of them although somewhat extreme, will make it clearer and easier to restore ones that are vital.

A lot of these icons may be better done in something like MediaWiki:Vector/Monobook/Common.css on a per project basis anyhow.

(In reply to Jared Zimmerman (WMF) from comment #23)

From my understand the external/file format link icons are only relevant
when you are actually leaving the site, or when it would cause your browsers
to download a file rather than loading it in some kind of built in player,
either on site or in-browsers. The original code for this I assume was
created prior to mediawiki being able to natively handle many of these file
formats.

Generally external links do leave the site. This is what 'external link' means.

(In reply to Jon from comment #24)

That said, I am happy to deal with any after effects of this change. I'd
hope such a drastic change would surface the icons that are important.

I think the removal of them although somewhat extreme, will make it clearer
and easier to restore ones that are vital.

I would suggest that breaking everything and then seeing what people complain about the most may not be the best way to determine what is important. Yes, it may work, but more than anything it's probably just going to annoy people, and needlessly alienating users is a trend that needs to stop, not continue.

To be clear, we still show an external link icon.
Yes I suspect it may annoy people but this can be easily remedied. Change is also a trend that needs to start.

Isarra, from a technical perspective a PDF hosted on commons while I'm on wikipedia is "leaving the site" but this isn't how most readers or casual editors see it. That was my point. I appreciate your point about it from a literal perspective.

(In reply to Jon from comment #26)

To be clear, we still show an external link icon.
Yes I suspect it may annoy people but this can be easily remedied. Change is
also a trend that needs to start.

And there is a right way to make change happen - by proposing changes and getting feedback. By implementing this, you have inherently proposed it, and now you are getting feedback. Whether or not they should be merged depends on what consensus is established either here or in other appropriate channels; we have these channels for a reason.

(In reply to Jared Zimmerman (WMF) from comment #27)

Isarra, from a technical perspective a PDF hosted on commons while I'm on
wikipedia is "leaving the site" but this isn't how most readers or casual
editors see it. That was my point. I appreciate your point about it from a
literal perspective.

External links appear if the external link format is used, regardless of where the link points. They are only supposed to be for links that go off the site (in other words, ones that are 'external'); we normally use wikilinks (ones formatted [[like this]]) to link to local and sister project pages, which do not result in external link formatting.

For more information about how links work in MediaWiki, please see https://www.mediawiki.org/wiki/Links

Isarra, I've mentioned it before and I'll mention it here again, that process makes total sense for editor or logged-in user features. It doesn't make sense for reader facing features, as these users are not part of this conversation.

You know how to do cross site links but many users do not, and the systems are not smart enough to automatically convert them in many cases.

(In reply to Jared Zimmerman (WMF) from comment #29)

Isarra, I've mentioned it before and I'll mention it here again, that
process makes total sense for editor or logged-in user features. It doesn't
make sense for reader facing features, as these users are not part of this
conversation.

You know how to do cross site links but many users do not, and the systems
are not smart enough to automatically convert them in many cases.

I agree, new users often don't appreciate the significance of many features. But how is this relevant to the matter at hand?

(In reply to Isarra from comment #28)

And there is a right way to make change happen - by proposing changes and
getting feedback. By implementing this, you have inherently proposed it, and
now you are getting feedback. Whether or not they should be merged depends
on what consensus is established either here or in other appropriate
channels; we have these channels for a reason.

Its relevant because asking for feedback on "the usual channels" completely omits the target user, when the target users is a reader. Jon's point that we can do it and then iterate speaks to the fact that it is one of our only ways to get reader feedback. To roll the change out publicly then iterate. We can get better at this eventually, but at the current juncture, I think this is a sound plan based on our infrastructure for eliciting feedback from readers and others who aren't aware of, or participate in "the usual channels"

Hope that makes sense.

Then again, can you expect a casual reader to ever appreciate that which is not there? A removal like this is hard to notice, because most links don't have these icons.

Your proposal is basically: Ah, people asked for this 10 years ago. but there is a bunch of annoying css rules, so let's remove them and wait how long it takes for people to ask if they can have the feature back. This feature contributes to the rule of least astonishment, and you are removing it without considering replacing it with an alternative.

I find that somewhat presumptuous if i'm honest. But then again, I thought messing with fonts was a dangerous plan and that turned out just splendid, so what do I know....

Users of a product are aware (although not always consciously) of the absence of something equally well as the awareness of and existence of something (cruft). A user does not need to be aware of something, an "absence," say to be aware that a product is better because of design choices to remove, and simplify.

"Good design is as little design as possible — Less is more – because it concentrates on the essential aspects and the products are not burdened with non-essentials. Back to purity, back to simplicity" —Dieter Rams

"Perfection is achieved perfection not when there is nothing left to add, but when there is nothing left to take away" –Antoine de Saint-Exupery

In reply to Jared Zimmerman (WMF) from comment #31)

Its relevant because asking for feedback on "the usual channels" completely
omits the target user, when the target users is a reader. Jon's point that
we can do it and then iterate speaks to the fact that it is one of our only
ways to get reader feedback. To roll the change out publicly then iterate.
We can get better at this eventually, but at the current juncture, I think
this is a sound plan based on our infrastructure for eliciting feedback from
readers and others who aren't aware of, or participate in "the usual
channels"

I would think that 'users' here would include readers and editors both, and even then, editors often know their readers better than one might expect (such as by talking to them, or being them). So talking to the editors is probably exactly the right place to start. They could tell you things like the fact that some of these external link styles, such as IRC, will never appear in article space (on a non-technical project, anyway - mw.org or archwiki and similar probably have quite a few irc links in various articles), but are often used in project and user space (so changes to that style would be purely editor-facing on projects like Wikipedia, not reader). Or that readers often complain when <blah> or have trouble with <blah>.

And there is also ample infrastructure in place to test potential changes, is there not? For instance you could do:

  • A/B tests to determine if readers are less likely to click on external if they are or aren't visibly marked as whatever kind of link they are.
  • Scripted tests on usertesting.com to gain insight into possible reactions to unexpected pdfs, audio files, IRC links, etc vs regular external links, and to the external link icons themselves when they are used, such as if they even figure out what they mean.
  • A betafeature for users to see if removing the special icons affects their workflows, or if they prefer it outright.

If there is a problem, you should be able to prove it without just deploying it first and asking questions later. Also what Derk-Jan said.

Its relevant because asking for feedback on "the usual channels" completely
omits the target user, when the target users is a reader. Jon's point that
we can do it and then iterate speaks to the fact that it is one of our only
ways to get reader feedback. To roll the change out publicly then iterate.
We can get better at this eventually, but at the current juncture, I think
this is a sound plan based on our infrastructure for eliciting feedback from
readers and others who aren't aware of, or participate in "the usual
channels"

Hope that makes sense.

breaking the world, and then seeing who complains isn't really a good way to get feedback from readers. First off, most people who complain aren't going to be "readers" they are going to be active users who are familiar with the regular channels. If you actually get readers angry enough that they figure out how to complain, you've probably massively screwed up the site. Second, that's prone to a massive confirmation bias of "Well nobody or not that many people complained out of a group of people who don't know how to complain, therefore it must be a good idea"

I really think this is an extraordinarily dangerous line of reasoning to go down.

(In reply to Isarra from comment #34)
We did a beta feature, it was called typography refresh, and little to no feedback (negative or positive) addressed the removal of these icons, that was the impetus for moving forward with this as a simple patch.

If you read on the Beta Features page, you'll see that Beta Features should not be only subtractive in nature, e.g. packaging up the removal of these icons as its own feature makes very little sense. — https://www.mediawiki.org/wiki/Beta_Features/Package

The world is not breaking. The world of Vector is just going through turbulence.

As stated above external links were removed as part of the typography beta feature. Actually, not a single person wrote about this on the talk page (https://www.mediawiki.org/wiki/Talk:Typography_Update) during the 6 months it was live. This evidence leads me to believe that they are /not/ important and it is the basis for removing them.

If this proves to be wrong, then we react and restore/amend it, it's no big deal. Commits and reverts are cheap.

The fact we have to actively discuss trivial changes like this concerns me greatly. We have a huge editor decline and here we are arguing about some icons next to links.

Also I think you mean /editors/ when you say /readers/ here. I think most /readers/ care simply if a link will take them to an external site or not.

I'm just going to leap in here in the middle of this heated discussion and change the title of the bug to be grammatically correct. It's been bugging me. Sorry.

(In reply to Jon from comment #37)

The world is not breaking. The world of Vector is just going through
turbulence.

I didn't mean to point fingers at any specific event, and wasn't actually thinking of typo refresh when making my comment (Or the actual subject of this bug. Oh well guess this bug is pretty hijacked). I wanted to address in the abstract the attitude in comment 23 of making changes willy nilly (which I flippantly phrased as "breaking the world") and then iterating on them until people like them (or cynically: until they become fait accompli), as being a good way to get feedback. I think its a good way to piss off people, and self-justify changes via confirmation bias.

The fact we have to actively discuss trivial changes like this concerns me
greatly. We have a huge editor decline and here we are arguing about some
icons next to links.

I'm not sure I follow. Not asking editors for feedback on changes is somehow going to increase editor retention?

Also I think you mean /editors/ when you say /readers/ here. I think most
/readers/ care simply if a link will take them to an external site or not.

I was responding as if "readers" = Users without an account who only read. For the actual change I think both groups care. My comment was aimed solely at attitudes for getting feedback. I'm not actually that concerned about the actual change

(In reply to Jared Zimmerman (WMF) from comment #36)

(In reply to Isarra from comment #34)
We did a beta feature, it was called typography refresh, and little to no
feedback (negative or positive) addressed the removal of these icons, that
was the impetus for moving forward with this as a simple patch.

Lumping multiple changes together and then testing the entire pile is problematic by its very nature: it can become very difficult to differentiate between the effects of different changes, and getting meaningful feedback on any specific one often becomes very difficult. Thus you can wind up with an overall positive response despite individual aspects which significantly detract from the experience, and then wind up thinking those, too, are positive when they really aren't (or visa versa).

Thus, to say anything specific about the external link icons and potential changes, I would suggest that you need to test or otherwise investigate those specifically, and distinctly, first. Jon's not wrong that this is a relatively minor thing, but while that means a full-on battery of tests probably isn't actually necessary here, like with any other change, discussion is definitely in order in order to establish some sort of consensus as to what best serves the users. So here we are. Discussing. Or we were, before the entire thing got a wee bit sidetracked.

(In reply to Jon from comment #37)

Commits and reverts are cheap.

No. There's almost no evidence to suggest that commits and reverts of this nature are cheap. There's a ton of evidence to suggest that commits and reverts of this nature are expensive (in terms of community time and developer time).

We have a huge editor decline and here we are arguing about some icons next to
links.

No fearmongering here, please. The stated editor decline is dubious, bordering on spurious. This bug is strictly about "some icons next to links." You (Jon) are sidetracking the technical conversation with FUD about an editor decline. Focus on the issue at hand, please.

(In reply to Jon from comment #37)

As stated above external links were removed as part of the typography beta
feature. Actually, not a single person wrote about this on the talk page
(https://www.mediawiki.org/wiki/Talk:Typography_Update) during the 6 months
it was live. This evidence leads me to believe that they are /not/ important
and it is the basis for removing them.

However, anyone who used a wiki (like English Wikipedia, which has such rules at https://en.wikipedia.org/wiki/MediaWiki:Common.css and https://en.wikipedia.org/wiki/MediaWiki:Vector.css) with local rules would not have seen any difference.

Thus, it's a valid question how many people who used the Beta feature saw a visible change to the main (PDF and HTTPS seem like the biggies for article space, with the audio being used a decent amount, but not as much, but I don't have data) icons. If a tiny number were affected, then "not a single person wrote about this" is not a convincing argument.

I think English Wikipedia Beta users saw neither change, since PDF is at [[MediaWiki:Common.css]] and HTTPS is at [[MediaWiki:Vector.css]].

However, it is certainly true that we are not going to be able to do dedicated research for every change, and this may be one such example. I agree that this feature affects both readers and editors, and we need to consider the impact on both. Having tools to do A/B tests on readers would certainly help, but it wouldn't solve everything.

It can definitely be iterated on regardless. In particular (if we proceed without the format-specific icons), there should be at least one follow-up change to do the following:

  1. Remove obsolete FIXME comment from externalLinks.css.
  2. Release notes
  3. Fixes from https://gerrit.wikimedia.org/r/#/c/123817/3/skins/vector/components/externalLinks.less . Note the point about div#content on the left, and might as well do the formatting fix on the right (space before right paren) since there needs to be a commit anyway.

Marking open again and assigning to Jon.

I can tell you that it's customary on sites like Reddit to add a "PDF warning" to a description of a link that points directly to a PDF document. (Other file types don't really come up often enough for such a custom to develop.)

Change 124618 had a related patch set uploaded by Jdlrobson:
Follow up to If985b16c4f682d737683597ed80951c6d6644c8f

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

Change 124618 merged by jenkins-bot:
Follow-up: If985b16c – Vector external link change release notes

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

Regardless of everyone's opinions the patch has been merged, so this is fixed.

I've looked at this again now that things have cooled down. I still do not like this solution, and I still like this alternative proposed a long time ago:

(In reply to Bartosz Dziewoński from comment #13)

(In reply to Jon from comment #11)

That aside use of a single class .link-https rather than a
combination of class .link-https, div#content a.external[href
^="https://"] would be preferable for a variety of reasons
readability, maintainability, reduced file size... are you really
telling me this is not a no-brainer?

Using the classes as you proposed does seem sane and mostly non-troublesome
(apart from the classic caching issues). I am perfectly fine with doing that
(as long as I'm not the one who will have to jerry-rig the Parser).

So I looked at the Parser, or in fact the Linker, and doing this is actually amusingly trivial. Pity no one did this the first time around.

A patch will follow momentarily.

Change 141973 had a related patch set uploaded by Bartosz Dziewoński:
Linker: Add CSS classes to external links to some protocols/filetypes

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

We'll have https (secure) and external link icons in wiki font soon (probably already) but wiki font isn't in core yet, these issues are related but could be separate too.

Hm, Parsoid does not tag links with classes. The only CSS would work fine for parsoid-generated HTML; the new CSS will not.

(In reply to C. Scott Ananian from comment #50)

Hm, Parsoid does not tag links with classes. The only CSS would work fine
for parsoid-generated HTML; the new CSS will not.

That is a bug in parsoid. We've required the .external class at minimum for years, parsoid should be able to handle link classes.

We have use cases where uncompressed HTML is sent over the wire (HTML save from VE), so uncompressed DOM size does matter.

Do we have actual measurements that verify that this benefits total page view performance in practice? If there is no measurable improvement on page view / rendering even on mobile, then I'd propose to keep this in CSS to avoid slowing down use cases like VE saves.

One more note on browser support: IIRC these selectors were already supported by IE 6 in 2004.

(In reply to Gabriel Wicke from comment #52)

Do we have actual measurements that verify that this benefits total page
view performance in practice?

We don't. Jon provided some funky tool in comment 11 but I've been unable to get it to work. Krinkle provided a theoretical explanation on why this should make a difference in comment 10.

If there is no measurable improvement on page
view / rendering even on mobile, then I'd propose to keep this in CSS to
avoid slowing down use cases like VE saves.

You are actually proposing restoring this to CSS :), because it's already been removed (in spite of working well since 2004). I wouldn't mind that, personally.

(In reply to Gabriel Wicke from comment #53)

One more note on browser support: IIRC these selectors were already
supported by IE 6 in 2004.

Not by IE 6, but by every other browser, yes. IE 7+ supports them too.

(In reply to Bartosz Dziewoński from comment #54)

(In reply to Gabriel Wicke from comment #52)

Do we have actual measurements that verify that this benefits total page
view performance in practice?

We don't. Jon provided some funky tool in comment 11 but I've been unable to
get it to work.

Same here, it wouldn't even load for me.

Krinkle provided a theoretical explanation on why this
should make a difference in comment 10.

Both of these aren't really telling us whether it's an overall win, draw or loss to add extra classes in the DOM & use those rather than match on existing properties. They both focus on one part of the picture only, and lack quantitative data. Only measurements can really answer this question.

(In reply to Gabriel Wicke from comment #53)

One more note on browser support: IIRC these selectors were already
supported by IE 6 in 2004.

Not by IE 6, but by every other browser, yes. IE 7+ supports them too.

Ah, okay. Misremembered that then ;)

(In reply to Gabriel Wicke from comment #52)

We have use cases where uncompressed HTML is sent over the wire (HTML save
from VE), so uncompressed DOM size does matter.

Why is it not compressed?

@daniel: long story, but basically gzip compression in HTTP only works in the download direction. There is no equivalent content negotiation for uploads in HTTP.

(In reply to C. Scott Ananian from comment #57)

@daniel: long story, but basically gzip compression in HTTP only works in
the download direction. There is no equivalent content negotiation for
uploads in HTTP.

JS implementations aren't universally a win on all browsers & hardware either. See https://bugzilla.wikimedia.org/show_bug.cgi?id=66914 for some related discussion.

Another reason is styles written in this way are not reusable.

What if we want to use the same audio icon elsewhere?

With an icon-audio selector class we can.
With a selector based on href we can't.

I'd like to see us moving away from descendant selectors and explore OOCSS [1] as a solution to our CSS mess we have created. We really should be making better uses of classes, and not be restricted by the implementation restrictions of Parsoid.

[1] http://www.smashingmagazine.com/2011/12/12/an-introduction-to-object-oriented-css-oocss/

(In reply to C. Scott Ananian from comment #57)

@daniel: long story, but basically gzip compression in HTTP only works in
the download direction. There is no equivalent content negotiation for
uploads in HTTP.

Oh you're talking about save sizes.

Are the byte sizes of a few link classes really a big deal for actions as infrequent as a save?

If they are, isn't there any serverside sanitization of the DOM? These are internally generated classes, so if it were an issue couldn't you just strip them before transmission and let the sanitizer or parsoid generate the classes?

Also I thought delta based saves were being done which make small byte size differences moot.

(In reply to Jon from comment #59)

Another reason is styles written in this way are not reusable.

I'll add one more reason.

What if we "don't" want an icon? This should be as simple as not including the class. Using selectors this means you have to override a skin specific css rule, with an unknown weight to it. The selectors aren't even standardized yet, Vector used to use #content, MonoBook uses #bodyContent, and commonElements of course uses .mw-body (though for non-icon styles).

(In reply to Jon from comment #59)

Another reason is styles written in this way are not reusable.

What if we want to use the same audio icon elsewhere?

With an icon-audio selector class we can.
With a selector based on href we can't.

This seems to overstate the case a bit. There's nothing stopping us from adding a selector in the CSS, and I don't see why that's more complex than adding the class in HTML.

I'd like to see us moving away from descendant selectors and explore OOCSS
[1] as a solution to our CSS mess we have created. We really should be
making better uses of classes, and not be restricted by the implementation
restrictions of Parsoid.

I don't have a strong opinion on what's done in skins. My interest is restricted to the content area. In general, we are currently moving away from any skin- and pref- specific HTML (including classes) in the page content. Our goal is to speed up page views for editors to the level of current anonymous views. This means that any variation in rendering between skins should be implemented in CSS only, and should not rely on different classes being set depending on skin or user preferences.

All these decision are engineering trade-offs, not implementation restrictions. If there are good engineering reasons (performance and CSS sanity for example) for generally adding more classes in the DOM & dumbing down the CSS selectors, then I'd be all for it.

So far our strongest performance data seems to be page sizes, as that's easy to measure. Quantitative data on the relative importance of selector performance on total page render time is missing so far. My anecdotal evidence is that it wasn't noticeable when I added external link icons in 2004, on a laptop that would be considered a feature phone these days.

(In reply to Daniel Friesen from comment #61)

What if we "don't" want an icon?

The .plainlinks class was normally used to mark this up, and applied to all links inside a content block. How does this work with classes on each link?

(In reply to Daniel Friesen from comment #60)

Are the byte sizes of a few link classes really a big deal for actions as
infrequent as a save?

Page size is a big deal for uncompressed uploads, as upload speeds are normally much slower than download speeds. Uploading a large article like [[San Francisco]] from a DSL connection in San Francisco takes many seconds (23 with current inline Parsoid metadata, around 7 without it), so a change of 5% in the page size does have a measurable impact on save times, even on a relatively fast connection. Things are worse than that when you are on a bad connection in India.

If they are, isn't there any serverside sanitization of the DOM? These are
internally generated classes, so if it were an issue couldn't you just strip
them before transmission and let the sanitizer or parsoid generate the
classes?

We are working towards using Parsoid HTML for both views and editing, so that you don't even loading a new page when switching to VE.

(In reply to Gabriel Wicke from comment #62)

I'd like to see us moving away from descendant selectors and explore OOCSS
[1] as a solution to our CSS mess we have created. We really should be
making better uses of classes, and not be restricted by the implementation
restrictions of Parsoid.

I don't have a strong opinion on what's done in skins. My interest is
restricted to the content area. In general, we are currently moving away
from any skin- and pref- specific HTML (including classes) in the page
content. Our goal is to speed up page views for editors to the level of
current anonymous views. This means that any variation in rendering between
skins should be implemented in CSS only, and should not rely on different
classes being set depending on skin or user preferences.

The suggested link classes are universal, they are not skin or user specific they are always in the dom dependent on what is in the dom.

(In reply to Daniel Friesen from comment #61)

What if we "don't" want an icon?

The .plainlinks class was normally used to mark this up, and applied to all
links inside a content block. How does this work with classes on each link?

.plainlinks will continue to work for content, in fact it'll work even better since classes have less specificity than attribute selectors.
But I'm not talking about content, but rather code. Adding links to elements or parts of the skin that happen to be under the root the link styles are defined on. With classes all an extension has to do is not use a link class. It can also add that link class to links that wouldn't normally match the selector (an extension could decide to link to a PDF reader with a document link style even though the url wouldn't have matched a .pdf$ selector.
And to top that off by using classes instead of attribute selectors we can drop the root (#content / #bodyContent / .mw-body) since interface links won't be ruined by them anymore, and code can opt-in to link styles outside of content.

(In reply to Daniel Friesen from comment #60)

Are the byte sizes of a few link classes really a big deal for actions as
infrequent as a save?

Page size is a big deal for uncompressed uploads, as upload speeds are
normally much slower than download speeds. Uploading a large article like
[[San Francisco]] from a DSL connection in San Francisco takes many seconds
(23 with current inline Parsoid metadata, around 7 without it), so a change
of 5% in the page size does have a measurable impact on save times, even on
a relatively fast connection. Things are worse than that when you are on a
bad connection in India.

Which brings me to my other point:
"Also I thought delta based saves were being done which make small byte size differences moot."

So in theory changing %5 of [[San Francisco]] should not involve uploading all of San Francisco. Unless VE is implemented really inefficiently, tiny additions like link classes shouldn't have as much effect on small edits. And if it is implemented that inefficiently, we have a larger problem than the bytes add to link classes.

If they are, isn't there any serverside sanitization of the DOM? These are
internally generated classes, so if it were an issue couldn't you just strip
them before transmission and let the sanitizer or parsoid generate the
classes?

We are working towards using Parsoid HTML for both views and editing, so
that you don't even loading a new page when switching to VE.

Not relevant, my suggestion is that if you want size reduction on upload, then you could strip the link classes right before upload (which are automatically generated based on a set of rules) since and the server-side sanitizer (which should exist anyways to prevent the rules of the parser being intentionally screwed with or broken by an old VE version) would add them back. The parsoid output and view content (which are compressed and delivered by the faster download bandwidth) would still have the classes.

Looking at the linked CSS [1], to me it seems that the most complex rules are those matching on file extensions to figure out video / audio / document links. As those links are not that common relative to the total number of links I'd expect classes to be a win overall for those.

Protocol matches on the other hand are a single selector only (no need for upper case variants), and a large chunk of external links is neither to videos, audio or document files. This seems to favor keeping those matches in CSS to keep the document size down.

Does this sound like a promising solution to you?

[1]: https://gerrit.wikimedia.org/r/#/c/85920/2/skins/vector/externalLinks.less

(In reply to Gabriel Wicke from comment #62)

The .plainlinks class was normally used to mark this up, and applied to all
links inside a content block. How does this work with classes on each link?

.plainlinks .link-https has higher specificity than .link-https, so it's easy to disable the icon for plain links, if desired.

(In reply to Gabriel Wicke from comment #64)

Looking at the linked CSS [1], to me it seems that the most complex rules
are those matching on file extensions to figure out video / audio / document
links. As those links are not that common relative to the total number of
links I'd expect classes to be a win overall for those.

Protocol matches on the other hand are a single selector only (no need for
upper case variants), and a large chunk of external links is neither to
videos, audio or document files. This seems to favor keeping those matches
in CSS to keep the document size down.

I don't really know the performance details, but that seems reasonable. PDF links are probably more common than audio/video, but still a minority.

(In reply to Gabriel Wicke from comment #64)

Looking at the linked CSS [1], to me it seems that the most complex rules
are those matching on file extensions to figure out video / audio / document
links. As those links are not that common relative to the total number of
links I'd expect classes to be a win overall for those.

Protocol matches on the other hand are a single selector only (no need for
upper case variants), and a large chunk of external links is neither to
videos, audio or document files. This seems to favor keeping those matches
in CSS to keep the document size down.

Does this sound like a promising solution to you?

Eh, I'd prefer if we decided on only one way of doing this and stuck to it. This seems to be the worst of both worlds.

As I said before, I'm okay with doing this either way (we can revert the old patch that removed this feature in Vector), but let's please not do both ways at once.

(In reply to Bartosz Dziewoński from comment #66)

As I said before, I'm okay with doing this either way (we can revert the old
patch that removed this feature in Vector), but let's please not do both
ways at once.

We are using attribute matches for a bunch of things in any case, so it's not like somebody working on MediaWiki CSS would now be exposed to new CSS 2 selectors. The main change would be the introduction of classes to eliminate overly complex attribute matches on file extensions, which I think is fairly easy to understand.

Oh, MediaWiki uses more advanced things for styling (like… LESS?), that was never the problem. The only problem was unspecified performance issues.

But I really don't feel like introducing two new ways of doing such a little feature is the right thing to do from a maintainability standpoint (both the maintainability of core, and maintainability of any skins that might want to use different icons).

One issue I didn't mention yet is editor complexity. Every editor for our HTML including VE will need to replicate the class logic from the parser. This includes future micro-contribution tools like 'fix this link'. It seems likely that this will lead to rendering inconsistencies when the logic replication is not perfect.

(In reply to Gabriel Wicke from comment #69)

One issue I didn't mention yet is editor complexity. Every editor for our
HTML including VE will need to replicate the class logic from the parser.
This includes future micro-contribution tools like 'fix this link'. It seems
likely that this will lead to rendering inconsistencies when the logic
replication is not perfect.

This is also a problem for VisualEditor. Even in the current system, not including any micro-contribution tools that Gabriel talks about, we would need three entirely separate copies of this logic, in:

  • MediaWiki's wikitext->HTML parser
  • Parsoid, to generate correct HTML
  • VisualEditor, to display newly created links correctly

VE generally deals well with styling that comes from the parser side and displays it correctly, but in this case the user could create a link entirely from scratch, and that link might be a .pdf link. Then even though there may not be another .pdf link anywhere on the page, we still need to know that .pdf is magical and should trigger a certain CSS class in order to produce the correct rendering. That's why we need a full duplicate of the icon logic.

So in general, we try to move this kind of logic fully *into* CSS, rather than *out of* CSS and into *code*. Last year this was done for auto-numbered external link numbering: instead of implementing a bunch of numbering logic both in Parsoid and in VE, we were able to number them entirely in CSS using CSS3 counters.

With that in mind, I would much prefer that link icons be done in pure CSS, with attribute selectors in their full "glory". That way VisualEditor and any other clients that need to generate new HTML will not have to reimplement the logic from scratch, and instead will just be able to load the CSS. Unless attribute selectors are horrendously slow (which they're probably not, the original CSS rules for link icons date back to 2004), moving to classes just makes things worse, not better.

Re performance, I have not been able to find anything on the web that indicates that CSS selector performance is a significant issue in practice.

This SO articles seems to provide a good overview:
http://stackoverflow.com/questions/12279544/which-css-selectors-or-rules-can-significantly-affect-front-end-layout-renderi

(In reply to Bartosz Dziewoński from comment #54)

(In reply to Gabriel Wicke from comment #52)

Do we have actual measurements that verify that this benefits total page
view performance in practice?

We don't. Jon provided some funky tool in comment 11 but I've been unable to
get it to work. Krinkle provided a theoretical explanation on why this
should make a difference in comment 10.

The performance impact is quite negligible actually. I'd rather let browsers worry about that. This shouldn't be a main driving factor in this case because it is too insignificant. My main point there was maintenance overhead:

(Citing comment #10 from Krinkle)

  • Hardcodes details that (other) stylesheets should not have to know or hardcode, thus causing maintenance issues (e.g. the fact that it is an <a>, and is inside #bodyContent. The container can change, and links might be wrapped for presentational reasons)

This would be mitigated by using a selector like:

.mw-body-content a[href^="https://"] {}

This doesn't have the added weight of an ID selector (#bodyContent) and is quite minimal and straight forward.

(In reply to Gabriel Wicke from comment #62)

(In reply to Daniel Friesen from comment #61)

What if we "don't" want an icon?

The .plainlinks class was normally used to mark this up, and applied to all
links inside a content block. How does this work with classes on each link?

That is indeed an important one to remember. Let's make sure we're compatible with that.

(In reply to Gabriel Wicke from comment #69)

One issue I didn't mention yet is editor complexity. Every editor for our
HTML including VE will need to replicate the class logic from the parser.
This includes future micro-contribution tools like 'fix this link'. It seems
likely that this will lead to rendering inconsistencies when the logic
replication is not perfect.

I agree. Adding all these classes to the Parser will inflate Parser output with information that can trivially be derived from the href attribute, doesn't really provide value to customers of the HTML (other than saving a few characters in stylesheet), and adds more or less random complexity to other producers of HTML such as VisualEditor content editable preview nodes and Parsoid. All the information about the url is right in the href. I think keeping classes like these out of page HTML would be optimal for maintenance overall:

  • Avoid imposing requirements on VE and Parsoid to add arbitrary attributes that the applied stylesheets should maintain instead. Separating responsibility and concerns; Unless it's a huge performance penalty we really shouldn't consider adding hooks like these inside the Parser for the skins' convenience.
  • Avoid optimising arbitrarily for the needs of one skin (the classes we add would only be for certain protocols and include certain file extensions in the media types. If another consumer doesn't agree with our grouping or wants more, they'd be back to square one in using href-matching; ergo, the approach doesn't scale and signifies that it'd wrongly divide responsibility between parser and stylesheet.

I'd recommend going with something like:

.mw-body-content .external[href^="https://"] {}

Phew .. long discussion. I could have gone either way (selectors or css classes) till I hit Gabriel and Roan's comments and now Krinkle's about client burden and code duplication to support classes/attributes. If there were no alternatives and this were the spec, of course, all clients would have to implement it, but given that there are no serious drawbacks to picking selectors over classes, I too think that it is a better idea to use css for icon-styling of links.

The icons for audio, video etc have been gone for some time without any complaints that I am aware of. Maybe because a lot of these styles are implemented in MediaWiki:Common.css

Let's not re-instate these classes and CSS rules. The status quo is currently fine. Let's not add back css rules for the sake of adding back css rules. I feel like we are wasting far too much energy on this bug.

(In reply to Jon from comment #75)

The icons for audio, video etc have been gone for some time without any
complaints that I am aware of. Maybe because a lot of these styles are
implemented in MediaWiki:Common.css

Yes, nobody noticing would make it more difficult for anyone to complain about it.

Let's not re-instate these classes and CSS rules. The status quo is
currently fine. Let's not add back css rules for the sake of adding back css
rules. I feel like we are wasting far too much energy on this bug.

MediaWiki serves more than just the English Wikipedia. While the icons often do serve little purpose on individual projects, many other projects do indeed make use of these icons, especially third-party technical projects which will not have seen that change at all due to generally being on stable releases (1.19, 1.22, 1.23 etc). This is not adding back css rules for the sake of adding them back, this is adding them back with a reasoned technical discussion about how best to do so considering the current technology, with the understanding that projects do use them, that there are real-life use cases speaking to their usefulness, and that there was no consensus to remove them in the first place.

Isarra. What was the motivation for putting them in in the first place?

Putting styles in both core and common.css that do the same thing is a terrible terrible terrible thing and we should at least resolve that problem at the same time. The fact those rules exist there show MediaWiki was not serving there purpose and they were /not/ useful.

Currently each project as complete free reign to add these icons in common.css if they want to. I truly don't see the value in them being here.

Isarra making use of icons is not the same as those icons giving value.

PS. How can I remove myself from this bug. Since I'm the creator, it doesn't seem possible. @Andre.. ?

(In reply to Jon from comment #77)

PS. How can I remove myself from this bug. Since I'm the creator, it doesn't
seem possible.

Sorry, only available in next version: https://bugzilla.mozilla.org/show_bug.cgi?id=148564 - For the time being, custom filters / thread muting.

(In reply to Jon from comment #77)

Isarra. What was the motivation for putting them in in the first place?

Given that this was orginally added in 2004, I couldn't say. I can, however, point you to comment #3 and comment #6, which detail specific reasons for keeping them in now.

Putting styles in both core and common.css that do the same thing is a
terrible terrible terrible thing and we should at least resolve that problem
at the same time. The fact those rules exist there show MediaWiki was not
serving there purpose and they were /not/ useful.

As I said, the English Wikipedia is not the only project that uses this software. What they do doesn't necessarily apply to other projects, especially when we don't even know why they did what they did.

Currently each project as complete free reign to add these icons in
common.css if they want to. I truly don't see the value in them being here.

Each project is free to write their entire skin css in common.css if they want to. Doesn't mean they should have to do that either.

Change 141973 abandoned by Bartosz Dziewoński:
Linker: Add CSS classes to external links to some protocols/filetypes

Reason:
Right.

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

So the general agreement is that it was good in the beginning and the change above was a bad idea. In other words, we should revert https://gerrit.wikimedia.org/r/#/c/123817/ in the Vector skin.

(Also, moving this to the Vector component, since the only changes that were implemented and the entire discussion involved only that skin.)

Change 154350 had a related patch set uploaded by Bartosz Dziewoński:
Restore external links icons

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

(In reply to Bartosz Dziewoński from comment #81)

So the general agreement is that it was good in the beginning and the change
above was a bad idea.

I don't know how you have determined this from the above. Please give me the precise quotes you are attributing to.

All I see is agreement that we shouldn't use classes and should use an approach such as [href^="https://"]

Whether they are a good idea is another question.
I wouldn't want them on my local wiki for example.

  1. As I said in comment #6 - the mailto, irc, audio/video, and pdf icons are frequently useful for:
  2. Readers to have "principle of least astonishment" when clicking on links which often have a non-typical-navigation result.
  3. Editors to see that an unusual link has been used/added, which might warrant further investigation.
  1. Our internal Pronunciation links all have an icon added automatically: e.g. https://en.wikipedia.org/wiki/Template:Pronunciation and https://en.wikipedia.org/wiki/Template:IPA - e.g. at [[Götterdämmerung]]

Then, 2 similar "bits of information - a difference that makes a difference" that any of our users might appreciate.

  1. we have a request from the Internet Archive to add icons to their links: #1 at https://www.mediawiki.org/wiki/Archived_Pages#Implementation_ideas (mentioned at https://lists.wikimedia.org/pipermail/design/2013-October/001065.html )
  1. we have a request from the community, (and I believe connected to The Wikipedia Library), to add icons that denote Open-Access resources: https://en.wikipedia.org/wiki/Wikipedia:WikiProject_Open_Access/Signalling_OA-ness

For all of these reasons, I hope we can re-instate these icons in Vector.
The patch to do so is currently -2'd based on lack of apparent consensus (https://gerrit.wikimedia.org/r/#/c/154350/).
I've also left a comment (the first...) at [[Help talk:External link icons]].
Comments here or there, with additional rationales for supporting or opposing the re-instatement (for use-case or technical or other reasons) would be appreciated.

PDF: To clarify this, the icon was always a generic document. On the English Wikipedia it uses an Adobe icon through Common.css.

The Typography Refresh removed the other icons in Vector. HTTPS icons were removed per Bug 61178.

So currently on the English Wikipedia we have the external link icon and the Adobe icon. Both are meaningless to our visually impaired readers since none of the icons have alt text; see Bug 45891.

Since many documents are now produced by dynamic web pages, they do not have the file extension that the CSS rules trigger on, thus those documents will show the simple external link icon.

Bottom line: How useful are the icons? Is there any other project using icons in this manner?

matmarex removed a project: Patch-For-Review.
matmarex set Security to None.

Meh.

Sigh.
I'm frustrated by the removal of icons that were useful.