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

replaceChild() has odd mutation observer behavior after #754 #814

Open
TimothyGu opened this issue Dec 31, 2019 · 7 comments
Open

replaceChild() has odd mutation observer behavior after #754 #814

TimothyGu opened this issue Dec 31, 2019 · 7 comments

Comments

@TimothyGu
Copy link
Member

After implementing #754 in jsdom, we are no longer passing wpt/dom/nodes/MutatioObserver-childList.html due to the following test:

<p id='n52'><span>NO </span><span>CHANGED</span></p>
<p id='n53'><span>text content</span></p>
   var n52 = document.getElementById('n52');
   runMutationTest(n52,
                   {"childList":true},
                   [{type: "childList",
-                    removedNodes: [n52.lastChild],
-                    previousSibling: n52.firstChild},
+                    removedNodes: [n52.lastChild]},
                    {type: "childList",
                     removedNodes: [n52.firstChild],
                     addedNodes: [n52.lastChild]}],
                   function() { n52.replaceChild(n52.lastChild, n52.firstChild); },
                   "childList Node.replaceChild: internal replacement mutation");
 
   var n53 = document.getElementById('n53');
   runMutationTest(n53,
                   {"childList":true},
                   [{type: "childList",
-                    removedNodes: [n53.firstChild]},
-                   {type: "childList",
-                    addedNodes: [n53.firstChild]}],
+                    addedNodes: [n53.firstChild],
+                    removedNodes: [n53.firstChild]}],
                   function() { n53.replaceChild(n53.firstChild, n53.firstChild); },
                   "childList Node.replaceChild: self internal replacement mutation");

(- is the previous expected result; + is what #754 gives.)

While the n53 change looks fine if in need of an update, n52 is perhaps more problematic.

Prior to #754, n52.lastChild first gets removed as part of adoption process in replace, between steps 9 and 10, leading to a mutation record. At the time of its removal, n52.firstChild is still in place, so it's recorded as previousSibling in the record.

After #754, however, the explicit adoption process was removed. So n52.firstChild first gets removed silently at step 11, and then n52.lastChild is removed as part of the adoption process in insert in step 13, leading to a mutation record – when n52.firstChild is no longer present in the document.

Looking only at the list of mutation records, the post-#754 flow of event is weird. Even though the removal of n52.lastChild is shown as the first event, the silent removal of n52.firstChild had already happened – and all this is observable through previousSibling. Another way to put this is that the events seem to no longer be atomic, as the second event happens both before and after the first.

annevk added a commit that referenced this issue Jan 10, 2020
This reverts commit c825cea. See #813 and #814 for context.
@annevk annevk changed the title replaceNode() has odd mutation observer behavior after #754 replaceChild() has odd mutation observer behavior after #754 Jan 10, 2020
@annevk
Copy link
Member

annevk commented Jan 10, 2020

The thing I don't understand is that previousSibling goes away as we do store that in a variable upfront before any mutations have happened. So how could it disappear?

@TimothyGu
Copy link
Member Author

TimothyGu commented Jan 10, 2020

child's previous sibling is saved, but not node's. The first mutation event results from step 13 of replace

Insert node into parent before referenceChild with the suppress observers flag set.

which eventually goes to step 7.1 of insert

Adopt node into parent’s node document.

which goes to step 2 of adopt

If node’s parent is non-null, then remove node.

which fires a mutation event. At that time, node's previous sibling has already been removed (in step 11.2 of replace), so the fired mutation event does not contain a previousSibling.


The second mutation event in the array is a result of "queue a tree mutation record" in replace itself. That works fine.

@annevk
Copy link
Member

annevk commented Jan 11, 2020

Thanks. Sigh this is hard.

So would reverting work, but then changing "adopt" in two ways:

  1. We give it an additional argument "do it anyway"
  2. Then we add 3.1.0 as a new step:

    If "do it anyway" is false and node is a DocumentFragment whose host is non-null, then continue.

    Note: This checks node and not inclusiveDescendant as ShadowRoot nodes are to be adopted, unless they are passed directly to adopt.

Then in the template element's adoption steps, we invoke "adopt" with "do it anyway" set to true.

cc @smaug---- @rniwa

@annevk
Copy link
Member

annevk commented Mar 27, 2020

@TimothyGu thoughts on the above? Does jsdom also have all the relevant tests imported? In that case I suppose I could play around with that too, it'd be nice to find a solution here.

@TimothyGu
Copy link
Member Author

TimothyGu commented Mar 27, 2020

@annevk Yes, definitely feel free to play around. Tests are all there, and you can change test exceptions in the to-run.yml file (look for 754 and 813) for a list.

Here's the patch for what you described above:

diff --git a/lib/jsdom/living/nodes/Document-impl.js b/lib/jsdom/living/nodes/Document-impl.js
index 94f8d484..ad642df5 100644
--- a/lib/jsdom/living/nodes/Document-impl.js
+++ b/lib/jsdom/living/nodes/Document-impl.js
@@ -816,7 +816,7 @@ class DocumentImpl extends NodeImpl {
   }
 
   // https://dom.spec.whatwg.org/#concept-node-adopt
-  _adoptNode(node) {
+  _adoptNode(node, doItAnyway = false) {
     const newDocument = this;
     const oldDocument = node._ownerDocument;
 
@@ -827,6 +827,9 @@ class DocumentImpl extends NodeImpl {
 
     if (oldDocument !== newDocument) {
       for (const inclusiveDescendant of shadowIncludingInclusiveDescendantsIterator(node)) {
+        if (!doItAnyway && DocumentFragment.isImpl(inclusiveDescendant)) {
+          continue;
+        }
         inclusiveDescendant._ownerDocument = newDocument;
       }
 
diff --git a/lib/jsdom/living/nodes/HTMLTemplateElement-impl.js b/lib/jsdom/living/nodes/HTMLTemplateElement-impl.js
index b0a2405b..1f829e49 100644
--- a/lib/jsdom/living/nodes/HTMLTemplateElement-impl.js
+++ b/lib/jsdom/living/nodes/HTMLTemplateElement-impl.js
@@ -43,7 +43,7 @@ class HTMLTemplateElementImpl extends HTMLElementImpl {
   // https://html.spec.whatwg.org/multipage/scripting.html#template-adopting-steps
   _adoptingSteps() {
     const doc = this._appropriateTemplateContentsOwnerDocument(this._ownerDocument);
-    doc._adoptNode(this._templateContents);
+    doc._adoptNode(this._templateContents, /* doItAnyway= */ true);
   }
 
   get content() {

You could apply it and run tests through yarn test.

Edit: It looks like this change made html/semantics/scripting-1/the-template-element/template-element/template-content-hierarcy.html pass, but not either custom-elements/adopted-callback.html or dom/nodes/adoption.window.js that were changed in web-platform-tests/wpt#16348.

For custom-elements/adopted-callback.html we have:

Failed in "Moving the shadow host's shadow of a custom element from the owner document into * must enqueue and invoke adoptedCallback": 
assert_array_equals: expected array ["disconnected", "adopted", Document node with 2 children, Document node with 1 child, "connected"]
                                got ["adopted", Document node with 2 children, Document node with 1 child, "disconnected", "connected"])

(but the "moving the <template>'s content of a custom element" test works fine)

For dom/nodes/adoption.window.js we have:

Failed in "adoptNode() and DocumentFragment with host": 
assert_equals: expected Document node with 0 children but got Document node with 2 children

Failed in "adoptNode() and DocumentFragment": 
assert_equals: expected Document node with 2 children but got Document node with 0 children

@TimothyGu
Copy link
Member Author

Sorry, it looks like I have gotten myself confused. The correct patch should be the following:

diff --git a/lib/jsdom/living/nodes/Document-impl.js b/lib/jsdom/living/nodes/Document-impl.js
index 94f8d484..3b486d84 100644
--- a/lib/jsdom/living/nodes/Document-impl.js
+++ b/lib/jsdom/living/nodes/Document-impl.js
@@ -816,7 +816,7 @@ class DocumentImpl extends NodeImpl {
   }
 
   // https://dom.spec.whatwg.org/#concept-node-adopt
-  _adoptNode(node) {
+  _adoptNode(node, doItAnyway = false) {
     const newDocument = this;
     const oldDocument = node._ownerDocument;
 
@@ -827,6 +827,9 @@ class DocumentImpl extends NodeImpl {
 
     if (oldDocument !== newDocument) {
       for (const inclusiveDescendant of shadowIncludingInclusiveDescendantsIterator(node)) {
+        if (!doItAnyway && DocumentFragment.isImpl(node) && node._host) {
+          continue;
+        }
         inclusiveDescendant._ownerDocument = newDocument;
       }
 
diff --git a/lib/jsdom/living/nodes/HTMLTemplateElement-impl.js b/lib/jsdom/living/nodes/HTMLTemplateElement-impl.js
index b0a2405b..1f829e49 100644
--- a/lib/jsdom/living/nodes/HTMLTemplateElement-impl.js
+++ b/lib/jsdom/living/nodes/HTMLTemplateElement-impl.js
@@ -43,7 +43,7 @@ class HTMLTemplateElementImpl extends HTMLElementImpl {
   // https://html.spec.whatwg.org/multipage/scripting.html#template-adopting-steps
   _adoptingSteps() {
     const doc = this._appropriateTemplateContentsOwnerDocument(this._ownerDocument);
-    doc._adoptNode(this._templateContents);
+    doc._adoptNode(this._templateContents, /* doItAnyway= */ true);
   }
 
   get content() {

In terms of tests, template-element/template-content-hierarcy.html works, custom-elements/adopted-callback.html has the same result as before, and dom/nodes/adoption.window.js has:

Failed in "appendChild() and DocumentFragment with host": 
assert_equals: expected Document node with 2 children but got Document node with 0 children

Failed in "appendChild() and DocumentFragment": 
assert_equals: expected Document node with 0 children but got Document node with 2 children

Failed in "appendChild() and ShadowRoot": 
assert_equals: expected Document node with 2 children but got Document node with 0 children

annevk added a commit that referenced this issue Mar 27, 2020
This reverts commit c825cea. See #813 and #814 for context.
annevk added a commit that referenced this issue Mar 27, 2020
Tests: ...

Corresponding HTML PR: ...

Closes #813 and closes #814.
@annevk
Copy link
Member

annevk commented Mar 27, 2020

I worked a bit more on this and put up jsdom/jsdom#2925, #819, and web-platform-tests/wpt#22504. I'll clean that all up for wider review soonish, but early feedback appreciated.

annevk added a commit that referenced this issue Nov 6, 2020
This reverts commit c825cea. See #813 and #814 for context.
annevk added a commit that referenced this issue Nov 6, 2020
Tests: ...

Corresponding HTML PR: ...

Closes #813 and closes #814.
annevk added a commit that referenced this issue Dec 1, 2020
This reverts commit c825cea. See #813 and #814 for context.
annevk added a commit that referenced this issue Dec 1, 2020
Tests: ...

Corresponding HTML PR: ...

Closes #813 and closes #814.
annevk added a commit that referenced this issue Jan 19, 2021
This reverts commit c825cea. See #813 and #814 for context.
annevk added a commit that referenced this issue Jan 19, 2021
Tests: ...

Corresponding HTML PR: ...

Closes #813 and closes #814.
annevk added a commit that referenced this issue Oct 18, 2022
This reverts commit c825cea. See #813 and #814 for context.
annevk added a commit that referenced this issue Oct 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants