fix(dom): auto-detect open popovers nested in shadow DOM

[aryanku-dev]

Problem

A customer (consumer_wellness_web, Q2 ebanking) opens a native popover and snapshots it, but it renders hidden in Percy. The popover is an inner <div popover="manual"> inside a <q2-popover> Stencil web component — i.e. nested inside open shadow DOM (confirmed via the customer's DevTools: q2-dropdown(shadow) → q2-popover(shadow) → [popover]). No iframe is involved.

The only existing popover handling (markPopoverIfOpen, added in #2141) runs solely on configured pseudoClassEnabledElements, resolved via top-document getElementById/querySelectorAll/etc. Those:

1. require explicit config (the customer's :q2-popover-open was invalid CSS and matched nothing), and
2. cannot pierce shadow DOM, and the <q2-popover> host doesn't carry the popover attribute.

So the open popover is never stamped data-percy-popover-open, the renderer can't re-open it, and the UA rule [popover]:not(:popover-open){display:none} drops it from the snapshot.

Fix

Add markOpenPopovers(ctx), called from markPseudoClassElements (the pre-clone marking pass). [popover]:popover-open is an unambiguous serialize-time state (like :checked/:disabled), so it's auto-detected page-wide with zero config, including inside shadow roots.

It walks the live document + every shadow root via a marker-independent walk, eachScopeIncludingShadow (descends through live el.shadowRoot / getShadowRoot) — deliberately not walkShadowDOM, whose data-percy-shadow-host markers aren't stamped until cloning, which happens after this marking pass. Stamps are recorded on ctx._liveMutations so cleanup unstamps the live DOM; it warns once and stops querying if :popover-open is unsupported.

No renderer change needed — percy-renderer's popover-element-helper.js already traverses shadow roots and re-opens [popover][data-percy-popover-open] via showPopover().

Tests

Adds a popover auto-detection inside shadow DOM suite (packages/dom/test/serialize-pseudo-classes.test.js):

• auto-stamps an open popover inside an open shadow root with no config
• removes the shadow-DOM popover marker from the live DOM on cleanup
• does NOT stamp a closed popover inside a shadow root

Existing @percy/dom suite passes (Chrome).

Verification

Headless serialize of a faithful repro (q2-dropdown→q2-popover→[popover], opened via showPopover()): patched → 1 data-percy-popover-open="true" stamp on the [popover] element → renders; unfixed (1.31.14) → 0 stamps → display:none / dropped. No live-DOM leak after serialize.

🤖 Generated with Claude Code

[aryanku-dev]

🤖 stack:pr-review — Automated review (BrowserStack AI harness)

Auto-detects open native popovers nested inside shadow roots during the pre-clone marking pass, by walking the live document and every (open or CDP-closed) shadow root directly rather than via the not-yet-stamped data-percy-shadow-host markers. Diff is two files (serialize-pseudo-classes.js + its test). Overall: Pass.

Area	Result	Notes
Security	✅ Pass	Selectors are static literals ('[popover]:popover-open', '*') — no injection surface. Live-DOM mutations are tracked on ctx._liveMutations via the existing stampOnce and reverted by cleanupInteractiveStateMarkers, so no Percy-attribute leak into the customer tab. Test innerHTML is test-only and carries the nosemgrep annotation.
Correctness	✅ Pass	eachScopeIncludingShadow (src serialize-pseudo-classes.js:154-161) correctly reaches nested shadow content: it visits the scope, then for each light-DOM descendant resolves getShadowRoot(el) (shadow-utils:33-36 handles both .shadowRoot and the CDP closed-root WeakMap) and recurses. It does not depend on data-percy-shadow-host (which walkShadowDOM needs but which isn't stamped yet at this pre-clone stage) — exactly the bug being fixed. Cleanup is correct (reuses the shared reverted-mutation mechanism). Unsupported :popover-open is caught, ctx.warnings.add(...) fires (warnings is a Set, src serialize-dom.js:109), and supported=false halts further per-scope querying.
Testing	✅ Pass	New suite covers the three meaningful cases: auto-stamp of an open popover in an open shadow root with no pseudoClassEnabledElements config, the negative (closed popover not stamped), and live-DOM marker removal on cleanup. Guards on popoverSupported() and tidies up in afterEach. Minor gap noted below.
Performance	✅ Pass (note)	querySelectorAll('*') per scope is O(N) but runs only once per snapshot on the live DOM, pre-clone — acceptable. See note re: a second independent shadow walk.
Quality	✅ Pass	Clear intent comments, consistent with the existing markInteractiveStates / stampOnce patterns; no dead code.

Findings (non-blocking)

• Note (perf), serialize-pseudo-classes.js:297-298: markInteractiveStates already performs a shadow-DOM walk (via walkShadowDOM over :checked/:disabled), and markOpenPopovers now adds a second, independent full shadow walk in the same pass. Functionally fine (the two use different traversal strategies for a reason — popovers must run pre-marker), but on shadow-heavy pages the two walks could eventually be fused to avoid traversing the tree twice. Not required for this fix.
• Test gap (minor), test popover auto-detection inside shadow DOM: all cases use mode: 'open' shadow roots. The closed-root (CDP __percyClosedShadowRoots WeakMap) branch of getShadowRoot is exercised only indirectly via shared coverage elsewhere; an explicit closed-root popover case would lock in that path. Low priority.
• Could-not-verify (out of diff): the comment at :166-167 states the renderer's popover-element-helper re-opens [popover][data-percy-popover-open] across shadow boundaries. That helper lives outside this repo/package and was not inspected here; the claim is plausible and consistent with the existing POPOVER_OPEN_ATTR contract, but it is the load-bearing downstream assumption for this fix to render correctly.

Note: the karma browser test suite was not executed in this run (the packages/dom dev dependencies are not installed in the review sandbox). The review is a static analysis of the diff and its surrounding context; recommend confirming CI green before merge.

Overall verdict: ✅ Pass — sound fix, correct shadow traversal and cleanup, adequate tests. No blocking issues; two minor non-blocking notes.

_{Posted by the BrowserStack stack:pr-review harness.}

[aryanku-dev]

Claude Code Review (automated) — 2 inline finding(s). Full report in the PR comment below. Verdict: Passed.

[aryanku-dev]

[Low] Narrow the catch to SyntaxError

This catch latches supported = false (skipping every remaining scope) and emits the "not supported" warning on any throw. In practice querySelectorAll only throws SyntaxError for an unparseable selector, so this is correct today — but narrowing makes the intent explicit and avoids a misleading warning if some other DOMException ever surfaces here.

Suggestion:

} catch (e) {
  if (e instanceof SyntaxError) {
    supported = false;
    ctx.warnings.add(POPOVER_UNSUPPORTED_WARNING);
  }
  return;
}

Reviewer: stack:code-reviewer (reconciled)

[aryanku-dev]

[Low] Duplicated warning string

This exact literal is also used in isPopoverOpen (line 196). Extract a module-level constant so both call sites stay in sync.

Suggestion: const POPOVER_UNSUPPORTED_WARNING = 'Browser does not support :popover-open pseudo-class.'; and reference it from both.

Reviewer: stack:code-reviewer (reconciled)

[aryanku-dev]

Claude Code PR Review

PR: #2321 • Head: fd69344 • Reviewers: stack:code-reviewer (reconciled by orchestrator)

Summary

Adds markOpenPopovers(ctx) to @percy/dom's pre-clone marking pass so open native popovers ([popover]:popover-open) are auto-detected page-wide — including inside open and CDP-exposed closed shadow roots — and stamped data-percy-popover-open="true" with zero config. Previously popovers were only marked when explicitly listed in pseudoClassEnabledElements, and that path could not pierce shadow DOM (the customer's q2-popover Stencil component case). Adds a focused test suite covering open-root stamping, cleanup, closed-root-not-stamped, and the closed-root-via-WeakMap path.

Review Table

Priority	Category	Check	Status	Notes
High	Security	No hardcoded secrets or credentials	Pass	None introduced.
High	Security	Authentication/authorization checks present	N/A	Client-side DOM serialization; no auth surface.
High	Security	Input validation and sanitization	Pass	New querySelectorAll uses a fixed literal selector; no user input flows into it. innerHTML in tests is nosemgrep-annotated test scaffolding.
High	Security	No IDOR — resource ownership validated	N/A	No resource access.
High	Security	No SQL injection (parameterized queries)	N/A	No DB.
High	Correctness	Logic is correct, handles edge cases	Pass	eachScopeIncludingShadow correctly descends live shadowRoot/CDP-WeakMap roots before markers exist; stampOnce is idempotent so the new auto-detect pass and the existing config markPopoverIfOpen path cannot double-stamp. querySelectorAll does NOT pierce shadow DOM, so each scope is walked exactly once (the reviewer's O(N²) claim does not hold).
High	Correctness	Error handling is explicit, no swallowed exceptions	Pass	:popover-open unsupported → caught, warns once, latches off. Stamp loop is correctly outside the try so a stamp throw isn't misreported as "unsupported". Minor: catch could be narrowed to SyntaxError (see Findings).
High	Correctness	No race conditions or concurrency issues	Pass	Synchronous DOM pass; mutations recorded on ctx._liveMutations and reverted in cleanupInteractiveStateMarkers.
Medium	Testing	New code has corresponding tests	Pass	Open-root, cleanup, closed-not-stamped, and closed-root-via-WeakMap all covered, each gated on Popover-API support.
Medium	Testing	Error paths and edge cases tested	Partial	No test for the supported-latch / unsupported-:popover-open path, nor for nested shadow-within-shadow (the recursion the helper exists for). Non-blocking.
Medium	Testing	Existing tests still pass (no regressions)	Pass	PR states @percy/dom suite passes (Chrome); change is additive and idempotent.
Medium	Performance	No N+1 queries or unbounded data fetching	Pass	Adds a second full live-DOM + shadow-root walk on every snapshot, plus a getShadowRoot(el) probe per element. Linear, but unconditional — see Findings.
Medium	Performance	Long-running tasks use background jobs	N/A	Synchronous serialization step.
Medium	Quality	Follows existing codebase patterns	Pass	Mirrors markInteractiveStates; reuses getShadowRoot/stampOnce. Deliberately uses a marker-independent walk (documented why).
Medium	Quality	Changes are focused (single concern)	Pass	Scoped to popover auto-detection + tests.
Low	Quality	Meaningful names, no dead code	Pass	Clear names; well-commented rationale.
Low	Quality	Comments explain why, not what	Pass	Strong "why" comments on the marker-independent walk and the try-scope reasoning.
Low	Quality	No unnecessary dependencies added	Pass	No new deps.

Findings

All findings below are Low severity / non-gating polish. None blocks merge. The reviewer's two "blocking" findings did not survive reconciliation (details below).

• File: packages/dom/src/serialize-pseudo-classes.js:171 (markOpenPopovers)
• Severity: Low (Performance)
• Reviewer: stack:code-reviewer (reconciled)
• Issue: This is a second unconditional full page-walk per snapshot (in addition to markInteractiveStates' walkShadowDOM pass), and probes getShadowRoot(el) for every element. On large component-heavy pages this is measurable, and it runs even when the page has no popovers.
• Suggestion: Consider folding popover detection into the existing markInteractiveStates walk so the tree is traversed once. Do not guard with ctx.dom.querySelectorAll('[popover]') as a cheap early-exit — that query does not pierce shadow DOM and would skip the exact shadow-nested popover this PR targets. A shadow-aware existence check (or merging the passes) is the only correct optimization.

---

• File: packages/dom/src/serialize-pseudo-classes.js:183 (markOpenPopovers catch)
• Severity: Low (Robustness)
• Reviewer: stack:code-reviewer (reconciled)
• Issue: The catch latches supported = false and warns on any exception. In practice querySelectorAll only throws SyntaxError for an unparseable selector, so this is fine today — but narrowing makes intent explicit and future-proof.
• Suggestion: if (e instanceof SyntaxError) { supported = false; ctx.warnings.add(...); } return; — a non-syntax throw then skips only the current scope without a misleading "not supported" warning.

---

• File: packages/dom/src/serialize-pseudo-classes.js:185
• Severity: Low (DRY)
• Reviewer: stack:code-reviewer (reconciled)
• Issue: 'Browser does not support :popover-open pseudo-class.' is duplicated verbatim here and in isPopoverOpen (line 196).
• Suggestion: Extract a module-level const POPOVER_UNSUPPORTED_WARNING and reference it from both call sites.

---

• File: packages/dom/test/serialize-pseudo-classes.test.js
• Severity: Low (Testing)
• Reviewer: stack:code-reviewer (reconciled)
• Issue: No test exercises a popover nested two levels deep (shadow-root → shadow-host → shadow-root → [popover]) — the recursive case eachScopeIncludingShadow exists for — nor the unsupported-:popover-open latch path.
• Suggestion: Add a nested-shadow case and an unsupported-selector case (stub a scope whose querySelectorAll throws SyntaxError, assert the warning is added once and no stamp occurs).

Reconciliation notes (reviewer findings adjusted)

• Reviewer Finding 1 (CRITICAL, "O(N²)") — rejected. It assumed document.querySelectorAll('*') returns shadow-root descendants; it does not (shadow encapsulation). Each scope is walked once → ~O(N). Kept only the legitimate "second unconditional walk" perf note above. The recursive-stack-overflow concern is real only at >~1000 levels of shadow nesting (not realistic); an iterative walk is optional hardening.
• Reviewer Finding 2 (HIGH) — downgraded to Low (see catch-narrowing finding); the misleading-warning path requires a non-SyntaxError throw from querySelectorAll, which does not occur in practice.
• Reviewer Finding 4 (visited Set) — not actionable: a ShadowRoot belongs to exactly one host, so distinct elements cannot yield the same root; no real double-visit path exists.

---

Verdict: PASS — correct, well-tested fix for a real customer bug; remaining items are optional low-severity polish.