fix(dom): make interactive-state serialization opt-in, default off (PER-10588)

[pranavz28]

Fixes PER-10588

Three commits, separable if you'd rather land them apart:

1. grouped-selector cascade fix — the reported symptom
2. make interactive-state serialization opt-in, default off — the strategic ask from the CSS sync
3. propagate the gate into iframe serialization — found while verifying (2); without it, opt-in users lose iframe coverage

---

Why turn it off by default

The interactive-state auto-detect path has shipped four regressions in three months, all the same shape: it copies customer CSS, rewrites a pseudo-class to a data-percy-* attribute, and re-injects the copy — reordering the cascade for rules nobody asked it to touch.

Ticket	PR	Damage
PER-9775	#2324	nested selectors leaked page-wide via a bare &
PER-9836	—	pseudoClassEnabledElements froze layout / transitions
PER-10077	#2342	:checked/:disabled copies recolored Angular Material buttons across 70+ snapshots
PER-10588	this	one :hover member of an 82-selector rule repainted every colored-card heading on an AEM site

Adoption does not justify that risk. On the PER-10588 customer's captured DOM:

• injected block: 289,676 bytes, containing 629 data-percy-hover selectors
• data-percy-* state attributes actually stamped on elements: 0

Not one rule it emitted could match anything. The feature contributed zero correct behavior and all of the breakage on that page.

What the gate does

New snapshot option enablePseudoClassSerialization (boolean, default false; enable_pseudo_class_serialization accepted for the SDK path). The predicate is computed once in serializeDOM and carried as ctx.pseudoClassSerialization — one auditable switch rather than a check per call site:

const pseudoClassSerialization = !!(enablePseudoClassSerialization || pseudoClassEnabledElements);

Configuring pseudoClassEnabledElements is itself an opt-in, so existing opt-in users are unaffected without touching their config.

|| (not ??) is deliberate: PercyConfig.getDefaults() materializes snapshot.enablePseudoClassSerialization to false, so ?? would short-circuit on that materialized false and silently break users who opt in only via pseudoClassEnabledElements. Verified against the real schema — pseudoClassEnabledElements carries no default, so it is never materialized — and covered by a test and a build using that exact production shape.

No pseudo-class logic is removed — both paths are intact behind the flag.

What is deliberately NOT gated

Both verified against percy-renderer:

• Open-popover stamping. src/script/popover-element-helper.js reopens [popover][data-percy-popover-open]; its own comment reads "CLI stamps data-percy-popover-open on the popover elements." Without the stamp an open popover renders hidden behind the UA [popover]:not(:popover-open){display:none} rule.
• Custom element :state() rewriting. serialize-custom-states rewrites <style> text in place, so it cannot reorder the cascade — a different mechanism from the one causing these regressions.

The renderer consumes only shadow-host, injected, popover-open, dialog-modal, scrolltop/scrollleft and send-freeze-animation-logs. It reads none of the gated focus/hover/active/pseudo-element-id stamps, so nothing downstream regresses.

Commit 1 — the grouped-selector fix

Still needed: it makes the path correct for people who do opt in.

extractPseudoClassRules() copied a rule's entire selector list when any one member carried a pseudo-class. In clientlib-site.css on the reported site:

	Byte offset	Selectors	Declaration	Pseudo?
A	1,226,827	82 grouped, incl. .dbk-basic-box … [class*="__title"]	color:#004b6f	1 of 82: … .dbk-readmore__trigger:hover
B	1,276,597 (later, same sheet)	243 grouped, incl. .dbk-basic-box--blue … [class*="__title"]	color:#fff	none

Both specificity (0,6,0); live, B wins on source order. That one :hover member dragged all 82 of A's selectors into the injected <style> → A after B → headings dark-on-dark.

PER-10077's per-sheet anchoring doesn't help — A and B share a sheet. Fix: split the selector list on top-level commas only (paren/bracket/quote aware) and copy only pseudo-bearing members.

Commit 3 — iframe propagation

serializeFrames recurses via serializeDOM({...}) but forwarded neither pseudo-class option. Pre-PR that was invisible because the auto-detect path ran unconditionally, so iframe content was serialized. With the gate added in commit 2 and nothing forwarded, iframes got zero interactive-state serialization in every configuration — including an explicit enablePseudoClassSerialization: true.

Measured on a host page with a srcdoc iframe carrying a :hover rule (injected bytes inside the iframe's serialized srcdoc):

Bundle	default	flag on	snake_case	pseudoClassEnabledElements
master	231 B	231 B	231 B	231 B
commit 2 only	0 B	0 B ❌	0 B ❌	0 B ❌
with commit 3	0 B ✅	231 B ✅	231 B ✅	231 B ✅

The fix forwards the already-computed parent gate, so the child inherits the decision rather than re-deriving it from raw options:

pseudoClassEnabledElements,
enablePseudoClassSerialization: pseudoClassSerialization

ctx carries the computed pseudoClassSerialization, not the raw enablePseudoClassSerialization — forwarding the raw name reads undefined and silently keeps iframes dark. Covered by 4 specs.

Verification — scenario matrix

Ran the real PercyDOM.serialize() from built bundles in headless Chrome over six purpose-built pages × five config variants × master vs this branch (60 combinations), then re-served each serialized document so relative stylesheet hrefs resolve exactly as the renderer sees them, and read getComputedStyle().

1. Grouped-selector cascade — the PER-10588 shape

82 grouped selectors declaring color:#004b6f, exactly one carrying :hover; a later equal-specificity rule in the same sheet declaring color:#fff. Live page renders the headings white.

Bundle / variant	Injected	blue / green / yellow heading	beige
master (all variants)	3,838 B	rgb(0, 75, 111) ❌	rgb(0, 75, 111) ✅
branch, default	0 B	rgb(255, 255, 255) ✅	rgb(0, 75, 111) ✅
branch, flag on	147 B	rgb(255, 255, 255) ✅	✅
branch, snake_case	147 B	rgb(255, 255, 255) ✅	✅
branch, pseudoClassEnabledElements	147 B	rgb(255, 255, 255) ✅	✅
branch, flag materialized false + elements	147 B	rgb(255, 255, 255) ✅	✅

3,838 B → 147 B when opted in: only the one pseudo-bearing member is copied instead of all 82. Both the default and the opt-in path now match the live page.

2. Selector-list splitting integrity

Byte-for-byte output of the injected block, master vs branch:

  :is(.alpha, .beta)[data-percy-hover] { … }        both — comma inside :is() preserved
  [data-k="x,y"][data-percy-hover] { … }            both — comma inside quoted attr preserved
  :not(.p, .q)[data-percy-hover] > .child { … }     both — comma inside :not() preserved
  .quoted[title="a, b"][data-percy-hover] { … }     both — comma inside quoted attr preserved
- .plain[data-percy-hover], .other { … }            master: copies the non-pseudo member too
+ .plain[data-percy-hover] { … }                    branch: only the pseudo-bearing member

Same 5 hover selectors emitted either way; the only difference is the dropped non-pseudo .other. Nothing is shredded.

3. Gate routes — opt-in output is unchanged from today

Injected bytes on a page with real :hover / :active / :focus / :focus-within rules:

Variant	master	branch
default	441 B	0 B
enablePseudoClassSerialization: true	441 B	441 B
enable_pseudo_class_serialization: true	441 B	441 B
pseudoClassEnabledElements only	441 B	441 B
flag materialized false + pseudoClassEnabledElements	441 B	441 B

Every opt-in route produces byte-identical output to master. The last row is the ||-vs-?? case.

4. Carve-outs hold with the feature off

Carve-out	Metric	master	branch, default	branch, flag on
Open popover	data-percy-popover-open stamps	1	1	1
Open popover	rendered display	block	block	block
Custom element :state()	in-place rewrites	2	2	2
Custom element :state()	rendered background	rgb(31, 122, 77)	rgb(31, 122, 77)	rgb(31, 122, 77)

5. Live DOM is left clean

data-percy-* stamps remaining on the live document after serialization: 0 in all 60 combinations. A dedicated probe also confirms 0 leaked <base> elements, 0 leftover style[data-percy-interactive-states], and unchanged relative-URL resolution (new URL('assets/x.webp', document.baseURI) identical before and after) on both bundles, both variants.

Verification — Percy builds

Project test-pranav. Build #520 is the reference: the same six pages captured with the master bundle, i.e. today's shipped behavior. #521–#523 run this branch against it, so every diff is exactly what this PR changes.

Build	Config	Link	Snapshots changed vs #520
#520 reference (master bundle)	—	https://percy.io/9560f98d/web/test-pranav-8a4f5725/builds/53294971	baseline
#521 branch, default	none	https://percy.io/9560f98d/web/test-pranav-8a4f5725/builds/53294992	grouped-cascade, interactive-states
#522 branch, flag on	enablePseudoClassSerialization: true	https://percy.io/9560f98d/web/test-pranav-8a4f5725/builds/53294994	grouped-cascade only
#523 branch, legacy opt-in	pseudoClassEnabledElements	https://percy.io/9560f98d/web/test-pranav-8a4f5725/builds/53294998	grouped-cascade, interactive-states
#524 matrix — all 4 variants side by side	24 snapshots	https://percy.io/9560f98d/web/test-pranav-8a4f5725/builds/53295014	n/a (distinct names)

Reading the results:

• ⬆️ Bump eslint-plugin-import from 2.24.0 to 2.24.1 #521 (the shipped default) changes exactly two pages. grouped-cascade is the PER-10588 correction. interactive-states is the intended consequence of turning the feature off — the forced :focus styling is no longer injected. popover, custom-state, selector-integrity and iframe-host are unchanged, which is the carve-out and no-collateral-damage evidence.
• ⬆️ Bump globby from 11.0.4 to 12.0.1 #522 (opt-in) changes only grouped-cascade — the commit-1 correction. interactive-states matching the reference is the compatibility result that matters: an opt-in customer's interactive-state rendering is untouched by this PR.
• ⬆️ Bump ws from 8.1.0 to 8.2.0 #523 additionally moves interactive-states because it enables the configured-elements path, which the reference build did not have configured — expected, not a regression.
• ⬆️ Bump jasmine from 3.8.0 to 3.9.0 #524 renders all four config variants of all six pages in one build for side-by-side inspection.

Also validated directly against the config schema: accepts true/false, materializes to false, and rejects a non-boolean with snapshot.enablePseudoClassSerialization: must be a boolean, received a string.

Testing

@percy/dom, Chrome, same command on both sides, run in a clean worktree rebased on current master (3886c115):

	Pass	Fail
master baseline	457	40
this branch	472	40

Identical pre-existing failure set on both sides — focus-dependent specs that need a focused browser window; they fail the same way on unmodified master in this environment and pass in CI. No new failures.

15 new specs:

• grouped selector lists — only pseudo-bearing members copied; no shredding of :is(.a, .b) / [data-k="x,y"]; all-pseudo lists kept whole
• opt-in gate — off by default (no injected block, no stamps); on via the flag; on via the snake_case alias; on via pseudoClassEnabledElements alone; on for configured elements even when the flag is materialized false; popovers still stamped while disabled; :state() still rewritten while disabled; no data-percy-* left on the live DOM either way
• iframe propagation — off by default; on via the flag, the snake_case alias, and pseudoClassEnabledElements

The iframe specs build their iframe in their own container and remove it in afterEach rather than going through withExample, which would create a second same-id iframe inside the shadow-DOM copy and perturb the shared karma document — that leaked a <base> into live iframe documents and broke the unrelated loadAllSrcsetLinks specs downstream.

Firefox is not installed on the dev machine, so only the Chrome leg ran locally — CI covers the rest.

@percy/core was not runnable locally. Its suite binds port 8000, which is held on this machine by a pre-existing listener (an ssh tunnel plus an unrelated node process). Unmodified master fails the same way — 342 EADDRINUSE errors on master against 964 on the branch, both runs swamped rather than comparable — so this is environmental, not a signal about the change. The core-side edits are a schema addition plus threading one option through page.js and a debugProp line; they are covered by the config-schema validation above (accepts true/false, materializes to false, rejects non-boolean with the right message), by the four Percy builds — which exercise the whole config -> snapshot options -> serializeDOM path end to end through the real CLI — and by CI.

Rollout

Snapshots currently distorted by the injected copies — not just this customer's — will diff once as they return to their true colors. That diff is the correction and should be approved to re-baseline. Worth a heads-up to CE ahead of the release, since it will land across multiple accounts at once.

Evidence — original report

What	Link
Affected build (CLI 1.32.3)	https://percy.io/6bc07296/web/Webauftritt-Test-2e8c6ad4/builds/52912661
Affected snapshot	https://percy.io/6bc07296/web/Webauftritt-Test-2e8c6ad4/builds/52912661/changed/2863001336
Baseline build (CLI 1.31.13)	https://percy.io/6bc07296/web/Webauftritt-Test-2e8c6ad4/builds/51540262
Baseline snapshot (correct)	https://percy.io/6bc07296/web/Webauftritt-Test-2e8c6ad4/builds/51540262/changed/2784826354
Comparison	4793907995, diff-ratio 0.00227
Head DOM	GET /api/v1/snapshots/2863001336/assets/head.html — 698,096 bytes
Baseline DOM	GET /api/v1/snapshots/2784826354/assets/head.html — 411,417 bytes
Head screenshot	https://images.percy.io/1a328cb4f0fd1e496f644893a9e502ae48bb21a62eb296cac7834d75dcc3eeba
Baseline screenshot	https://images.percy.io/c0d6fc8cd8e5156b47f972a749231475a194ec89733042410aeed90f9817c80c
Honeycomb render pipeline (clean)	https://ui.honeycomb.io/percy/environments/production/datasets/render-pipeline-prod/result/9PUn4uqxfni
Honeycomb sidekiq (clean)	https://ui.honeycomb.io/percy/environments/production/datasets/sidekiq-prod/result/z5VsfWHZXPZ

🤖 Generated with Claude Code

[pranavz28]

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

[pranavz28]

[Medium] Backslash escapes are only honored inside quotes

This skip lives in the if (quote) branch, so a comma escaped outside a string is still treated as a list separator. \, is valid inside a CSS identifier, so a rule like .foo\,bar:hover, .baz splits mid-identifier and can emit a mangled selector into the injected <style>. Reachability is low — CSSOM usually re-escapes such identifiers when serializing selectorText — but the omission currently reads as an oversight rather than a decision.

Suggestion: hoist the escape skip out of the quote branch so \ consumes the next character in any state, and add a test with .foo\,bar:hover, .baz. If the case is genuinely unreachable via CSSOM, say so in a comment instead.

Reviewer: stack-code-reviewer

[pranavz28]

[Low] An empty pseudoClassEnabledElements object silently re-enables the page-wide path

Any object is truthy here, so pseudoClassEnabledElements: {} — valid against the schema — turns the full auto-detect path back on, including the rule-copying behind the four regressions this PR is narrowing exposure to. Same for a config whose selector lists are all empty: the user's intent there is closer to "nothing configured" than "enable everything".

Suggestion: treat a config with no non-empty member list as absent (check that at least one recognized key holds a non-empty array). A one-line test with {} would pin the intended behavior either way.

Reviewer: orchestrator (aggregation pass)

[pranavz28]

Claude Code PR Review

PR: #2399 • Head: 60de756 • Reviewers: stack-code-reviewer

Summary

Makes @percy/dom interactive-state (pseudo-class) serialization opt-in behind a new enablePseudoClassSerialization snapshot option (default false), propagates the gate into iframe recursion, and fixes extractPseudoClassRules() so it copies only the pseudo-bearing members of a grouped selector list instead of the whole list.

Review Table

Priority	Category	Check	Status	Notes
High	Security	No hardcoded secrets or credentials	Pass	No credentials, tokens, or URLs introduced.
High	Security	Authentication/authorization checks present	N/A	No auth surface; client-side DOM serialization only.
High	Security	Input validation and sanitization	Pass	Selector text comes from CSSOM selectorText (already normalized by the engine), not raw user strings. The rewrite only substitutes pseudo tokens for attribute selectors; the emitted <style> uses textContent, not innerHTML. Existing nosemgrep annotations unchanged.
High	Security	No IDOR — resource ownership validated	N/A	No resource access in this change.
High	Security	No SQL injection (parameterized queries)	N/A	No database access.
High	Correctness	Logic is correct, handles edge cases	Pass	Gate predicate computed once and threaded via ctx; || over ?? is correct because PercyConfig.getDefaults() materializes the flag to false, so ?? would break pseudoClassEnabledElements-only opt-in. The comma splitter correctly respects ()/[] nesting and quoted strings. One narrow unhandled edge case — escaped commas outside quotes (Finding 1).
High	Correctness	Error handling is explicit, no swallowed exceptions	Pass	The try/finally around live-DOM stamping and cleanupInteractiveStateMarkers is untouched; markPseudoClassElements initializes ctx._liveMutations before the new early return, so cleanup stays safe when the gate is off.
High	Correctness	No race conditions or concurrency issues	Pass	Single-pass synchronous serialization. Reordering markOpenPopovers ahead of markInteractiveStates is safe — stampOnce is idempotent per attribute and the two write disjoint attributes.
Medium	Testing	New code has corresponding tests	Pass	8 new gate tests in serialize-dom.test.js, 4 iframe-propagation tests in serialize-frames.test.js, 3 grouped-selector tests in serialize-pseudo-classes.test.js, plus defaults coverage in percy.test.js.
Medium	Testing	Error paths and edge cases tested	Pass	Covers off-by-default, flag-on, snake_case alias, config-only opt-in, flag explicitly false + config (the materialized-default trap), popover and :state() carve-outs, and no-leak-on-live-DOM. Commas inside :is() and inside attribute values are both asserted. Not covered: escaped commas (Finding 1) and 2+ level nested iframe propagation.
Medium	Testing	Existing tests still pass (no regressions)	Pass	All CI suites green at 60de756, including Test @percy/dom, Test @percy/core, Typecheck, Regression, CodeQL and Semgrep. Every pre-existing ctx fixture in serialize-pseudo-classes.test.js was migrated to set pseudoClassSerialization: true, so the old paths remain exercised rather than silently skipped.
Medium	Performance	No N+1 queries or unbounded data fetching	Pass	Net improvement: with the gate off, the full stylesheet walk and rule-copy pass are skipped entirely. The PR reports 289,676 bytes of injected CSS avoided on the reported customer page.
Medium	Performance	Long-running tasks use background jobs	N/A	No job or async work introduced.
Medium	Quality	Follows existing codebase patterns	Pass	Schema wiring matches sibling options exactly (configSchema property + snapshotSchema $ref, same as ignoreIframeSelectors / pseudoClassEnabledElements); debugProp added in discovery.js; camelCase/snake_case dual read matches the other serializeDOM options. One gap in the TypeScript surface (Finding 2).
Medium	Quality	Changes are focused (single concern)	Pass	Three related commits, each independently revertable, all scoped to the same subsystem.
Low	Quality	Meaningful names, no dead code	Pass	splitSelectorList / rewritePseudoSelectorList are accurately named; no pseudo-class logic was deleted — both paths remain intact behind the flag.
Low	Quality	Comments explain why, not what	Pass	The module header documents what is gated and, more usefully, why popover stamping and :state() rewriting are deliberately not gated.
Low	Quality	No unnecessary dependencies added	Pass	No dependency changes.

Findings

• File: packages/dom/src/serialize-pseudo-classes.js:99

• Severity: Medium

• Reviewer: stack-code-reviewer

• Issue: splitSelectorList only honors backslash escapes inside quoted strings — the if (char === '\\') i++ skip lives in the if (quote) branch. A comma escaped outside a string (valid CSS: \, inside an identifier, e.g. .foo\,bar:hover, .baz) is treated as a list separator, so the member is split mid-identifier and a mangled selector can be emitted into the injected <style>. Real-world reachability is low, since CSSOM generally re-escapes such identifiers when it serializes selectorText. The single depth counter also does not verify that (/[ pairs match, which is harmless for valid CSS but worth knowing.

• Suggestion: Hoist the escape skip out of the quote branch so \ consumes the next character in any state, and add a test with .foo\,bar:hover, .baz. If the case is judged unreachable, say so in a comment instead — right now the omission reads as an oversight rather than a decision.

• File: packages/core/types/index.d.ts:69

• Severity: Medium

• Reviewer: orchestrator (aggregation pass)

• Issue: enablePseudoClassSerialization is not declared on CommonSnapshotOptions, even though its sibling pseudoClassEnabledElements is (line 69). TypeScript consumers of @percy/core therefore cannot set the new flag without an as any cast — and this flag is the only way to keep the behavior they have today. Typecheck CI passes because the type tests only assert declared properties. Note the precedent is already inconsistent (ignoreIframeSelectors is also missing), so this is a pre-existing pattern rather than a new one.

• Suggestion: Add enablePseudoClassSerialization?: boolean; beside pseudoClassEnabledElements in CommonSnapshotOptions, and a line in index.test-d.ts next to the existing pseudoClassEnabledElements cases.

• File: packages/dom/src/serialize-dom.js:107

• Severity: Low

• Reviewer: orchestrator (aggregation pass)

• Issue: !!(enablePseudoClassSerialization || pseudoClassEnabledElements) treats any object as opt-in, so pseudoClassEnabledElements: {} — valid against the schema — silently re-enables the full page-wide auto-detect path, including the rule-copying that caused the four regressions this PR is reducing exposure to. Same for a config whose selector lists are all empty. The user's intent in that case is closer to "nothing configured" than "enable everything".

• Suggestion: Treat a config with no non-empty member list as absent — e.g. check that at least one recognized key holds a non-empty array. A one-line test with pseudoClassEnabledElements: {} would pin the intended behavior either way.

• File: packages/core/src/config.js:337

• Severity: Low

• Reviewer: stack-code-reviewer

• Issue: This is a default-behavior change, not only a fix: anyone relying on auto-detected :focus/:focus-within/:hover/:active serialization without any config loses it on upgrade and may see visual diffs. The reasoning is documented thoroughly in the PR body and commit messages, but nothing in the diff surfaces it to end users, and the repo carries no CHANGELOG.md to carry the note.

• Suggestion: Make sure the release notes call this out as a default change with the opt-back-in flag named, and confirm the external Percy docs for snapshot options gain enablePseudoClassSerialization.

Raised by other reviewers (not independently confirmed)

None — no human comments or reviews on this PR at review time.

Verification notes

• The local working tree at bd141070 does not match the pushed head; this review was performed against the diff and file contents fetched from 60de756f.
• stack-code-reviewer hit its 25-turn limit and was resumed to emit findings. Its self-reported gaps: page.js / discovery.js read only as diff hunks rather than in full, no runtime execution of the test suite (CI covers that), no constructed test for 2+ level nested iframes, and no visibility into out-of-repo SDKs that call percy-dom directly.
• Independently confirmed by the orchestrator: serializeFrames(ctx) receives pseudoClassSerialization through ctx, so the iframe propagation is real and not merely declared; all four stamping/rewrite entry points are accounted for (two gated, two deliberately not); schema coverage is at parity with sibling options (configSchema + snapshotSchema, with comparisonSchema correctly untouched); and packages/core/src/api.js:226 passes SDK options straight through, so no allowlist blocks the new flag.

---

Verdict: PASS — the gate is wired correctly and thoroughly tested; the four findings are non-blocking (two Medium polish items, two Low).