Correct ChildNode replacement and insertion order

.changeset/correct-child-replace-with.md

@@ -0,0 +1,5 @@
+---
+'@remote-dom/polyfill': patch
+---
+
+Make `ChildNode.replaceWith()`, `before()`, and `after()` validate all arguments before changing existing trees, preserve sibling argument order, and commit each operation before custom-element reactions run.

packages/polyfill/source/ChildNode.ts

@@ -1,7 +1,12 @@
-import {NEXT} from './constants.ts';
+import {HOST, NEXT, PARENT, PREV} from './constants.ts';
+import {performWithCustomElementReactions} from './custom-element-reactions.ts';
+import {performHookEffects, type HookEffect} from './hook-effects.ts';
 import type {ParentNode} from './ParentNode.ts';
 import {Node} from './Node.ts';
 
+export const INSERT_NODE = Symbol('insertNode');
+export const REPLACE_NODE = Symbol('replaceNode');
+
 export class ChildNode extends Node {
   remove() {
     const parent = this.parentNode;
@@ -10,39 +15,120 @@ export class ChildNode extends Node {
   }
 
   replaceWith(...nodes: (Node | string)[]) {
+    const staged = stageNodes(nodes);
     const parent = this.parentNode;
     if (!parent) return;
-    // Anchor on the first following sibling that isn't itself being moved, so
-    // that replacing a node with one of its own siblings still has a reference
-    // node left to insert before.
-    let next = this[NEXT];
-    while (next && nodes.includes(next)) next = next[NEXT];
-    parent.removeChild(this);
-    for (const node of nodes) {
-      parent.insertBefore(toNode(parent, node), next);
-    }
+
+    return performChildNodeMutation((hookEffects) => {
+      validateNodesForInsertion(parent, staged);
+
+      let next = this[NEXT];
+      while (next && staged.includes(next)) next = next[NEXT];
+
+      const replacement = convertNodesIntoNode(parent, staged, hookEffects);
+      if (this.parentNode === parent) {
+        parent[REPLACE_NODE](replacement, this, hookEffects);
+      } else {
+        parent[INSERT_NODE](replacement, next, hookEffects);
+      }
+    });
   }
 
   before(...nodes: (Node | string)[]) {
+    const staged = stageNodes(nodes);
     const parent = this.parentNode;
     if (!parent) return;
-    for (const node of nodes) {
-      parent.insertBefore(toNode(parent, node), this);
-    }
+
+    return performChildNodeMutation((hookEffects) => {
+      validateNodesForInsertion(parent, staged);
+
+      let previous = this[PREV];
+      while (previous && staged.includes(previous)) previous = previous[PREV];
+
+      const node = convertNodesIntoNode(parent, staged, hookEffects);
+      parent[INSERT_NODE](
+        node,
+        previous ? previous[NEXT] : parent.firstChild,
+        hookEffects,
+      );
+    });
   }
 
   after(...nodes: (Node | string)[]) {
+    const staged = stageNodes(nodes);
     const parent = this.parentNode;
     if (!parent) return;
-    const next = this[NEXT];
-    for (const node of nodes) {
-      parent.insertBefore(toNode(parent, node), next);
-    }
+
+    return performChildNodeMutation((hookEffects) => {
+      validateNodesForInsertion(parent, staged);
+
+      let next = this[NEXT];
+      while (next && staged.includes(next)) next = next[NEXT];
+
+      const node = convertNodesIntoNode(parent, staged, hookEffects);
+      parent[INSERT_NODE](node, next, hookEffects);
+    });
   }
 }
 
+function performChildNodeMutation(
+  mutation: (hookEffects: HookEffect[]) => void,
+) {
+  return performWithCustomElementReactions(() => {
+    const hookEffects: HookEffect[] = [];
+    try {
+      mutation(hookEffects);
+    } catch (error) {
+      try {
+        performHookEffects(hookEffects);
+      } catch {}
+      throw error;
+    }
+    performHookEffects(hookEffects);
+  });
+}
+
+function stageNodes(nodes: (Node | string)[]) {
+  return nodes.map((node) => (node instanceof Node ? node : String(node)));
+}
+
 export function toNode(parent: ParentNode, node: Node | any) {
   if (node instanceof Node) return node;
   const ownerDocument = parent.ownerDocument;
   return ownerDocument.createTextNode(String(node));
 }
+
+function validateNodesForInsertion(
+  parent: ParentNode,
+  nodes: (Node | string)[],
+) {
+  for (const node of nodes) {
+    if (!(node instanceof Node)) continue;
+
+    let ancestor: Node | null = parent;
+    while (ancestor) {
+      if (ancestor === node) {
+        throw Error(
+          'cannot insert a node into itself or one of its descendants',
+        );
+      }
+      ancestor = ancestor[PARENT] ?? ancestor[HOST];
+    }
+  }
+}
+
+function convertNodesIntoNode(
+  parent: ParentNode,
+  nodes: (Node | string)[],
+  hookEffects: HookEffect[],
+): Node {
+  const convertedNodes: Node[] = [];
+  for (const node of nodes) convertedNodes.push(toNode(parent, node));
+  if (convertedNodes.length === 1) return convertedNodes[0]!;
+
+  const fragment = parent.ownerDocument.createDocumentFragment();
+  for (const node of convertedNodes) {
+    fragment[INSERT_NODE](node, null, hookEffects);
+  }
+  return fragment;
+}

packages/polyfill/source/ParentNode.ts

@@ -12,7 +12,7 @@ import {
 } from './constants.ts';
 import type {Node} from './Node.ts';
 import type {Element} from './Element.ts';
-import {ChildNode, toNode} from './ChildNode.ts';
+import {ChildNode, INSERT_NODE, REPLACE_NODE, toNode} from './ChildNode.ts';
 import {NodeList} from './NodeList.ts';
 import {querySelectorAll, querySelector} from './selectors.ts';
 import {isElementNode, selfAndDescendants} from './shared.ts';
@@ -51,24 +51,22 @@ export class ParentNode extends ChildNode {
   readonly children = new NodeList<Element>();
 
   appendChild<T extends Node>(child: T) {
-    return performWithCustomElementReactions(() => {
-      this.insertInto(child, null);
-      return child;
-    });
+    return performWithCustomElementReactions(() =>
+      this[INSERT_NODE](child, null),
+    );
   }
 
   insertBefore<T extends Node>(child: T, ref?: Node | null) {
-    return performWithCustomElementReactions(() => {
-      this.insertInto(child, ref || null);
-      return child;
-    });
+    return performWithCustomElementReactions(() =>
+      this[INSERT_NODE](child, ref || null),
+    );
   }
 
   append(...nodes: (Node | string)[]) {
     return performWithCustomElementReactions(() => {
       for (const child of nodes) {
         if (child == null) continue;
-        this.insertInto(toNode(this, child), null);
+        this[INSERT_NODE](toNode(this, child), null);
       }
     });
   }
@@ -78,7 +76,7 @@ export class ParentNode extends ChildNode {
       const before = this.firstChild;
       for (const child of nodes) {
         if (child == null) continue;
-        this.insertInto(toNode(this, child), before);
+        this[INSERT_NODE](toNode(this, child), before);
       }
     });
   }
@@ -91,7 +89,7 @@ export class ParentNode extends ChildNode {
       }
       for (const child of nodes) {
         if (child == null) continue;
-        this.insertInto(toNode(this, child), null);
+        this[INSERT_NODE](toNode(this, child), null);
       }
     });
   }
@@ -126,52 +124,59 @@ export class ParentNode extends ChildNode {
   }
 
   replaceChild(newChild: Node, oldChild: Node) {
-    return performWithCustomElementReactions(() => {
-      if (oldChild.parentNode !== this) {
-        throw Error('reference node is not a child of this parent');
-      }
+    return performWithCustomElementReactions(() =>
+      this[REPLACE_NODE](newChild, oldChild),
+    );
+  }
+
+  [REPLACE_NODE](newChild: Node, oldChild: Node, hookEffects?: HookEffect[]) {
+    if (oldChild.parentNode !== this) {
+      throw Error('reference node is not a child of this parent');
+    }
 
-      const previous = oldChild[PREV];
-      const next = oldChild[NEXT];
-      this.validateInsertion(newChild, next);
-
-      const insertion = this.prepareInsertion(newChild);
-      const removedNodes = this[IS_CONNECTED]
-        ? selfAndDescendants(oldChild)
-        : undefined;
-      const insertionRoots = new Set(insertion.map(({node}) => node));
-      let before = next;
-      while (before && insertionRoots.has(before)) before = before[NEXT];
-
-      const oldChildIndex = this.detachChild(oldChild);
-      if (removedNodes) {
-        for (const node of removedNodes) node[IS_CONNECTED] = false;
-
-        const removedNodeSet = new Set(removedNodes);
-        for (const prepared of insertion) {
-          if (removedNodeSet.has(prepared.node)) {
-            prepared.shouldDisconnect = false;
-          }
+    const previous = oldChild[PREV];
+    const next = oldChild[NEXT];
+    this.validateInsertion(newChild, next);
+
+    const insertion = this.prepareInsertion(newChild);
+    const removedNodes = this[IS_CONNECTED]
+      ? selfAndDescendants(oldChild)
+      : undefined;
+    const insertionRoots = new Set(insertion.map(({node}) => node));
+    let before = next;
+    while (before && insertionRoots.has(before)) before = before[NEXT];
+
+    const oldChildIndex = this.detachChild(oldChild);
+    if (removedNodes) {
+      for (const node of removedNodes) node[IS_CONNECTED] = false;
+
+      const removedNodeSet = new Set(removedNodes);
+      for (const prepared of insertion) {
+        if (removedNodeSet.has(prepared.node)) {
+          prepared.shouldDisconnect = false;
         }
       }
+    }
 
-      this.commitInsertion(insertion, before);
-      const destinationIsConnected = this[IS_CONNECTED];
+    this.commitInsertion(insertion, before);
+    const destinationIsConnected = this[IS_CONNECTED];
 
-      this.queueRemovalMutationRecord(this, oldChild, previous, next);
-      this.queueInsertionMutationRecords(insertion);
+    this.queueRemovalMutationRecord(this, oldChild, previous, next);
+    this.queueInsertionMutationRecords(insertion);
 
-      if (removedNodes) {
-        this.enqueueTreeReactions(removedNodes, 'disconnectedCallback');
-      }
-      this.enqueueInsertionReactions(insertion, destinationIsConnected);
-      performHookEffects([
+    if (removedNodes) {
+      this.enqueueTreeReactions(removedNodes, 'disconnectedCallback');
+    }
+    this.enqueueInsertionReactions(insertion, destinationIsConnected);
+    this.performOrCollectHookEffects(
+      [
         ...this.collectRemovalHookEffects(oldChild, oldChildIndex),
         ...this.collectInsertionHookEffects(insertion),
-      ]);
+      ],
+      hookEffects,
+    );
 
-      return oldChild;
-    });
+    return oldChild;
   }
 
   querySelectorAll(selector: string) {
@@ -182,9 +187,14 @@ export class ParentNode extends ChildNode {
     return querySelector(this, selector);
   }
 
-  private insertInto(child: Node, before: Node | null) {
+  [INSERT_NODE]<T extends Node>(
+    child: T,
+    before: Node | null,
+    hookEffects?: HookEffect[],
+  ) {
     this.validateInsertion(child, before);
-    this.insertIntoValidated(child, before);
+    this.insertIntoValidated(child, before, hookEffects);
+    return child;
   }
 
   private validateInsertion(child: Node, before: Node | null) {
@@ -203,15 +213,30 @@ export class ParentNode extends ChildNode {
     }
   }
 
-  private insertIntoValidated(child: Node, before: Node | null) {
+  private insertIntoValidated(
+    child: Node,
+    before: Node | null,
+    hookEffects?: HookEffect[],
+  ) {
     if (child === before) before = child[NEXT];
 
     const insertion = this.prepareInsertion(child);
     this.commitInsertion(insertion, before);
     const destinationIsConnected = this[IS_CONNECTED];
     this.queueInsertionMutationRecords(insertion);
     this.enqueueInsertionReactions(insertion, destinationIsConnected);
-    performHookEffects(this.collectInsertionHookEffects(insertion));
+    this.performOrCollectHookEffects(
+      this.collectInsertionHookEffects(insertion),
+      hookEffects,
+    );
+  }
+
+  private performOrCollectHookEffects(
+    effects: HookEffect[],
+    hookEffects?: HookEffect[],
+  ) {
+    if (hookEffects) hookEffects.push(...effects);
+    else performHookEffects(effects);
   }
 
   private prepareInsertion(child: Node) {