Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug with script-blocking <style> elements #7469

Closed
xiaochengh opened this issue Jan 6, 2022 · 11 comments · Fixed by #7470
Closed

Bug with script-blocking <style> elements #7469

xiaochengh opened this issue Jan 6, 2022 · 11 comments · Fixed by #7470

Comments

@xiaochengh
Copy link
Contributor

If a script-blocking <style> element has critical subresources, we should increment the script-blocking stylesheet counter when we start to fetch those subresources, and decrement the counter when the fetching finishes.

The decrement step is at here, but the increment step seems missing, as none of the references of script-blocking stylesheet counter is related.

@domenic
Copy link
Member

domenic commented Jan 6, 2022

Wow, that is embarassing! A pull request to fix this would be very welcome.

@xiaochengh
Copy link
Contributor Author

Some notes on the parts of the specs that seem problematic to me...


The current specs are pretty vague on stylesheet critical subresources... It says

The specification that defines a link type's critical subresources (e.g., CSS) is expected to describe how these subresources are fetched and processed.

But the CSSOM spec doesn't say anything about it. I can't even find any occurrence of "subresource" in it.

As a result, there's no spec on how to create a CSS style sheet for an @import-ed style sheet. Both HTML and CSSOM have only specified how to create a CSS style sheet for a root sheet.


Besides, in the style element section, the HTML spec doesn't say when to start fetching the subresources.


And there's an inconsistency between style and link elements: style runs the create a CSS style sheet steps before the critical subresources finish, but link runs those steps after the critical subresources finish. I think both should wait until the subresources finish, because a style sheet might not be useable before the @import-ed sheets are ready. Not sure if this should be a separate issue though.

@domenic
Copy link
Member

domenic commented Jan 7, 2022

The current specs are pretty vague on stylesheet critical subresources... It says

Yep. This is basically something we've been waiting for the CSS community to help with for some time. #3532 and #2263 are the relevant issues, I think. The former even has some unfinished WPT pull requests, it looks like!

As a result, there's no spec on how to create a CSS style sheet for an @import-ed style sheet. Both HTML and CSSOM have only specified how to create a CSS style sheet for a root sheet.

Oh, nice find. I think that's a separate issue from the question of load event timing, which is what we've mostly been focused on so far.

Besides, in the style element section, the HTML spec doesn't say when to start fetching the subresources.

Both style and link just kind of assume that fetching the stylesheet will fetch subresources somehow. Then they set up some stuff to run (about the load event and decrementing the relevant counter) once they've finished loading. Yeah, this is not great.

From what I understand @tabatkins recently wrote https://drafts.csswg.org/css-values-4/#fetch-a-style-resource but I don't know who is supposed to call that and whether it applies to @imports or @font-faces or background-images or... It doesn't seem like it's meant to apply to <link>s.

There's also https://drafts.csswg.org/cssom/#fetching-css-style-sheets . Historically we've ignored that, having our own spec for fetching stylesheet <link>s, because it was very outdated (e.g. didn't integrate with the actual Fetch spec). However it looks like recently @tabatkins and @noamr have updated it to integrate with Fetch. We could start using that, but that doesn't appear to do any recursion into @imports either, so although it solves issues like #2997 , it doesn't solve the issue you're raising. And, I tend to prefer to keep all the <link> fetching algorithms in HTML anyway, where possible.

And there's an inconsistency between style and link elements: style runs the create a CSS style sheet steps before the critical subresources finish, but link runs those steps after the critical subresources finish.

That seems reasonable, although we'd need tests to show that. (I.e., tests where a.css imports slow.css, and we confirm that a.css never shows up in document.styleSheets before slow.css does.)

Help on any of this stuff would be really welcome!!

@tabatkins
Copy link
Contributor

From what I understand @tabatkins recently wrote drafts.csswg.org/css-values-4/#fetch-a-style-resource

Nah, that was Noam. I presume this is meant to be a generic URL-fetcher for anything CSS related that sets up all the boilerplate for you, and other algos he's written or is writing call into it.

I've spent some decent effort trying to dive in and fix all of this, but Fetch's significant under-specification (for usage and guidelines, at least) has stymied me twice now. It also didn't help that HTML's link fetching algos are out-of-date and no longer work against the current Fetch algos (a few months ago, at least; dunno if they've been fixed), so I couldn't interoperate with them or use them as guidance for writing my own. :(

@xiaochengh
Copy link
Contributor Author

Besides, in the style element section, the HTML spec doesn't say when to start fetching the subresources.

Both style and link just kind of assume that fetching the stylesheet will fetch subresources somehow. Then they set up some stuff to run (about the load event and decrementing the relevant counter) once they've finished loading. Yeah, this is not great.

There's a subtle difference that style doesn't need to fetch its stylesheet, as it's already inline. So we are basically assuming that it's the parsing of the stylesheet that triggers the fetching of the subresources.

And since style doesn't have a resource, even the term "subresource" seems a bit ill-defined here...

@noamr
Copy link
Contributor

noamr commented Jan 8, 2022

The current specs are pretty vague on stylesheet critical subresources... It says

Yep. This is basically something we've been waiting for the CSS community to help with for some time. #3532 and #2263 are the relevant issues, I think. The former even has some unfinished WPT pull requests, it looks like!

As a result, there's no spec on how to create a CSS style sheet for an @import-ed style sheet. Both HTML and CSSOM have only specified how to create a CSS style sheet for a root sheet.

Oh, nice find. I think that's a separate issue from the question of load event timing, which is what we've mostly been focused on so far.

Besides, in the style element section, the HTML spec doesn't say when to start fetching the subresources.

Both style and link just kind of assume that fetching the stylesheet will fetch subresources somehow. Then they set up some stuff to run (about the load event and decrementing the relevant counter) once they've finished loading. Yeah, this is not great.

From what I understand @tabatkins recently wrote https://drafts.csswg.org/css-values-4/#fetch-a-style-resource but I don't know who is supposed to call that and whether it applies to @imports or @font-faces or background-images or... It doesn't seem like it's meant to apply to <link>s.

It applies to imports, font-faces, images etc. explicitly - all those specs call fetch a style resource directly.
This is the only place where subresources of a style are fetched, and I think it shouldn't be difficult to patch them through to increment/decrement the counter.

@noamr
Copy link
Contributor

noamr commented Jan 8, 2022

As a result, there's no spec on how to create a CSS style sheet for an @import-ed style sheet. Both HTML and CSSOM have only specified how to create a CSS style sheet for a root sheet.

Of course there is!
fetch an import calls parse which in turn calls create a CSS style sheet.

@domenic
Copy link
Member

domenic commented Jan 8, 2022

Nice! Who calls "fetch an import"?

@noamr
Copy link
Contributor

noamr commented Jan 8, 2022

Nice! Who calls "fetch an import"?

The answer to this question is less nice. It's not called explicitly, but is implied.
As with the rest of style resources, it's specified how they are fetched but not precisely when.

For imports it can probably be done in a more observable way. For images/fonts etc. this is more challenging - user agents can (and do) make a lot of decisions about when to fetch them, e.g. web fonts are only fetched when there is text in the document that uses the relevant font, and I believe the same goes for SVG shapes and background images. They're not downloaded as soon as the stylesheet is downloaded, but rather wnen the resource is used.

Thus, I'm not sure fonts etc. (anything besides CSS imports) can be considered "subresources of the stylesheet". They're perhaps a subresource of the stylesheet+the relevant DOM, and perhaps a consistent way to have them block the load event is to block that event in case the fetch started, regardless of how the fetch was initiated.

@domenic
Copy link
Member

domenic commented Jan 8, 2022

Right. In particular for imports getting this straightened out is important since it affects what's visible in document.styleSheets before/after the load event fires.

@xiaochengh
Copy link
Contributor Author

Didn't know we have fetch an import. Thanks for the pointer!

For the other types of resources, I'm pretty sure fonts are not considered critical subresources, since they should be lazy-loaded on use, so there's no guarantee that they will be loaded.

The sitation with images seems worse. Current spec didn't say whether images are critical or not, so I made a test case (https://purring-various-felidae.glitch.me/stylesheet-resources.html) to compare browsers' behaviors.

In Chrome, images do not block the load event on the style element, but do block the global load event on window. Not sure if this is the correct behavior.

In Firefox, it looks like the loading of @import and images are sequential... So both block the global load event. And no load event is fired on the style element.

Anyway, since the spec is intentionally vague on other subresources, maybe we can live with that. But we should at least make imports work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
5 participants