-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Use the new definition of the insertion steps #4354
Conversation
source
Outdated
<span data-x="nodes are inserted">inserted</span> into the <code>script</code> element, after any | ||
<code>script</code> elements <span data-x="nodes are inserted">inserted</span> at that time.</li> | ||
<li><p><span>Enqueue</span> the steps to <span data-x="prepare a script">prepare</span> | ||
<var>script</var> to <var>deferredStepsQueue</var>.</p></li> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if another script has removed this script meanwhile so it's no longer connected? I thought we had to split script execution into a preparation step that had to happen directly, and then an execution step that's deferred.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Step 6 of prepare a script is "If the element is not connected, then return. The script is not executed."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would match Chrome, but not Firefox, per whatwg/dom#575 (comment). That might be okay, but I'll double check if there's test coverage.
So I found two main differences between Chrome and Firefox:
My own opinion is that Chrome is more logical on both accounts, i.e. it seems better to me to always create stylesheets before executing scripts, and it seems better to me to always execute scripts in tree order. |
Just to be clear, we're talking about inserting a DOM subtree that has both Why does doing that make more sense than just doing the whole thing in tree order?
I really don't have time to dig through the whole WPT PR, unfortunately. Can you please point to a specific testcase (ideally on http://software.hixie.ch/utilities/js/live-dom-viewer/ so it can just be run easily) that shows this behavior difference? |
Because all other insertion steps are done before executing scripts, except style sheet creation in Firefox. I don't see why style sheet creation should be postponed like scripts.
Sure: |
The link for "stylesheets created before scripts are executed" was wrong, here is the correct one. I fixed it in my comment too. |
I don't believe that's true in Firefox, at least. There's all sorts of stuff that gets run at a "safe point" once insertion is complete, in tree order. Generally stuff that can cause script to execute when it happens. That includes stylesheet creation, script executions, custom element creation callbacks, changes to fullscreen state, changes to what ids are targeting, focus/blur events, etc, etc. See https://searchfox.org/mozilla-central/search?q=AddScriptRunner&case=true for the places that work that way. I'm not sure whether the spec has this concept of "run at a safe point" (apart from very async things like stable state), but my impression was that part of the question around insertion steps was whethere there should be such a concept and what should use it. Thank you, that's very helpful. So what happens there in Gecko (and I am open to changing the behavior in this weird edge case, to be clear), is that we go and insert the kids into the parent, as an atomic uninterruptible operation. The The current spec status of this stuff is, of course, unclear and hinges on what "a node or document fragment is inserted into the script element" means in item 2 of https://html.spec.whatwg.org/multipage/scripting.html#script-processing-model:prepare-a-script-4, which is exactly what we are trying to define here. It happened to be convenient to implement that item 2 on top of the generic mutation-notification mechanism in Gecko. As I said above, I'm open to some other approach, but not that happy with the typical WebKit approach of just scattering element name checks all over the place to implement things like this (which more naturally leads to the behavior observed in Chrome in this specific edge case). |
Thank you I'll skim through that. I don't understand what you mean by "changes to what ids are targeting" though. What I meant by "insertion steps" are all the things in the HTML that uses that concept from the HTML script, such as setting form owners on form elements, setting the selectedness of options in select elements, etc. Basically all the things that are observable from JS during script execution. At the very least, Chrome seems to do stylesheet creation way before Firefox. |
(Oops submitted too early)
This is kinda what my DOM changes are trying to create with the "deferredStepsQueue" used in my new insertion algorithm in the DOM PR.
Yeah that's what I gathered, and it seems quite complex to specify to me, and Chrome does things differently: it starts with the parent script because it executes things in tree order.
Step 2 is changed in this very PR to use the "children added steps" concept I introduced in the DOM PR, specifically because I thought all browsers agreed that the parent script should be executed last, but it turns out that Chrome currently executes it first. |
I don't understand what you mean about element name checks, the changes I made to DOM and HTML don't rely on element name checks, they just make script elements' own "insertion steps" enqueue "prepare a script" to the passed "deferredStepsQueue" thing. |
When you have an idref, what element it references can change when the DOM mutates. This appears in various places in HTML; "setting form owners" is an example of this, because of the "form" attribute.
Right.
Sure. In Firefox, stylesheet creation at one point involved doing things that might run script (not page script, but extension script). Those things had to happen at a safe point. Chrome may not have had a way for extensions to hook stylesheet creation, and it's possible that Firefox may not have a way to hook it anymore either. Would need careful auditing. Anyway, as a general rule, requiring things to happen "more sync" places more constraints on how those things can work and what they're allowed to do. We just need to watch out for that.
It's not that hard to specify... You do need to specify the "stuff changed" notification system.
How does the parent script know that it needs to execute at all? And in particular, how does it know that before the child script is inserted and queues its own execution? Or are you saying that each script queues the execution on itself, not in an actual queue, and then there is a treewalk to collect up the queued-up things?
The way I see it, you have a few options for implementing this thing. You have a generic "add some child nodes" thing on Element. You can then do one of:
An ideal solution would have zero cost in the case when the parent is not a script. Firefox has more or less solution 4, but the notification is after insertion, which is why the behavior is different. Solution 2 is closest to zero cost of the remaining things, but is what I meant by "explicit name checks". That said, I looked at Blink's code just now and their execution of the parent script also comes from a "you had children inserted" notification, which also happens after the children are inserted. So it's worth asking some of the Blink folks exactly what Blink is doing here to produce the observed ordering... |
Oh, and the zero cost thing is somewhat important. We basically don't want a bunch of virtual dispatch on the fast paths here if we can avoid it... |
As far I can tell it follows the model I specified here: The insertion algorithm passes "deferredStepsQueue" to insertion steps, which are invoked on each inclusive descendants of the nodes we insert (scripts enqueue "prepare a script" in "deferredStepsQueue" here), then the "children added steps" are invoked on the parent (the script parent runs here), then each element in deferredStepsQueue is dequeued and ran, executing the children scripts. |
Ah, so the key part is that insertion doesn't immediately queue things to a "safe point" queue but to some other queue (which may effectively work to implement the safe point, maybe)? Again, I would be interested in hearing from other implementors what they actually do. |
Yes exactly! Basically my thing executes all scripts that were inserted or mutated during a DOM insertion just after this DOM insertion (so all scripts inserted or mutated by inserting a So I guess the real question is whether the behaviour I suggest is a "safe enough" point for you. :)
I would love to ping Edge or Safari people but I don't know who they are. |
I don't think there's any "extra" delaying in Firefox.
So to be clear, the way "safe point" works in Gecko is that when you start doing "unsafe" stuff you increment a counter; when you stop, you decrement it. When the counter goes to zero, that's a safe point. Generally the end of a mutation is a safe point, but not necessarily.
@rniwa and @travisleithead might know. |
So the Firefox behaviour for the insertion steps of script elements with the new insertion steps model from my DOM PR is:
This neuters any nested DOM mutation that may trigger again the insertion steps of the script element, as found out in some of the WPT tests I made for this set of change, and as described by @bzbarsky in earlier comments on this PR. I'll add tests on my WPT PR to check what happens with focused elements and whatnot. |
I couldn't come up with a scenario that implies focused elements. I found other things to test for though:
|
It turns out that neither Chrome nor Safari load iframes before scripts when inserted both at the same time. I observed that both when inserting from a div or from a document fragment. |
Only Chrome loads stylesheet links before running scripts, couldn't reproduce that in either Safari or Firefox. |
Only Safari executes scripts before handling new sources in a media element, but that's unsurprising given we already know it inserts nodes one by one when inserting from a document fragment. |
All browsers ( |
My test was wrong, it turns out that I forgot to escape a After some experimenting, I ended up finding out that |
@tkent-google Could anyone from the Chrome side of things chime in this thread, please? |
I tried to summarise the discussions here but I failed to do so, IMO every comment from @bzbarsky is important to read because that will make it clear to the Chrome side how they differ from Firefox. |
@domenic @tkent-google the problem is that HTML currently executes script elements upon insertion, but this breaks down if the script element is inserted as part of a Now given this separate queue, this makes all kinds of thing observable, as illustrated by the previous comments. So the question is what's Chrome's opinion on these various scenarios. E.g., is it a good idea that you can have a connected |
Sounds good to me but even just for |
(I also wonder if we can move to only have child node notifications and not separate Text node child notifications. And then filter for nodes that only care about Text node children changing. That seems like a slightly better and future-proof setup.) |
@tkent-google so with two scripts, script1 modifying script2, the order in Chrome is different for:
So based on that I kinda have to question "general child node notifications", no? web-platform-tests/wpt#15886 has these tests in more detail. |
HTMLScriptElement doesn't trigger script execution for child node removal and child text data change. |
Okay, so there's a general notification, but it comes with context (say a parameter), and how that context is used can be determined by looking at the code, but the rationale for it will likely remain unclear. (It's also not entirely clear to me how that code works, e.g., IsChildRemoval does not return true when type is kAllChildrenRemoved.) It makes sense to me to add that kind of children changed algorithm to DOM. It's not entirely clear to me how we decide how various elements are to use it. |
whatwg/dom#732 We defer preparing scripts and updating style blocks during insertion to make sure all DOM mutations are finished before executing scripts. This is what Chrome and Firefox seem to already do anyway.
<p>The user agent must run the <span>update a <code>style</code> block</span> algorithm whenever | ||
one of the following conditions occur:</p> | ||
<p>To run the <span data-x="concept-node-insert-ext">insertion steps</span> for a | ||
<code>style</code> element with <var>style</var> and <var>deferredStepsQueue</var>, the user |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, I'm inclined to remove this from this PR as I don't think we should use deferredStepsQueue for the style
element.
Given the old Chromium change that introduced this behavior: - https://source.chromium.org/chromium/chromium/src/+/604e798ec6ee30f44d57a5c4a44ce3dab3a871ed ... the old discussion in: - whatwg/dom#732 (review) - whatwg/html#4354 (comment) ... and the much newer discussion in: - whatwg/dom#1261 - whatwg/html#10188 This CL upstreams an old test ensuring that removal of a child node from a script node that has not "already started" [1], does not trigger script execution. Only the *insertion* of child nodes (to a non-already-started script node) should trigger that script's execution. [1]: https://html.spec.whatwg.org/C#already-started [email protected] Bug: 40150299 Change-Id: I750ccee0a2be834360c557042e975547c8ddd32c
Given the old Chromium change that introduced this behavior: - https://source.chromium.org/chromium/chromium/src/+/604e798ec6ee30f44d57a5c4a44ce3dab3a871ed ... the old discussion in: - whatwg/dom#732 (review) - whatwg/html#4354 (comment) ... and the much newer discussion in: - whatwg/dom#1261 - whatwg/html#10188 This CL upstreams an old test ensuring that removal of a child node from a script node that has not "already started" [1], does not trigger script execution. Only the *insertion* of child nodes (to a non-already-started script node) should trigger that script's execution. [1]: https://html.spec.whatwg.org/C#already-started [email protected] Bug: 40150299 Change-Id: I750ccee0a2be834360c557042e975547c8ddd32c Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5367238 Reviewed-by: Noam Rosenthal <[email protected]> Commit-Queue: Dominic Farolino <[email protected]> Cr-Commit-Position: refs/heads/main@{#1272330}
Given the old Chromium change that introduced this behavior: - https://source.chromium.org/chromium/chromium/src/+/604e798ec6ee30f44d57a5c4a44ce3dab3a871ed ... the old discussion in: - whatwg/dom#732 (review) - whatwg/html#4354 (comment) ... and the much newer discussion in: - whatwg/dom#1261 - whatwg/html#10188 This CL upstreams an old test ensuring that removal of a child node from a script node that has not "already started" [1], does not trigger script execution. Only the *insertion* of child nodes (to a non-already-started script node) should trigger that script's execution. [1]: https://html.spec.whatwg.org/C#already-started [email protected] Bug: 40150299 Change-Id: I750ccee0a2be834360c557042e975547c8ddd32c Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5367238 Reviewed-by: Noam Rosenthal <[email protected]> Commit-Queue: Dominic Farolino <[email protected]> Cr-Commit-Position: refs/heads/main@{#1272330}
…er execution, a=testonly Automatic update from web-platform-tests WPT: Script child removal does not trigger execution Given the old Chromium change that introduced this behavior: - https://source.chromium.org/chromium/chromium/src/+/604e798ec6ee30f44d57a5c4a44ce3dab3a871ed ... the old discussion in: - whatwg/dom#732 (review) - whatwg/html#4354 (comment) ... and the much newer discussion in: - whatwg/dom#1261 - whatwg/html#10188 This CL upstreams an old test ensuring that removal of a child node from a script node that has not "already started" [1], does not trigger script execution. Only the *insertion* of child nodes (to a non-already-started script node) should trigger that script's execution. [1]: https://html.spec.whatwg.org/C#already-started [email protected] Bug: 40150299 Change-Id: I750ccee0a2be834360c557042e975547c8ddd32c Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5367238 Reviewed-by: Noam Rosenthal <[email protected]> Commit-Queue: Dominic Farolino <[email protected]> Cr-Commit-Position: refs/heads/main@{#1272330} -- wpt-commits: cc99c62762135f7e193941e32c3f738960c256be wpt-pr: 45085
…er execution, a=testonly Automatic update from web-platform-tests WPT: Script child removal does not trigger execution Given the old Chromium change that introduced this behavior: - https://source.chromium.org/chromium/chromium/src/+/604e798ec6ee30f44d57a5c4a44ce3dab3a871ed ... the old discussion in: - whatwg/dom#732 (review) - whatwg/html#4354 (comment) ... and the much newer discussion in: - whatwg/dom#1261 - whatwg/html#10188 This CL upstreams an old test ensuring that removal of a child node from a script node that has not "already started" [1], does not trigger script execution. Only the *insertion* of child nodes (to a non-already-started script node) should trigger that script's execution. [1]: https://html.spec.whatwg.org/C#already-started [email protected] Bug: 40150299 Change-Id: I750ccee0a2be834360c557042e975547c8ddd32c Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5367238 Reviewed-by: Noam Rosenthal <[email protected]> Commit-Queue: Dominic Farolino <[email protected]> Cr-Commit-Position: refs/heads/main@{#1272330} -- wpt-commits: cc99c62762135f7e193941e32c3f738960c256be wpt-pr: 45085
Given the old Chromium change that introduced this behavior: - https://source.chromium.org/chromium/chromium/src/+/604e798ec6ee30f44d57a5c4a44ce3dab3a871ed ... the old discussion in: - whatwg/dom#732 (review) - whatwg/html#4354 (comment) ... and the much newer discussion in: - whatwg/dom#1261 - whatwg/html#10188 This CL upstreams an old test ensuring that removal of a child node from a script node that has not "already started" [1], does not trigger script execution. Only the *insertion* of child nodes (to a non-already-started script node) should trigger that script's execution. [1]: https://html.spec.whatwg.org/C#already-started [email protected] Bug: 40150299 Change-Id: I750ccee0a2be834360c557042e975547c8ddd32c Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5367238 Reviewed-by: Noam Rosenthal <[email protected]> Commit-Queue: Dominic Farolino <[email protected]> Cr-Commit-Position: refs/heads/main@{#1272330}
Use the newly-introduced DOM Standard "post-connection steps" (see whatwg/dom@0616094), which are run for all nodes in a batch of freshly-inserted nodes, after all DOM insertions take place. The purpose of these steps is to provide an opportunity for script executing side effects to take place during the insertion flow, but after after all DOM mutations are completed atomically. Before this, the HTML standard executed scripts during the <script> HTML element insertion steps. This means that when a batch of script elements were "atomically" inserted, each script would run synchronously after its DOM insertion and before the next DOM insertion took place. After this PR, to make progress on whatwg/dom#808 and move to a more "atomic" model where script execution only takes place after all pending DOM tree insertions happen, script execution moves to a model that more closely resembles that of Chromium and Gecko. We push script execution back to the post-connection steps, which run after all DOM insertions are complete. This gives two notable observable differences: 1. All text nodes atomically inserted as children to a script will run when their parent script executes. Imagine you have an empty parser-inserted script element. Before, doing script.append(new Text("..."), new Text("..."), ...) would "prepare" and "execute" the script synchronously after the first text node was inserted, because previously any child node insertion would cause script preparation. With this change, the execution of script is run after the entire batch of children get inserted, because the execution is tied to the "children changed steps", which run after all nodes are inserted. 2. The post-connection steps run after a parent's "children changed steps" run. This means any nested script elements inserted as children to a parent script element will run (as a result of its "post-connection steps") after the parent script gets a chance at running (as a result of its "children changed steps", which run before any post-connection steps). The new spec text has an example of this. This PR supersedes a portion of #4354.
@domfarolino the remaining work to obsolete this would be to do a similar PR to #10188 for style elements, right? Is that on your roadmap? |
@domenic I don't think so. I agree with @annevk in #4354 (review) (alluded to in whatwg/dom#1261)) in that I've left a comment on this in https://bugs.webkit.org/show_bug.cgi?id=276545#c3, but generally I think WebKit's architecture is more similar to Chromium's in this case, so I don't think there is a strong case to run style application in the post-connection steps. |
Hmm OK, then it sounds like we should close this. Let me know if I misunderstood and there's more work here that we still should work on. |
Use the newly-introduced DOM Standard "post-connection steps" (see whatwg/dom@0616094), which are run for all nodes in a batch of freshly-inserted nodes, after all DOM insertions take place. The purpose of these steps is to provide an opportunity for script executing side effects to take place during the insertion flow, but after after all DOM mutations are completed atomically. Before this, the HTML standard executed scripts during the <script> HTML element insertion steps. This means that when a batch of script elements were "atomically" inserted, each script would run synchronously after its DOM insertion and before the next DOM insertion took place. After this PR, to make progress on whatwg/dom#808 and move to a more "atomic" model where script execution only takes place after all pending DOM tree insertions happen, script execution moves to a model that more closely resembles that of Chromium and Gecko. We push script execution back to the post-connection steps, which run after all DOM insertions are complete. This gives two notable observable differences: 1. All text nodes atomically inserted as children to a script will run when their parent script executes. Imagine you have an empty parser-inserted script element. Before, doing script.append(new Text("..."), new Text("..."), ...) would "prepare" and "execute" the script synchronously after the first text node was inserted, because previously any child node insertion would cause script preparation. With this change, the execution of script is run after the entire batch of children get inserted, because the execution is tied to the "children changed steps", which run after all nodes are inserted. 2. The post-connection steps run after a parent's "children changed steps" run. This means any nested script elements inserted as children to a parent script element will run (as a result of its "post-connection steps") after the parent script gets a chance at running (as a result of its "children changed steps", which run before any post-connection steps). The new spec text has an example of this. This PR supersedes a portion of whatwg#4354.
whatwg/dom#732
We defer preparing scripts and updating style blocks during insertion to
make sure all DOM mutations are finished before executing scripts.
This is what Chrome and Firefox seem to already do anyway.
💥 Error: Wattsi server error 💥
PR Preview failed to build. (Last tried on Jan 15, 2021, 7:59 AM UTC).
More
PR Preview relies on a number of web services to run. There seems to be an issue with the following one:
🚨 Wattsi Server - Wattsi Server is the web service used to build the WHATWG HTML spec.
🔗 Related URL
If you don't have enough information above to solve the error by yourself (or to understand to which web service the error is related to, if any), please file an issue.