Page MenuHomePhabricator

Implement ParserOutput::addModuleStyles
Closed, ResolvedPublic

Description

Currently to load a CSS file for an extension there are three options:

  • use OutputPage::addModuleStyles - does not cache, so has to be included on each page, if needed or not
  • use ParserOutput::addModules - does not work on JS-disabled browsers
  • use ParserOutput::addHeadItem - does not use ResourceLoader (i.e. no minifying, no batching), requires the full html text of the head item instead of just the module name

To improve this it should be possible to add style-only modules to the ParserOutput using a method ParserOutput::addModuleStyles.
The list of style-only modules should be cached.
The modules should be added to the output in OutputPage::addParserOutputNoText using OutputPage::addModuleStyles (not OutputPage::addModules).


Version: 1.20.x
Severity: enhancement

Details

Reference
bz29308

Event Timeline

bzimport raised the priority of this task from to Medium.Nov 21 2014, 11:28 PM
bzimport set Reference to bz29308.
bzimport added a subscriber: Unknown Object (MLST).
Foxtrott created this task.Jun 7 2011, 9:46 PM
brion added a comment.Jun 8 2011, 6:34 PM

Hmm...

Roan, is there a reason that styles for modules requested at page output time don't have their styles loaded up via <link rel="stylesheet">? Is this to force load order, or perhaps because we don't have a good way to distinguish that the style's already been grabbed but JS still has to be loaded?

There's nothing terribly clear in the doc comments on addModules() vs addModuleScripts() vs addModuleStyles() on OutputPage.

(In reply to comment #1)

Hmm...
Roan, is there a reason that styles for modules requested at page output time
don't have their styles loaded up via <link rel="stylesheet">? Is this to force
load order, or perhaps because we don't have a good way to distinguish that the
style's already been grabbed but JS still has to be loaded?
There's nothing terribly clear in the doc comments on addModules() vs
addModuleScripts() vs addModuleStyles() on OutputPage.

Yeah this isn't really documented, but there's a very good reason.

We serve CSS together with JS and load it dynamically (we insert it into the DOM in a <style> tag). So that kind of precludes the <link> tag thing, but even if we would load them as separate requests (which we don't because fewer requests == win) we'd still have another problem. We want to cache requests forever whenever possible. The way we accomplish this is by having one startup module which contains the last-modified timestamps of all other modules, and use a short cache timeout for that (5 minutes). The other modules have cache timeouts of 30 days, and we use a cache-busting version parameter that is set to the last modified timestamp. However, in order to be able to append the version parameter and put the right timestamp in it, we have to do the request from JS. We can't do this in the HTML output, because the timestamp would get Squid-cached and that would take us straight back to the pre-ResourceLoader $wgStyleVersion hell.

For that reason, we only load styles as a <link> tag if they absolutely must be present in a no-JS environment (skin styles are a good example), and we put them in short cache timeout mode because that's the worst of the two evils.

In the not-too-distant future, we'll be able to put this link tag in a <noscript> tag and load it with JS if JS is available. The only reason we're not doing that yet is because the 11.0x versions of Opera have a bug causing <noscript><link ... ></noscript> not to work. All other major browsers, including newer and older versions of Opera, support this. However, this regression is fixed in Opera 11.10, which came out a while ago. We're just waiting for the buggy versions to go extinct (Opera 11.00 is at like 0.25% right now, IIRC), which should happen pretty quickly. I'll look at our browser stats again in mid-July, when we'll be doing a ResourceLoader sprint, and hopefully they'll be low enough to allow doing this then.

Method from bug title was added with r93758