[fix]: screenshot not working on <dialog> elements
#1624
Conversation
🦋 Changeset detectedLatest commit: 4781745 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Greptile OverviewGreptile SummaryFixes screenshot masking for The PR correctly identifies that top-layer UI (dialogs, popovers) renders above the normal document stacking context, so masks appended to
The implementation is well-structured with defensive programming (try-catch blocks, safe helper functions). The test coverage validates the core fix. Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Page as Page.screenshot()
participant Utils as screenshotUtils
participant Locator as Locator
participant Script as resolveMaskRect
participant Dialog as Dialog Element
participant DOM as Document
Page->>Utils: applyMaskOverlays(locators, color)
Note over Utils: Generate unique maskToken
loop For each locator
Utils->>Utils: resolveMaskRects(locator, maskToken)
Utils->>Locator: resolveNodesForMask()
Locator-->>Utils: Array of objectIds
loop For each objectId
Utils->>Script: call resolveMaskRect(maskToken)
Script->>Script: findTopLayerRoot(element)
alt Element in dialog[open]
Script->>Dialog: getBoundingClientRect()
Dialog-->>Script: rootRect
Script->>Dialog: setAttribute(data-stagehand-mask-root)
Script-->>Utils: rect with rootToken
else Element in normal flow
Script-->>Utils: rect with null rootToken
end
end
end
loop For each frame with rects
alt Has rootToken
Utils->>Dialog: querySelector(data-stagehand-mask-root)
Utils->>Dialog: Check position style
Utils->>Dialog: Save original, set relative
Utils->>Dialog: appendChild(maskOverlay)
else No rootToken
Utils->>DOM: documentElement.appendChild(maskOverlay)
end
end
Page->>Page: Capture screenshot
Page->>Utils: cleanup()
loop For each frame
Utils->>DOM: Remove all mask overlays
alt Has rootTokens
Utils->>Dialog: Restore original position
Utils->>Dialog: Remove mask-root attributes
end
end
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
No issues found across 4 files
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Architecture diagram
sequenceDiagram
participant Client
participant Util as "ScreenshotUtil (Node)"
participant DOM as "Browser DOM"
Note over Client, DOM: NEW: Top-Layer Aware Masking Flow
Client->>Util: page.screenshot({ mask: [elements] })
Util->>Util: Generate unique session maskToken
loop For each mask locator
Util->>DOM: callFunction(resolveMaskRect, maskToken)
activate DOM
DOM->>DOM: NEW: Check closest dialog[open] / :popover-open
alt Element inside Top Layer (NEW)
DOM->>DOM: Generate/Get unique rootToken
DOM->>DOM: Mark container: data-stagehand-mask-root={rootToken}
DOM->>DOM: Compute rect relative to container
DOM-->>Util: Rect + rootToken
else Standard Element
DOM->>DOM: Compute rect relative to document
DOM-->>Util: Rect (no rootToken)
end
deactivate DOM
end
Util->>DOM: applyMaskOverlays(rects, token)
activate DOM
loop For each rect
alt Has rootToken (NEW)
DOM->>DOM: Select container via [data-stagehand-mask-root]
opt Container position is static
DOM->>DOM: Save current pos -> data-stagehand-mask-root-pos
DOM->>DOM: Set style.position = "relative"
end
DOM->>DOM: Append mask <div> to container (top layer)
else Standard
DOM->>DOM: Append mask <div> to Document Body
end
end
deactivate DOM
Util->>Client: Return Screenshot Buffer
Note over Client, DOM: Cleanup Phase
Client->>Util: Cleanup callback
Util->>DOM: Remove masks & Restore DOM
activate DOM
DOM->>DOM: Remove all mask elements by token
loop For each used rootToken (NEW)
DOM->>DOM: Find container
opt Modified Position
DOM->>DOM: Restore style.position from data attribute
end
DOM->>DOM: Remove data-stagehand-mask-root attributes
end
deactivate DOM
|
One note, although not sure if relevant - I think you can have a |
|
@theoephraim good call out! i did test locally on dialogs opened via both |
|
Just if someone (or a framework) wrote html that had a dialog and used the open property directly. Your code might assume it’s in the top layer, but calling out that you may need to verify that assumption. |
|
@theoephraim that makes sense! probably could have been more clear in my above comment, but was trying to explain that this PR fixes the mask issue for both cases. ie, after this is merged, the mask will work on dialogs regardless of whether the dialog was opened via |
why
<dialog>and popovers) is painted above the normal document stacking context, so a mask appended todocument.bodycan never cover it. in order to mask dialog content, the overlay must be inserted inside the top‑layer container itself<dialog>nodes (and other top‑layer elements) didn’t work because the overlay divs were always appended to the document root, which renders underneath the dialogdialog#1616what changed
resolveMaskRect()to detect top‑layer roots (dialog[open], :popover-open), compute rects relative to that root, and return arootTokenso the caller can target the correct containerapplyMaskOverlays()to insert mask overlays inside the top‑layer root when arootTokenis present, and to restore any temporary position changes on cleanup.resolveMaskRectForObject()now passes amaskTokenand preserves therootTokenoutputtest plan
page-screenshot.spec.tsto check whether masks are correctly injected into the dialog nodeSummary by cubic
Fixes screenshot masking for elements inside dialog and other top‑layer UIs by inserting overlays into the top‑layer root instead of the document root. Masks now correctly cover dialogs and are fully cleaned up after the screenshot.
Written for commit 4781745. Summary will update on new commits. Review in cubic