Improve clarity in link and style sheet resource fetching #3544

domenic · 2018-03-07T07:34:15Z

Defines the critical subresources for CSS as @imported style sheets, including
indirectly imported ones. This was previously defined nowhere; probably it
should live in CSS, but for now it lives here.
Makes it clearer that the steps to obtain the resource are the default steps,
which some link types override.
Adds explicit steps for fetching critical subresources, and ties those to the
load/error events.
Uses fetch primitives like network error and ok status instead of vague
language.
Makes it clearer how incorrect Content-Types for CSS generate error events, by
defining the new "resource-specific fetch error" concept.
Makes it clearer that resources apply if and only if their main fetch
succeeds. (Critical subresource fetches generate error events, but do not
impact the "obtained successfully" or "apply" concepts.)

Of these only the first is really a normative fix; the rest were present in the existing spec if you squint hard enough.

/infrastructure.html ( diff )
/links.html ( diff )
/semantics.html ( diff )

domenic · 2018-03-16T06:48:37Z

@bzbarsky @annevk review ping

annevk · 2018-03-16T09:10:07Z

source

@@ -2025,8 +2025,14 @@ a.setAttribute('href', 'https://example.com/'); // change the content attribute
  <ref spec=HTTP></p>

  <p>A resource's <dfn>critical subresources</dfn> are those that the resource needs to have
-  available to be correctly processed. Which resources are considered critical or not is defined by
-  the specification that defines the resource's format.</p>
+  available to be correctly processed. Which resources are considered critical or not generally


are generally*

annevk · 2018-03-16T09:11:21Z

source

+  stylesheets.</p>
+
+  <p class="&#x0058;&#x0058;&#x0058;">This definition should be moved to CSS, alongside more detail
+  around how they are fetched.</p>


It'd be great if we could turn this into an issue link.

annevk · 2018-03-16T09:12:40Z

source

@@ -13612,38 +13621,66 @@ interface <dfn>HTMLLinkElement</dfn> : <span>HTMLElement</span> {
    </ol>
   </li>

-   <!--FETCH--><li><p><span data-x="concept-fetch">Fetch</span> <var>request</var>.</p></li>
+   <!--FETCH-->


Maybe we should start dropping these now we have concept-fetch?

annevk · 2018-03-16T09:13:20Z

source

+    <ol>
+     <li><p>If <var>response</var> is a <span>network error</span>, or its <span
+     data-x="concept-response-status">status</span> is not an <span>ok status</span>, or it has a
+     <dfn>resource-specific fetch error</dfn>, then <span>queue a task</span> using the <span>DOM


This seems a little COMEFROM?

How so? It's defining a concept which each resource is allowed to define; basically polymorphism.

annevk · 2018-03-16T09:14:11Z

source

-  resources. (For example, redirects will be followed and 404 responses will cause the external
-  resource to not be applied.)</p>
+     <li><p>Once the original fetch and all of the <span data-x="critical subresources">critical
+     subresource</span> fetches have completed (i.e. have reached the <span>process response


annevk · 2018-03-16T09:17:09Z

source

+     <code data-x="event-error">error</code> at the <code>link</code> element, and abort these
+     steps. The resource was not <span>obtained successfully</span>.</p></li>
+
+     <li><p>At this point, the resource has been <span>obtained successfully</span>.</p></li>


This seems wrong. I think this needs to be done as part of the task that is queued. Otherwise we update .sheet and other state too early, no?

THis whole step is inside of the task that is queued by https://fetch.spec.whatwg.org/#ref-for-process-response%E2%91%A0 , right?

annevk · 2018-03-16T09:18:36Z

source

+   <span>resource-specific fetch error</span>, then <span>queue a task</span> on the
+   <span>DOM manipulation task source</span> to <span data-x="concept-event-fire">fire an
+   event</span> named <code data-x="event-error">error</code> at the <code>style</code> element, and
+   abort these steps.</p></li>


It'd be nice to abstract this error handling. Seems reasonable as a follow-up. The less duplication the better imo.

annevk · 2018-03-16T09:18:48Z

source

-  <p>The <span>task source</span> for these <span data-x="concept-task">tasks</span> is the <span>DOM
-  manipulation task source</span>.</p>
+   <li><p>Once all of the <span data-x="critical subresources">critical subresource</span> fetches
+   have completed (i.e. have reached the <span>process response done</span> phase), <span>queue a


annevk · 2018-03-16T09:19:18Z

source

-  <span>same origin</span> as the <span>URL</span> of the external resource<!-- CVE-2010-0654 -->,
-  and the <span data-x="Content-Type">Content-Type metadata</span> of the external resource is not a
-  supported style sheet type, the user agent must instead assume it to be <code>text/css</code>.</p>
+  <p>For style sheet resources, a <span data-x="concept-response">response</span>


CSS style sheet*

@imported

* Defines the critical subresources for CSS as @imported style sheets, including indirectly imported ones. This was previously defined nowhere; probably it should live in CSS, but for now it lives here. * Makes it clearer that the steps to obtain the resource are the default steps, which some link types override. * Adds explicit steps for fetching critical subresources, and ties those to the load/error events. * Uses fetch primitives like network error and ok status instead of vague language. * Makes it clearer how incorrect Content-Types for CSS generate error events, by defining the new "resource-specific fetch error" concept. * Makes it clearer that resources apply if and only if their main fetch succeeds. (Critical subresource fetches generate error events, but do not impact the "obtained successfully" or "apply" concepts.) Fixes #3532.

domenic · 2018-04-04T21:10:42Z

@annevk ready for another pass

annevk · 2018-04-05T07:04:54Z

source

+
+  <p>For CSS style sheets, we tentatively define here that their critical subresources are other
+  style sheets imported via <code data-x="">@import</code> rules, including those indirectly
+  imported by other imported stylesheets.</p>


style sheets*

annevk · 2018-04-05T07:05:33Z

source

+  <p class="&#x0058;&#x0058;&#x0058;">This definition is not fully interoperable; furthermore, some
+  user agents seem to count resources like background images or web fonts as critical subresources.
+  Ideally, the CSS Working Group should define this; see <a
+  href="https://github.com/w3c/csswg-drafts/issues/1088">w3c/csswg-drafts</a> to track progress on


Add 1088 to the link text in case we end up linking to more of their issues?

annevk · 2018-04-05T07:07:59Z

source

+     status</span>, or that has a <span>resource-specific fetch error</span>, then <span>queue a
+     task</span> using the <span>DOM manipulation task source</span> to <span
+     data-x="concept-event-fire">fire an event</span> named <code data-x="event-error">error</code>
+     at the <code>link</code> element, and abort these steps.</p></li>


I think the main problem here is that you queue another task. If something ran before that task but after the networking event it could observe sheet being non-null. I don't think that's reality in implementations.

Ah, right, good catch; I'll remove the second task queuing.

annevk · 2018-04-05T07:08:26Z

source

+     subresource</span> fetches have completed (i.e., have reached the <span>process response
+     done</span> task), <span>queue a task</span> using the <span>DOM manipulation task
+     source</span> to <span data-x="concept-event-fire">fire an event</span> named <code
+     data-x="event-load">load</code> at the <code>link</code> element.</p></li>


annevk · 2018-04-05T07:09:53Z

source

+   <li><p>If <var>response</var>'s <span data-x="Content-Type">Content-Type metadata</span> is
+   <code>text/css</code>, return false.</p></li>
+
+   <li><p>If the document has been set to <span>quirks mode</span>, and <var>response</var>'s <span


Where does this document come from?

Jeez, now we have a can of worms regarding the question of what happens if you move between a quirks and non-quirks document while the response is coming in. Please hold while I write some tests...

At least in Gecko, I believe anything that moves the <link> in the DOM drops the old request and starts a new one. Certainly if the move is across documents. The document affects not only the MIME type bit but also the encoding you have to use to decode the bytes, various security properties, etc.

Yeah, in the processing of the response it looks like there's a similar check in the spec already, so maybe it's already accounted for.

OK, so I wrote up some tests. web-platform-tests/wpt#10350

I think the best model is the one depicted in those tests, where you get an error or load event based on whether the document you were in at the time the fetch started was quirks or no-quirks. Do you agree, @bzbarsky? Firefox doesn't pass all of the tests; in particular for the last test it hits the "load event must not be fired yet" assert.

I would have to pretty carefully dig though exactly what Gecko does here, but in terms of expected behavior... I actually don't know what the behavior should be if loads are canceled before completing.

Furthermore, I'm not sure that any spec says that any loads should start in this case before the quirksDocument.body.appendChild(link) call...

The thing is, nothing in the spec suggests loads should be canceled. Just that, once the load finishes, the stylesheet should not be applied, if another load has been started. So the tests above support firing the appropriate error/load event regardless if another load has been started.

Furthermore, I'm not sure that any spec says that any loads should start in this case before the quirksDocument.body.appendChild(link) call...

Well, before that we have noQuirksDocument.body.appendChild(link);, which is one of the relevant mutations which would cause a load, right? It becomes browsing-context connected.

It becomes browsing-context connected.

Ah, looks like this is much better-defined than it used to be.

That said, nothing in the spec suggests the loads should be canceled, but that might be a bug in the spec. What do UAs do? I think it would be pretty surprising for web developers to have these racing loads and be getting a "load" event when the sheet is not ready to be applied yet!

This will also fix #3610

domenic · 2018-04-06T22:31:26Z

@bzbarsky, also, given the discussion in w3c/csswg-drafts#1088, I wonder if we should change course and just get rid of the "critical subresources" concept entirely? At least for CSS? It certainly would be simpler.

bzbarsky · 2018-04-07T02:59:19Z

and just get rid of the "critical subresources" concept entirely?

What would that mean in terms of observable behavior? What do authors want to happen?

domenic · 2018-04-09T18:59:43Z

Sorry, upon re-reviewing the thread, let's scratch that suggestion. If everyone interoperably delays the load event for at least top-level @imports, we probably shouldn't move away from that.

bzbarsky · 2018-04-09T19:44:32Z

Note that the things that browsers delay for and the things that might lead to error instead of load may not be the same set.

That is, I expect even browsers that don't let non-toplevel imports make the load error out still wait for them before firing load... Testcase:

<!DOCTYPE html>
<script>
  var start = performance.now();
</script>
<link rel="stylesheet"
      href="data:text/css,@import url(&quot;data:text/css,@import url('http://software.hixie.ch/utilities/cgi/test-tools/delayed-file?pause=4&mime=text%2Fhtml&text=NOT+A+STYLESHEET');&quot;)"
      onload="alert('loaded in: ' + (performance.now() - start))"
      onerror="alert('errored in: ' + (performance.now() - start))">

and that shows "loaded in" followed by a 4+s time in Chrome. So it's waiting for that innermost import, but then ignoring that it failed.

domenic · 2018-04-09T21:16:12Z

It's amazing what you can inline into a single-file test case...

To me a reasonable model would be that error events only fire if the top-level sheet itself fails to come over the network. Any ideas if that is consistent with browsers at all? (I can write tests if you haven't already.) It's certainly not allowed by the current spec, which says that if any critical subresource fails, you must fire error and must not fire load.

It does seem like this whole area needs a more comprehensive test suite. I guess I'm up for taking over that enterprise. You have some tests already written, right? Can you point me to them?

bzbarsky · 2018-04-09T21:25:42Z

It's amazing what you can inline into a single-file test case...

Yeah... arguably there's all sorts of violations of same-origin policy in there. I wonder whether that affects anyone's behavior...

You have some tests already written, right? Can you point me to them?

Sure. There's https://searchfox.org/mozilla-central/source/layout/style/test/test_bug1443344-2.html and https://searchfox.org/mozilla-central/source/layout/style/test/test_bug1443344-1.html (not testharness tests, but should be pretty self-explanatory and not too bad to port to testharness).

There's also https://searchfox.org/mozilla-central/source/layout/style/test/test_load_events_on_stylesheets.html which I modified in the course of making the changes in https://bugzilla.mozilla.org/show_bug.cgi?id=1442126

Plus the bits I already posted in #3532

gsnedders · 2018-05-22T15:55:55Z

@domenic web-platform-tests/wpt#5525 also has some tests for this

* Introduce new link loading infrastructure, specifically algorithms to fetch and process linked resources that are overridable by the individual link types. This replaces the previous "obtain the resource" algorithms with something more structured and rigorous. * Specify the order in which style sheets are removed, created/added, their corresponding load/error events are fired, and scripting is unblocked from a loading style sheet. * Replace "style sheet done" flag with Document's "script-blocking style sheet counter", which works in tandem with the "contributes a script-blocking style sheet" conditions, which replaces the definition of "a style sheet that is blocking scripts". * Change <link> load/fire events to be fired from tasks queued on the networking task source, instead of the DOM manipulation task source. * Handle much of what #3544 set out to do, minus defining and explicitly fetching a style sheet's critical subresources. Fixes #3610. Tests: web-platform-tests/wpt#14899

* Introduce new link loading infrastructure, specifically algorithms to fetch and process linked resources that are overridable by the individual link types. This replaces the previous "obtain the resource" algorithms with something more structured and rigorous. * Specify the order in which style sheets are removed, created/added, their corresponding load/error events are fired, and scripting is unblocked from a loading style sheet. * Replace "style sheet done" flag with Document's "script-blocking style sheet counter", which works in tandem with the "contributes a script-blocking style sheet" conditions, which replaces the definition of "a style sheet that is blocking scripts". * Change <link> load/fire events to be fired from tasks queued on the networking task source, instead of the DOM manipulation task source. * Handle much of what whatwg#3544 set out to do, minus defining and explicitly fetching a style sheet's critical subresources. Fixes whatwg#3610. Tests: web-platform-tests/wpt#14899

annevk · 2020-05-30T16:21:09Z

Is this obsolete @domenic?

domenic · 2020-06-01T14:29:12Z

Yeah, it looks like @domfarolino's work has done pretty much everything this was hoping to accomplish.

There's still some messiness around stylesheet links, e.g. I think

Quirk: If the document has been set to quirks mode, has the same origin as the URL of the external resource, and the Content-Type metadata of the external resource is not a supported style sheet type, the user agent must instead assume it to be text/css.

should be integrated into the processing model (like this PR tried to do), and of course we have the confusion with CSSOM and HTML both attempting to fetch the stylesheet. But this seems reasonable for now.

domenic added topic: link topic: style labels Mar 7, 2018

domenic requested a review from bzbarsky March 7, 2018 07:34

domenic mentioned this pull request Mar 7, 2018

Something needs to define the actual behavior of load/error events on <link> #3532

Open

annevk reviewed Mar 16, 2018

View reviewed changes

domenic mentioned this pull request Apr 4, 2018

Define which subresources block the DOM load event w3c/csswg-drafts#1088

Open

Address most feedback

50c4168

domenic force-pushed the link-loading-clearer branch from a4ccf1c to 50c4168 Compare April 4, 2018 21:10

annevk requested changes Apr 5, 2018

View reviewed changes

annevk mentioned this pull request Apr 5, 2018

Consider changing task source for link "error"/"load" events #3610

Closed

Fix the easy things

0448bd2

This will also fix #3610

domenic mentioned this pull request Apr 6, 2018

WIP tests for text/css link quirk web-platform-tests/wpt#10350

Open

Pass through and check the specific document

f61f9e0

domfarolino mentioned this pull request Sep 13, 2018

Specify when style sheets are no longer script-blocking #4031

Merged

hiroshige-g mentioned this pull request Oct 4, 2018

Need to define what fetch groups various loads from the sheet go in WICG/construct-stylesheets#15

Closed

domenic closed this Jun 1, 2020

domenic deleted the link-loading-clearer branch June 1, 2020 14:29

Improve clarity in link and style sheet resource fetching #3544

Improve clarity in link and style sheet resource fetching #3544

Conversation

domenic commented Mar 7, 2018 • edited by pr-preview bot Loading

domenic commented Mar 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domenic commented Apr 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domenic commented Apr 6, 2018

bzbarsky commented Apr 7, 2018

domenic commented Apr 9, 2018

bzbarsky commented Apr 9, 2018

domenic commented Apr 9, 2018

bzbarsky commented Apr 9, 2018

gsnedders commented May 22, 2018

annevk commented May 30, 2020

domenic commented Jun 1, 2020

domenic commented Mar 7, 2018 •

edited by pr-preview bot

Loading