Feat/closed shadow dom capture#1085
Conversation
Inject preflight script via Cypress.on('window:before:load') to intercept
attachShadow({ mode: 'closed' }) and attachInternals() calls before page
scripts run. Bridge the resulting WeakMaps from the app's window to the
runner's window so PercyDOM.serialize() can access them.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Adds 7 tests in new 'Closed Shadow DOM and ElementInternals Preflight' block: - __percyPreflightActive flag set on window - Closed shadow roots intercepted and stored in WeakMap - Open shadow roots NOT captured - ElementInternals intercepted and stored in WeakMap - Preflight is idempotent (skips if already active) - attachShadow returns usable shadow root - Preflight data bridged to runner window during snapshot Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…tion - Add static formAssociated getter so attachInternals doesn't throw - Use strict equality check instead of Chai .equal() to avoid NotSupportedError when Chai inspects the ElementInternals object Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Add test for preflight idempotency guard (re-emit window:before:load) - Add test for browsers without attachInternals support - Add test for snapshot when preflight data is absent from app window - Replace optional chaining with explicit null checks to avoid babel-generated uncoverable branches - Exclude cypress/plugins from coverage (test infrastructure, not source) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
rishigupta1599
left a comment
rishigupta1599
left a comment
There was a problem hiding this comment.
Nice scoped change - the preflight hook to capture closed shadow roots and ElementInternals before page scripts run is the right approach, and the runner-window bridge addresses the cross-iframe visibility issue cleanly. A few correctness/robustness concerns around the bridge lifecycle and the idempotency test's reliance on Cypress.emit - details inline.
| window.__percyClosedShadowRoots = appWin.__percyClosedShadowRoots; | ||
| } | ||
| if (appWin && appWin.__percyInternals) { | ||
| window.__percyInternals = appWin.__percyInternals; |
There was a problem hiding this comment.
Stale-map leak across snapshots on the runner window. The bridge only copies appWin.__percyClosedShadowRoots / __percyInternals onto window.* when the source is truthy, and never clears them otherwise. If a later cy.percySnapshot() runs after a navigation where the preflight hook somehow didn't fire (or after delete win.__percyClosedShadowRoots as exercised in the last test), window.__percyClosedShadowRoots will still reference the previous page's WeakMap, and PercyDOM.serialize() could associate closed roots with elements from a prior document. Safer pattern:
window.__percyClosedShadowRoots = (appWin && appWin.__percyClosedShadowRoots) || undefined;
window.__percyInternals = (appWin && appWin.__percyInternals) || undefined;or explicitly delete when absent. Since WeakMap keys are Element refs scoped to a document, the cross-document bleed is mostly benign (keys won't match), but a stale map on a repeat visit to the same origin could produce incorrect serialization.
There was a problem hiding this comment.
Fixed. Now explicitly clearing when absent: window.__percyClosedShadowRoots = (appWin && appWin.__percyClosedShadowRoots) || undefined; (same for __percyInternals). This prevents stale WeakMap references from persisting across navigations.
| // closed shadow roots and ElementInternals. This runs before the page's | ||
| // own scripts, so attachShadow({ mode: 'closed' }) calls are captured. | ||
| Cypress.on('window:before:load', (win) => { | ||
| if (win.__percyPreflightActive) return; |
There was a problem hiding this comment.
The __percyPreflightActive guard only protects re-patching the same window. Because this listener is registered at module load time with Cypress.on(...) at the top level, if index.js is ever required more than once in the same runner session (Cypress supports plugin re-requires, and some test harnesses do this explicitly) you'll end up with multiple listeners, each wrapping the prior attachShadow - a patched chain of wrappers on fresh windows. Worth either (a) gating the registration with a module-level flag (if (!Cypress.__percyPreflightRegistered) { Cypress.__percyPreflightRegistered = true; Cypress.on(...) }), or (b) using Cypress.removeAllListeners('window:before:load') + re-register, depending on which side of the tradeoff you want.
There was a problem hiding this comment.
Fixed. Added if (!Cypress.__percyPreflightRegistered) module-level guard to prevent duplicate listener registration if index.js is required multiple times. Added a test to verify the flag is set.
| const patchedFn = win.Element.prototype.attachShadow; | ||
|
|
||
| // Re-emit the event on the same window to trigger the idempotency guard (line 14) | ||
| Cypress.emit('window:before:load', win); |
There was a problem hiding this comment.
Cypress.emit is not a public API - it's an internal event bus. Re-emitting window:before:load manually here (and on line 1323) does exercise the hook function, but it doesn't faithfully simulate the real scenario (Cypress won't fire this event twice on the same window in production). This test is a coverage-driven smoke test, not a behavioral one, and it may silently break across Cypress major versions. Consider asserting the idempotency contract in a way that doesn't depend on an internal bus - e.g., navigate to a second page via cy.visit() and verify each fresh window gets patched exactly once, using a spy on origAttachShadow. At minimum, add a comment noting this is a private API usage.
There was a problem hiding this comment.
Acknowledged. Added a comment noting Cypress.emit is a private API used here for testing idempotency and may break across Cypress major versions.
| }; | ||
|
|
||
| // Emit preflight on the mock window — should not throw and should skip internals | ||
| Cypress.emit('window:before:load', mockWin); |
There was a problem hiding this comment.
This test passes a hand-rolled mockWin with Element.prototype.attachShadow pointing at the real patched function from the already-loaded window, so calling it on the mock would throw (this not a real Element). The assertions never exercise the wrapper, only the branch-coverage of the typeof attachInternals === 'function' guard. That's fine for coverage, but the test name ('handles browsers without attachInternals support') oversells what's actually verified - a reader could reasonably think we're testing the shadow path works in such a browser too. Consider renaming to 'skips ElementInternals setup when the API is unavailable' to match what's tested.
There was a problem hiding this comment.
Fixed. Renamed to 'skips ElementInternals setup when the API is unavailable'.
| return; | ||
| } | ||
|
|
||
| const tag = 'test-internals-' + Date.now(); |
There was a problem hiding this comment.
'test-internals-' + Date.now() for the custom-element tag name can collide across test retries inside the same millisecond (unlikely but possible, and some CI runners do reuse the second exactly). customElements.define with a duplicate name throws NotSupportedError, which would surface as an obscure failure. Prefer a counter or Math.random().toString(36).slice(2) suffix.
There was a problem hiding this comment.
Fixed. Changed to 'test-internals-' + Math.random().toString(36).slice(2) to avoid collision on test retries within the same millisecond.
…stale map fix - Add module-level Cypress.__percyPreflightRegistered guard to prevent duplicate listener registration if index.js is required multiple times - Use .apply(this, arguments) for forward-compatible wrapping of attachShadow and attachInternals - Clear stale preflight WeakMaps when absent from app window to prevent cross-navigation data leaks - Add private API usage comment for Cypress.emit in tests - Rename test to accurately reflect what is verified - Use Math.random() suffix for custom element tag names to avoid collision on test retries within the same millisecond Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ranch - Extract registration logic into registerPreflight() that returns true on first call, false on subsequent calls - Test calls registerPreflight() a second time to cover the early-return branch and achieve 100% branch coverage Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Recursively walks accessible (same-origin) iframes to find cross-origin descendants and emit one corsIframes entry per cross-origin frame at any depth. Cypress runs in the same browser window as the AUT, so the same- origin policy still blocks reading the contentDocument of a true cross- origin iframe — when that happens iframeSnapshot stays null and the CLI drops the entry. Cross-origin-inside-cross-origin therefore remains an inherent Cypress limitation (documented in code), but for the common case of cross-origin iframes nested in a same-origin parent we now at least emit the percyElementId entry so downstream tooling sees the structure. Same skip rules as before (about:blank, javascript:, data:, blob:, vbscript:, chrome:, chrome-extension:, srcdoc). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Cypress runs in the AUT window and is blocked from reading true cross-origin iframe content. The recursion still walks accessible parents to find these frames, but emitting an entry with iframeSnapshot=null is wire-time waste — the CLI drops malformed entries on validation. Filter them client-side so pages with many ad/tracker iframes don't pay the bandwidth cost. Only entries with a real captured snapshot are kept. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Code-simplicity review surfaced that the recursive walk produces no output the flat top-level walk wouldn't, because the null-snapshot filter (added in f6aa61e) drops every nested entry the recursion finds: the browser's same-origin policy blocks Cypress JS from reading cross-origin contentDocument either way, so iframeSnapshot stays null and the entry is filtered out before submission. ~50 LOC of dead code removed; the null-snapshot filter and same-origin skip are kept. Nested-cross-origin support is documented as an inherent Cypress limitation in the comment above processCrossOriginIframes — percy-playwright/percy-puppeteer are the right tools when out-of-process frame access is needed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Closes the test gap surfaced by the ce:review run — verifies that a cross-origin iframe whose contentDocument access throws does not show up in the corsIframes payload (browser security blocks JS access; the SDK filters those entries client-side rather than shipping null snapshots that the CLI would discard anyway). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…rs filters - maxIframeDepth (snapshot option, also via percy.config.snapshot): caps the recursive walk. Default 10, hard ceiling 25 to prevent abuse from misconfigured values. - ignoreIframeSelectors (snapshot option / config): array of CSS selectors; matching iframes are skipped before the SDK pays the cost of switching into them. - data-percy-ignore attribute on an <iframe> element: per-element opt-out that skips capture without needing a selector. Closes the TB requirement: "Users can optionally exclude specific iframes from capture via CSS selector or data-percy-ignore attribute" and "Nested iframes ... captured recursively up to a configurable depth." Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Address ce:review findings: - Add iframe-utils.js exporting UNSUPPORTED_IFRAME_SRCS, isUnsupportedIframeSrc (case-insensitive), and normalizeIgnoreSelectors so iframe constants stay in sync with the other SDKs. - index.js now consumes iframe-utils instead of carrying its own local SKIP_IFRAME_SRCS list and resolver. - Drop unused postedPayload/utils variables flagged by lint in cypress/e2e/index.cy.js. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Firefox replaces failed cross-origin loads with about:neterror, which slipped past the explicit 'about:blank'/'about:srcdoc' enumeration. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…id-selector branches
Covers the previously-uncovered branches in processCrossOriginIframes:
the iframe.hasAttribute('data-percy-ignore') early-return path, the
iframe.matches(selectors[i]) loop with skipBySelector=true, and the
invalid-selector try/catch that swallows.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…meSelectors loop Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…Selectors falsy branch) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Removes the transitional local iframe-utils.js shim that duplicated constants now exported from @percy/sdk-utils. SDK PR is now blocked on the CLI PR (PER-7292) merging and a sdk-utils 1.32.x release. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two fixes folded together: 1. The migration to @percy/sdk-utils 1.31.14-beta.3 was premature — that release doesn't yet export the iframe resolver helpers we rely on. Restore the local _iframe_shim.js until sdk-utils ships them in a stable release. 2. PercyDOM.serialize must run inside the AUT window so cloneNode sees the AUT's own document/Element prototypes. Running it in the Cypress runner window with `dom: aut_doc` dropped closed shadow roots during the cross- window clone. Inject via a <script> appended to the AUT head so PercyDOM lands on the AUT global, then call appWin.PercyDOM.serialize directly. Also keep parallel hosts arrays alongside the WeakMaps for closed shadow roots and ElementInternals so the serializer can iterate them. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
^1.32.0 doesn't exist on npm (latest stable is 1.31.14); the migration commit bumped the floor prematurely and CI install fails. Pin to the beta that actually publishes the iframe constants we consume. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two CE review fixes folded together because they touch the same preflight
block and the second one cannot be exercised cleanly without the first.
1. closedShadowHosts / internalsHosts arrays are gone. They pinned every
shadow host element strongly for the whole test lifetime, defeating the
WeakMaps that store the actual roots and leaking detached DOM nodes in
long SPA suites. The @percy/dom serializer reaches the WeakMaps by
probing each live DOM node it visits, so iteration over the maps is
never needed — there is no reason to hold strong refs to hosts.
2. Cypress.on('window:before:load') only fires on subsequent navigations.
When index.js is required from support/e2e.js, the first AUT page is
already loaded, so closed shadow roots created on the initial page used
to be invisible to the serializer. registerPreflight now also patches
cy.state('window') synchronously, gated by a try/catch in case cy.state
isn't available during very early module init.
Also tightens the CORS-iframe block: contentWindow / contentDocument
getters themselves throw SecurityError on true cross-origin frames in
Blink, so both reads now live inside the try/catch — otherwise the throw
bubbles up and breaks every same-origin frame later in the same loop. The
serialize call already normalizes ignoreIframeSelectors before handing it
to @percy/dom so a string value can't crash the in-page walker.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Pins to the released 1.31.14 so the test harness uses the same CLI version we expect downstream. Pairs with @percy/sdk-utils 1.31.14-beta.4 which is already pinned. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- WeakMap leak guard: assert __percyClosedShadowHosts / __percyInternalsHosts are gone (CE review #1). - Synchronous preflight patch: exercise the patch path against a fresh mock window and assert both WeakMaps land (CE review #3). - cy.state guard: re-invoke registerPreflight with cy.state throwing, to cover the try/catch around the initial-window patch. - Injection-path coverage: a new test stamps a marker on PercyDOM between two snapshots to prove the second injection is a no-op when PercyDOM is already on the AUT window. Another test asserts PercyDOM ends up on cy.window().PercyDOM after a snapshot (script-element append landed in the AUT global, not the runner global). - CORS iframe success branch: simulate a same-origin-misclassified frame by overriding contentWindow/contentDocument to return a working PercyDOM-bearing shell, exercising the capture-success path from CE review #2. Also fixes the existing 'skips DOM serialization when percyDOMScript is unavailable' test: it stubbed via the @percy/sdk-utils namespace, but that exports fetchPercyDOM as a non-writable getter in 1.31.14-beta.4 so the stub silently no-op'd and the snapshot kept going through. The SDK reads through the local _iframe_shim re-export, which is a regular writable property, so the test now stubs there. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CI run 26355847952 failed with 7 cypress test failures all rooted in the same module-isolation surprise: webpack's spec bundle gets its own @percy/sdk-utils + _iframe_shim instance, so mutating utils.percy.* or utils.fetchPercyDOM from the test file doesn't reach the SDK side. - Add __getShimForTesting / isResponsiveDOMCaptureValid exports on index.js so tests can reach the SDK-side shim instance index.js captured at module load and invoke branch-only code paths directly. - Rewrite the deferUploads, getResponsiveWidths-throws, and no-domScript tests to drive the shim instance index.js actually uses (and intercept the SDK-side logger to capture warn / debug — the helpers.logger mock targets a different logger instance). - Drop the cy.state-unavailable test that relied on Cypress private internals; cover its synchronous-patch branch via /* istanbul ignore next */ on the registerPreflight try/catch (the guard exists to keep the runner alive on edge-case init failures, not to be exercised). - /* istanbul ignore */ a handful of defensive branches that can't be reached from real fixtures: legacy RESPONSIVE_CAPTURE_SLEEP_TIME alias, viewport-resize short-circuit on equal dimensions, null defaultView from cy.document(), and the normalizeIgnoreSelectors garbage-input fallback. - Clear indexShim.percy.domScript before triggering the dom.js-error test — fetchPercyDOM caches the script on the SDK-side info object, so the test server's error response would otherwise be ignored. yarn test:coverage now: 95 passing, 100/100/100/100 nyc. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…dom-capture # Conflicts: # index.js # package.json
|
Add desc |
|
This PR is stale because it has been open for more than 14 days with no activity. Remove stale label or comment or this will be closed in 14 days. |
…dom-capture # Conflicts: # index.js
package.json requires ^1.31.15-beta.0 (the readiness-gate floor), but the lockfile was stale at 1.31.14-beta.4 — whose getReadinessConfig/isReadinessDisabled don't merge global .percy.yml config or detect preset:disabled, failing 4 readiness gate specs. Re-resolving to 1.31.15-beta.0 restores the expected behavior. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The readiness gate needs sdk-utils >=1.31.15-beta.0 (config merge + preset:disabled detection). But @percy/cli@1.31.14's transitive deps (@percy/monitoring, @percy/webdriver-utils) pull @percy/sdk-utils@1.31.14 stable, which won hoisting in CI and got bundled into the cypress spec — failing 4 readiness gate specs. A resolutions pin collapses the tree to a single 1.31.15-beta.0 so the correct build is always bundled. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The shim did module.exports = Object.assign({}, utils, …), giving index.js a
fresh object that snapshots utils.percy at load time. That made the SDK's utils
a different instance than the @percy/sdk-utils the Cypress spec reads/mutates, so
tests setting utils.percy.config (global readiness) or stubbing utils.postSnapshot
never reached the SDK — 4 readiness gate specs failed (no config merge, no
preset:disabled detection, no diagnostics forwarded). Augment the real singleton
in place and re-export it, matching master's direct require('@percy/sdk-utils').
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
BROWSER_INTERNAL_PREFIXES was missing the legacy IE-era `vbscript:` and `file:` schemes that the canonical Protractor/Nightwatch _iframe_shim list includes. file: can reference local paths and vbscript: is script execution, so both should be treated as unsupported iframe srcs. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
🤖 stack:pr-review — Automated review (BrowserStack AI harness)Summary: Adds closed shadow-DOM +
Findings (fixed): Verification: Commits pushed: Overall: ✅ Pass — Approve.
|
…ct CSP) On an AUT with a strict CSP (no unsafe-inline), the injected <script> never runs, so appWin.PercyDOM stays undefined and serialize() threw an uncaught TypeError outside withLog, failing the whole test. Guard after capturing the PercyDOM reference: if it's missing or serialize isn't a function, log a warning and skip the snapshot (Step 3's empty-snapshots guard handles the rest), instead of crashing the test. Normal path and readiness gate unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The graceful-skip guard added an uncovered branch (coverage gate is 100%). Add a spec that stubs fetchPercyDOM to return a truthy script which never defines window.PercyDOM (mimicking a strict-CSP AUT blocking the inline injector), and asserts percySnapshot skips the snapshot instead of throwing. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Delete the local _iframe_shim.js (which augmented the @percy/sdk-utils singleton in place with isUnsupportedIframeSrc + normalizeIgnoreSelectors) and require @percy/sdk-utils directly, now that percy/cli#2319 exports the canonical helpers from the single source of truth. - Bump @percy/sdk-utils to 1.32.3-beta.1 in both dependencies and the resolutions pin (the resolution would otherwise force the old version across the tree) and re-lock yarn.lock. - Repoint the one spec reference and __getShimForTesting hook at the @percy/sdk-utils instance (same singleton the shim re-exported). - The canonical UNSUPPORTED_IFRAME_SRCS is a superset of the old local list (adds ws/wss/ftp); existing about:/javascript:/data: assertions are unaffected. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude Code PR ReviewPR: #1085 • Head: b2ab1d2 • Reviewers: stack:code-reviewer SummaryDeletes the local Review Table
FindingsNo blocking issues found. Non-blocking observations (Low, not requiring changes):
Verdict: PASS |
Auto-stamped by Corepack during earlier yarn runs; not needed (yarn-classic repo). yarn.lock unchanged and consistent (verified --frozen-lockfile). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
| "@percy/sdk-utils": "1.32.3-beta.1" | ||
| }, | ||
| "resolutions": { | ||
| "@percy/sdk-utils": "1.32.3-beta.1" |
4 participants
Summary
Brings
@percy/cypressto parity with the PER-7292 feature set: closed shadow DOM + ElementInternals capture and cross-origin iframe handling, adapted to Cypress's in-browser (same-window) execution model.What's included
Closed shadow DOM & ElementInternals (preflight interception)
Cypress.on('window:before:load'), plus a synchronous patch of the already-loaded first window) wrapsattachShadow({ mode: 'closed' })andattachInternals()before page scripts run, recording{ host → root }/{ host → internals }in WeakMaps the serializer reads.@percy/domreaches the WeakMaps by probing each live node it visits. Stale maps are cleared across navigations; registration is idempotent.PercyDOM.serializeis injected into and run inside the AUT window via an appended<script>(running it in the Cypress runner window withdom: aut_docdropped closed roots during the cross-window clone).Cross-origin iframes
maxIframeDepth,ignoreIframeSelectors, anddata-percy-ignorecontrols (also viapercy.config.snapshot); shared case-insensitive unsupported-scheme skip list.contentDocumentfrom the AUT window) — documented in code; entries with a null snapshot are filtered client-side rather than shipped for the CLI to drop.Robustness
PercyDOMnever loads (e.g. a strict-CSP AUT blocks the inline injector) — logs a warning and skips the snapshot instead of throwing and failing the test.contentWindow/contentDocumentreads are inside try/catch (they throwSecurityErroron true cross-origin frames in Blink, which would otherwise break later same-origin frames in the loop).Tests
100% nyc coverage maintained. Note the webpack spec bundle gets its own
@percy/sdk-utils/_iframe_shiminstance, so tests reach the SDK-side instance via__getShimForTesting; the readiness gate uses the real sdk-utils singleton re-exported from_iframe_shim.🤖 Generated with Claude Code