Last modified: 2014-09-28 12:26:23 UTC
ResourceLoader sorts the modules for stylesheets added with addModuleStyles alphabetically ignoring dependencies. In other words if you create the module 'a' and make it depend on the module 'b'. And then do `$out->addModuleStyles( 'b' ); $out->addModuleStyles( 'a' );`. ResourceLoader will load 'a' first and then load 'b', even though both the dependencies and addModuleStyles say that's wrong. Skins use addModuleStyles to add skin styles to the page. The setupSkinUserCss was crafted previously so that addStyle could be used in the correct order by overriding the function and using parent::setupSkinUserCss at the right spots. And this works for subskins too since they can let the parent skin add it's styles before adding their own styles. However the alphabetical sorting done by RL goes and undoes any precise ordering required by that. This is problematic for the creation of subskins. If you create a 'skin.foo' that depends on 'skin.vector' and setup setupSkinUserCss properly RL will go and put skin.vector after your The only reason every skin isn't already broken by the shared styles is that 'mediawiki.legacy.' alphabetically sorts before 'skin.'. If we fixed that and started using 'skins.common.' some skins could start to break. Also note that most skins use mediawiki.legacy.shared and mediawiki.legacy.commonPrint which are not added by any core or third party skin to the dependency setup of the skin's module. So even if we fix this to not sort things before the things they depend on it would be best to make sure we also fix it to take addModuleStyles into account. The ideal situation of course is to fix addModules to know that some modules should not be loaded by JS (another key on the module registration) and just use addModules inside of initPage, deprecating setupSkinUserCss for anything besides addStyle usage (eg: IE stylesheets) and old skin compatibility. But we have skins that use addModuleStyles right now so we'll need to fix this bug at minimum for compatibility.
There may be a way to address this, but in general you shouldn't use addModuleStyles and other mw.loader-circumventing methods in OutputPage. They don't resolve dependencies, so why should they do use dependency information for the order? The easiest improvement may be to keep them in order of addition (simply not alphabetise them).
(moving out of ResourceLoader component as this is not (yet) related to the framework itself, but how MediaWiki's OutputPage (ab)uses it by calling it's entry point directly).
(In reply to comment #1) > There may be a way to address this, but in general you shouldn't use > addModuleStyles and other mw.loader-circumventing methods in OutputPage. addModules loads stylesheets via JavaScript. This is absolutely unacceptable for the css loaded by skins. Hence addModuleStyles has been used from the start. And this is a problem for the entire interface, not the installer.
(In reply to comment #3) > (In reply to comment #1) > > There may be a way to address this, but in general you shouldn't use > > addModuleStyles and other mw.loader-circumventing methods in OutputPage. > > addModules loads stylesheets via JavaScript. This is absolutely unacceptable > for the css loaded by skins. Hence addModuleStyles has been used from the > start. > > And this is a problem for the entire interface, not the installer. I agree that's unacceptable for the skin. However if there's no dynamic client scripting involved, it means no dependencies can be resolved since we don't do that server side (not yet anyway). So in the current system, addModuleStyles doesn't (nor claims to) support dependencies. It adds that one module as a raw stylesheet to the page. As a short term fix it may help (not saying it is pretty, but it may improve the situation) to at least preserve the order in which the addModuleStyles() calls occurred, as generally these sort of things match execution order. e.g. { parent::myMethod(); // adds module $out->addModuleStyles( .. ); }
(see bug 61577)
Change 133843 had a related patch set uploaded by Daniel Friesen: Do not sort modules alphabetically https://gerrit.wikimedia.org/r/133843
Note that one reason to do sort them alphabetically is to share cache between pages. This is why mw.loader.load calls outputted by the server as well as the actual http requests made my mw.loader always sort alphabetically, since implementation ensures proper execution order. When code adds things using addModuleStyles that aren't order-sensitive per se, this keeps them consistently in the same order regardless of whether two pieces of code execute in a slightly different order (e.g. as result of refactoring in the caller, or, changing the order in which hooks are registered when you reorder things in LocalSettings.php, etc.). That can all lead to inconsistencies (e.g. if a module is loaded on a Special page via hook, and in the parser on regular pages, if those happen in different orders then they wouldn't consistently load after or before the skin). So I can imagine it may be interesting to investigate the possibility of keeping the forced sort with the option to perhaps hoist important ones in a fixed order. Anyway, that opens another can of worms and an arms race of sorts, so let's not for now :)
(In reply to Krinkle from comment #7) > So I can imagine it may be interesting to investigate the possibility of > keeping the forced sort with the option to perhaps hoist important ones in a > fixed order. ;) Like a dependency system to declare which ones need to be loaded first?
It seems to be that even a very rudimentary broken-by-design dependencies support in addModuleStyles() (bug 61577) would be a better fix for this than preserving the call order (or no fix at all). That said, I'd support just preserving the order too. (In reply to Daniel Friesen from comment #8) > (In reply to Krinkle from comment #7) > > So I can imagine it may be interesting to investigate the possibility of > > keeping the forced sort with the option to perhaps hoist important ones in a > > fixed order. > > ;) Like a dependency system to declare which ones need to be loaded first? Yeah, we really shouldn't create a separate system for dependencies. Sure, we could add a key like 'stylesAfter' to ResourceLoaderFileModule that would effectively implement half-asses dependency support, but why do that if we already implemented dependencies once?
*** Bug 69159 has been marked as a duplicate of this bug. ***
