Skip to content

fix(dom): resolve <slot> projection when flattening shadow DOM (PER-10812) #2437

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ninadbstack merged 2 commits into from
Sep 22, 2026
Merged

fix(dom): resolve <slot> projection when flattening shadow DOM (PER-10812) #2437

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f7db6ce
fix(dom): resolve <slot> projection when flattening shadow DOM (PER-1…
ninadbstack Sep 21, 2026
0f0913a
test(dom): cover closed shadow roots and nested hosts in slot project…
ninadbstack Sep 21, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 27 additions & 3 deletions packages/dom/src/clone-dom.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,24 @@ export function cloneNodeAndShadow(ctx) {
return;
}

// Slotted content lives in the host's light DOM and is only *rendered* at
// the slot's position by the shadow root. Flattening the root away leaves
// an inert <slot> and strands that content outside its container, so
// resolve the projection here instead (PER-10812). Every shadow root is
// flattened when forceShadowAsLightDOM is set, so any slot belonging to
// one needs resolving — including slots reached indirectly, as projected
// content nested inside another component's slotted markup. The slot
// itself never renders and isn't cloned.
if (forceShadowAsLightDOM && node.nodeName === 'SLOT' &&
typeof node.assignedNodes === 'function' && node.getRootNode()?.host) {
for (let assigned of node.assignedNodes({ flatten: true })) {
if (!ignoreTags.includes(assigned.nodeName)) {
cloneNode(assigned, parent);
}
}
return;
}

// mark the node before cloning
markElement(node, disableShadowDOM, forceShadowAsLightDOM);

Expand Down Expand Up @@ -116,8 +134,10 @@ export function cloneNodeAndShadow(ctx) {
// clone shadow DOM (including closed shadow roots captured via CDP
// and stored on window.__percyClosedShadowRoots)
let nodeShadowRoot = node.shadowRoot || getClosedShadowRoot(node);
let flattenedHost = false;
if (nodeShadowRoot && !disableShadowDOM) {
if (forceShadowAsLightDOM) {
flattenedHost = !!forceShadowAsLightDOM;
if (flattenedHost) {
// When forceShadowAsLightDOM is true, treat shadow content as normal DOM
walkTree(nodeShadowRoot.firstChild, clone);
} else {
Expand All @@ -136,8 +156,12 @@ export function cloneNodeAndShadow(ctx) {
}
}

// clone light DOM
walkTree(node.firstChild, clone);
// clone light DOM — skipped for a flattened host, whose light children were
// already placed at their <slot> positions above. Children assigned to no
// slot are dropped, matching what the browser renders for them: nothing.
if (!flattenedHost) {
walkTree(node.firstChild, clone);
}
} catch (err) {
if (!err.handled) {
handleErrors(err, 'Error cloning node: ', node);
Expand Down
4 changes: 4 additions & 0 deletions packages/dom/src/serialize-canvas.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,10 @@ export function serializeCanvas(ctx) {
// skip empty canvases
if (!dataUrl || dataUrl === 'data:,') continue;

// no counterpart in the clone means the canvas wasn't serialized — e.g.
// light DOM content no <slot> projected, which the browser doesn't render
if (!clone.querySelector(`[data-percy-element-id=${percyElementId}]`)) continue;

@ninadbstack ninadbstack Sep 22, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Low] Unquoted attribute-selector template, inconsistent with the sibling serializers

serialize-inputs.js, serialize-video.js, serialize-frames.js, serialize-cssom.js and serialize-dom.js all quote the attribute value. This line matches the pre-existing unquoted selector in createAndInsertImageElement in the same file, so it's locally consistent — but it copies a latent fragility: an unquoted CSS attribute value must be a valid CSS identifier. Today uid() returns _${random} and is always valid, so it works; if the ID format ever changes, querySelector throws a SyntaxError instead of returning null, turning a correct "not projected, skip" into a reported canvas-serialization error.

Suggestion:

Suggested change
if (!clone.querySelector(`[data-percy-element-id=${percyElementId}]`)) continue;
if (!clone.querySelector(`[data-percy-element-id="${percyElementId}"]`)) continue;

Reviewer: stack-code-reviewer


// create a resource for the canvas data
let resource = resourceFromDataURL(percyElementId, dataUrl);
resources.add(resource);
Expand Down
4 changes: 2 additions & 2 deletions packages/dom/src/serialize-frames.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,12 +147,12 @@ export function serializeFrames({ dom, clone, warnings, resources, enableJavaScr
cloneEl.setAttribute('srcdoc', p.createHTML ? p.createHTML(serialized.html) : serialized.html);
} catch { }

cloneEl.removeAttribute('src');
cloneEl?.removeAttribute('src');

@ninadbstack ninadbstack Sep 22, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Medium] Optional chaining is a partial fix — the accessible-frame branch above still assumes a non-null cloneEl

When a flattened host projects no <iframe>, cloneEl (line 75) is null, but the loop doesn't bail out. It enters the frame.contentDocument && frame.contentDocument.documentElement branch (a bare iframe's contentDocument points at about:blank), runs a full recursive serializeDOM(), adds that frame's resources to the resource set, then throws a TypeError on the unguarded cloneEl.setAttribute('srcdoc', …) — silently swallowed by the adjacent empty catch { }. Sandboxed-iframe warnings are emitted for content that never renders too. Output HTML is unaffected, so this is wasted recursion, spurious resources and a masked exception rather than a visual regression.

Suggestion: Bail out once right after cloneEl is computed on line 75, matching the pattern this PR already applies in serialize-canvas.js, serialize-inputs.js and serialize-video.js:

let cloneEl = clone.querySelector(`[data-percy-element-id="${percyElementId}"]`);
// no counterpart in the clone means the iframe wasn't rendered — e.g. light
// DOM content no <slot> projected, which the browser doesn't render
if (!cloneEl) continue;

Both ?. guards then become unnecessary. Worth also strengthening the 'does not throw for unprojected media elements in a flattened host' test to assert the recursive serialization does not run, rather than only that nothing throws — as written it tolerates this defect.

Reviewer: stack-code-reviewer


// delete inaccessible frames built with js when js is disabled because they
// break asset discovery by creating non-captured requests that hang
} else if (!enableJavaScript && builtWithJs) {
cloneEl.remove();
cloneEl?.remove();
}
}
}
Expand Down
3 changes: 3 additions & 0 deletions packages/dom/src/serialize-inputs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,9 @@ export function serializeInputElements(ctx) {
try {
let inputId = elem.getAttribute('data-percy-element-id');
let cloneEl = clone.querySelector(`[data-percy-element-id="${inputId}"]`);
// no counterpart in the clone means the element wasn't serialized — e.g.
// light DOM content no <slot> projected, which the browser doesn't render
if (!cloneEl) continue;

switch (elem.type) {
case 'checkbox':
Expand Down
3 changes: 3 additions & 0 deletions packages/dom/src/serialize-video.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,9 @@ export function serializeVideos(ctx) {

let videoId = video.getAttribute('data-percy-element-id');
let cloneEl = clone.querySelector(`[data-percy-element-id="${videoId}"]`);
// no counterpart in the clone means the element wasn't serialized — e.g.
// light DOM content no <slot> projected, which the browser doesn't render
if (!cloneEl) continue;
let canvas = document.createElement('canvas');
let width = canvas.width = video.videoWidth;
let height = canvas.height = video.videoHeight;
Expand Down
7 changes: 4 additions & 3 deletions packages/dom/test/serialize-dom.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -457,10 +457,11 @@ describe('serializeDOM', () => {

const html = serializeDOM({ forceShadowAsLightDOM: true }).html;

// When forceShadowAsLightDOM is true, shadow content becomes light DOM
// The slot element from shadow DOM will be present, and slotted content remains in light DOM
// When forceShadowAsLightDOM is true, shadow content becomes light DOM and
// slotted content is projected into the slot's position. The <slot> itself
// is dropped — outside a shadow root it renders nothing.
expect(html).toMatch('Slotted content as light DOM');
expect(html).toMatch('<slot name="title"></slot>');
expect(html).not.toMatch('<slot');
expect(html).not.toMatch('<template shadowrootmode="open"');
expect(html).not.toMatch('shadowrootserializable');
});
Expand Down
Loading
Loading