Page MenuHomePhabricator

[carryover] ResourceLoaderSkinModule should load core styles before skin styles
Closed, ResolvedPublic0 Estimated Story PointsBUG REPORT

Description

Currently it's not possible for Vector to override certain rules in MediaWiki core defined by ResourceLoader that are scoped to @screen. Without fixing this Vector is forced to write overly-specific CSS rules or use !important which can have consequences on other extension styles that we'd like to avoid. This is expanded upon later in the ticket in the Example section.

Given the following "skin feature" definitions in SkinModule.php along with the skin.json config in Vector 2022

core/includes/ResourceLoader/SkinModule.php
// NOTE: The order of the keys defines the order in which the styles are output.
private const FEATURE_FILES = [
	'normalize' => [
		'all' => [ 'resources/src/mediawiki.skinning/normalize.less' ],
	],
	'elements' => [
		'screen' => [ 'resources/src/mediawiki.skinning/elements.less' ],
		'print' => [ 'resources/src/mediawiki.skinning/elements-print.less' ],
	],
	'content-links' => [
		'screen' => [ 'resources/src/mediawiki.skinning/content.links.less' ]
	],
];
Vector/skin.json
"skins.vector.styles": {
  class: "ResourceLoaderSkinModule",
  features: {
    "normalize": true,
    "elements": true,
    "content-links": true
  },
  styles: [
    "resources/skins.vector.styles/skin.less"
  ]
}

CSS is output in the following order

Current CSS output

- normalize /* with "all" media type, no @media block */
- resources/skins.vector.styles/skin.less /* 👈 no specified media block, treated as "all" */

@media screen {
	- elements /*(screen specific)*/
	- content-links
}

@media print {
	- elements /*(print specific)*/
}

Styles are grouped by media-query type (all, screen, print), with FEATURE_FILES maintaining the order defined in SkinModule.php and skin related styles coming last in the "all" block if they don't define a media query type.

This output order makes it hard for skins to override core styles because styles in the @screen block can override them with source order. This forces the skin to increase specificity instead of leveraging the cascade. Preferably, if a skin doesn't specified a media query type, it should be placed at the end of the skin output.

Preferred CSS output

- normalize /* with "all" media type, no @media block */

@media screen {
	- elements /*(screen specific)*/
	- content-links
}

@media print {
	- elements /*(print specific)*/
}

- resources/skins.vector.styles/skin.less /* 👈 Placed at end of CSS output, after core styles */

Example

  • Create a ResourceLoaderSkiNModule using the content-links and normalize ResourceLoader features.
  • Define a rule like so in a "styles": [ "foo.css" ]

where foo.css is:

a.new {
        color: yellow;
}

Expected: all links will be yellow. The normalize CSS should appear first.
Actual: Links remain @color-red - this is because the foo.css rule is defined to apply without @screen and @screen rules are added after skin rules.

QA

  • no pixel regressions
  • remove the !important in typography.less and ensure no regressions (search for "// FIXME: Remove !important with T354975" comment)

Verifying in production

QA Results - Prod

ACStatusDetails
1T354975#9614127
2T354975#9614127

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Meeting update:
@Jdrewniak will meet @Jdlrobson to get the ordering of dependencies correctly for media queries

Change 1003134 had a related patch set uploaded (by Jdrewniak; author: Jdrewniak):

[mediawiki/core@master] SkinModule - Ensure skins can easily override skin feature styles

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

Change 1003134 merged by jenkins-bot:

[mediawiki/core@master] SkinModule - Ensure skins can easily override skin feature styles

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

After r1003134, I get the follow on my localhost using MediaWiki-Docker:

The resource from “http://localhost:8080/w/load.php?lang=en&modules=startup&only=scripts&raw=1&skin=vector-2022” was blocked due to MIME type (“text/html”) mismatch (X-Content-Type-Options: nosniff).

I've tried rebuilding Docker and have had no luck. Core and Vector are up-to-date. Any ideas?


Resolved now courtesy of @Jdlrobson. Thanks!

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

[mediawiki/core@master] Fixes issue with unusual mediaTypes

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

Change 1003868 merged by jenkins-bot:

[mediawiki/core@master] Fixes issue with unusual mediaTypes

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

Edtadros added subscribers: bwang, Edtadros.

Test Result - Beta

Status: ✅ PASS
Environment: beta
OS: macOS Sonoma
Browser: Chrome
Device: MBA
Emulated Device:NA

Test Artifact(s):

QA Steps

✅ AC1: no pixel regressions
The regressions are only Legacy Vector

screenshot 530.png (1×791 px, 189 KB)

❓ AC2: remove the !important in typography.less and ensure no regressions (search for " FIXME: Remove !important with T354975" comment)
@bwang , I'm not sure how to validate this (it seems like a dev validation). I thought I'd get some clarity by looking at the linked task, but that is this task.//

Change 1004712 had a related patch set uploaded (by Esanders; author: Esanders):

[mediawiki/core@master] Revert "SkinModule - Ensure skins can easily override skin feature styles"

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

Change 1004712 merged by jenkins-bot:

[mediawiki/core@master] Revert "SkinModule - Ensure skins can easily override skin feature styles"

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

These changes caused incorrect rendering of headings on talk pages with DiscussionTools (T357929), and on all pages when using the Parsoid parser (T357831).

Consider adding these scenarios to the Pixel regression test suite.

I proposed a patch for Vector that will resolve these issues if the reverted patch is reapplied: https://gerrit.wikimedia.org/r/c/mediawiki/skins/Vector/+/1004259

Change 1005070 had a related patch set uploaded (by Jdrewniak; author: Jdrewniak):

[mediawiki/core@master] Ensure skins can easily override skin feature styles (Take 2)

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

ovasileva lowered the priority of this task from High to Medium.Feb 20 2024, 4:47 PM

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

[mediawiki/skins/Vector@master] Remove redundant site notice styles

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

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

[mediawiki/skins/Vector@master] Correct how font sizes apply to headings

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

Change 1005126 merged by jenkins-bot:

[mediawiki/skins/Vector@master] Remove redundant site notice styles

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

Change 1005144 merged by jenkins-bot:

[mediawiki/skins/Vector@master] Correct how font sizes apply to headings

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

Jdlrobson renamed this task from ResourceLoaderSkinModule should load core styles before skin styles to [carryover] ResourceLoaderSkinModule should load core styles before skin styles.Feb 23 2024, 7:59 PM
Jdlrobson changed the point value for this task from 2 to 0.Feb 26 2024, 7:06 PM

As discussed in standup https://gerrit.wikimedia.org/r/c/mediawiki/core/+/1005070 is ready to go, we are just waiting on input from @Krinkle

Recap of issue at hand

As I understand it, the work that led to this task started with a design tweak to list items in Vector 2022 (per T352875). Vector change 989911 (Jan 16) attempted to change list item margins. The same commit also weakens several other selectors rules in Vector, and adds the style .mw-body p { padding-bottom: 0.5em;.

This change led to two regressions:

  • T355805 in VisualEditor's new wikitext editor mode (via CodeMirror extension). I can reproduce this when I check out the above Vector change (with VE workaround change 992760 (Feb 1) locally git-revert'ed), the textarea contents indeed looks blurry due the line-height of the textarea no longer corresponding to the paragraph. The latter now has non-zero padding between. From what I can tell, this isn't caused by selector strength or load order. There exists no rules in core that set or unset padding in .mw-body p. The Vector stylesheet is the only one setting this, and would "win" (and cause that bug) no matter which order styles are applied in.
    • This was appropriately fixed within VE by ignoring padding on paragraphs within the editor, so that it always matches its textarea. This avoids future coupling to the skin.
  • T355698: MediaWiki core redirect pages glitch. Core styles for .redirectMsg render invisible text, but this started glitching in Vector 2022 due to the extra padding not being accounted for.
    • This was worked around in MediaWiki core change 997958 (Feb 7) by levering a *stronger selector*. These styles exist in a dedicated module and are, also, unaffected by the order of media-blocks in SkinModule.

The Vector change was reverted Vector change 992774 (Jan 24), and after the above regressions were fixed in future-proof way, it was re-applied in Vector change 993505 (Feb 7) for the original purpose of improving the list item design (per T352875). This second attempt applies !important. This is similar to a stronger selector, but has risks due to often being too strong in practice. Understandably, this task was then filed to investigate how to solve this without !important, hinting at a root cause in ResourceLoader.

MediaWiki core change 1003134 (Feb 14) was proposed and merged to re-order the media groups in ResourceLoader. This in turn caused a third regression:

  • T357929: Headings too large on talk pages due to applying the em-based font-size increase for h2, twice, in two different ways. This appears to expose an issue whereby the order of SkinModule is intended to offer skins a feature, but the changed order is breaking this. The latest draft above has not resolved this, and it appears resolving it is mutually-exclusive with the intent of the patch. Back in November 2023 with change 975362, Scott and Bartosz already anticipated this issue and ensured that core provides as part of the elements option in SkinModule, a rule that ensures the inner element inherits the outer elements automatically. The proposed re-order broke this.

The MediaWiki core change was reverted meanwhile in change 1004712 (Feb 19).

With the original list-item design completed (T352875), I understand there is no longer an immediate problem or urgency.

Alternative 1:

  • Keep 1 extra "stronger selector" in Vector for list-item design.

The option is essentially what has ended up in Vector already, albeit using the very strong !important instead. Perhaps the team prefers that, fine from a platform perspective either way.

Current proposal:

If I understand correctly, if the ResourceLoader patch were merged again, the fixes and workarounds listed at T354975#9579070 would have to remain in-place to mitigate the would-be breakage. As such, the proposal is really a package deal:

  • Merge breaking change in ResourceLoader.
  • And – Keep 2-3 extra "stronger selectors" as fixes in various repos to unbreak things.
  • Give up on allowing core to provide elements (change 975362) to skins like Vector, with a workaround for each future rule.

Changing the order of the existing groups as we have them, not only might break things down the line, we already know that it does per T354975#9579070. There exists no group order that doesn't break something. We save one line of code in 1 place, and exchange it for 3+ lines somewhere else. And next time we run into a similar issue, do we re-order again? Doing so would save one line then, and instead introduce the lines we saved this time. If this is acceptable, we might as well go with Alternative 1 and saves ourselves the effort, keeping the current order that we at least know is compatible with the ecosystem today.

Recall also that SkinModule has previously addresses a different ordering issue. The "features" option from a skin.json, is reliably iterated in the order declared by the FEATURE_FILES constant.

Alternative 2:

Zooming out, I very much agree that the way SkinModule injects custom skins in-between core feature files is confusing. I would support fixing that. When I first saw Jan's patch in my Gerrit Notifications folder, I actually thought it might do this as describing it sounds the same, and the implementation perhaps simpler than what was submitted.

  • Change SkinModule to output core's style features first, and append the skin's custom style after that.

This will be easy to reason about and explain to a skin developer. No caveats to the future.
It also maintains backward-compatibility by preserving the current core order as declared, as well as the order of any media queries declared in skin.json, consistent with other File modules in core and extensions. Basically, you shift the customisations as a whole, without internal re-ordering or splitting, to the end of the module output.

The Module::getStyles interface, and ResourceLoader::makeCombinedStyles utility method can help facilitate this.

@Krinkle thanks for the detailed response, it looks like we agree on the desired outcome

Change SkinModule to output core's style features first, and append the skin's custom style after that.

Looking back on the patch I submitted, I can see how it doesn't really achieve that goal. As you commented in the patch, it changes the output order like so:

Before:
all (core+custom)
print (core+custom)
screen (core+custom)
other custom

After:
all (core)
screen (core+custom)
print (core+custom)
all (custom)
other custom

Whereas the desired output should look more like this:

Preferred:
all (core)
print (core)
screen (core)
other (core)
all (custom)
print (custom)
screen (custom)
other (custom)

I'll take another stab at this with the suggested approach of using Module::getStyles and ResourceLoader::makeCombinedStyles instead.

Change 1007343 had a related patch set uploaded (by Jdrewniak; author: Jdrewniak):

[mediawiki/core@master] [WIP] - Output skin related styles after SkinModule feature styles

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

Jdlrobson updated Other Assignee, added: Krinkle.
Jdlrobson added a subscriber: Krinkle.

Change 1005070 abandoned by Jdrewniak:

[mediawiki/core@master] Ensure skins can easily override skin feature styles (Take 2)

Reason:

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

Jdlrobson updated Other Assignee, added: Jdrewniak; removed: Krinkle.

LGTM from our side. @Krinkle did say LGTM on https://gerrit.wikimedia.org/r/c/mediawiki/core/+/1007343/comment/28ba7db3_bfa2c578/ but I don't want to make any assumptions that he's okay with merging it so I'll wait for him to +1 or +2 before merging.

Test Result - Prod

Status: ✅ PASS / ❓Need More Info
Environment: enwiki
OS: macOS Sonoma
Browser: Chrome
Device: MBA
Emulated Device:NA

Test Artifact(s):

QA Steps

Verifying in production
Use https://en.wikipedia.org/wiki/Wikipedia:Requests_for_adminship/2024_review/Phase_I
❓ AC1: Confirm h2 font size is 24px.
@Jdlrobson, please take a look at the font size below. I'm not sure if this is a valid way to verify this AC or if this is a regression

screenshot 617.png (1×1 px, 565 KB)

✅ AC2: Double check: Inspect the h2 it should say font: inherit;
screenshot 616.png (1×1 px, 564 KB)

Change 1007343 merged by jenkins-bot:

[mediawiki/core@master] Output skin related styles after SkinModule feature styles

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

Test Result - Prod

Status: ✅ PASS
Environment: enwiki
OS: macOS Sonoma
Browser: Chrome
Device: MBA
Emulated Device:NA

Test Artifact(s):

QA Steps

Verifying in production
Use https://en.wikipedia.org/wiki/Wikipedia:Requests_for_adminship/2024_review/Phase_I
✅ AC1: Confirm h2 font size is 24px in standard mode, 21px in small mode.

smallstandard
screenshot 676.png (1×1 px, 597 KB)
screenshot 677.png (1×1 px, 576 KB)

✅ AC2: Double check: Inspect the h2 it should say font: inherit;
see above

This LGTM and resolves the original issue that was causing us issues (follow up patch in https://gerrit.wikimedia.org/r/c/mediawiki/skins/Vector/+/1009814)

Thanks all for the help here and for the simplified solution we ended up with.