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 a queue of deferred steps in the node insertion algorithm #732

Closed
wants to merge 1 commit into from

Conversation

nox
Copy link
Member

@nox nox commented Feb 8, 2019


💥 Error: 500 Internal Server Error 💥

PR Preview failed to build. (Last tried on Jan 15, 2021, 7:33 AM UTC).

More

PR Preview relies on a number of web services to run. There seems to be an issue with the following one:

🚨 CSS Spec Preprocessor - CSS Spec Preprocessor is the web service used to build Bikeshed specs.

🔗 Related URL

<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML 2.0//EN">
<html><head>
<title>500 Internal Server Error</title>
</head><body>
<h1>Internal Server Error</h1>
<p>The server encountered an internal error or
misconfiguration and was unable to complete
your request.</p>
<p>Please contact the server administrator at 
 [no address given] to inform them of the time this error occurred,
 and the actions you performed just before this error.</p>
<p>More information about this error may be available
in the server error log.</p>
<hr>
<address>Apache/2.4.10 (Debian) Server at api.csswg.org Port 443</address>
</body></html>

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.

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.

This looks good. I guess the way to go here is that you also create patches for the HTML Standard, and then we all review those, the tests, and this change a couple times until we're sure it all works?

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
nox added a commit to nox/html that referenced this pull request Feb 10, 2019
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.
nox added a commit to nox/html that referenced this pull request Feb 10, 2019
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.
dom.bs Outdated Show resolved Hide resolved
nox added a commit to nox/html that referenced this pull request Feb 11, 2019
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.
@annevk
Copy link
Member

annevk commented Feb 26, 2019

We should note in the eventual commit that this helps fixing whatwg/html#1127 and #575

dom.bs Outdated Show resolved Hide resolved
annevk added a commit that referenced this pull request Dec 5, 2019
It needs to be "children changed steps" as the various consumers appear to react to changes of all sorts.

Tests: web-platform-tests/wpt#15871.

This builds on work started in #732 and will further refine that eventually.
@annevk
Copy link
Member

annevk commented Dec 5, 2019

The other piece of relevant feedback here is whatwg/html#4611. We need these deferred steps for removal too. Hurray!

@domenic
Copy link
Member

domenic commented Dec 5, 2019

I don't believe that's true? At least, not if we want to match 2/3 browsers; for those the current spec suffices.

@annevk
Copy link
Member

annevk commented Dec 5, 2019

That would effectively mean keeping around mutation events though and I doubt the current specification accurately accounts for that.

@domenic
Copy link
Member

domenic commented Dec 5, 2019

Hmm, maybe that argument could convince Chrome or WebKit to add the extra complexity; I'm not sure. unload events are kind of a compat minefield though so it'd be a hard sell.

annevk added a commit that referenced this pull request Dec 6, 2019
It needs to be "children changed steps" as consumers appear to react to changes of all sorts.

Tests: web-platform-tests/wpt#15871.

This builds on work started in #732 and will further refine that eventually. Meta follow-up: #802.

<p><a lt="other applicable specifications">Specifications</a> may define
<dfn export id=concept-node-children-changed-ext>children changed steps</dfn> for all or some
<a for=/>nodes</a>. The algorithm is passed no argument and is called from <a for=/>insert</a>,
<a for=/>remove</a>, and <a for=/>replace data</a>.
<a for=/>nodes</a>. The algorithm is passed <var ignore>deferredStepsQueue</var> and is called from
Copy link
Member

Choose a reason for hiding this comment

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

Addressing whatwg/html#4354 (comment) by @tkent-google in a way that aligns with Chrome would also require passing "insert"/"change"/"remove" as an argument here and acting on it accordingly. (Script would ignore anything but "insert".)

Probably would have to double check whether setting textContent counts as "change" or "insert".

annevk pushed a commit to nox/html that referenced this pull request Dec 9, 2019
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.
Base automatically changed from master to main January 15, 2021 07:32
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}
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
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
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}
@annevk annevk closed this in 0616094 Jun 5, 2024
vinhill pushed a commit to vinhill/dom that referenced this pull request Jun 20, 2024
For any given insert operation, these steps run for each inserted node synchronously after all node insertions are complete. This closes whatwg#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
    - This includes creating an iframe's document and synchronously firing its load event

For any given call to 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 whatwg#808. This PR also helps out with whatwg/html#1127 and whatwg#575 (per whatwg#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 here:

- whatwg/html#10188
- whatwg/html#10241
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.

3 participants