Rewrite node insertion algorithm to match the spec (#35999)

Per [spec](https://dom.spec.whatwg.org/#concept-node-insert), adoption
of new node should be done while inserting the node. This patch moves
the call site of `adopt` to inside `insert` to match it.

It also rewrites some existing code to better match the spec without any
behavioral changes.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by
`[X]` when the step is complete, and replace `___` with appropriate
data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #___ (GitHub issue number if applicable)

<!-- Either: -->
- [X] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox
is checked, so that we can help you if you get stuck somewhere along the
way.-->

<!-- Pull requests that do not address these steps are welcome, but they
will require additional verification as part of the review process. -->

---------

Signed-off-by: Xiaocheng Hu <xiaochengh.work@gmail.com>

components/script/dom/node.rs

@@ -2246,9 +2246,6 @@ impl Node {
         };
 
         // Step 4.
-        Node::adopt(node, &parent.owner_document(), can_gc);
-
-        // Step 5.
         Node::insert(
             node,
             parent,
@@ -2257,7 +2254,7 @@ impl Node {
             can_gc,
         );
 
-        // Step 6.
+        // Step 5.
         Ok(DomRoot::from_ref(node))
     }
 
@@ -2269,49 +2266,64 @@ impl Node {
         suppress_observers: SuppressObserver,
         can_gc: CanGc,
     ) {
-        node.owner_doc().add_script_and_layout_blocker();
-        debug_assert!(*node.owner_doc() == *parent.owner_doc());
         debug_assert!(child.is_none_or(|child| Some(parent) == child.GetParentNode().as_deref()));
 
-        // Step 1.
+        // Step 1. Let nodes be node’s children, if node is a DocumentFragment node; otherwise « node ».
-        let count = if node.is::<DocumentFragment>() {
-            node.children_count()
-        } else {
-            1
-        };
-        // Step 2.
-        if let Some(child) = child {
-            if !parent.ranges_is_empty() {
-                let index = child.index();
-                // Steps 2.1-2.
-                parent.ranges().increase_above(parent, index, count);
-            }
-        }
         rooted_vec!(let mut new_nodes);
         let new_nodes = if let NodeTypeId::DocumentFragment(_) = node.type_id() {
-            // Step 3.
+            new_nodes.extend(node.children().map(|node| Dom::from_ref(&*node)));
-            new_nodes.extend(node.children().map(|kid| Dom::from_ref(&*kid)));
+            new_nodes.r()
-            // Step 4.
+        } else {
-            for kid in &*new_nodes {
+            from_ref(&node)
+        };
+
+        // Step 2. Let count be nodes’s size.
+        let count = new_nodes.len();
+
+        // Step 3. If count is 0, then return.
+        if count == 0 {
+            return;
+        }
+
+        // Script and layout blockers must be added after any early return.
+        // `node.owner_doc()` may change during the algorithm.
+        let parent_document = parent.owner_doc();
+        let from_document = node.owner_doc();
+        from_document.add_script_and_layout_blocker();
+        parent_document.add_script_and_layout_blocker();
+
+        // Step 4. If node is a DocumentFragment node:
+        if let NodeTypeId::DocumentFragment(_) = node.type_id() {
+            // Step 4.1. Remove its children with the suppress observers flag set.
+            for kid in new_nodes {
                 Node::remove(kid, node, SuppressObserver::Suppressed, can_gc);
             }
-            // Step 5.
+            vtable_for(node).children_changed(&ChildrenMutation::replace_all(new_nodes, &[]));
-            vtable_for(node).children_changed(&ChildrenMutation::replace_all(new_nodes.r(), &[]));
 
+            // Step 4.2. Queue a tree mutation record for node with « », nodes, null, and null.
             let mutation = LazyCell::new(|| Mutation::ChildList {
                 added: None,
-                removed: Some(new_nodes.r()),
+                removed: Some(new_nodes),
                 prev: None,
                 next: None,
             });
             MutationObserver::queue_a_mutation_record(node, mutation);
+        }
 
-            new_nodes.r()
+        // Step 5. If child is non-null:
-        } else {
+        //     1. For each live range whose start node is parent and start offset is
-            // Step 3.
+        //        greater than child’s index, increase its start offset by count.
-            from_ref(&node)
+        //     2. For each live range whose end node is parent and end offset is
-        };
+        //        greater than child’s index, increase its end offset by count.
-        // Step 6.
+        if let Some(child) = child {
+            if !parent.ranges_is_empty() {
+                parent
+                    .ranges()
+                    .increase_above(parent, child.index(), count.try_into().unwrap());
+            }
+        }
+
+        // Step 6. Let previousSibling be child’s previous sibling or parent’s last child if child is null.
         let previous_sibling = match suppress_observers {
             SuppressObserver::Unsuppressed => match child {
                 Some(child) => child.GetPreviousSibling(),
@@ -2319,9 +2331,14 @@ impl Node {
             },
             SuppressObserver::Suppressed => None,
         };
-        // Step 7.
+
+        // Step 7. For each node in nodes, in tree order:
         for kid in new_nodes {
-            // Step 7.1.
+            // Step 7.1. Adopt node into parent’s node document.
+            Node::adopt(kid, &parent.owner_document(), can_gc);
+
+            // Step 7.2. If child is null, then append node to parent’s children.
+            // Step 7.3. Otherwise, insert node into parent’s children before child’s index.
             parent.add_child(kid, child, can_gc);
 
             // Step 7.4 If parent is a shadow host whose shadow root’s slot assignment is "named"
@@ -2350,12 +2367,17 @@ impl Node {
             kid.GetRootNode(&GetRootNodeOptions::empty())
                 .assign_slottables_for_a_tree();
 
-            // Step 7.7.
+            // Step 7.7. For each shadow-including inclusive descendant inclusiveDescendant of node,
+            // in shadow-including tree order:
             for descendant in kid
                 .traverse_preorder(ShadowIncluding::Yes)
                 .filter_map(DomRoot::downcast::<Element>)
             {
+                // Step 7.7.1. Run the insertion steps with inclusiveDescendant.
+                // This is done in `parent.add_child()`.
+
                 // Step 7.7.2, whatwg/dom#833
+                // Enqueue connected reactions for custom elements or try upgrade.
                 if descendant.is_custom() {
                     if descendant.is_connected() {
                         ScriptThread::enqueue_callback_reaction(
@@ -2369,13 +2391,18 @@ impl Node {
                 }
             }
         }
+
         if let SuppressObserver::Unsuppressed = suppress_observers {
+            // Step 9. Run the children changed steps for parent.
+            // TODO(xiaochengh): If we follow the spec and move it out of the if block, some WPT fail. Investigate.
             vtable_for(parent).children_changed(&ChildrenMutation::insert(
                 previous_sibling.as_deref(),
                 new_nodes,
                 child,
             ));
 
+            // Step 8. If suppress observers flag is unset, then queue a tree mutation record for parent
+            // with nodes, « », previousSibling, and child.
             let mutation = LazyCell::new(|| Mutation::ChildList {
                 added: Some(new_nodes),
                 removed: None,
@@ -2408,7 +2435,7 @@ impl Node {
         // 2) post_connection_steps from Node::insert,
         // we use a delayed task that will run as soon as Node::insert removes its
         // script/layout blocker.
-        node.owner_doc().add_delayed_task(task!(PostConnectionSteps: move || {
+        parent_document.add_delayed_task(task!(PostConnectionSteps: move || {
             // Step 12. For each node of staticNodeList, if node is connected, then run the
             //          post-connection steps with node.
             for node in static_node_list.iter().map(Trusted::root).filter(|n| n.is_connected()) {
@@ -2416,7 +2443,8 @@ impl Node {
             }
         }));
 
-        node.owner_doc().remove_script_and_layout_blocker();
+        parent_document.remove_script_and_layout_blocker();
+        from_document.remove_script_and_layout_blocker();
     }
 
     /// <https://dom.spec.whatwg.org/#concept-node-replace-all>
@@ -3239,10 +3267,16 @@ impl NodeMethods<crate::DomTypeHolder> for Node {
         // Step 9.
         let previous_sibling = child.GetPreviousSibling();
 
-        // Step 10.
+        // NOTE: All existing browsers assume that adoption is performed here, which does not follow the DOM spec.
+        // However, if we follow the spec and delay adoption to inside `Node::insert()`, then the mutation records will
+        // be different, and we will fail WPT dom/nodes/MutationObserver-childList.html.
         let document = self.owner_document();
         Node::adopt(node, &document, can_gc);
 
+        // Step 10. Let removedNodes be the empty set.
+        // Step 11. If child’s parent is non-null:
+        //     1. Set removedNodes to « child ».
+        //     2. Remove child with the suppress observers flag set.
         let removed_child = if node != child {
             // Step 11.
             Node::remove(child, self, SuppressObserver::Suppressed, can_gc);

tests/wpt/meta/MANIFEST.json

@@ -624655,6 +624655,13 @@
        {}
       ]
      ],
+     "Node-childNodes-cache-2.html": [
+      "9079fc6ea7a146cb854389bf4f1b8c302a102746",
+      [
+       null,
+       {}
+      ]
+     ],
      "Node-childNodes-cache.html": [
       "da9e32c6a9cf579f3d7c6ce2e3208e04f90d7105",
       [

tests/wpt/meta/custom-elements/adopted-callback.html.ini

@@ -1,39 +0,0 @@
-[adopted-callback.html]
-  [Moving the shadow host's shadow of a custom element from the owner document into the document of the template elements must enqueue and invoke adoptedCallback]
-    expected: FAIL
-
-  [Moving the shadow host's shadow of a custom element from the owner document into a new document must enqueue and invoke adoptedCallback]
-    expected: FAIL
-
-  [Moving the <template>'s content of a custom element from the owner document into a new document must enqueue and invoke adoptedCallback]
-    expected: FAIL
-
-  [Moving the shadow host's shadow of a custom element from the owner document into a cloned document must enqueue and invoke adoptedCallback]
-    expected: FAIL
-
-  [Moving the <template>'s content of a custom element from the owner document into a cloned document must enqueue and invoke adoptedCallback]
-    expected: FAIL
-
-  [Moving the shadow host's shadow of a custom element from the owner document into a document created by createHTMLDocument must enqueue and invoke adoptedCallback]
-    expected: FAIL
-
-  [Moving the <template>'s content of a custom element from the owner document into a document created by createHTMLDocument must enqueue and invoke adoptedCallback]
-    expected: FAIL
-
-  [Moving the shadow host's shadow of a custom element from the owner document into an HTML document created by createDocument must enqueue and invoke adoptedCallback]
-    expected: FAIL
-
-  [Moving the <template>'s content of a custom element from the owner document into an HTML document created by createDocument must enqueue and invoke adoptedCallback]
-    expected: FAIL
-
-  [Moving the shadow host's shadow of a custom element from the owner document into the document of an iframe must enqueue and invoke adoptedCallback]
-    expected: FAIL
-
-  [Moving the <template>'s content of a custom element from the owner document into the document of an iframe must enqueue and invoke adoptedCallback]
-    expected: FAIL
-
-  [Moving the shadow host's shadow of a custom element from the owner document into an HTML document fetched by XHR must enqueue and invoke adoptedCallback]
-    expected: FAIL
-
-  [Moving the <template>'s content of a custom element from the owner document into an HTML document fetched by XHR must enqueue and invoke adoptedCallback]
-    expected: FAIL

tests/wpt/meta/dom/nodes/adoption.window.js.ini

@@ -1,12 +1,3 @@
 [adoption.window.html]
-  [appendChild() and DocumentFragment with host]
-    expected: FAIL
-
   [adoptNode() and DocumentFragment with host]
     expected: FAIL
-
-  [appendChild() and DocumentFragment]
-    expected: FAIL
-
-  [appendChild() and ShadowRoot]
-    expected: FAIL

tests/wpt/meta/html/semantics/document-metadata/the-style-element/mutations.window.js.ini

@@ -1,6 +1,3 @@
 [mutations.window.html]
   [Mutating the style element: mutating a Comment node]
     expected: FAIL
-
-  [Mutating the style element: inserting an empty DocumentFragment node]
-    expected: FAIL

tests/wpt/tests/dom/nodes/Node-childNodes-cache-2.html

@@ -0,0 +1,16 @@
+<!DOCTYPE html>
+<meta charset=utf-8>
+<title>Node.childNodes caching bug with replaceChild</title>
+<link rel=help href="https://dom.spec.whatwg.org/#dom-node-childnodes">
+<link rel=author title="Xiaocheng Hu" href="mailto:xiaochengh.work@gmail.com">
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<div id="target"><div id="first"></div><div id="second"></div><div id="third"></div><div id="last"></div></div>
+<script>
+test(function() {
+  let target = document.getElementById("target");
+  assert_array_equals(Array.from(target.childNodes).map(node => node.id), ["first", "second", "third", "last"]);
+  target.replaceChild(target.childNodes[2], target.childNodes[1]);
+  assert_array_equals(Array.from(target.childNodes).map(node => node.id), ["first", "third", "last"]);
+});
+</script>