Skip to content

Add closed shadow DOM capture via CDP#33

Open
aryanku-dev wants to merge 10 commits into
mainfrom
PER-7292
Open

Add closed shadow DOM capture via CDP#33
aryanku-dev wants to merge 10 commits into
mainfrom
PER-7292

Conversation

@aryanku-dev

Copy link
Copy Markdown

Summary

  • Add exposeClosedShadowRoots() method using CDP to discover and expose closed shadow roots before DOM serialization
  • Non-fatal: catches exceptions for non-Chromium browsers and CDP errors

Ported from percy/percy-playwright#609

Test plan

  • Verify snapshot works on Chromium (closed shadow roots captured)
  • Verify snapshot works on Firefox/WebKit (CDP silently skipped)
  • Verify no regression on pages without shadow DOM

🤖 Generated with Claude Code

Use CDP to discover closed shadow roots before DOM serialization.
Closed shadow roots are inaccessible from JS (element.shadowRoot === null),
but CDP's DOM domain can pierce them. We resolve each closed shadow root
to a JS object and store it in a WeakMap that PercyDOM.serialize() reads.

- Add exposeClosedShadowRoots() using CDPSession
- Add walkNodes() helper to traverse CDP DOM tree
- Skip iframe contentDocument nodes (cross-frame not yet supported)
- Non-fatal: catches exceptions for non-Chromium browsers and CDP errors
- Called after PercyDOM injection, before DOM serialization

Ported from percy/percy-playwright#609

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@aryanku-dev
aryanku-dev requested a review from a team as a code owner April 20, 2026 18:47
aryanku-dev and others added 8 commits May 22, 2026 17:47
- page.frames() returns the entire frame tree, including same-origin
  grandchildren of cross-origin frames. Switching to mainFrame().childFrames()
  scopes detection to top-level iframes; nested capture is handled inside
  processFrame via recursion when supported.
- Pair every successful DOM.enable with DOM.disable in finally so the CDP
  session doesn't keep emitting DOM events after capture.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Commit 4afca05 switched from page.frames() to mainFrame().childFrames()
on the assumption that processFrame recurses. It doesn't — processFrame
only serializes and returns. As a result, topologies like
main(A) -> same-origin(A) -> cross-origin(B) silently dropped B.

Option A (chosen, minimal): walk the full page.frames() tree and skip
frames whose ancestor is already a known cross-origin frame. This
preserves correctness for all observed topologies while keeping each
top-level CORS frame processed exactly once (descendants are stitched
in by the in-frame PercyDOM.serialize call).

Picked over Option B (recursive processFrame) because the diff is
smaller and the existing test suite's mocking around mockPage.frames()
keeps working as-is.

Also leaves a TODO in processFrame for closed-shadow exposure inside
CORS iframes — that requires a CDP session attached to the OOPIF
target, which is materially more than ~30 LOC and is tracked separately.
Top-page closed-shadow capture is unaffected.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
A single detached backendNodeId (host or shadow) caused DOM.resolveNode
to throw, which previously aborted the whole loop and lost every
remaining closed shadow root on the page. Wrap each iteration in its
own try/catch so one bad pair only skips itself.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Covers the three regressions guarded by the BLOCKER fix and the
per-host try/catch:

- crossOriginGrandchildOfSameOriginFrameIsStillCaptured
  main(A) -> same-origin(A) -> cross-origin(B): B is emitted.
- descendantsOfACorsFrameAreNotRecapturedIndependently
  main(A) -> cors(B) -> inner(B) -> nested-cors(C): only B is emitted;
  C is handled inside B's PercyDOM.serialize.
- domEnableIsPairedWithDomDisableOnSuccess / OnException
  DOM.disable always runs after a successful DOM.enable, even when
  DOM.getDocument throws. The CDP session is always detached.
- domDisableNotCalledWhenDomEnableFails
  If DOM.enable itself throws, DOM.disable is not called (correct
  protocol shape) but detach still runs.
- oneBadBackendNodeIdDoesNotAbortRemainingShadowHosts
  One DOM.resolveNode failure skips only its pair; the remaining
  closed shadow hosts are still exposed via Runtime.callFunctionOn.

Tests are pure-mock (Mockito) — no Playwright browser required, so
they run on the full Java 8/11/17/21 CI matrix.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Percy.java (PER-7292) imports com.google.gson.* directly for parsing the
CDP DOM tree when exposing closed shadow roots, but gson was only present
transitively via the Playwright artifact. A direct compile-time source
dependency must be declared explicitly so a future Playwright upgrade that
drops or relocates gson cannot silently break compilation.

Pinned to 2.10.1 to match the version Playwright 1.44.0 currently resolves,
so no version skew is introduced. Build and offline test suites unaffected.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@aryanku-dev

Copy link
Copy Markdown
Author

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

Summary: Adds (1) closed shadow DOM capture via CDP (DOM.getDocument pierce:true → resolve hosts/roots into a __percyClosedShadowRoots WeakMap read by PercyDOM.serialize) and (2) a readiness gate (PercyDOM.waitForReady) run before serialize with diagnostics. Also fixes a CORS-iframe topology bug (main → same-origin → cross-origin frames were lost) and bumps version to 1.2.1-beta.0.

Priority Category Check Status
High Security No hardcoded secrets Pass
High Security Input validation Pass — readiness config passed as structured arg, not string-interpolated
High Security AuthZ / IDOR / SQLi N/A
High Correctness Logic & edge cases Pass — CORS dedup via hasCrossOriginAncestor; readiness preset:disabled short-circuits; merge precedence correct
High Correctness Explicit error handling Pass — catches log at debug & degrade gracefully; DOM.disable guarded by domEnabled flag
High Correctness No race conditions Pass — single CDP session, sequential per-pair loop
Medium Testing New code / error paths tested Pass — 6 topology/CDP + 3 readiness tests
Medium Testing Existing tests pass Pass — 5 SdkTest failures are pre-existing on main (live-CLI/network), not regressions
Medium Performance No unbounded fetch Pass — DOM walk bounded by page tree
Medium Quality Patterns / focused Pass — mirrors canonical percy/percy-playwright index.js
Low Quality Names / comments / deps Pass (after fix) — see Findings

Findings (fixed): Percy.java newly imports com.google.gson.* directly, but gson was undeclared in pom.xml (resolved only transitively via Playwright) — a future Playwright upgrade could silently break compilation. Fix: added com.google.code.gson:gson:2.10.1 as a direct dependency, pinned to the version Playwright 1.44.0 already resolves (no version skew). Build + offline tests still green.

Verification: JDK 17 / Maven 3.9.15. mvn dependency:tree + compile succeed; new tests 6/6 + 3/3 pass; existing CORS/cookie mock tests pass. The 5 remaining SdkTest failures are identical on main (verified via worktree).

Commits pushed: 8c4d1ad (declare gson as direct dependency).

Overall: ✅ Pass — Approve. No human-decision blockers; pre-existing live-CLI SdkTest failures predate this PR.

Posted by the BrowserStack stack:pr-review harness.

…e capture

The Java SDK captured cross-origin iframes but ignored the two opt-out
controls the JS SDK already supports, so an iframe a user marked to skip was
still captured. Port from percy-playwright/index.js:
- resolveIgnoreSelectors: per-snapshot ignoreIframeSelectors, then global
  cliConfig.snapshot.ignoreIframeSelectors; normalized to a String list.
- Per-frame lookup in the parent frame's DOM for data-percy-ignore and
  ignoreIframeSelectors matches; skip (debug log) before the switch/serialize.
  Invalid selectors swallowed; empty/missing -> no-op.
Adds PercyIframeIgnoreTest (5 tests).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@aryanku-dev

Copy link
Copy Markdown
Author

Claude Code PR Review

PR: #33Head: 824737eReviewers: stack:code-reviewer

Summary

Adds (1) closed-shadow-DOM capture via a page-level CDP session (DOM.getDocument with pierce:true, a recursive walkNodes collector, and Runtime.callFunctionOn to stash each closed root in a window.__percyClosedShadowRoots WeakMap), and (2) honoring data-percy-ignore / ignoreIframeSelectors on cross-origin iframes plus a corrected CORS frame-walk for main(A) → same-origin(A) → cross-origin(B) topologies with descendant dedupe. gson is promoted to a direct dependency for CDP JSON parsing. Two new test classes (522 LOC) cover the topology fix, DOM.enable/disable pairing (success/exception/enable-failure), per-host loop isolation, and the ignore-selector matrix. CI is fully green (Java 8/11/17/21, Analyze×3, CodeQL, Semgrep, submit-maven).

Review Table

Priority Category Check Status Notes
High Security No hardcoded secrets/credentials Pass None introduced.
High Security Input validation / injection safe Pass JS sent to evaluate/callFunctionOn is static; iframe URL/selectors passed as bound args, not concatenated into JS. No SQL/shell.
High Security No unsafe deserialization / eval Pass gson parses CDP responses into typed JsonObject; el.matches try/caught; no eval of remote content.
High Security AuthZ/AuthN sound N/A No auth surface touched.
High Correctness No null-pointer / unchecked access Pass walkNodes guards every field with node.has(...); cliConfig null-checked; flag map cast guarded by instanceof.
High Correctness Error handling / resource cleanup Pass CDP session detached in finally; DOM.disable gated on domEnabled; per-pair and outer try/catch isolate failures (3 pairing tests).
High Correctness Concurrency / thread safety N/A Single-threaded snapshot flow; no shared mutable state added.
Medium Testing New code has tests Pass New tests cover the topology fix, dedupe, enable/disable pairing (3 paths), per-host isolation, selector resolution matrix.
Medium Testing Error/edge cases tested Pass about:blank/empty/no-host filtered; empty closedPairs early-returns; non-array/non-string selectors → empty no-op.
Medium Testing Existing tests pass Pass CI Test on Java 8/11/17/21 green.
Medium Performance No N+1 / unbounded Pass DOM.getDocument(depth:-1,pierce:true) once per snapshot; ignore-flag check runs before the expensive serialize.
Medium Performance Heavy work backgrounded N/A Synchronous snapshot flow.
Medium Quality Follows patterns Pass Consistent with existing file; package-private visibility intentional for testability.
Medium Quality Focused change Pass Scoped to closed-shadow + CORS-iframe ignore handling.
Low Quality Names / no dead code Pass Thorough Javadoc; Option A vs B trade-off documented; only intentional debug logs.
Low Quality Comments explain why Pass Code-level docs comprehensive; deferred OOPIF work has a tracked TODO.
Low Quality No unnecessary deps Pass gson made a direct dep with a comment explaining why; version matches Playwright 1.44.0's transitive.

Findings

No blocking issues found.

Non-blocking observations (informational):

  • The gson version (2.10.1) is manually kept in sync with Playwright 1.44.0's transitive version; a future Playwright bump could drift. The comment documents this and a direct pin is the safer choice.
  • OOPIF closed-shadow capture is deferred (TODO at processFrame); top-page capture works. Matches the documented design and is ticketed. The closed-shadow walker intentionally skips iframe documents (same approach as the Python SDK).

Note: the PR's Percy "visual change needs review" status is a manual dashboard approval, separate from this code review.


Verdict: PASS

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