Skip to content

Distinguish DOM mutation and selector errors - #692

Draft
olavoasantos wants to merge 1 commit into
polyfill-correctness-prerequisitesfrom
distinguish-dom-errors
Draft

olavoasantos wants to merge 1 commit into
polyfill-correctness-prerequisitesfrom
distinguish-dom-errors

Conversation

@olavoasantos

@olavoasantos olavoasantos commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Problem

The polyfill reported invalid tree mutations with generic Error objects and treated some malformed or unsupported selectors as non-matches. Callers could not reliably distinguish a missing child or reference node, a hierarchy violation, and invalid selector syntax from other failures or an ordinary empty result.

Impact

Minor. Code using the exposed DOM-like API cannot make DOM-style error decisions when every mutation failure is a generic error or invalid selector input silently produces no match. Mutation failures also need to report the error before they move nodes or emit Remote DOM hooks, so the local tree and remote receiver remain synchronized.

Reproduction

const parent = document.createElement('div');
const foreignParent = document.createElement('section');
const foreignChild = document.createElement('span');
foreignParent.appendChild(foreignChild);

parent.removeChild(foreignChild);
parent.querySelectorAll('');

Before this change, the first operation threw a generic Error, and the empty selector returned an empty result. They now throw errors named NotFoundError and SyntaxError, respectively. The regression tests likewise verify HierarchyRequestError for an attempted ancestor insertion; in every mutation-error case, parent/child links and insert/remove hook calls remain unchanged.

Change

  • Add a shared DOM-exception factory that returns a named DOMException when available and a named Error fallback when it is not.
  • Report NotFoundError for invalid child or reference nodes and HierarchyRequestError for invalid insertion hierarchy, with hierarchy validation taking precedence where both conditions apply.
  • Stage and validate all arguments to variadic insertion APIs before converting values or moving a cross-document node, preserving state if validation or conversion fails.
  • Reject empty, malformed, and unsupported selector syntax with SyntaxError rather than silently returning no matches; supported Unicode and double-hyphen identifiers and relative :has() selectors remain covered.
  • Reuse the shared factory for existing named custom-element, namespace, document-cloning, and event redispatch errors so the fallback behavior is consistent.

Tests

Adds focused error-contract coverage for invalid children and references, hierarchy violations (including template host relationships), all variadic insertion APIs, and conversion failures after a cross-document node has been staged. The tests assert error names, unchanged links, no hook calls, and no custom-element reactions. Selector coverage exercises malformed/unsupported syntax through the parser, querySelector(), and querySelectorAll(), and verifies the no-DOMException fallback.

Stack

This draft is the final fan-in for the subsystem stacks. The live PR remains parked on polyfill-correctness-prerequisites; its historical head also contains a bundle-size gate update that was separately landed by PR #706. An unpublished maintainer-local reconstruction isolates this error-contract layer on the fully integrated post-#706 base, but it is supporting evidence rather than a reviewable branch. After the prerequisite stacks merge, rebase this PR onto main and rerun the complete validation suite.

Validation

The live PR’s historical CI is not a current green validation signal: its lint, type-check, bundle-size, Playwright, and classified Web Platform Test checks passed, but its unit-test check failed.

An unpublished maintainer-local reconstruction passed:

  • lint;
  • type-check and build;
  • unit tests with coverage (56 files / 665 tests);
  • bundle-size checks;
  • the classified Web Platform Test suite (4/4 files); and
  • Playwright end-to-end tests (13/13).

These local results are supporting evidence only. Fresh GitHub CI is still required after the live PR is rebased.

@olavoasantos
olavoasantos changed the base branch from type-comment-hooks to polyfill-correctness-prerequisites September 3, 2026 15:28
@henrytao-me

Copy link
Copy Markdown
Member

Thanks for this. The named errors and conversion staging look right, including the narrow conversion-order fix discussed on #681. I compared 23f091e7 against its holding base 7ec42505 and native Chrome 152. There are a few corrections before this is ready:

1. Preserve CSS end-of-input recovery for supported function selectors

const root = document.createElement('section');
root.innerHTML = '<div data-kind="item"><span></span></div>';

root.querySelector('div:has(span'); // Deliberately no closing parenthesis
  • Native Chrome and the base return the div.
  • This head throws SyntaxError.
  • The same regression affects div:not(.missing and div:has(> span.

readFunctionArgument() now returns null at EOF, but the CSS Syntax function-consumption algorithm returns the function at EOF. Missing closing delimiters at EOF are not automatically invalid selectors.

Please preserve this recovery and correct the test that treats :has(.item as invalid. Keep rejecting genuinely invalid cases such as div:has(), div:has(span)), and trailing/repeated combinators. Add assertions through the parser and both query APIs.

2. Avoid the duplicate ancestor walk for single-node append/prepend

append() prevalidates, then calls appendChild(), which validates the same host-inclusive ancestry again. prepend() follows the same pattern through insertBefore().

I ran the existing 6,000-template fixture directly against both source snapshots, separating construction from serialization. Three rounds with alternating base/head order, Node 24.15.0, without Vitest/coverage:

Phase Base PR
Construction 290 / 294 / 284ms 756 / 549 / 538ms
Serialization 1-2ms 1-4ms

The warm construction runs are roughly twice as expensive. Please avoid redundant validation on the single-node path while preserving the intended multi-argument safeguards.

This demonstrates added overhead in the exact fixture that timed out in CI; it does not independently reproduce the five-second CI timeout. I would address the duplicate traversal before considering a timeout increase.

3. Cover compound-selector grammar, not just individual tokens

With the same fixture:

root.querySelector('[data-kind]div');

Native throws SyntaxError, but both base and head return the div. A type selector cannot follow the attribute selector in that compound. :not(.missing)div has the same problem.

This acceptance is pre-existing, not an introduced matcher regression, but it is a gap in the new syntax-error contract. Please validate type/universal-selector placement and cover these cases through parseSelector(), querySelector(), and querySelectorAll().

Contract and validation notes

  • The stronger atomicity for multi-argument hierarchy failures is a deliberate difference from native DOM. For parent.append(movable, parent), native can detach the supplied nodes before throwing, while this head preserves both original trees. Please distinguish that contract from native conformance; I am not suggesting moving fragment assembly ahead of validation wholesale.
  • The 37 dedicated error-contract cases passed in the recorded CI run; the full run failed on the deep-template timeout. After the corrections and final restack onto integrated main, fresh CI on the published head is still needed. The unpublished local reconstruction is useful supporting evidence, not validation of this live head.

@olavoasantos

Copy link
Copy Markdown
Contributor Author

i'm keeping this branch parked until the prerequisite stacks land, so i haven't updated this head. The structural nested-:has() rejection moved down to #679 where acceptance begins. EOF recovery, compound-selector grammar, the duplicate append/prepend validation, and the remaining named-error coverage will stay in the final #692 reconstruction on current main, followed by fresh full CI.

@henrytao-me

Copy link
Copy Markdown
Member

Thanks, keeping this parked until the prerequisites land and taking the structural nested-:has() guard from #679 makes sense. One addition to the existing EOF correction for the final reconstruction: some of the new malformed-attribute fixtures also need recovery rather than SyntaxError.

const root = document.createElement('section');
const child = document.createElement('div');
child.setAttribute('data-kind', 'item]');
root.appendChild(child);

root.querySelector('[data-kind="item]') === child;
// Native: true, despite the deliberately missing closing quote/bracket.
// Current head: SyntaxError. Base: no match.

I confirmed this in isolated Chrome 153 with CSS1Compat, against unchanged head 23f091e7 / base 7ec42505. The first two quoted-attribute fixtures and their nested :has() variants are accepted natively at EOF. Positive fixtures with the literal values item] and item"] actually select the element; these are not just silently ignored invalid selectors.

Please include string/attribute EOF recovery alongside the function recovery already discussed, and replace those error expectations with exact matching assertions through both query APIs plus parser coverage. Keep the genuinely malformed operator/trailing-junk cases rejected. Base's missing positive match is an inherited gap; head's new exception is separate. Deliberately unsupported CSS remains a different policy boundary.

Fresh validation of this unchanged head:

  • 469 polyfill and 59 focused core tests pass locally, using borrowed Vitest 4.1.2; locked polyfill TypeScript and formatting checks pass. The green suite currently includes these incorrect EOF expectations.
  • Seven public API error paths preserve the named fallback when DOMException is absent.
  • Repeated 6,000-template construction remains about 1.8x base: 291/296/291ms versus 541/534/538ms, with serialization under 3ms. This confirms redundant traversal overhead, not reproduction of the historical five-second CI timeout.

The earlier compound-selector and duplicate single-node validation corrections still apply to the planned reconstruction. No need to duplicate the upstream nested-:has() implementation here. Fresh full CI on the final restacked head is still the gate; these local checks are not integrated-stack or live Shell validation.

@olavoasantos

Copy link
Copy Markdown
Contributor Author

yea, added string/attribute EOF recovery to the final #692 reconstruction checklist alongside function EOF recovery, compound-selector grammar, and duplicate single-node validation. The live branch stays untouched until the prerequisites land; then i'll rebuild it on current main and run fresh CI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants