DOM: Added a few batch-insertion scenarios that are incompatible across engines #44658

noamr · 2024-02-19T20:53:44Z

Repurposing #15264 to clean up the mess around deferred insertion steps.
Made the tests tentative, and cleaned up a few of the results.

Currently the expected results match Firefox.
They're marked tentative, and we can reason about the differences and fix the tests when we come to a conclusion.

Together with @annevk and @nox

domfarolino

Mostly small stuff so far about test grouping and making the expectations more clear by using string/state variables instead of booleans that are equal to foo === null. Separately, I think it would be good to add test descriptions to each of the test(() => {}, "<description here>")s, just to make it more clear what each is asserting, and maybe add a little information about why.

dom/nodes/insertion-effects/Node-appendChild-iframe-before-script.tentative.html

dom/nodes/insertion-effects/Node-appendChild-script-and-button-from-div.tentative.html

dom/nodes/insertion-effects/Node-appendChild-script-and-custom-from-fragment.tentative.html

...nsertion-effects/Node-appendChild-script-and-default-style-meta-from-fragment.tentative.html

dom/nodes/insertion-effects/Node-appendChild-script-and-iframe-from-fragment.tentative.html

dom/nodes/insertion-effects/Node-appendChild-script-and-iframe.tentative.html

...s/insertion-effects/Node-appendChild-script-and-stylesheet-link-from-fragment.tentative.html

dom/nodes/insertion-effects/Node-appendChild-script-and-stylesheet-link.tentative.html

domfarolino · 2024-02-26T15:47:31Z

dom/nodes/insertion-effects/Node-appendChild-iframe-before-script.tentative.html

+const iframe = document.createElement("iframe");
+
+test(() => {
+  iframe.src = "data:text/html,";


We don't need this right?

domfarolino · 2024-02-26T16:25:24Z

dom/nodes/insertion-effects/Node-appendChild-script-and-iframe.tentative.html

+test(() => {
+  window.state = "script not run yet";
+  window.iframe = document.createElement("iframe");
+  iframe.src = "data:text/html,"


Same, is this important, or will the about:blank Document suffice?

domfarolino · 2024-02-26T20:36:56Z

...s/insertion-effects/Node-appendChild-script-and-stylesheet-link-from-fragment.tentative.html

+
+  assert_array_equals(happened, []);
+  document.body.appendChild(df);
+  assert_array_equals(happened, [true]);


It depends on what we mean by "insertion effects". I don't think the goal is to eradicate all insertion side effects from taking place during DOM mutations. I think the goal is to eradicate all synchronous script-execution from the DOM mutation flow, to be queued until after all DOM mutations are done. The mutations themselves may have script-observable side effects by definition, which can be observed by the time earlier-inserted scripts run. (Although I'm not sure what we want to do about that in this case specifically).

dom/nodes/insertion-effects/Node-appendChild-script-and-style-from-fragment.tentative.html

dom/nodes/insertion-effects/Node-appendChild-script-and-style.tentative.html

dom/nodes/insertion-effects/Node-appendChild-script-in-script.tentative.html

dom/nodes/insertion-effects/Node-appendChild-text-and-script-in-style.tentative.html

domfarolino · 2024-02-27T00:19:36Z

(I've added a few changes, test description updates, and test coalescing in response to this and my prior review; feel free to check them out).

dom/nodes/insertion-effects/Node-appendChild-script-before-iframe.tentative.html

The test follows the spec, but the spec is followed only by Safari.

This test too follows the spec, but only Safari defines the custom element before inserting one in the document, other browsers upgrade the custom-element at the time of the customElements.define call because it is already in tree.

This test once again follows the spec, but only Safari executes it correctly, with no form owner set on the button at the time of the execution of the script.

Safari and Chrome pass that test, Firefox fails it.

…ement

…-from-div.tentative.html Co-authored-by: Dominic Farolino <domfarolino@gmail.com>

…heet-link-from-fragment.tentative.html Co-authored-by: Dominic Farolino <domfarolino@gmail.com>

Co-authored-by: Dominic Farolino <domfarolino@gmail.com>

domfarolino · 2024-02-29T22:46:23Z

I've rebased this on top of master and moved the tests to the insertion-removing-steps folder where other tests related to this have been landing. I think this is good to merge now.

wpt-pr-bot added the dom label Feb 19, 2024

wpt-pr-bot assigned jdm Feb 19, 2024

wpt-pr-bot requested review from annevk and jdm February 19, 2024 20:53

noamr changed the title ~~Weird insertion~~ DOM: Added a few batch-insertion scenarios that are incompatible across engines Feb 19, 2024

noamr requested a review from domfarolino February 19, 2024 21:02

domfarolino mentioned this pull request Feb 22, 2024

Side effects due to tree insertion or removal (script, iframe) whatwg/dom#808

Closed

domfarolino reviewed Feb 22, 2024

View reviewed changes

domfarolino mentioned this pull request Feb 22, 2024

Add insertion tests for the new changes with the "deferred steps queue" #15264

Closed

domfarolino reviewed Feb 27, 2024

View reviewed changes

dom/nodes/insertion-effects/Node-appendChild-script-before-iframe.tentative.html Outdated Show resolved Hide resolved

domfarolino mentioned this pull request Feb 28, 2024

Introduce DOM post-connection steps whatwg/dom#1261

Merged

5 tasks

nox and others added 17 commits February 29, 2024 17:29

Test inserting a script and a div from a DocumentFragment

dd49336

The test follows the spec, but the spec is followed only by Safari.

Test inserting a script and a custom element

b8c4374

This test too follows the spec, but only Safari defines the custom element before inserting one in the document, other browsers upgrade the custom-element at the time of the customElements.define call because it is already in tree.

Test inserting a script and button in a form from a div

05694a9

This test once again follows the spec, but only Safari executes it correctly, with no form owner set on the button at the time of the execution of the script.

Test inserting three scripts where the first one modifies the third one

6e602f6

Safari and Chrome pass that test, Firefox fails it.

Test inserting two text nodes from a fragment into an empty script el…

8a43ca1

…ement

Test inserting text and script nodes in a style element

11214ff

Test inserting a script and a style where the script modifies the style

631c9ac

Test inserting a script and some code in an empty script

2ddd125

Test inserting a script and an iframe both at the same time

7643207

Test inserting a script and a stylesheet link both at the same time

66357e1

Test inserting a script and a source in a media element

d104e23

Test inserting a script and a default-style meta

1e52f0f

rewrite first four files to work again

ae30cc5

more

392eed7

wip

a339b35

more cleanup

982a1f4

Fix a couple of tests to have more coherent results

55767c7

noamr and others added 13 commits February 29, 2024 17:29

Add .tentative

3ee6105

Change expected results to match batch insertion

d598cb5

Update dom/nodes/insertion-effects/Node-appendChild-script-and-button…

68ee4c1

…-from-div.tentative.html Co-authored-by: Dominic Farolino <domfarolino@gmail.com>

Update dom/nodes/insertion-effects/Node-appendChild-script-and-styles…

b8c3dd0

…heet-link-from-fragment.tentative.html Co-authored-by: Dominic Farolino <domfarolino@gmail.com>

Apply suggestions from code review

70b0596

Co-authored-by: Dominic Farolino <domfarolino@gmail.com>

Remove redundant log divs

e5eaa85

Apply PR fixes

7da6f2d

Some changes

4573bb4

Fix assertion

6bfc45b

Combine tests

5a010df

Update expectations for style tests

d7174e5

Write explicit test description and combine a few more tests

98aa182

Move to insertion-removing-steps

76b9cb2

domfarolino force-pushed the weird-insertion branch from dc2c7ac to 76b9cb2 Compare February 29, 2024 22:39

Unnecessary src assignment

f818763

domfarolino approved these changes Feb 29, 2024

View reviewed changes

domfarolino merged commit 870bb35 into master Feb 29, 2024
19 checks passed

domfarolino deleted the weird-insertion branch February 29, 2024 23:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DOM: Added a few batch-insertion scenarios that are incompatible across engines #44658

DOM: Added a few batch-insertion scenarios that are incompatible across engines #44658

noamr commented Feb 19, 2024 •

edited

Loading

domfarolino left a comment

domfarolino Feb 26, 2024

domfarolino Feb 26, 2024

domfarolino Feb 26, 2024

domfarolino commented Feb 27, 2024

domfarolino commented Feb 29, 2024

DOM: Added a few batch-insertion scenarios that are incompatible across engines #44658

DOM: Added a few batch-insertion scenarios that are incompatible across engines #44658

Conversation

noamr commented Feb 19, 2024 • edited Loading

domfarolino left a comment

Choose a reason for hiding this comment

domfarolino Feb 26, 2024

Choose a reason for hiding this comment

domfarolino Feb 26, 2024

Choose a reason for hiding this comment

domfarolino Feb 26, 2024

Choose a reason for hiding this comment

domfarolino commented Feb 27, 2024

domfarolino commented Feb 29, 2024

noamr commented Feb 19, 2024 •

edited

Loading