feat(pds-modal): add disableTopLayer to opt out of the browser top layer - #802
Conversation
✅ Deploy Preview for pine-design-system ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
@cursor review |
pixelflips
left a comment
There was a problem hiding this comment.
Reviewed through a3c820d. Four high-confidence findings left inline on the relevant lines: two in the component, two in the new docs section. The aria-modal prose fix in a3c820d resolves the earlier inaccuracy in the Technical Notes.
Lower-confidence items (no e2e coverage for the real dialog.show() path, the isInnermostModal() z-index comparison across mixed top-layer/non-modal modals, and the unrelated pds-link turbo* drift in components.d.ts) are not raised inline.
a3c820d to
83d2b3a
Compare
pixelflips
left a comment
There was a problem hiding this comment.
Thanks for addressing the feedback. LGTM! 👍🏼
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit c16b5ac. Configure here.
Description
pds-modalopens via the nativedialog.showModal(), which promotes the dialog to the browser top layer. The top layer always paints above every normal-flow element regardless ofz-index, so any overlay a consumer mounts elsewhere in the DOM (typically appended todocument.body) renders behind an openpds-modaland is unreachable.This is a recurring, cross-team issue — commerce file picker / media-library card / Adobe Express "Edit image", TinyMCE "Formats" dropdown, and now Clubs' image uploader (its own
z-indexmodal sits behind the Pine edit modal). Patchingz-indexcan't beat the top layer, moving nodes into the modal via aMutationObserverwas abandoned (kajabi-products#51160, closed), and the only reliable workaround has been falling back to the legacy Sage modal.This adds the escape hatch phillip proposed: a prop to render the modal outside the top layer.
What changed: new boolean prop
disableTopLayer(defaultfalse):false): unchanged —dialog.showModal()(top layer, page inert). No behavior change for existing consumers.true:dialog.show()— a non-modal dialog that stays in the normal stacking context at--pine-z-index-modal, so consumer overlays with a higherz-indexcan display above it.The dimming backdrop still renders (it's the
<dialog>element styled via.pds-modal__backdrop, not the::backdroppseudo) and outside-click dismiss still works — this mode is for letting a higher-z-indexoverlay appear above the modal, not for making the modal non-blocking. What changes in this mode: the page is not madeinert,aria-modalisfalse, and the Tab focus trap is disabled so controls in an overlay stacked above the modal stay reachable by keyboard/AT. Escape closes the modal while focus is inside it, but is left to an overlay above the modal when that overlay owns focus — so a keyboard user can dismiss the overlay without losing the modal underneath it. The prop is read when the modal opens (documented as open-time-only; changing it mid-open is unsupported). Caveats (documented): the dialog now participates in normal stacking (a page element with a higher stacking context can overlap it), and don't mix adisable-top-layermodal with a default top-layer modal in one stack (dismiss targeting isz-index-based).Fixes DSS-240 (https://linear.app/kajabi/issue/DSS-240). Consumer follow-up: adopt
disable-top-layeron the Clubs post edit modal (kajabi-products#56131).Type of change
How Has This Been Tested?
disable-top-layer.Spec (stubbed
modalRef): default →showModal(),disableTopLayer→show(),aria-modalreflection. E2e (real Chromium — the only place the top-layer contract and focus/Escape behavior are observable): default is:modaltrue,disableTopLayeris:modalfalse + open +aria-modal="false", a higher-z-indexbody overlay paints above the non-modal dialog, and Escape is left to an overlay that owns focus (modal stays open) but closes the modal when focus is not held outside it. Full@pine-ds/coresuite green (3018 tests). Storybook DisableTopLayer story + MDX example demonstrate a focusable, self-cleaningdocument.bodyoverlay stacking above the modal.Reviewed via
pine-run-gauntlet(code + security + design/a11y) plus Cursor Bugbot + @pixelflips. Findings addressed: Tab focus trap disabled in non-top-layer mode; Escape left to an overlay that owns focus;aria-modal/prop documented as open-time-only; MDX heading blank-line parse fix; demo overlay now self-removes (click / Escape / modal close) so it can't orphan over the docs page.Test Configuration:
@pine-ds/core(main)Checklist:
Note
Medium Risk
Changes modal open semantics, focus trapping, and Escape handling for opt-in consumers; default top-layer behavior is preserved but a11y/stacking caveats apply when the flag is set.
Overview
Adds
disableTopLayer(defaultfalse) onpds-modalso consumers can open the dialog withdialog.show()instead ofshowModal(), keeping it in normal stacking so body-mounted overlays (file pickers, editor menus) can paint above it viaz-index.When enabled, the modal sets
aria-modal="false", skips Tab focus trapping, and only handles Escape while focus stays inside the modal (overlays above can keep Escape). Default behavior is unchanged. Docs, Storybook, generated types, and unit/e2e tests cover the new mode.Reviewed by Cursor Bugbot for commit c16b5ac. Bugbot is set up for automated code reviews on this repo. Configure here.