Wikimedia Bugzilla is closed!

Created attachment 9230 [details]
List of stylesheets dynamically loaded

Looks like there is a lot of opportunity for combining styles. Here's a list of the CSS files being dynamically loaded on our MediaWiki 1.17.0 site using WikiEditor, as displayed by Firebug 1.8.3. (See attached image.)

My site is impacted by this problem now, so I wrote an extension (a hack) to work around it.  This extension converts multiple <link> tags into a single <style> tag to include the stylesheets. This reduces the number of style inclusions below 31.

I am not recommending this technique as a permanent fix.

$wgHooks['BeforePageDisplay'][] = 'efCssFixHackForIE';

// Convert CSS <link> tags into a single <style> tag with @import statements.
// Runs only for Internet Explorer.
function efCssFixHackForIE($outputPage, &$skin) {
  // Run only for IE
  if (! $outputPage
      || !isset($_SERVER['HTTP_USER_AGENT'])
      || (strpos($_SERVER['HTTP_USER_AGENT'], 'MSIE') === false)) {
    return true;
  }

  // List of @import statements, space-delimited
  $importStatements = '';

  // Iterate through link tags
  foreach ($outputPage->mLinktags as $index => $value) {
    // Current link tag
    $linkTag = $outputPage->mLinktags[$index];
    // If it's a stylesheet, take action
    if ($linkTag['rel'] == 'stylesheet') {
      // Generate an @import statement for the stylesheet
      $importStatements .= sprintf(" @import url('%s');", $linkTag['href']);
      // Delete the <link> tag for this stylesheet
      unset($outputPage->mLinktags[$index]);
    }
  }

  // Register the import statements
  if ($importStatements) {
    $outputPage->addInlineStyle($importStatements);
  }

  return true;
}

Argh, we just got hit by this again. All the backgrounds of our (custom) dialogs disappeared because styles were missing. I guess my "fix" above isn't completely working.

Which versions of IE does this bug affect?

All versions from IE4 through IE9.  (According to the Microsoft knowledgebase article cited in the bug report, http://support.microsoft.com/kb/262161.)

I don't think changing <link> tags to <style> tags from OutputPage won't do much as most of the resources are dynamically loaded in packaged modules and inserted on the client side as separate <style> tags.

We should probably consider appending to an existing (by resourceloader inserted) <style> tag instead of appending a new element (ofcourse taking into account that the cascading order is not affected, so this can only be done if there is no other <style> or <link> tag after the last one inserted by ResourceLoader).

Too simple , or plausible ?

I'd like to work on this. Could someone please tell me where in the code that ResourceLoader's registered stylesheets are converted to <link> tags for a given page?

FYI, we are unable to upgrade to MediaWiki 1.18 because of this bug, as the number of stylesheets we load (including extensions) has hit 34 as of this release.  I think we were right at 31 in 1.17.

Here is a proposed approach for solving this problem.

In IE only, for the first N stylesheets served, ResourceLoader should have normal behavior for CSS in modules. It responds to load.php by serving any stylesheets normally. (Probably N should be 20 or so, in case MediaWiki is loading other CSS outside of the ResourceLoader framework.) The number N would be tracked by load.php, not in JavaScript.

Beyond N, for any requested module that hasn't already been served, load.php would instead cause the requested stylesheet to be queued. Maybe an additional query parameter to load.php would indicate "queue the CSS for this module", e.g., "load.php?...&modules=...&queuecss=1".

At the end of page-loading (at an appropriate time, maybe at a particular hook), MediaWiki makes one more special load.php call to serve all the queued CSS (e.g., "load.php?retrievecss=1") in one shot. This retrieval would:

1. Concatenate all the queued CSS into one big string.
2. Serve the whole blob of CSS as it were a single stylesheet.

To make things friendly to MediaWiki admins & developers:

* The number N should be configurable in LocalSettings.php, e.g., "$wgResourceLoaderCssHackThreshhold = 15"

* The "concatenate" step should prepend a comment to each stylesheet with the name of the module, e.g., "/* CSS from module foo.bar.blat */\n". So developers can see where the CSS came from.

Comments?

Why would we use an argument to queue css? Moreover where would the state even come from when css might not be loaded in order and there shouldn't be any background server state for these things.

And really, why do we even have 32 stylesheets being loaded? Wasn't the whole point of ResourceLoader to try and kill all the unnecessary extra http requests to speed up the site? Why can't we just find ways to continue combining as much as we possibly can together?

Daniel - you make good points. I am not on the MW development team, and my understanding of ResourceLoader is limited. (I was the discoverer of the problem, which led Roan to file the ticket.)

Aggregating the CSS more effectively would be fine with me. Heck, any reasonable solution is fine for me at this point, since this bug is blocking the 1.18 upgrade at my site (see comment #8) and IMHO is a ticking time bomb for all MW sites.

I should probably mention what the fun symptoms of this problem are. Once the page goes beyond 31 stylesheets, pages start breaking because styles are missing. For us, it's the edit page.  Imagine clicking the "Link" button in WikiEditor and seeing a corrupted link dialog. That's the danger of this problem.

Bumping up to high priority, but I think we should rethink the 1.19 deployment blocker status.  This isn't a regression from 1.18 (is it?) so this shouldn't cause us to stop progress on 1.19.

IMHO, it could be considered a regression from 1.18. Our site works in 1.17.1 but breaks in 1.18.1 due to this bug; the number of ResourceLoader-handled stylesheets must have increased from one release to the next.

Hi Dan, what I'm saying is that this is broken in 1.18 today.  We should try to get this fixed soon (hence my raising the priority), but we're really trying to trim the list of stuff that will block us from deploying 1.19 to the Wikimedia sites mainly to those things that will cause us to go backwards from 1.18 if we deploy.

Thanks Rob.

FYI, I created a workaround (read "UGLY HACK") on our wiki so we can update to MediaWiki 1.18 before this issue is fixed. I moved all of our custom stylesheets into one extension temporarily, so they are loaded as a unit by ResourceLoader.

In case this helps anyone else, here's the slightly-clever implementation so it's easy to back out later when this bug gets fixed.

1. Set up the subdirectory "CssHoldingBin" in extensions:

mkdir extensions/CssHoldingBin
cd extensions/CssHoldingBin
# Symbolic link to each extension whose CSS will be moved:
ln -s ../Extension1 .
ln -s ../Extension2 .
ln -s ../Extension3 .
...

2. Create CssHoldingBin.php, which defines one big batch of CSS files to be loaded together:

# Styles on the edit page
$wgResourceModules['ext.CssHoldingBin'] = array(
   'styles' => array(
     'Extension1/css/file1.css',
     'Extension2/css/file2.css',
     'Extension3/whatever.css',
     ...
   ),
   'localBasePath' => dirname( __FILE__ ),
   'remoteExtPath' => 'CssHoldingBin',
);

3. In each affected extension, comment out the 'styles' element and add 'ext.CssHoldingBin' as a dependency.

Voila, fewer stylesheets loaded by ResourceLoader. And it's easy to back out this change because no files were moved and only a few files were edited.

Actually, our CssHoldingBin defines two resources instead of one -- one for articles and the other for the edit page -- and sets up the CSS files and dependencies appropriately.

I've implemented the fix suggested in comment 6 in r110988. As of version 1.19, MediaWiki will use one <style> tag that all dynamic CSS is written into.

Thanks Roan!

FWIW, I tried patching this into 1.18.1 but it didn't seem to work. I get a JS error:

Unexpected call to method or property access.  load.php?debug=false&lang=en&modules=jquery%2Cmediawiki&only=scripts&skin=vector&version=20120209T141628Z, line 84 character 984

The code around that point is:

append:function(){return this.domManip(arguments,true,function(elem){if(this.nodeType===1){this.appendChild(
elem);}});}

and it's complaining about the "this.appendChild" call.

Hmm. I must admit I'm using .append() to append text (i.e. a string) to a <style> tag, and the documentation isn't very clear on whether that's allowed. On which browser did you get this error? Maybe this is a new jQuery feature that was introduced recently?

I received this error in IE8, version 8.0.7600.16385, running on Windows Server 2008 R2 Enterprise.

My jQuery version is 1.6.4 as installed with MW 1.18.1.

(In reply to comment #20)
> I received this error in IE8, version 8.0.7600.16385, running on Windows Server
> 2008 R2 Enterprise.
> 
> My jQuery version is 1.6.4 as installed with MW 1.18.1.
Turns out this was an IE issue, not a jQuery issue, as I could reproduce on trunk in IE8. It's kind of typical that code written to work around one IE bug is broken by another IE bug :S

Fixed in r111067.

Some related links for bug 34669 in this context:

* http://john.albin.net/css/ie-stylesheets-not-loading
* http://blogs.msdn.com/b/ieinternals/archive/2011/05/14/internet-explorer-stylesheet-rule-selector-import-sheet-limit-maximum.aspx
* http://podlipensky.com/post/2011/09/01/Stylesheet-limits-in-IE.aspx
* http://www.automation-excellence.com/blog/internet-explorer-31-stylesheet-limit

There's also the selector limit in IE, aside from the stylesheet limit of 31 (which is where this bug 31676 is about). We might have to implement a fix for that as well ( http://blesscss.com/ claims to be able to do that).

I patched r110988 and r111067 into our MediaWiki 1.18.1 installation, and the code seems to work. No more CSS problems with IE, as the number of stylesheets on a WikiEditor page has decreased by 12.

Thank you very much for implementing this!!

(In reply to comment #23)
> I patched r110988 and r111067 into our MediaWiki 1.18.1 installation, and the
> code seems to work. No more CSS problems with IE, as the number of stylesheets
> on a WikiEditor page has decreased by 12.
> 
> Thank you very much for implementing this!!

Be careful though! These patches have not been fully approved and there is at least 1 known bug: "@import no longer works in CSS"

Just for the record, this was never deployed/released and reverted due to the @import issues and other concerns.

*** Bug 34276 has been marked as a duplicate of this bug. ***

Solving bug 38024 will reduce the visibility of this problem.

Recently there have been a few bug reports about "Invalid argument" exceptions being thrown in IE9.

Presumably only in debug mode, but I don't know.

Reason is exactly this bug. Where other IE versions silently ignored the too-many (plus, its hard to keep track of the number manually since anything can insert a stylesheet in many different ways) - in IE9 they decided to screw the world even more and throw exceptions at the time of cssText property assingment if the limit is reached.

Probably we added a few more modules and the default number is getting close to 31, add a few gadgets and bingo. 

* Lets wrap a try/catch to at least make stuff not fail (and call log() from production)

* Solving bug 38024 will reduce the number of stylesheets but as before, it can still happen. But at least it won't break stuff and (at least for now) there is no reliable way to work around that limit because
** stuff like @import needs to be on top, so concatenating unrelated modules is a bad idea
** syntax errors...
** On top of the 31-stylesheet limit IE also a max of 4095 rules per stylesheet[1]. So concatenating everything doesn't help either.

By the way, there is a node library that tries to work around this[2].


[1] http://blogs.msdn.com/b/ieinternals/archive/2011/05/14/internet-explorer-stylesheet-rule-selector-import-sheet-limit-maximum.aspx
[2] http://blesscss.com/

See bug 38032 (closed as dupe of this one). Upload Wizard (and other stuff) is not usable by the poor IE lovers

bug 38024 has been fixed, merged in master and backported to 1.20wmf6. This should hopefully reduce the number of stylesheets for most if not all users to below 31 in IE9.

(In reply to comment #28)
> Presumably only in debug mode
Even when turning off IE debug mode and not using &debug=true, UpWiz did not load.

> Where other IE versions silently ignored
Tested with IE 6 yesterday and it did not ignore. Also IE 8 did not. Instead the yellow ! appeared in the status bar. And got a user complaint on [[:commons:COM:HD]] using IE 7.

*** Bug 38513 has been marked as a duplicate of this bug. ***

Fix in I3e8227ddb87fd9441071ca935439fc6467751dab.

queued for merge req to 1.20

(In reply to comment #34)
> queued for merge req to 1.20

Hm.. what queue are you referring to? I don't see it in gerrit (other than aforementioned pending change in refs/for/master)

(In reply to comment #35)
> Hm.. what queue are you referring to?

The one in my head :P

See Change-Id: I3c76e9a8cebfa993d51bfd4b9b4a0963c03cf370

(In reply to comment #36)
> (In reply to comment #35)
> > Hm.. what queue are you referring to?
> 
> The one in my head :P
> 
> See Change-Id: I3c76e9a8cebfa993d51bfd4b9b4a0963c03cf370

Abandoned as it created a new change-id, was actually an early version of the patch.

(In reply to comment #33)
> Fix in I3e8227ddb87fd9441071ca935439fc6467751dab.

Landed in master, and merged to REL1_20.