Skip to content

PPLT-5148: Add dialog serialization for showModal() support#2185

Open
yashmahamulkar-bs wants to merge 6 commits intomasterfrom
PPLT-5148
Open

PPLT-5148: Add dialog serialization for showModal() support#2185
yashmahamulkar-bs wants to merge 6 commits intomasterfrom
PPLT-5148

Conversation

@yashmahamulkar-bs
Copy link
Copy Markdown

@yashmahamulkar-bs yashmahamulkar-bs commented Apr 16, 2026

Summary

  • Add serializeDialogs() to stamp data-percy-dialog-modal on <dialog> elements opened via showModal(), enabling the renderer to restore top-layer positioning and ::backdrop during visual snapshots.
  • Add comprehensive tests covering showModal(), show(), setAttribute('open'), closed dialogs, and mixed scenarios.
  1. Percy's serialization works on a clone of the DOM, not the original, so that any modifications (adding attributes, rewriting
    elements) don't affect the live page the user is testing.

Looking at serialize-dom.js:115:
ctx.clone = cloneNodeAndShadow(ctx);

The pattern across all serializers (serialize-inputs.js, serialize-canvas.js, serialize-video.js, etc.) is the same:

  • Read state from dom (the original) — e.g., checking elem.matches(':modal')
  • Write changes to clone — e.g., stamping data-percy-dialog-modal

The clone is what gets serialized to HTML and sent to Percy's rendering servers. The original DOM stays untouched so the user's
page isn't disrupted during snapshot capture.

  1. These are doing very different things. serialize-pseudo-classes handles computed CSS styles for pseudo-class elements — it's about
    capturing :hover, :focus styles and injecting CSS rules. It's complex (200 lines) with XPath lookups, style computation, and CSS
    injection. Our dialog logic is about detecting showModal() and stamping a data attribute. It follows the same pattern as serialize-inputs.js, serialize-canvas.js, serialize-video.js — small, focused serializers that each handle one element type.

Output after fix

https://storage.googleapis.com/percy-dev-images/263646a46264af1f177d1270e7e091b91810b075851050f3a38a0fee50cf33be
image

Test plan

  • Run yarn workspace @percy/dom test to verify dialog serialization tests pass
  • Verify no regressions in existing DOM serialization tests

🤖 Generated with Claude Code

Serialize open <dialog> elements and stamp data-percy-dialog-modal
on dialogs opened via showModal() so the renderer can restore
top-layer positioning and ::backdrop during visual snapshots.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
yashmahamulkar-bs and others added 4 commits April 16, 2026 23:28
Without this import and call, dialog elements were never processed
during serialization, causing showModal() detection tests to fail.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Dialog elements need data-percy-element-id to be matched between
the original DOM and clone during serialization.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Remove unreachable defensive branch (clone always has open attribute)
and add error handling test to cover the catch block.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Test the early-continue paths for missing data-percy-element-id
and missing clone element to reach 100% branch coverage.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Comment thread packages/dom/src/prepare-dom.js
if (['input', 'textarea', 'select', 'iframe', 'canvas', 'video', 'style'].includes(domElement.tagName?.toLowerCase())) {
if (['input', 'textarea', 'select', 'iframe', 'canvas', 'video', 'style', 'dialog'].includes(domElement.tagName?.toLowerCase())) {
if (!domElement.getAttribute('data-percy-element-id')) {
domElement.setAttribute('data-percy-element-id', uid());
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

domElement is this domElement is cloned dom or actual dom ?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

It's the actual/original DOM. markElement is called at line 64 of clone-dom.js before the node is cloned: markElement(node, disableShadowDOM, forceShadowAsLightDOM); // line 64
let clone = cloneElementWithoutLifecycle(node); // line 66
So it stamps data-percy-element-id on the original element first, then the clone inherits it since cloneNode() copies all attributes.

Comment on lines +19 to +20
let cloneEl = clone.querySelector(`[data-percy-element-id="${dialogId}"]`);
if (!cloneEl) continue;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why not marking this clonedDom, if the actual dom have data-percy-element-id and your cloned dom is not having one, then you should setAttribute for this cloned dom as well

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

The clone already has data-percy-element-id — it's copied during cloning since cloneNode() copies all attributes. The if (!cloneEl) continue is a defensive guard for edge cases (e.g., if the clone tree structure diverges unexpectedly). This is the same pattern used in serialize-inputs.js:9, serialize-canvas.js:26, serialize-video.js:12 — none of them re-mark the clone either.


// Detect showModal() vs show()/setAttribute:
// :modal pseudo-class matches only dialogs in the top layer (opened via showModal())
if (!elem.matches(':modal')) continue;
Copy link
Copy Markdown
Contributor

@this-is-shivamsingh this-is-shivamsingh Apr 17, 2026

Choose a reason for hiding this comment

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

double check

  1. dialogElement.show() v/s dialogElement.showModal(), does this comes true in both cases
  2. Did you checked for the case when a multiple dialog elements open
  3. When dialogElement have a button click on that open another dialog elements, then we need to set priority on which dialogElement to call showModal at last. On the top most one, we should call showModal at last

Copy link
Copy Markdown
Author

@yashmahamulkar-bs yashmahamulkar-bs Apr 17, 2026

Choose a reason for hiding this comment

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

  1. Checked it
  2. Yes in that we iterate dom.querySelectorAll('dialog[open]') so every open dialog is evaluated independently.
    The "multiple dialogs with mixed open methods" test covers this one showModal(), one setAttribute('open'), one closed — and only the showModal() one gets stamped.
  3. Our code stamps all of them with data-percy-dialog-modal. The renderer then calls showModal() on each, and the browser restores the correct stacking order. Also added test cases in cli.
    Also in renderer we sort dialog element via their z index.

Verify that both outer and inner dialogs opened via showModal()
are stamped with data-percy-dialog-modal, confirming the browser's
top-layer stack is handled correctly.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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.

3 participants