Page MenuHomePhabricator

IE9 executing our MediaWiki:Print.css stylesheet in screen media
Closed, ResolvedPublic

Event Timeline

NQ created this task.Aug 11 2015, 8:06 PM
NQ updated the task description. (Show Details)
NQ raised the priority of this task from to Needs Triage.
NQ added a subscriber: NQ.
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptAug 11 2015, 8:06 PM
TheDJ added a subscriber: TheDJ.Aug 12 2015, 11:03 AM

I have also made tests with IE9 on Windows Vista now. Hatnotes were visible in mobile but missing for desktop in mainspace and no other tested namespaces. They were missing in all four skins both logged in and out. They appeared for a fraction of a second each time the page was loaded. Some other things like navboxes were also missing. I searched the MediaWiki namespace for hatnote and found MediaWiki:Print.css which is apparently the culprit. -- PrimeHunter

TheDJ renamed this task from Hatnotes not displaying in IE9 to IE9 executing our MediaWiki:Print.css stylesheet in screen media.Aug 12 2015, 11:03 AM
TheDJ set Security to None.
ori added a subscriber: ori.Aug 12 2015, 4:23 PM

Would wrapping the contents of MediaWiki:Print.css in @media print{ } solve this?

TheDJ added a comment.Aug 12 2015, 8:13 PM

@ori it already is wrapped in media print.

See bottom of:
https://en.wikipedia.org/w/load.php?debug=false&lang=en&modules=site&only=styles&skin=vector&*

Perhaps the IE CSS parser is confused by something preceding it ?

This also happens at de.wikipedia.

@media print{div.NavFrame,div.BoxenVerschmelzen{display:none}span.mw-cite-backlink{display:none}#content .plainlinks-print a.external.text:after,#content .plainlinks-print a.external.autonumber:after{content:""}}

from https://de.wikipedia.org/w/load.php?debug=false&lang=de&modules=site&only=styles&skin=vector&* is applied in IE9 on screen, too.

The behavior seems to be introduced with the recent changes to ResourceLoader.

That change went live with 1.26wmf18, i.e. yesterday for Wikipedias, didn't it? The issue already existed before (1.26wmf17).

That change went live with 1.26wmf18, i.e. yesterday for Wikipedias, didn't it? The issue already existed before (1.26wmf17).

It was backported to 1.26wmf17: https://gerrit.wikimedia.org/r/#/c/230575/

I agree 11e47561e4652a is a likely culprit (though haven't verified yet). Specifying both <link media="print"> and using @media in the CSS content could cause a problem.

This is why we previously refrained from doing so for other modules and instead opt for the flexibility of providing a single CSS response and allowing individual modules and their stylesheets to specify the intended media target in the ResourceLoader module definition array, and wrapping each portion accordingly. (Leaving the media target of the link element implied "all".)

For statically loaded stylesheets (from the HTML directly with <link>) this wastes a small amount of bandwidth (but saved an expensive http request in the pre-HTTP2 era).

For dynamically loaded stylesheet (from mw.loader) it doesn't matter as the module is loaded as a whole regardless and is merely injected as unused stylesheet, and allowed us to extend the same <style> sheet instead of creating new ones for each media attribute, and other concatenation flexibilities.

I understand the potential gain from this commit by omitting print styles from the main CSS payload (bandwidth and CSS parsing/memory performance gain) but the way it is implemented pokes a hole through ResourceLoader and bypasses its safe guards and consistency guarantees by deferring control of this aspect to OutputPage in an unenforced way. The resulting behaviour is almost impossible to document or explain, and creates yet-another magical reserved "group" – something we've been trying to phase out (Documentation about "Groups").

For example, loading this module on-demand with mw.loader results in no such media restriction. And what if a module has group => print but also contains stylesheets with media targets other than print (granted, garbage in, garbage out). We previously denied support for nested @media blocks (e.g. nested @media blocks within a file, or even having a file with a single @media and also specifying a media in the module definition; which is effectively the same thing).

The logic added in Ife1d65b31bc basically adds another implied nesting layer. If we really want this, I propose the following approach:

  • Require (hard error) that all stylesheets in group=print have media=print.
  • Omit the media wraps for those modules from any only=styles response, .. but keep them if loaded through mw.loader (without only=styles) – and also add any midding 'media=>print' to stylesheets that don't have it in this group (to enforce the same media target when not loaded by OutputPage).
  • Enforce somehow that the HTML <link> element created by OutputPage has media=print for group=print when loaded raw with only=styles.

However the above shows just that RL is not designed for this kind of media separation.

I'd say try to have a deployer verify the bug in IE9 on testwiki and see if a local revert of this commit fixes it. If so, we can revert and re-implement at a later time as imho the bug is more important than the performance potential in this case.

Krinkle triaged this task as Normal priority.Aug 19 2015, 4:50 PM
Krinkle added a project: Regression.
Krinkle moved this task from Inbox to Accepted: Enhancement on the MediaWiki-ResourceLoader board.
Krinkle updated the task description. (Show Details)Aug 21 2015, 2:58 PM
revi added a comment.Sep 7 2015, 8:51 AM

Korean Wikipedia VP (Tech) had same issue (not sure tbh since I don't use IE nor I care about it) reported, screenshot below for convinience.

The change 11e47561e4652a is definitely a wrong thing to do. We even have unit tests that would prevent you from doing this if you tried to do it the normal way.

Change 237435 had a related patch set uploaded (by Bartosz Dziewoński):
Revert "Load 'mediawiki.legacy.commonPrint' styles with a media type property"

https://gerrit.wikimedia.org/r/237435

Change 237435 merged by jenkins-bot:
Revert "Load 'mediawiki.legacy.commonPrint' styles with a media type property"

https://gerrit.wikimedia.org/r/237435

LeadSongDog reopened this task as Open.Sep 22 2015, 5:26 PM
LeadSongDog added a subscriber: LeadSongDog.

Still not working for IE9/Win7-64 at en.wikipedia (logged in or not) as of 22 Sep.

Elitre added a subscriber: Elitre.Sep 22 2015, 5:49 PM

I'll look into this again over the weekend.

I've downloaded a Win7/IE9 virtual machine from https://dev.modern.ie/tools/vms/ and I can reproduce the issue with it. I think we're running into the same issue as discussed at https://github.com/webpack/style-loader/issues/39.

Change 241696 had a related patch set uploaded (by Bartosz Dziewoński):
Work around IE9's broken handling of .styleSheet.cssText

https://gerrit.wikimedia.org/r/241696

Change 241696 merged by jenkins-bot:
Work around IE9's broken handling of .styleSheet.cssText

https://gerrit.wikimedia.org/r/241696

matmarex closed this task as Resolved.Sep 29 2015, 7:21 PM
matmarex removed a project: Patch-For-Review.

Actually, I'll go ahead and have it backported (to WMF wikis and to 1.26 release).

Change 242508 had a related patch set uploaded (by Bartosz Dziewoński):
Work around IE9's broken handling of .styleSheet.cssText

https://gerrit.wikimedia.org/r/242508

Change 242509 had a related patch set uploaded (by Bartosz Dziewoński):
Work around IE9's broken handling of .styleSheet.cssText

https://gerrit.wikimedia.org/r/242509

Change 242510 had a related patch set uploaded (by Bartosz Dziewoński):
Work around IE9's broken handling of .styleSheet.cssText

https://gerrit.wikimedia.org/r/242510

Change 242509 merged by jenkins-bot:
Work around IE9's broken handling of .styleSheet.cssText

https://gerrit.wikimedia.org/r/242509

Change 242510 merged by jenkins-bot:
Work around IE9's broken handling of .styleSheet.cssText

https://gerrit.wikimedia.org/r/242510

Change 242508 merged by jenkins-bot:
Work around IE9's broken handling of .styleSheet.cssText

https://gerrit.wikimedia.org/r/242508