Page MenuHomePhabricator

[carryover] ResourceLoaderSkinModule should load core styles before skin styles
Open, MediumPublic2 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

Event Timeline

@Mooeypoo this is one of those code areas that overlaps web and platform team. Does the platform team have any opinions/concerns about changing this behaviour?

Jdlrobson triaged this task as Medium priority.Jan 18 2024, 6:11 PM
Jdlrobson raised the priority of this task from Medium to High.

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.Tue, Feb 20, 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.Fri, Feb 23, 7:59 PM