Correct ChildNode replacement and insertion order

[olavoasantos]

Problem

Upstream PR #623 already landed the baseline replaceWith() repair: ordinary replacement no longer passes the old and new nodes backwards, and zero- and multi-argument calls have their basic placement semantics. The remaining issue was that replaceWith(), before(), and after() still changed the tree one argument at a time. A later cyclic node or throwing string conversion could leave earlier nodes moved and the receiver removed; custom-element reactions could also observe or reenter an incomplete multi-node operation.

Impact

Important. A throwing convenience mutation could leave connected trees partially changed, with sibling links, lifecycle callbacks, and hook effects reflecting only a prefix of the requested operation. This is especially hazardous for a worker-side DOM that must stay coherent with its remote receiver.

Upstream #623 landed the baseline fix while this stack was being prepared, narrowing this PR to the remaining ordering, validation, atomicity, and reentrancy cases.

Reproduction

const parent = document.createElement('div');
const receiver = document.createElement('em');
const replacement = document.createElement('span');
const source = document.createElement('div');

parent.appendChild(receiver);
source.appendChild(replacement);
document.body.append(parent, source);

receiver.replaceWith(replacement, parent);

The second replacement is the receiver’s parent and is invalid. Before this change, the operation could remove receiver and move replacement before reaching the invalid argument. It now throws before mutating either tree: parent still contains receiver and source still contains replacement.

Change

Validate every node argument before changing links, then convert the complete argument list into one node or a document fragment and commit it in one parent operation. Anchor around arguments that are already adjacent to the receiver so their supplied order is preserved. Wrap replaceWith(), before(), and after() in the custom-element reaction boundary, ensuring all structural changes are committed before reactions can run or reenter.

This PR deliberately builds on #623 rather than redescribing its already-landed baseline fix; its scope is the remaining argument validation, ordering, atomicity, and reentrancy behavior.

Tests

New regression coverage verifies replaceWith() with mixed nodes and strings, adjacent/self receiver arguments, an invalid first or later cyclic argument, and a reentrant disconnect callback that observes the complete replacement before removing one replacement node. Companion before() and after() coverage verifies argument order, complete-tree visibility to connected callbacks, late conversion failures without moves or hooks, lifecycle-error behavior, and late cyclic arguments without partial mutation.

Stack

• Stack: Tree and adoption (#699), layer 1 of 2
• Base: normalize-dom-namespaces
• This is the bottom of its stack.
• This stack depends on Core foundation PR Normalize DOM names and attribute namespaces #677.

Validation

Fresh GitHub CI on restacked head 6676e1d passes:

• lint;
• type-check and build;
• unit tests with coverage;
• bundle-size checks;
• Playwright end-to-end tests;
• the classified Web Platform Test suite.

[henrytao-me]

Could we stage argument coercion before validateNodesForInsertion() in all three methods? The current order introduces an error-precedence regression: a later cyclic node prevents an earlier argument's toString() from running.

Repro (before, after, and replaceWith are all affected):

const parent = document.createElement('div');
const receiver = document.createElement('span');
parent.appendChild(receiver);

let conversionCalls = 0;
const conversionError = new TypeError('conversion failed');
const value = {
  toString() {
    conversionCalls++;
    throw conversionError;
  },
};

receiver.before(value, parent);

Compared native Chrome 152, this PR's namespace base, and head 6676e1d:

• Native Chrome and the base call toString() once and throw conversionError.
• This head skips conversion (conversionCalls === 0) and throws Error: cannot insert a node into itself or one of its descendants instead.

The issue is the ordering in ChildNode.ts, not just the exception name.

Suggested fix: bring the narrow argument-staging approach from #692 into this PR:

const staged = nodes.map((node) =>
  node instanceof Node ? node : String(node),
);
validateNodesForInsertion(parent, staged);
// Use staged arguments for the remaining operation.

Keep coercion separate from fragment assembly/tree mutation; don't simply move the entire convertNodesIntoNode() call ahead of validation. This is only the coercion-order fix, not a request to move all of #692's validation changes here.

The existing conversion-failure test is useful, but doesn't combine a throwing value with a later cyclic argument. Please add that combination for all three methods, asserting the original error object is thrown, conversion runs exactly once, and this failed conversion leaves the tree unchanged.

[JoviDeCroock]

Could this use the same host-inclusive ancestor walk as ParentNode.validateInsertion (PARENT ?? HOST). That would add automatic support for template.content

[olavoasantos]

yea, switched the ancestor walk to PARENT ?? HOST so the prevalidation catches cycles through template.content before any earlier argument is moved.

[olavoasantos]

Agreed. before(), after(), and replaceWith() now stage every Node / string conversion once before hierarchy validation, while fragment assembly and tree mutation still happen afterward. The combined throwing-conversion + later-cycle cases preserve the original trees and throw the original conversion error.

[andrewiggins]

IIUC, won't this fire custom element reactions prematurely, when instead we would want to delay those until after this entire operation is done? I.e. if a custom element is already mounted, and then it is moved, it should never see this move to the fragment. My agent came up with this test case to demonstrate the issue:

it('commits replacements before disconnecting a moved custom element', () => {
 const parent = document.createElement('div');
 const receiver = document.createElement('em');
 const second = document.createElement('span');
 let observedChildren: unknown[] = [];

 class MovingElement extends PolyfillHTMLElement {
   disconnectedCallback() {
     observedChildren = [...parent.childNodes];
   }
 }

 polyfillWindow.customElements.define(
   'moving-element',
   MovingElement as unknown as CustomElementConstructor,
 );

 const first = document.createElement('moving-element');
 parent.appendChild(receiver);
 document.body.append(parent, first); // first must already be connected

 receiver.replaceWith(first, second);

 expect([...parent.childNodes]).toEqual([first, second]); // passes
 expect(observedChildren).toEqual([first, second]); // fails: [receiver]
});

Should we expose a private method on ParentNode that any public insertion method can use to do the actual insertion that takes an array of children to insert so we can avoid having to go through fragments all together? Just one thought off the top of my head.

[olavoasantos]

yea, this reproduced. convertNodesIntoNode() was staging through public fragment.appendChild(), so that nested reaction boundary flushed while the destination still contained receiver.

i kept the fragment conversion instead of adding the array-based mutation path, but routed both staging and the final commit through internal ParentNode primitives and collect hook effects until all links are committed. before(), after(), and replaceWith() now cover moved connected custom elements, and argument conversion happens before reading parentNode too. Your exact case passes on 9904cdd, and fresh CI is green. i force-updated the head, so this needs another look.