Page MenuHomePhabricator

skins.vector.styles.legacy sets hr{height:0}, hiding horizontal rules
Closed, ResolvedPublic

Description

@CptViraj noticed today that horizontal rules (<hr>, ----) are not appearing on Commons. The ---- markup is still creating a <hr> tag, but it is invisible.

I also noticed that redlinks were looking a bit off, and suspected CSS trickery. I noticed that the height for hr tags was set to 0. Through process of elimination, I was able to determine that the skins.vector.styles.legacy module was causing the offending CSS:
https://commons.wikimedia.org/w/load.php?lang=en&modules=skins.vector.styles.legacy&only=styles&skin=vector

hr{box-sizing:content-box;height:0;overflow:visible}

This occurred today on Commons, but isn't broken yet on enwiki, so it's likely that the patch that caused it is from 1.36.0. I wasn't able to determine exactly what commit or even what file is causing this CSS rule.

Event Timeline

hashar added subscribers: Jdlrobson, Volker_E, Mainframe98, hashar.

The HR height being set to 0 seems to come from mediawiki/core:

resources/src/mediawiki.skinning/normalize.less
hr {
    // Support Firefox: Add the correct box sizing.
    box-sizing: content-box;
    height: 0;
    // Support Edge and IE: Show the overflow.
    overflow: visible; 
}

Made by 6923d35ab31d575d439c03177270f49d695e5aaf which has been merged back in July. So potentially the hr bars have disappeared since then? Or something has changed in Vector which trigger that rule. In Firefox the element shows:

hr {
    box-sizing: content-box;
    height: 0;
    overflow: visible;
}
hr {
    height: 1px;  // Striked and overriden by above
    background-color: #a2a9b1;
    border: 0;
    margin: 0.2em 0;
}

The later block comes from:

src/mediawiki.skinning/elements.css
hr {
    height: 1px;
    background-color: #a2a9b1;
    border: 0;
    margin: 0.2em 0;
}

Maybe it is a load order issue? With normalize.less being applied after elements.css and thus overriding the height.

The patch above adds normalize as an option of ResourceLoaderSkinModule and has been applied in Vector with 9efd8a9268831e3d09a49fab11a8ef8d299f2362:

--- a/skin.json
+++ b/skin.json
@@ -52,7 +52,7 @@
                },
                "skins.vector.styles": {
                        "class": "ResourceLoaderSkinModule",
-                       "features": [ "elements", "content", "interface", "legacy" ],
+                       "features": [ "normalize", "elements", "content", "interface", "legacy" ],

Which comes from July as well.

That leads me to ResourceLoaderSkinModule recent modifications:

35d03b1168cbe032a2c690e18d9b93f9b870f24a (deployed this week) and 6ab14857b972eea9a0fb94f8d50e5d098b00be51 (not deployed).

So the regression is probably in the load order of those various resources.

That leads me to ResourceLoaderSkinModule recent modifications:
35d03b1168cbe032a2c690e18d9b93f9b870f24a (deployed this week) and 6ab14857b972eea9a0fb94f8d50e5d098b00be51 (not deployed).
So the regression is probably in the load order of those various resources.

@hashar That's really thorough research. Calling array_unshift (prepend in English) one-by-one on the features list reverses the order of those as "normalize" is prepended, then "elements", etc.
What it should do is to accumulate the ResourceLoaderFilePaths in a new array then array_unshift that new array to $styles[$mediaType].

@hashar: Should this be UBN with it blocking the train?

Jdforrester-WMF triaged this task as Unbreak Now! priority.Sep 10 2020, 8:48 AM
Jdforrester-WMF subscribed.

Train blocker -> UBN.

Change 626340 had a related patch set uploaded (by Mainframe98; owner: Mainframe98):
[mediawiki/core@master] resourceloader: Fix incorrect order of feature stylesheets

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

@Danny_B reported on irc:

[13:58:02] <Danny_B> red links are shown in #d33 (lighter, more "marker" color) than original (actually again overriden #ba0000)

Which I guess is related ;)

Just curious: if something was broken in July is it really UBN?
Personally I'd like some time with this patch rather than rush out a fix today.

[Update 1] Apologies: just saw T262507#6449694 - in which case maybe reverting the new change might be the appropriate fix for now.
[Update 2] Ok looks like @Krinkle is on this and has capacity so ignore me :)

Change 626264 had a related patch set uploaded (by Krinkle; owner: Mainframe98):
[mediawiki/core@wmf/1.36.0-wmf.8] resourceloader: Fix incorrect order of feature stylesheets

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

@Jdlrobson I think the prepending logic from last week month, and the new defaulting logic we landed this week, together probably justify a few integration tests for this class. Could you add that to the backlog?

Change 626340 merged by jenkins-bot:
[mediawiki/core@master] resourceloader: Fix incorrect order of feature stylesheets

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

Change 626264 merged by jenkins-bot:
[mediawiki/core@wmf/1.36.0-wmf.8] resourceloader: Fix incorrect order of feature stylesheets

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

Krinkle claimed this task.

Thanks all for taking care of this one!

@Jdlrobson I think the prepending logic from last week month, and the new defaulting logic we landed this week, together probably justify a few integration tests for this class. Could you add that to the backlog?

Added to T252774 which I'm currently working on.