Page MenuHomePhabricator

OutputPage::addModuleStyles should support module dependencies
Closed, DeclinedPublic

Description

(There was no bug yet for "addModuleStyles should support dependencies" - which I've commonly said is WONTFIX, so I'll put it here so I can refer to it)

Just semantics, but there is no case of ResourceLoader not being able to handle skin css dependencies. It is impossible by design due to restrictions outside ResourceLoader.

Some of these restrictions are:

  1. Skin css can't require javascript, which means we can't just list the top level modules and handle dependencies client-side, we'd have to flatten and hardcode dependencies in the page html <link> tag
  2. page html must not be required to change for things not to break, cached page html must continue to work. This means:

    2a) version can't be in page html, 2b) This can't work because if a dependency is added or things moved into a lower-level module, then all existing page cache will be broken as it would now have a <link> that soon generates an incomplete stylesheet where a dependency is missing.

Possible solutions involve ESI, which is fine for enhancement (we can make the <link> tag contain a version number that way for better caching), but we don't want to depend on it for proper functionality, which means we can't have it replace dependencies.

Alternatively (current practice[1]) you can continue the convention that one *should not* use addModuleStyles for anything other than the core basic css (which must not have dependencies or concerns about module order).

If you want dependencies, you'll have to join the regular load stack, which won't run without javascript.


See:

  • bug 45229
  • Ia5eadcfcd4b98685
  • ...

[1] https://www.mediawiki.org/wiki/ResourceLoader/Developing_with_ResourceLoader#CSS_2


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

Details

Reference
bz61577

Event Timeline

bzimport raised the priority of this task from to Needs Triage.Nov 22 2014, 3:07 AM
bzimport set Reference to bz61577.
bzimport added a subscriber: Unknown Object (MLST).

I've got two problems with these assertions.

Firstly, on (2b) the skin stack is already under this situation. If we change basically anything like css selectors, module names, or add a new module to addModuleStyles we'll have to leave things duplicated on WMF for 30 days for the caches to expire. So supporting dependencies and replacing addModuleStyles with a key to tell addModules that a hardcoded <link> should be used instead just leaves us in the same situation, it doesn't make anything worse.

Secondly, on (2a) our current assertion seems to be that we always want to serve up to date CSS. But given all the trouble we have with changing selectors, renaming modules, etc... it seems that that perception is incorrect, and the reality is that we DON'T want up to date css, if a cached page with old HTML is being served then we actually want to serve it old CSS.

That would mean we DO want &version= in the HTML and we want a ResourceLoader snapshot system that will allow WMF to snapshot the current state of RL modules before a scap and have that served to cached pages with old HTML after the new code is pushed out.

That would mean server side dependencies wouldn't be a problem, nor would we need selector, css, or module duplication when we change things.

It seems to be that even a very rudimentary broken-by-design dependencies support in addModuleStyles() would be a better fix for bug 45229 than preserving the call order (or no fix at all).

Situations like you describe in point 2) can be easily handled by duplicating the module under a new name until cached page HTML expires. This of course sucks a lot, but *we already do this all the time* to make incompatible changes in HTML.

Skins need to be able to specify module dependencies. Otherwise the only way to make more specific skin styles (such as for a specific page) load after the main styles is to name them such that they come later in alphabetical order. This is unintuitive and stupid, as it results in poorly named modules and an unclear hierarchy, and also comes with no assurance that the load order will not break down the road.

You can see this, for instance, in the bluesky skin, which has special mainpage formatting; the theme extension, which applies theme css on top of the skin css; and also, from what I understand, wikihow's custom styling of the mobilefrontend extension. These are all stacks of cards just waiting to topple.

(In reply to Isarra from comment #3)

This is unintuitive and stupid, as it results in poorly named modules
and an unclear hierarchy, and also comes with no assurance that the load
order will not break down the road.

Forwards-compatibility -- or documentation for that matter -- has never been MediaWiki's strongest suit. You can live with it when you have only one site to fix after upgrading, but when you have tens or hundreds of sites to fix, it gets kinda old really fast.

You can see this, for instance, in the
bluesky skin, which has special mainpage formatting; the theme extension,
which applies theme css on top of the skin css;

Theme used previously (MW 1.22 and older versions) the naming schema skins.skinname.themename, i.e. skins.monobook.dark for a Dark MonoBook theme. This worked fine, because in 1.22 MonoBook's main.css etc. was loaded by a module called "skins.monobook". Come 1.23, the skins.monobook module was renamed skins.monobook.styles and because the letter d (as in the word "dark") sorts alphabetically before the letter s (as in "styles"), the skin defaults ended up overriding the theme-defined rules, and the end result was a horribly butchered MonoBook on the sites, unhappy users and unhappy developers.

Nobody saw this coming, given that the Theme extension worked consistently from MediaWiki 1.16 to 1.22.
This particular issue was "fixed" by prefixing Theme's modules with themeloader., since t sorts alphabetically after s, but if history's anything to go by, it'll only be so long until it breaks again.

wikihow's custom styling of the mobilefrontend extension

wikiHow essentially had the same issue with their MobileFrontend customizations (/extensions/wikihow/MobileFrontendWikihow) as ShoutWiki had with the Theme extension, and the solution was, obviously, similar: certain modules were prefixed with zzz. to ensure the correct loading order. Hardly the ideal solution, but for the time being, it works. It's not intuitive, obvious or otherwise ideal in any case.

These are all stacks of cards just waiting to topple.

"to topple *again*", you mean.

(In reply to Isarra from comment #3)

Skins need to be able to specify module dependencies. Otherwise the only way
to make more specific skin styles (such as for a specific page) load after
the main styles is to name them such that they come later in alphabetical
order.

This only addresses one your scenarios. However, for this issue (targeting a specific page), you can use CSS specificity, in which case the order won't matter (order only applies if they have the same specificity).

First, you need a class. MediaWiki provides these based on page titles (e.g. page-Main_Page). That probably doesn't work for this use case, though. Instead, you can use Skin->addToBodyAttributes and $out->getTitle()->isMainPage() to set a class for the main page (or maybe MW core should do this; MW.org already uses something similar at https://www.mediawiki.org/wiki/MediaWiki:Gadget-site.js).

Then, you can do something like:

.mw-bluesky-mainpage {

// Custom main page styling

}

in LESS.