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

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

Merged
merged 18 commits into from
Jan 17, 2019

Conversation

domfarolino
Copy link
Member

@domfarolino domfarolino commented Sep 13, 2018

Rough (very WIP) Attempt at specifying when exactly a style sheet no longer blocks scripts. Introduces style sheet done flag on style sheets in this standard, in place of the style sheet ready flag which was previously never used or set, and IMO is nominally indicative of a style sheet being successfully obtained (not ideal, since even style sheets that have failed to load should no longer block scripts).

This is WIP is because I don't even use the style sheet done flag in the context of <link>s yet, due to the underlying infrastructure. The setting of the style sheet done flag can happen in one of two places for <link>s (+ implicitly Link headers).

I was going for the former, but we can't do that because the corresponding CSS style sheet does not get created until after this algorithm runs, and we drop into the more specific Link type "stylesheet" section saying "Once the resource has been obtained......create the style sheet". We can't set the flag in that section either, because setting the flag should be part of the task that gets queued when a style sheet is obtained if we want to specify that these events must be fired before blocked scripts execute (see #4020 & #3610).

With this, I think there are at least two reasonable options forward. The Link type "stylesheet" section can provide its own specific "obtain" algorithm, which:

  1. Creates a CSS style sheet (same properties as now)
  2. Defers to the general link obtain algorithm
    • This would can say something like "...when obtained, queue a task that fires(...), and also sets the associated style sheet's style sheet done flag (if applicable)
  3. Waits for the general obtain algorithm to finish asynchronously, continuing to do what it does now

Or maybe define something like "custom task steps", that specific link types could pass to the general obtain algorithm, which would queue tasks to fire the appropriate event, and run the "custom task steps", if provided (not sure if it's worth the churn though).

I'm happy to make a follow-up that takes care of points 1 and 2 from #3532 (comment) to harden some of the vague language. Seems like the notion of critical subresources will need more work though, since it seems there isn't much interop IIRC.


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

@domfarolino domfarolino added do not merge yet Pull request must not be merged per rationale in comment needs tests Moving the issue forward requires someone to write tests interop Implementations are not interoperable with each other topic: events labels Sep 13, 2018
@annevk
Copy link
Member

annevk commented Sep 21, 2018

FWIW, I think it's fine to fix the things we can fix without doing a complete overhaul, especially if the overhaul isn't moving forward.

@domfarolino domfarolino changed the title [WIP]: Specify when style sheets are no longer script-blocking Specify when style sheets are no longer script-blocking Nov 25, 2018
source Show resolved Hide resolved
source Show resolved Hide resolved
@domfarolino
Copy link
Member Author

Would appreciate some feedback on the direction this is going @annevk. As it stands, this PR attempts to specify:

Building off of #696 (comment), with this PR I think the order of loading <link> style sheets would be as such:

  1. Create the CSS style sheet
    • It should not be available to scripts in any way yet though
  2. Obtain the style sheet
  3. Queue a microtask to:
    • Fire a load/error event at the link element
    • Set the associated style sheet's "done" flag, if there is an assoc ss
  4. (Somehow magically use the response body to create CSS rules for the style sheet)
  5. "Add" the style sheet, making it available via document.styleSheets, element.sheet, and I think scripts in general (via its rules)
    .....
  6. Finally later the microtask is queued, and the style sheet no longer blocks scripts

@annevk
Copy link
Member

annevk commented Nov 26, 2018

I think what you want is roughly this:

  1. In parallel:
    1. Fetch the style sheet.
    2. Parse the style sheet.
    3. Queue a task to:
      1. Create "main thread" data structures, add them to the appropriate places.
      2. Set the done flag.
      3. Fire an event.

Hope that helps. Microtasks aren't the right fit here, since the work is happening "in parallel".

@annevk
Copy link
Member

annevk commented Nov 26, 2018

(Let me know if you still need detailed feedback on the existing PR, given the above.)

@domfarolino
Copy link
Member Author

@annevk Ping for review /cc @domenic as well since it is somewhat related to #3544 (I plan on breaking that PR into stuff that can be merged now, and stuff that needs more work (critical subresource hairiness etc).

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not so sure about this strategy. First, I don't understand why you say

in place of the style sheet ready flag which was previously never used or set

From what I can tell from the diff, you basically replaced "style sheet ready flag" with "style sheet done flag". "ready flag" was used in the same place (line 15187) that "done flag" is used after your changes. And although the PR appears to set the done flag more explicitly, the previous version was also OK-ish; it said "When a style sheet is ready to be applied, its style sheet ready flag must be set" at the start of a paragraph which, arguably, defines when a style sheet is ready to be applied.

Next, I have the model-related concerns, mentioned in the review, about the overriding of link obtain, and how CSSOM seems to assume that "CSS style sheets" always get created-and-added together, not separately.

Finally, it seems like a lot of the work here is coming from an additional constraint you've placed on yourself, that the "style sheet done flag" must exist on the CSSOM "CSS style sheet" object. I don't think that's a necessary constraint, and indeed is probably inaccurate for XSL stylesheets (which are generally poorly specced). It would probably be a lot simpler to add the flag to style and link elements themselves, since that's what the algorithm "a style sheet that is blocking scripts" refers to. Or, perhaps better for future work on Link headers and XSL stylesheets, just add a set, or counter, or something to the Document object.

I'd be curious how this is implemented in browsers, and if there are lessons we could learn there. How do they track blocking stylesheets? Do they use a counter on Document; do they put somethign on link/style elements; do they put it on CSSOM CSS style sheet-equivalents?

source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
@domfarolino
Copy link
Member Author

We talked on IRC about changing/refining directions a bit, so just for the record here, I'll also respond to a few points on this thread before pushing changes up later.

From what I can tell from the diff, you basically replaced "style sheet ready flag" with "style sheet done flag". "ready flag" ..... And although the PR appears to set the done flag more explicitly, the previous version was also OK-ish;

It is mostly a name change and more explicit "setting", yeah (original comment here), with the intention of more-clearly specifying the ordering of the error/load event and when scripting is unblocked.

and how CSSOM seems to assume that "CSS style sheets" always get created-and-added together, not separately.

I agree; it seems my first go at this rather weakly ensures a created style sheet is only added/available to scripts after the error/load event fires, stemming from an assumption WRT when these steps run in relation to the task queued in the para under concept-link-obtain, but that will be cleaned up!

or counter, or something to the Document object.

ACK. As discussed, a counter seems like a nice way to do it and bring some simplicity, and happens to be how Chromium implements this.

Copy link
Member Author

@domfarolino domfarolino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ping for re-review (with some questions attached!) If the direction is looking good, I can then sum up normative changes and necessary browser bugs.

Mostly it revolves around the ordering of scripting becoming unblocked on styles being obtained, and the load event being fired (simple test here), as well as when styles fetched from a <link> are no longer available to scripts when a new sheet is to be fetched from the same <link> (simple test here).

source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for cleaning this up, I left some initial comments on the upper half of the PR. The main problem I see is that MIME types might not be checked at the appropriate time. It seems at some point you need that to make a decision and start the CSS parser, before you can even talk about @import rules and such.

source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved

<li><p>If <var>success</var> is true, <span data-x="concept-fetch">fetch</span> the
<span>critical subresources</span> of the <span data-x="external resource link">link
resource</span>.</p></li>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps we should simply wait for them to be fetched? Ideally CSS defines the fetching.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, we shouldn't fetch critical subresources if the MIME type was wrong or some such.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think for now just adding a warning that this is underdefined is fine. This is at least as good as the current text, I believe?

source Show resolved Hide resolved
source Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly looks good, just a bit more to do! Heroic work! Let me know if there were any questions not yet answered.

source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Getting there!

source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
@@ -24655,97 +24796,132 @@ document.body.appendChild(wbr);</code></pre>

<ol>

<li><p>Let <var>element</var> be the <code>link</code> element that created the
<span>external resource link</span>.</p></li>
<li><p>If the resource's <span data-x="Content-Type">Content-Type metadata</span> is not
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A follow-up could improve this by inspecting response and incorporating the quirk mentioned above directly into this step. But, this is fine for now.

source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
… "Create a CSS style sheet" in link-type-stylesheet
@domfarolino
Copy link
Member Author

Ok I think I addressed everything :) Thanks for the heavy review churn here. Perhaps we can make this followup into a good first issue?

Latest commit here moves the link style sheet event firing after the ss gets "created", and guards both the creation and load firing with a success==true check.

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Although as noted in IRC I think you could clean this up by moving the "queue a task" into the "fetch and process" algorithm, so that when link types write their "process this type of linked resource" algorithm they don't all need to wrap most of their steps in a "Queue a task".

source Show resolved Hide resolved
source Outdated Show resolved Hide resolved
@domfarolino domfarolino removed do not merge yet Pull request must not be merged per rationale in comment needs tests Moving the issue forward requires someone to write tests labels Jan 16, 2019
@domenic
Copy link
Member

domenic commented Jan 16, 2019

@domfarolino do you want to try to write a commit message that summarizes all the great work here? One that tries to give a sense of both normative changes and editorial ones, and also points to any followup or related issues (such as my PR which this mostly replaces).

Looks like tests are in progress over at web-platform-tests/wpt#14899, so this looks ready to land to me!

@domfarolino
Copy link
Member Author

domfarolino commented Jan 16, 2019

Sounds good I will get on it!


Tests: web-platform-tests/wpt#14899. It might be worth testing the DOM manipulation task source => networking task source change a bit more however...

Bugs:

@domenic
Copy link
Member

domenic commented Jan 16, 2019

Task source changes seem almost untestable to me; I'd be curious if you come up with something. Maybe stochastic tests, but nobody likes putting those on their bots :-/.

@domfarolino
Copy link
Member Author

Commit message:

Specify when style sheets no longer block scripts

 * Introduces new link loading infrastructure, specifically algorithms
   to fetch and process linked resources that are overridable by the
   individual link types
 * 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"
 * <link> load/fire events are fired from tasks queued on the networking
   task source
 * Handles much of what #3544 set out to do, minus defining and
   explicitly fetching a style sheet's critical subresources

Fixes #3610. 

Tests: https:/web-platform-tests/wpt/pull/14899

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
interop Implementations are not interoperable with each other topic: events topic: link topic: style
Development

Successfully merging this pull request may close these issues.

3 participants