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

Use DOM's post-connection steps for <script> elements #10188

Merged
merged 14 commits into from
Aug 29, 2024

Conversation

domfarolino
Copy link
Member

@domfarolino domfarolino commented Mar 8, 2024

This PR (part 1 of probably 2) uses the newly-introduced DOM Standard "post-connection steps" (see whatwg/dom#1261), 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. This PR effectively supersedes a portion of #4354.

Before this PR, 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 Firefox. We push script execution back to the post-connection steps, which run after all DOM insertions are complete. This gives two notable observables differences from the old spec:

  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 this PR, doing script.append(new Text("..."), new Text("..."), ...) would "prepare" and "execute" the script synchronously after the first text node was inserted, because HTML says that whenever a child element is inserted to a connected script element, that's when the script gets prepared. All subsequent text nodes that are appended as children would come after the script has already started, so they would not execute. What I think the standard really means though, is to count on the script's "children changed steps" that DOM offers. After this PR, 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. Therefore, execution would include all text that was atomically added to the script. This is tested here; Safari is the odd one out (I guess because it's not using the "children changed steps" properly?)
  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). See this concrete example I added in the spec. This has a WPT too, but it assumes a blend of the old a new models (which forced nested script execution before parent script execution). I'm updating it to match this spec, in DOM: Correct script-in-script atomic move WPT expectations web-platform-tests/wpt#45094.

(See WHATWG Working Mode: Changes for more details.)


/infrastructure.html ( diff )
/scripting.html ( diff )

@domfarolino domfarolino marked this pull request as ready for review March 8, 2024 19:48
@domfarolino domfarolino requested a review from annevk March 8, 2024 19:48
source Show resolved Hide resolved
source Show resolved Hide resolved
@domfarolino
Copy link
Member Author

/cc @noamr @mfreed7

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Mar 13, 2024
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

R=nrosenthal@chromium.org

Bug: 40150299
Change-Id: I750ccee0a2be834360c557042e975547c8ddd32c
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Mar 13, 2024
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

R=nrosenthal@chromium.org

Bug: 40150299
Change-Id: I750ccee0a2be834360c557042e975547c8ddd32c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5367238
Reviewed-by: Noam Rosenthal <nrosenthal@chromium.org>
Commit-Queue: Dominic Farolino <dom@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1272330}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Mar 13, 2024
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

R=nrosenthal@chromium.org

Bug: 40150299
Change-Id: I750ccee0a2be834360c557042e975547c8ddd32c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5367238
Reviewed-by: Noam Rosenthal <nrosenthal@chromium.org>
Commit-Queue: Dominic Farolino <dom@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1272330}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Mar 13, 2024
This CL corrects the script-in-script insertion test, to assert that
when an "outer" script element (that has not yet been prepared/executed)
gets an "inner" script element inserted into it, the outer script runs
before the inner one.

This is a break in what the HTML Standard previously required, before
whatwg/html#10188.

R=nrosenthal@chromium.org

Bug: 40150299
Change-Id: If8cdce18a13678ac0646d81bc14850b2f26b6eb7
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Mar 13, 2024
This CL corrects the script-in-script insertion test, to assert that
when an "outer" script element (that has not yet been prepared/executed)
gets an "inner" script element inserted into it, the outer script runs
before the inner one.

This is a break in what the HTML Standard previously required, before
whatwg/html#10188.

R=nrosenthal@chromium.org

Bug: 40150299
Change-Id: If8cdce18a13678ac0646d81bc14850b2f26b6eb7
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Mar 14, 2024
This CL corrects the script-in-script insertion test, to assert that
when an "outer" script element (that has not yet been prepared/executed)
gets an "inner" script element inserted into it, the outer script runs
before the inner one.

This is a break in what the HTML Standard previously required, before
whatwg/html#10188.

R=nrosenthal@chromium.org

Bug: 40150299
Change-Id: If8cdce18a13678ac0646d81bc14850b2f26b6eb7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5367667
Reviewed-by: Noam Rosenthal <nrosenthal@chromium.org>
Commit-Queue: Dominic Farolino <dom@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1272720}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Mar 14, 2024
This CL corrects the script-in-script insertion test, to assert that
when an "outer" script element (that has not yet been prepared/executed)
gets an "inner" script element inserted into it, the outer script runs
before the inner one.

This is a break in what the HTML Standard previously required, before
whatwg/html#10188.

R=nrosenthal@chromium.org

Bug: 40150299
Change-Id: If8cdce18a13678ac0646d81bc14850b2f26b6eb7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5367667
Reviewed-by: Noam Rosenthal <nrosenthal@chromium.org>
Commit-Queue: Dominic Farolino <dom@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1272720}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Mar 15, 2024
…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

R=nrosenthal@chromium.org

Bug: 40150299
Change-Id: I750ccee0a2be834360c557042e975547c8ddd32c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5367238
Reviewed-by: Noam Rosenthal <nrosenthal@chromium.org>
Commit-Queue: Dominic Farolino <dom@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1272330}

--

wpt-commits: cc99c62762135f7e193941e32c3f738960c256be
wpt-pr: 45085
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Mar 15, 2024
…e WPT expectations, a=testonly

Automatic update from web-platform-tests
DOM: Correct script-in-script atomic move WPT expectations

This CL corrects the script-in-script insertion test, to assert that
when an "outer" script element (that has not yet been prepared/executed)
gets an "inner" script element inserted into it, the outer script runs
before the inner one.

This is a break in what the HTML Standard previously required, before
whatwg/html#10188.

R=nrosenthal@chromium.org

Bug: 40150299
Change-Id: If8cdce18a13678ac0646d81bc14850b2f26b6eb7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5367667
Reviewed-by: Noam Rosenthal <nrosenthal@chromium.org>
Commit-Queue: Dominic Farolino <dom@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1272720}

--

wpt-commits: e25a0c84dac911c78265c142ab9e178e3936fee2
wpt-pr: 45094
jamienicol pushed a commit to jamienicol/gecko that referenced this pull request Mar 17, 2024
…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

R=nrosenthal@chromium.org

Bug: 40150299
Change-Id: I750ccee0a2be834360c557042e975547c8ddd32c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5367238
Reviewed-by: Noam Rosenthal <nrosenthal@chromium.org>
Commit-Queue: Dominic Farolino <dom@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1272330}

--

wpt-commits: cc99c62762135f7e193941e32c3f738960c256be
wpt-pr: 45085
jamienicol pushed a commit to jamienicol/gecko that referenced this pull request Mar 17, 2024
…e WPT expectations, a=testonly

Automatic update from web-platform-tests
DOM: Correct script-in-script atomic move WPT expectations

This CL corrects the script-in-script insertion test, to assert that
when an "outer" script element (that has not yet been prepared/executed)
gets an "inner" script element inserted into it, the outer script runs
before the inner one.

This is a break in what the HTML Standard previously required, before
whatwg/html#10188.

R=nrosenthal@chromium.org

Bug: 40150299
Change-Id: If8cdce18a13678ac0646d81bc14850b2f26b6eb7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5367667
Reviewed-by: Noam Rosenthal <nrosenthal@chromium.org>
Commit-Queue: Dominic Farolino <dom@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1272720}

--

wpt-commits: e25a0c84dac911c78265c142ab9e178e3936fee2
wpt-pr: 45094
BruceDai pushed a commit to BruceDai/wpt that referenced this pull request Mar 25, 2024
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

R=nrosenthal@chromium.org

Bug: 40150299
Change-Id: I750ccee0a2be834360c557042e975547c8ddd32c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5367238
Reviewed-by: Noam Rosenthal <nrosenthal@chromium.org>
Commit-Queue: Dominic Farolino <dom@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1272330}
BruceDai pushed a commit to BruceDai/wpt that referenced this pull request Mar 25, 2024
This CL corrects the script-in-script insertion test, to assert that
when an "outer" script element (that has not yet been prepared/executed)
gets an "inner" script element inserted into it, the outer script runs
before the inner one.

This is a break in what the HTML Standard previously required, before
whatwg/html#10188.

R=nrosenthal@chromium.org

Bug: 40150299
Change-Id: If8cdce18a13678ac0646d81bc14850b2f26b6eb7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5367667
Reviewed-by: Noam Rosenthal <nrosenthal@chromium.org>
Commit-Queue: Dominic Farolino <dom@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1272720}
source Show resolved Hide resolved
source Show resolved Hide resolved
@domfarolino
Copy link
Member Author

@annevk @smaug---- could you take a look please?

@domfarolino domfarolino changed the title Use DOM's post-insertion steps for <script> elements Use DOM's post-connection steps for <script> elements Jul 12, 2024
source Show resolved Hide resolved
source Show resolved Hide resolved
@domfarolino domfarolino requested a review from annevk July 24, 2024 14:16
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.

Looks good modulo a question about the src attribute change steps. @smaug---- please review now if you still want to review. @domenic do you want to have a look as well since this touches script?

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.

Huge improvement! But yeah, let's clean up the src attribute change handling, since everything else is so nice.

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
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Aug 26, 2024
See this HTML Standard PR thread:
whatwg/html#10188 (comment).

R=domenic

Bug: N/A
Change-Id: I31065f76e324f71d2b4ae52813b3b37d3b4467c0
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Aug 26, 2024
See this HTML Standard PR thread:
whatwg/html#10188 (comment).

R=domenic

Bug: N/A
Change-Id: I31065f76e324f71d2b4ae52813b3b37d3b4467c0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5809759
Reviewed-by: Domenic Denicola <domenic@chromium.org>
Commit-Queue: Dominic Farolino <dom@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1346661}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Aug 26, 2024
See this HTML Standard PR thread:
whatwg/html#10188 (comment).

R=domenic

Bug: N/A
Change-Id: I31065f76e324f71d2b4ae52813b3b37d3b4467c0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5809759
Reviewed-by: Domenic Denicola <domenic@chromium.org>
Commit-Queue: Dominic Farolino <dom@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1346661}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Aug 28, 2024
…ger preparation, a=testonly

Automatic update from web-platform-tests
WPT: Ensure `src` attribute changes trigger preparation

See this HTML Standard PR thread:
whatwg/html#10188 (comment).

R=domenic

Bug: N/A
Change-Id: I31065f76e324f71d2b4ae52813b3b37d3b4467c0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5809759
Reviewed-by: Domenic Denicola <domenic@chromium.org>
Commit-Queue: Dominic Farolino <dom@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1346661}

--

wpt-commits: cab88353d0991229e6d81f8fb6b0862fe9495de8
wpt-pr: 47782
ErichDonGubler pushed a commit to erichdongubler-mozilla/firefox that referenced this pull request Aug 29, 2024
…ger preparation, a=testonly

Automatic update from web-platform-tests
WPT: Ensure `src` attribute changes trigger preparation

See this HTML Standard PR thread:
whatwg/html#10188 (comment).

R=domenic

Bug: N/A
Change-Id: I31065f76e324f71d2b4ae52813b3b37d3b4467c0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5809759
Reviewed-by: Domenic Denicola <domenic@chromium.org>
Commit-Queue: Dominic Farolino <dom@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1346661}

--

wpt-commits: cab88353d0991229e6d81f8fb6b0862fe9495de8
wpt-pr: 47782
i3roly pushed a commit to i3roly/firefox-dynasty that referenced this pull request Aug 29, 2024
…ger preparation, a=testonly

Automatic update from web-platform-tests
WPT: Ensure `src` attribute changes trigger preparation

See this HTML Standard PR thread:
whatwg/html#10188 (comment).

R=domenic

Bug: N/A
Change-Id: I31065f76e324f71d2b4ae52813b3b37d3b4467c0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5809759
Reviewed-by: Domenic Denicola <domenic@chromium.org>
Commit-Queue: Dominic Farolino <dom@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1346661}

--

wpt-commits: cab88353d0991229e6d81f8fb6b0862fe9495de8
wpt-pr: 47782
@domenic domenic merged commit ddd2d0d into main Aug 29, 2024
2 checks passed
@domenic domenic deleted the domfarolino/post-insertion-steps branch August 29, 2024 19:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants