ResourceLoader should work around IE stylesheet limit
Closed, ResolvedPublic

Description

Per http://lists.wikimedia.org/pipermail/wikitech-l/2011-October/055704.html and http://support.microsoft.com/kb/262161 , the 32nd <style> tag and beyond are ignored by IE. RL should combine <style> tags as much as it can to prevent the page from containing more than 31 style tags.


Version: 1.20.x
Severity: major
See Also:
https://bugzilla.wikimedia.org/show_bug.cgi?id=47277

Catrope created this task.Via LegacyOct 13 2011, 7:55 PM
maiden_taiwan added a comment.Via ConduitOct 13 2011, 8:37 PM

Created attachment 9230
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.)

Attached:

maiden_taiwan added a comment.Via ConduitOct 14 2011, 5:11 PM

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;

}

maiden_taiwan added a comment.Via ConduitNov 10 2011, 5:48 PM

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.

RobLa-WMF added a comment.Via ConduitJan 12 2012, 6:14 PM

Which versions of IE does this bug affect?

maiden_taiwan added a comment.Via ConduitJan 12 2012, 8:52 PM

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

Krinkle added a comment.Via ConduitJan 13 2012, 10:23 PM

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 ?

maiden_taiwan added a comment.Via ConduitJan 20 2012, 7:02 PM

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?

maiden_taiwan added a comment.Via ConduitJan 20 2012, 8:47 PM

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.

maiden_taiwan added a comment.Via ConduitJan 25 2012, 12:46 PM

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?

DanielFriesen added a comment.Via ConduitJan 25 2012, 12:56 PM

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?

maiden_taiwan added a comment.Via ConduitJan 25 2012, 1:09 PM

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.

maiden_taiwan added a comment.Via ConduitJan 25 2012, 1:13 PM

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.

RobLa-WMF added a comment.Via ConduitJan 25 2012, 5:15 PM

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.

maiden_taiwan added a comment.Via ConduitJan 25 2012, 5:59 PM

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.

RobLa-WMF added a comment.Via ConduitJan 25 2012, 6:30 PM

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.

maiden_taiwan added a comment.Via ConduitJan 26 2012, 5:15 PM

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 .
...

  1. 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',

);

  1. 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.

Catrope added a comment.Via ConduitFeb 9 2012, 12:21 AM

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.

maiden_taiwan added a comment.Via ConduitFeb 9 2012, 2:24 PM

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.

Catrope added a comment.Via ConduitFeb 9 2012, 3:03 PM

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?

maiden_taiwan added a comment.Via ConduitFeb 9 2012, 3:10 PM

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.

Catrope added a comment.Via ConduitFeb 9 2012, 7:04 PM

(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.

Krinkle added a comment.Via ConduitFeb 27 2012, 8:05 PM

Some related links for bug 34669 in this context:

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).

maiden_taiwan added a comment.Via ConduitFeb 28 2012, 8:24 PM

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!!

Krinkle added a comment.Via ConduitFeb 28 2012, 8:32 PM

(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"

Krinkle added a comment.Via ConduitJun 28 2012, 12:40 PM

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

Krinkle added a comment.Via ConduitJun 28 2012, 12:42 PM
  • Bug 34276 has been marked as a duplicate of this bug. ***
Krinkle added a comment.Via ConduitJun 28 2012, 12:54 PM

Solving bug 38024 will reduce the visibility of this problem.

Krinkle added a comment.Via ConduitJun 28 2012, 7:44 PM

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/

bzimport added a comment.Via ConduitJun 28 2012, 11:25 PM

saibotrash wrote:

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

Krinkle added a comment.Via ConduitJun 28 2012, 11:31 PM

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.

Rillke added a comment.Via ConduitJun 29 2012, 2:25 PM

(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.

Krinkle added a comment.Via ConduitJul 24 2012, 9:26 PM
  • Bug 38513 has been marked as a duplicate of this bug. ***
Krinkle added a comment.Via ConduitSep 25 2012, 5:15 PM

Fix in I3e8227ddb87fd9441071ca935439fc6467751dab.

MarkAHershberger added a comment.Via ConduitSep 30 2012, 6:03 PM

queued for merge req to 1.20

Krinkle added a comment.Via ConduitSep 30 2012, 10:36 PM

(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)

MarkAHershberger added a comment.Via ConduitOct 2 2012, 2:05 PM

(In reply to comment #35)

Hm.. what queue are you referring to?

The one in my head :P

See Change-Id: I3c76e9a8cebfa993d51bfd4b9b4a0963c03cf370

Krinkle added a comment.Via ConduitOct 2 2012, 11:24 PM

(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.

Add Comment

Column Prototype
This is a very early prototype of a persistent column. It is not expected to work yet, and leaving it open will activate other new features which will break things. Press "\" (backslash) on your keyboard to close it now.