Bug 88869 – renderer should not block on script-inserted stylesheets

• Component: CSS
• Status: REOPENED

[Bryan]

The HTML5 spec, section 4.2.7, reads:

"A style sheet in the context of the Document of an HTML parser or XML parser is said to be a style sheet that is blocking scripts if the element was created by that Document's parser..."

I read this to mean that script-inserted sheets should not block the parser. Webkit seems to partially follow this, but there are cases where webkit could be more efficient.

For instance, if you look at the network waterfall for http://stevesouders.com/cuzillion/?c0=hc1dfff4_0_f&c1=hj1wfff0_0_f&c2=hb0hfff0_0_f&c3=hj1wfff2_0_f&t=1339507371 in a webkit browser:
http://www.webpagetest.org/result/120612_FQ_M0G/1/details/

You see that the first document-written script after the script-inserted stylesheet is not blocked on the parser, but the second one is. I get the impression that webkit is not immediately marking the sheet as renderer blocking (otherwise the inline script that does the doc-write of the first script would not execute until after the sheet is applied) however webkit does seem to block the second document.write on the sheet being applied, which is unnecessary.

By contrast, the same load in Firefox does not block either script on the sheet:
http://www.webpagetest.org/result/120612_9A_KS7/1/details/

This ends up causing a substantial renderer delay for sites like www.nytimes.com, which inserts a sheet via script which has a series of serialized @imported sheets that cause excess renderer delay due to the serialization of the sheet loads (see homepage.css, which @imports base.css, which @imports tooltip.css). There are other scripts that could be executed during the time these sheets are loading, but they end up getting blocked on each of these sheets getting downloaded serially and finally being applied.

[Tony Gentilcore]

If FF is matching the spec already, it is probably safe for us to do that as well. Patch should be straightforward: probably removing a certain call to Document::addPendingSheet().

[Shrey Banga]

Created attachment 147411 [details]
Patch

[WebKit Review Bot]

Attachment 147411 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/http..." exit_code: 1
LayoutTests/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
LayoutTests/ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 2 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Shrey Banga]

Created attachment 147416 [details]
Patch

[Tony Gentilcore]

Patch looks reasonable. Some quick questions:
- what does this do with @import stylesheets?
- can you verify other browsers' behavior here?
- Bryan mentions nytimes suffering from this issue. If you are able to measure a percent improvement here, that would be worth mentioning in the change log.

[Shrey Banga]

Created attachment 147455 [details]
Patch

[Shrey Banga]

(In reply to comment #5)
> Patch looks reasonable. Some quick questions:
> - what does this do with @import stylesheets?
Essentially for any stylesheet to be counted as blocking, its highest ancestor in the import chain should be an inline <style> or <link> tag. So if a document.write is used to insert a stylesheet which further @imports other stylesheets, all of them will be considered non-blocking.

> - can you verify other browsers' behavior here?
Our behavior matches Firefox's after this fix.

> - Bryan mentions nytimes suffering from this issue. If you are able to measure a percent improvement here, that would be worth mentioning in the change log.
web page replay showed ~100ms (2.7%) improvement in page load time for www.nytimes.com when tested with a connection of 2mbps/400kbps/40ms connection. Added that to the changelog.

[Darin Adler]

Hyatt, what do you think of this change?

[Tony Gentilcore]

Comment on attachment 147455 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=147455&action=review

A couple of suggestions on the test, but the rest looks good to me.

Hyatt, do you want a chance to review?

> LayoutTests/http/tests/css/script-inserted-stylesheet.html:39
> +      timerID = window.setInterval(function() {checkStyleSheetLoaded(elem)}, 30);

elem.onload would be your friend here.

But rather than checking times at all, why not just verify that the sheet is not in the document at the time the below scripts run. Then you don't even have to wait the 1s for the sheet to load.

> LayoutTests/http/tests/css/script-inserted-stylesheet.html:53
> +    document.write('<scr' + 'ipt type="text/javascript" src="resources/script-inserted-stylesheet-external-script-2.js"></scr' + 'ipt>');

Is there any reason to doc.write these as opposed to including them directly.

Also, it would be a little cleaner to do <script ... onload="onScript1Loaded()"> instead of having to have special script resources that call methods in the root doc.

[Antti Koivisto]

Won't this cause flash of unstyled content with script-inserted stylesheets?

[Tony Gentilcore]

(In reply to comment #10)
> Won't this cause flash of unstyled content with script-inserted stylesheets?

Good point. We want to hold up render tree construction even though (I think) we agree we don't need to hold up script execution.

Currently we marry the concepts into a single list of pending sheets. What if we separated the two. Perhaps give Document two fields: m_pendingStylesheets and m_pendingParserBlockingStylesheets (or maybe the latter fits better on HTMLDocumentParser)?

[Antti Koivisto]

Right, we should separate those concepts. Not sure what the best factoring is.

[Antti Koivisto]

Comment on attachment 147455 [details]
Patch

r- due to FOUC concerns.

[Antti Koivisto]

We should have a tracking mechanism that has more data than just the pending count (which is awfully fragile anyway). A CachedCSSStyleSheet* -> BlockingInfo map or similar maybe.

[Ilya Grigorik]

(In reply to comment #11)
> (In reply to comment #10)
> > Won't this cause flash of unstyled content with script-inserted stylesheets?
> 
> Good point. We want to hold up render tree construction even though (I think) we agree we don't need to hold up script execution.

Wouldn't this break getComputedStyle in script execution? Which brings us back to square one.

FWIW, I don't think its unreasonable to say that script inserted stylesheets will not block the renderer at all. If you inject *all* of your stylesheets via JS, then yes, you have just created a FOUC scenario. However, you can (should) rely on parser created stylesheets to be blocking, and you have the flexibility to inject non-blocking stylesheets via JS. 

I was trying to think through this scenario last night, take a look here: https://gist.github.com/2935269 (WIP)

[Ilya Grigorik]

On some further thinking about this.. It's not clear that FF behavior is what you want all the time. We already have a *working* mechanism to determine if a stylesheet should block renderer / parsing, or not.. media-queries. 

If no media query is specified on the inserted element, then it *should* block - the implicit media is "screen", which always applies. However, if a media query is specified and evaluates to false, then we *should not* hold up the parser or rendering. 

The good news is, that's behavior as it exists today: http://jsbin.com/3/omulof/9/quiet

Making all injected stylesheets non-blocking would break the media query interface - I don't think that's what we want. Perhaps the conditional scenario is: if no media query is specified, then default to non-blocking, but if there is a media query, than make sure to evaluate it. If media is simply "screen" or evaluates to true, then it should hold up the parser and everything else. Although I'm not convinced this is a great idea... Thoughts?

[Bryan]

I can understand where you are coming from, Ilya. On first glance it seems odd to allow sheets to be non-blocking. But (1) script-inserted scripts have worked this way for a long time (I claim the same arguments could be made for scripts: that the side-effects of the node-inserted script are not visible to subsequent scripts, which also can be argued as odd but is the way HTML5 specs it), and (2) the HTML5 spec explicitly calls out that only parser-inserted sheets should block ("A style sheet in the context of the Document of an HTML parser or XML parser is said to be a style sheet that is blocking scripts if the element was created by that Document's parser..."). If someone really needs to make a script or sheet blocking and it is being inserted via javascript, there is a mechanism to make this happen: document.write. If it is critical to make the sheet or script blocking, the spec is clear that document.write is the way to do this.

I'm open to blocking construction of the render tree on these sheets but it seems pretty clear that (1) the HTML5 spec does not indicate that these need to be blocking and (2) there already exists a mechanism to insert either scripts or sheets in a blocking manner from JS (doc.write). So I do not think it would be correct to block the renderer here. Given that Firefox has implemented this in a non-blocking way it also seems clear that sites like nytimes.com that insert sheets via script do not depend on it blocking the renderer. Blocking construction of the render tree does seem reasonable to me, though.

[Ilya Grigorik]

Hmm, so what about the hybrid scenario? Default to nonblocking on script inserted stylesheets without a media-query, but do respect the media-query if one is provided? Not doing so would only create pain and head-scratching from developers (and yet another "browser quirk" for libraries to pave over).

Seems like above would give us the performance benefit you're looking for, while preserving the functionality of the element.

[Bryan]

Sure, if the webkit devs think that it makes sense to key off the media query, I'm not opposed to it. It's really their call. I do think that if a web dev is counting on having a sheet being blocking they should be using doc.write, since that's the only cross-browser way to guarantee blocking loading of a sheet inserted via JS. Can you test and see what FF does (blocking vs non-blocking) when a media query is present in a script-node inserted sheet?

[Tony Gentilcore]

I agree w/ comment 17. Let's stick to the spec and get the performance benefit for existing web content that FF is getting here.

If we want to do something else, that discussion should take place on the whatwg.

[Ilya Grigorik]

(In reply to comment #19)
> Can you test and see what FF does (blocking vs non-blocking) when a media query is present in a script-node inserted sheet?

FF doesn't appear to respect media queries on script inserted stylesheets. WebKit does (currently). 

With respect to doc.write's, there are also differences between WebKit and FF: 

Test: http://jsbin.com/3/omulof/12/quiet
Result: https://img.skitch.com/20120618-twkgw1c7jrsp9c2c4gwrm35k6d.jpg

- FF loads the script inserted stylesheet in parallel with doc.write
- multiple doc.write calls in FF result in parallel downloads, and serialized downloads in WebKit

So, in FF, doc.write is "sort off" blocking - you need to rely on script tag boundaries.

[Ojan Vafai]

Created attachment 167836 [details]
test case for getComputedStyle

Firefox 15 outputs:
appended link
initial font-weight: 400
second font-weight: 400
link loaded
body loaded

WebKit r130046 outputs:
appended link
initial font-weight: normal
second font-weight: bold
link loaded
body loaded

So, it looks like we can get away with treating these loads as truly async, e.g. getComputedStyle doesn't block on the stylesheet loading.

[Ojan Vafai]

Performance aside, the Firefox 15 behavior seems preferable to me for web developers. The WebKit behavior is inconsistent.

[Ojan Vafai]

Relatedly, we seem to have some layout issues when blocking on stylesheets: bug 98821.

[Ahmad Saleem]

Created attachment 460137 [details]
Safari 15.5 matches other browsers

From the attached test cases, Safari 15.5 matches other browsers in the output. If it intended / expected result then we can close this issue. If not, please retest accordingly. Thanks!

[Alexey Proskuryakov]

The test is broken now. In the browser, loading the subresource is blocked because of mixed content (https vs. http), but also, the remote server is gone anyway.

So comparing the result with other browsers tells us nothing.

The patch has a test case; applying it would tell us more about whether this is still an issue. But also, one would need to convert it from PHP to Python, because PHP doesn't ship with macOS since 2021.

[Ahmad Saleem]

@[e-mail] - thanks for correction, I was able to retrieve the URL from the Wayback Archive and have now modified the test as follow:

Link - https://jsfiddle.net/8yg1kbdh/show

It shows following across browsers:

*** Safari 15.5 on macOS 12.4 ***

appended link
initial font-weight: 400
second font-weight: 400
body loaded

*** Firefox Nightly 104 ***

appended link
initial font-weight: 400
second font-weight: 400
body loaded

*** Chrome Canary 105 ***

appended link
initial font-weight: 400
second font-weight: 400
link loaded
body loaded

___________

Let me know if it is now correct test or not. Thanks for correcting and helping me to push myself more. Thanks!