Last modified: 2014-08-19 03:07:29 UTC

Wikimedia Bugzilla is closed!

Wikimedia migrated from Bugzilla to Phabricator. Bug reports are handled in Wikimedia Phabricator.
This static website is read-only and for historical purposes. It is not possible to log in and except for displaying bug reports and their history, links might be broken. See T63577, the corresponding Phabricator task for complete and up-to-date bug report information.
Bug 61577 - OutputPage::addModuleStyles should support module dependencies
OutputPage::addModuleStyles should support module dependencies
Status: RESOLVED WONTFIX
Product: MediaWiki
Classification: Unclassified
ResourceLoader (Other open bugs)
unspecified
All All
: Unprioritized normal (vote)
: ---
Assigned To: Nobody - You can work on this!
:
Depends on:
Blocks: 66508
  Show dependency treegraph
 
Reported: 2014-02-20 04:41 UTC by Krinkle
Modified: 2014-08-19 03:07 UTC (History)
8 users (show)

See Also:
Web browser: ---
Mobile Platform: ---
Assignee Huggle Beta Tester: ---


Attachments

Description Krinkle 2014-02-20 04:41:03 UTC
(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
Comment 1 Daniel Friesen 2014-05-16 21:22:00 UTC
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.
Comment 2 Bartosz Dziewoński 2014-06-12 16:03:35 UTC
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.
Comment 3 Isarra 2014-08-16 17:13:29 UTC
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.
Comment 4 Jack Phoenix 2014-08-16 17:52:03 UTC
(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.
Comment 5 Matthew Flaschen 2014-08-19 03:07:29 UTC
(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.

Note You need to log in before you can comment on or make changes to this bug.


Navigation
Links