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

Introduce DOM post-connection steps #1261

Merged
merged 14 commits into from
Jun 5, 2024
Merged

Introduce DOM post-connection steps #1261

merged 14 commits into from
Jun 5, 2024

Conversation

domfarolino
Copy link
Member

@domfarolino domfarolino commented Feb 28, 2024

This PR introduces a node's set of post-connection steps. For any given #concept-node-insert operation, these steps run for each inserted node synchronously after all DOM insertions are complete. This PR is meant to supersede the similar #732.

The goal here is to separate the following:

  1. Script-observable but not-script-executing insertion side effects
    • This includes synchronously applying styles to the document, etc...
  2. Script-executing insertion side effects

For any given call to #concept-node-insert, the above model allows us to keep all of (1) running synchronously after each node's insertion (as part of its insertion steps), while pushing all script-executing (or DOM tree-modifying or frame tree-modifying etc.) side effects to the new set of post-connection steps, which run synchronously during insertion, but after all nodes finish their insertion.

This essentially makes insertions "atomic" from the perspective of script, since script will not run until a given batch of DOM insertions are complete. This two-stage approach aligns the spec with a model most similar to Blink & Gecko's implementation, and fixes #808. This PR also helps out with whatwg/html#1127 and #575 (per #732 (comment)).

To accomplish, this we audited all insertion side effects on the web platform in https://docs.google.com/document/d/1Fu_pgSBziVIBG4MLjorpfkLTpPD6-XI3dTVrx4CZoqY/edit#heading=h.q06t2gg4vpw, and catalogued whether they have script-observable side-effects, whether they invoke script, whether we have tests for them, and how each implementation handles them. This gave us a list of present "insertion steps" that should move to the "post-connection steps", because they invoke script and therefore prevent insertions from being "atomic". This PR is powerless without counterpart changes to HTML, which will actually use the post-connection steps for all current insertion steps that invoke script or modify the frame tree. The follow-up HTML work is tracked, at the time of writing this:

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


Two things need more discussion:

  • Script child element mutation:
  • Dealing with "removal" steps
    • In general, removal is distinct from insertion, and I think has no bearing on the decisions we make in this PR and its corresponding HTML one. However, through some offline discussion, we didn't want to commit to this insertion model without having done some investigation into the removal side of things. I've started this in this doc, but see the summary here. Basically there are only two places during Node removal where browsers ever synchronously execute script: (1) pagehide which all browsers do on same-origin iframes, and (2) blur events (only Chrome does this).
    • Chrome is committed to experimenting with not firing in either of these scenarios. Along with the deprecation of mutation events, this should mark the end of all synchronous script execution on Node removal

Preview | Diff

Footnotes

  1. The only reason WebKit fails this is because due to the lack of post-connection steps, the script executes after the first text node is appended, so by the time the second text node gets attached, the script is already started

dom.bs Outdated Show resolved Hide resolved
dom.bs Outdated Show resolved Hide resolved
dom.bs Show resolved Hide resolved
@domfarolino domfarolino marked this pull request as ready for review March 8, 2024 03:16
@domfarolino domfarolino requested a review from annevk March 8, 2024 03:16
@domfarolino
Copy link
Member Author

PTAL @annevk! I'll prepare an HTML PR and commit message for this, as well as file the appropriate browser bugs. But I'd love your thoughts on the meat of this so far, since it should be good to once those particulars are done.

dom.bs Show resolved Hide resolved
@annevk annevk requested review from rniwa and smaug---- March 13, 2024 12:58
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

[email protected]

Bug: 40150299
Change-Id: I750ccee0a2be834360c557042e975547c8ddd32c
@domfarolino
Copy link
Member Author

Just a quick note for reviewers that have already taken a look: I've just edited the OP to elaborate on the fact that all browsers never execute a script as a result of removing a child node from it. This was previously pointed out in #732 (review) and whatwg/html#4354 (comment) as a maybe-Chromium-only thing — although it appears at the time, at least Chromium & WebKit behaved this way, and as of now, all browsers agree on this. Above, I've elaborated on what follow-up work can be done to DOM/HTML to align the spec with implementations. (Like I said, I don't think it needs to be handled immediately in this PR or in whatwg/html#10188.)

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

[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}
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

[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}
@domfarolino
Copy link
Member Author

domfarolino commented Mar 14, 2024

FWIW, even though much of the prior discussion about this and #808 makes it seem like WebKit doesn't implement the post-insertion steps that this PR introduces, the only reason that Blink implements this correctly is because it inherited the code from WebKit!

See https://github.com/WebKit/WebKit/blob/main/Source/WebCore/dom/ContainerNodeAlgorithms.cpp#L54 in WebKit, where we track which elements have post-insertion steps to call later, as well as the and the equivalent code in Blink https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/dom/container_node.cc;l=1095-1096;drc=54b5f6dcc0e51751aec186771b4012ef663b538b.

Also see https://github.com/WebKit/WebKit/blob/main/Source/WebCore/dom/ContainerNode.cpp#L302-L307 in WebKit, where we run each eligible element's post-insertion steps after the children change steps, and the equivalent code in Blink https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/dom/container_node.cc;l=1071-1077;drc=9ab453c8e7d336f430c50488e8638ca802b69d80.

For script execution, WebKit even requests that post-insertion steps are used, I just don't really understand why WebKit fails tests like this then: https://wpt.fyi/results/dom/nodes/insertion-removing-steps/Node-appendChild-script-and-button-from-div.tentative.html?label=experimental&label=master&aligned. Someone like @rniwa probably knows though.

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

[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
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

[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
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

[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}
dom.bs Outdated Show resolved Hide resolved
@rniwa
Copy link
Collaborator

rniwa commented Mar 26, 2024

Also see https://github.com/WebKit/WebKit/blob/main/Source/WebCore/dom/ContainerNode.cpp#L302-L307 in WebKit, where we run each eligible element's post-insertion steps after the children change steps, and the equivalent code in Blink https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/dom/container_node.cc;l=1071-1077;drc=9ab453c8e7d336f430c50488e8638ca802b69d80.

For script execution, WebKit even requests that post-insertion steps are used, I just don't really understand why WebKit fails tests like this then: https://wpt.fyi/results/dom/nodes/insertion-removing-steps/Node-appendChild-script-and-button-from-div.tentative.html?label=experimental&label=master&aligned. Someone like @rniwa probably knows though.

It's because in WebKit, updating of element.form also happens in the post insertion steps.

@domfarolino
Copy link
Member Author

It's because in WebKit, updating of element.form also happens in the post insertion steps.

Good to know. Do you think WebKit would be willing to change that? It's not a decision that needs to block this PR, since this PR just gives us the primitives available for HTML to do whatever we decide. But so far it seems 2/3 impls agree on that specific case.

@rniwa
Copy link
Collaborator

rniwa commented Mar 26, 2024

It's because in WebKit, updating of element.form also happens in the post insertion steps.

Good to know. Do you think WebKit would be willing to change that? It's not a decision that needs to block this PR, since this PR just gives us the primitives available for HTML to do whatever we decide. But so far it seems 2/3 impls agree on that specific case.

Unlikely. There are some design & performance constraints about this.

@domfarolino
Copy link
Member Author

Unlikely. There are some design & performance constraints about this.

I'd appreciate some elaboration on this, just to understand why other browsers seem to be OK not doing this. However, it is a low priority discussion relative to this PR, since this PR doesn't change anything WRT that scenario specifically. But if you'd be willing to review this PR that would be great.

@domfarolino
Copy link
Member Author

Gentle ping for reviews (and for a response to #1261 (comment)).

@domfarolino domfarolino requested review from annevk and smaug---- May 1, 2024 16:16
dom.bs Outdated Show resolved Hide resolved
dom.bs Outdated Show resolved Hide resolved
@domfarolino domfarolino changed the title Introduce DOM post-insertion steps Introduce DOM post-connection steps May 13, 2024
dom.bs Outdated Show resolved Hide resolved
dom.bs Outdated Show resolved Hide resolved
dom.bs Outdated Show resolved Hide resolved
dom.bs Outdated Show resolved Hide resolved
dom.bs Outdated Show resolved Hide resolved
dom.bs Outdated Show resolved Hide resolved
dom.bs Outdated Show resolved Hide resolved
@domfarolino domfarolino requested review from annevk and smaug---- May 31, 2024 11:47
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.

I plan on merging this next week Wednesday assuming there's no further comments.

dom.bs Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Side effects due to tree insertion or removal (script, iframe)
6 participants