Merge pull request #1732 from lightpanda-io/custom_element_clone_take_2

Fix cloning custom element with constructor which attaches the element

src/browser/tests/custom_elements/constructor.html

@@ -72,3 +72,59 @@
         testing.expectEqual(2, calls);
     }
 </script>
+
+<div id=fragment_clone_container></div>
+
+<script id=clone_fragment>
+    {
+        let calls = 0;
+        class MyFragmentCloneElement extends HTMLElement {
+            constructor() {
+                super();
+                calls += 1;
+                $('#fragment_clone_container').appendChild(this);
+            }
+        }
+        customElements.define('my-fragment-clone-element', MyFragmentCloneElement);
+
+        // Create a DocumentFragment with a custom element
+        const fragment = document.createDocumentFragment();
+        const customEl = document.createElement('my-fragment-clone-element');
+        fragment.appendChild(customEl);
+
+        // Clone the fragment - this should trigger the crash
+        // because the constructor will attach the element during cloning
+        const clonedFragment = fragment.cloneNode(true);
+        testing.expectEqual(2, calls);
+    }
+</script>
+
+<div id=range_clone_container></div>
+
+<script id=clone_range>
+    {
+        let calls = 0;
+        class MyRangeCloneElement extends HTMLElement {
+            constructor() {
+                super();
+                calls += 1;
+                $('#range_clone_container').appendChild(this);
+            }
+        }
+        customElements.define('my-range-clone-element', MyRangeCloneElement);
+
+        // Create a container with a custom element
+        const container = document.createElement('div');
+        const customEl = document.createElement('my-range-clone-element');
+        container.appendChild(customEl);
+
+        // Create a range that includes the custom element
+        const range = document.createRange();
+        range.selectNodeContents(container);
+
+        // Clone the range contents - this should trigger the crash
+        // because the constructor will attach the element during cloning
+        const clonedContents = range.cloneContents();
+        testing.expectEqual(2, calls);
+    }
+</script>

src/browser/webapi/DocumentFragment.zig

@@ -195,10 +195,11 @@ pub fn cloneFragment(self: *DocumentFragment, deep: bool, page: *Page) !*Node {
 
         var child_it = node.childrenIterator();
         while (child_it.next()) |child| {
-            const cloned_child = try child.cloneNode(true, page);
+            if (try child.cloneNodeForAppending(true, page)) |cloned_child| {
                 try page.appendNode(fragment_node, cloned_child, .{ .child_already_connected = self_is_connected });
             }
         }
+    }
 
     return fragment_node;
 }

src/browser/webapi/Element.zig

@@ -1328,22 +1328,14 @@ pub fn clone(self: *Element, deep: bool, page: *Page) !*Node {
     if (deep) {
         var child_it = self.asNode().childrenIterator();
         while (child_it.next()) |child| {
-            const cloned_child = try child.cloneNode(true, page);
+            if (try child.cloneNodeForAppending(true, page)) |cloned_child| {
-            if (cloned_child._parent != null) {
-                // This is almost always false, the only case where a cloned
-                // node would already have a parent is with a custom element
-                // that has a constructor (which is called during cloning) which
-                // inserts it somewhere. In that case, whatever parent was set
-                // in the constructor should not be changed.
-                continue;
-            }
-
                 // We pass `true` to `child_already_connected` as a hacky optimization
                 // We _know_ this child isn't connected (Because the parent isn't connected)
                 // setting this to `true` skips all connection checks.
                 try page.appendNode(node, cloned_child, .{ .child_already_connected = true });
             }
         }
+    }
 
     return node;
 }

src/browser/webapi/Node.zig

@@ -751,6 +751,29 @@ pub fn cloneNode(self: *Node, deep_: ?bool, page: *Page) CloneError!*Node {
     }
 }
 
+/// Clone a node for the purpose of appending to a parent.
+/// Returns null if the cloned node was already attached somewhere by a custom element
+/// constructor, indicating that the constructor's decision should be respected.
+///
+/// This helper is used when iterating over children to clone them. The typical pattern is:
+///   while (child_it.next()) |child| {
+///       if (try child.cloneNodeForAppending(true, page)) |cloned| {
+///           try page.appendNode(parent, cloned, opts);
+///       }
+///   }
+///
+/// The only case where a cloned node would already have a parent is when a custom element
+/// constructor (which runs during cloning per the HTML spec) explicitly attaches the element
+/// somewhere. In that case, we respect the constructor's decision and return null to signal
+/// that the cloned node should not be appended to our intended parent.
+pub fn cloneNodeForAppending(self: *Node, deep: bool, page: *Page) CloneError!?*Node {
+    const cloned = try self.cloneNode(deep, page);
+    if (cloned._parent != null) {
+        return null;
+    }
+    return cloned;
+}
+
 pub fn compareDocumentPosition(self: *Node, other: *Node) u16 {
     const DISCONNECTED: u16 = 0x01;
     const PRECEDING: u16 = 0x02;

src/browser/webapi/Range.zig

@@ -446,11 +446,12 @@ pub fn cloneContents(self: *const Range, page: *Page) !*DocumentFragment {
             var offset = self._proto._start_offset;
             while (offset < self._proto._end_offset) : (offset += 1) {
                 if (self._proto._start_container.getChildAt(offset)) |child| {
-                    const cloned = try child.cloneNode(true, page);
+                    if (try child.cloneNodeForAppending(true, page)) |cloned| {
                         _ = try fragment.asNode().appendChild(cloned, page);
                     }
                 }
             }
+        }
     } else {
         // Complex case: different containers
         // Clone partial start container
@@ -468,9 +469,11 @@ pub fn cloneContents(self: *const Range, page: *Page) !*DocumentFragment {
         if (self._proto._start_container.parentNode() == self._proto._end_container.parentNode()) {
             var current = self._proto._start_container.nextSibling();
             while (current != null and current != self._proto._end_container) {
-                const cloned = try current.?.cloneNode(true, page);
+                const next = current.?.nextSibling();
+                if (try current.?.cloneNodeForAppending(true, page)) |cloned| {
                     _ = try fragment.asNode().appendChild(cloned, page);
-                current = current.?.nextSibling();
+                }
+                current = next;
             }
         }