dom icon indicating copy to clipboard operation
dom copied to clipboard

Introduce DOM post-connection steps

Open domfarolino opened this issue 1 year ago • 11 comments
trafficstars

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 https://github.com/whatwg/dom/pull/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 https://github.com/whatwg/dom/issues/808. This PR also helps out with https://github.com/whatwg/html/issues/1127 and https://github.com/whatwg/dom/issues/575 (per https://github.com/whatwg/dom/pull/732#issuecomment-467403090).

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:

  • https://github.com/whatwg/html/pull/10188

  • https://github.com/whatwg/html/issues/10241

  • <another incoming PR will be made for iframes>

  • [x] At least two implementers are interested (and none opposed):

    • ✅ Chrome (mostly implements this model; fails at least one "nested script ordering" test)
    • ✅ Firefox (mostly implements this model; fails a few tests that rely on iframe load event being fired synchronously, since I guess it fires this event asynchronously..)
    • 🟠 Safari: @annevk would you be able to provide a WebKit signal here or should I file a standards position issue?
  • [x] Tests are written and can be reviewed and commented upon at:

    • WPT results can be seen here
    • https://github.com/web-platform-tests/wpt/pull/44308
    • https://github.com/web-platform-tests/wpt/pull/44658
    • https://github.com/web-platform-tests/wpt/pull/44774
    • https://github.com/web-platform-tests/wpt/pull/44906
  • [ ] Implementation bugs are filed: N/A given https://github.com/whatwg/dom/pull/1261#issuecomment-2037596814.

  • [ ] MDN issue is filed: …

  • [x] The top of this comment includes a clear commit message to use.

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


Two things need more discussion:

  • Script child element mutation:
    • https://github.com/whatwg/dom/pull/732#pullrequestreview-328249015 and https://github.com/whatwg/html/pull/4354#issuecomment-476038918 discuss Chromium & WebKit's model of not executing scripts when their children are modified except for insertion.
      • Regarding insertion only: All browsers are aligned on executing a script (that hasn't "already started") when a single child node is inserted. For insertion multiple child nodes atomically, see this WPT which Chrome & Firefox pass, and WebKit barely misses due to the post-connection steps[^1]. (Related: all browsers are consistent (see this test case) about not executing newly-inserted text nodes in a script that has "already started")
      • Regarding child removal/change: As a follow-up to https://github.com/whatwg/dom/pull/732#pullrequestreview-328249015 and https://github.com/whatwg/html/pull/4354#issuecomment-476038918, I've investigated this a little more and found that actually all browsers follow the Chromium behavior here. See this WPT (from https://github.com/web-platform-tests/wpt/pull/45085), where all browsers never trigger the execution of a script when a child node is removed. To accomplish this with the spec, we should probably just add an argument to the child changed steps, but that can be done in a follow-up PR to https://github.com/whatwg/html/pull/10188, since it is slightly orthogonal
    • The only oddity I can find with scripts in Chromium's model is that we don't follow this rule exactly, which mandates that when a script (that hasn't already started yet) gets a child <script> inserted into it, it must execute after that child script does. Chromium does not do that; the child mutation of the outer script (that inserts the inner <script>) triggers the execution of the outer script in its post-connection steps, and then after, the execution of the inner script during its post-connection steps. WebKit is aligned with Chrome on this and Firefox is the odd one out. Maybe we're OK aligning with Chrome/Firefox in this case?
  • 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

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


Preview | Diff

domfarolino avatar Feb 28 '24 16:02 domfarolino

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.

domfarolino avatar Mar 08 '24 03:03 domfarolino

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 https://github.com/whatwg/dom/pull/732#pullrequestreview-328249015 and https://github.com/whatwg/html/pull/4354#issuecomment-476038918 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 https://github.com/whatwg/html/pull/10188.)

domfarolino avatar Mar 13 '24 19:03 domfarolino

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.

domfarolino avatar Mar 14 '24 13:03 domfarolino

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.

rniwa avatar Mar 26 '24 21:03 rniwa

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.

domfarolino avatar Mar 26 '24 21:03 domfarolino

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.

rniwa avatar Mar 26 '24 21:03 rniwa

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 avatar Mar 27 '24 12:03 domfarolino

Gentle ping for reviews (and for a response to https://github.com/whatwg/dom/pull/1261#issuecomment-2022663904).

domfarolino avatar Apr 01 '24 20:04 domfarolino

I don't think we need implementation bugs for this PR given its largely non-normative nature. Can you update the proposed commit message so it's clearer where the corresponding HTML work happens? I found these two so far:

  • https://github.com/whatwg/html/pull/10188
  • https://github.com/whatwg/html/issues/10241

Given that we have a concrete HTML PR I think we can merge this, but I'd like to give folks a bit more time to comment. So let's say I merge this next week Wednesday, barring any other feedback.

annevk avatar Apr 04 '24 15:04 annevk

Let's see if we can briefly discuss this tomorrow. I do see the appeal of Gecko's script runner approach as it avoids the ordering issue. On the other hand, thus far the ordering issue is largely theoretical given the limited number of things we expect will need to be changed.

annevk avatar Apr 10 '24 15:04 annevk

Can you update the proposed commit message so it's clearer where the corresponding HTML work happens?

Done. I've updated the first few paragraphs in the OP to include this, and I think those paragraphs would make a good commit message, but let me know if you'd like me to distill it down further.

domfarolino avatar Apr 11 '24 15:04 domfarolino