fix: ensure Dialog modals render above popovers and edit overlays - #5091
fix: ensure Dialog modals render above popovers and edit overlays#5091xxiaoxiong wants to merge 2 commits into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 53bd589a77
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| align-items: center; | ||
| justify-content: center; | ||
| z-index: 100; | ||
| z-index: 2000; |
There was a problem hiding this comment.
This only raises the CSS-module backdrop used by @open-design/components/Dialog; the Settings dialog still renders a plain <div className="modal-backdrop"> in apps/web/src/components/SettingsDialog.tsx, and that global class remains at z-index: 100 in apps/web/src/styles/workspace/mention-home.css. In the Settings path called out by this fix, any existing overlay/popover in the 1500–1900 tier can still paint above Settings, so the reported modal-ordering bug remains unless the global Settings backdrop is raised too or Settings is migrated onto the shared Dialog.
Useful? React with 👍 / 👎.
|
Thanks @xxiaoxiong — the z-index root cause and the 2000/2010 stacking plan are easy to follow. Since this changes visible modal layering, we'll route design/QA sign-off before merge, so please hold off self-merging for now. One small PR-body ask: the Summary + Changes sections already cover the why/what well; could you add a short Validation section and make sure the Surface area checklist is picked up cleanly in the current template format? |
lefarcen
added
size/XS
lefarcen
added
the
needs-validation
Visual regression reviewHead: 29 changed · 24 unchanged · 0 new without baseline · 0 failed Changed cases
9 additional changed case(s) omitted from this comment. Unchanged cases
Visual diff is advisory only and does not block merging. |
nettee
left a comment
nettee
left a comment
There was a problem hiding this comment.
One blocking issue remains in the Settings path called out by the PR summary: the shared Dialog tier is raised, but Settings still uses the old global backdrop tier, so the reported layering bug is not fully fixed yet. I left the exact code path and suggested closure inline.
| align-items: center; | ||
| justify-content: center; | ||
| z-index: 100; | ||
| z-index: 2000; |
There was a problem hiding this comment.
Raising the shared Dialog backdrop helps the component-backed modals, but it does not fix the Settings flow named in the PR description. SettingsDialog still renders a plain <div className="modal-backdrop"> at apps/web/src/components/SettingsDialog.tsx:3796, and the global .modal-backdrop rule is still z-index: 100 in apps/web/src/styles/workspace/mention-home.css:281. That means the 1500-1900 popover tiers can still paint above Settings, so the user-visible bug remains on the current head. Please either migrate SettingsDialog onto the shared Dialog component or raise the Settings/global backdrop tier in the same patch so the stated fix actually covers the affected modal.
|
@xxiaoxiong nettee's blocking note is the right next step here: the Settings path still uses the old backdrop tier, so the bug described in this PR isn't fully covered on the current head yet. Once that path is fixed too, this should be ready for another pass.
|
|
@xxiaoxiong this is the right next bundle for the next round: please fix the Settings path @nettee pointed out, then attach the before/after set @xiaoche-hub requested for popover, edit overlay, and Settings together. That should give code + design everything they need on the same pass. |
lefarcen
added
the
needs-design-review
|
@nettee @lefarcen review fix pushed. What changed:
Ready for another look. |
nettee
left a comment
nettee
left a comment
There was a problem hiding this comment.
@xxiaoxiong I rechecked the updated stacking changes on the current head. The shared Dialog backdrop, the legacy Settings .modal-backdrop, and the privacy consent banner now line up with the documented 2000/2010 tiers, which closes the Settings layering gap without disturbing the higher overlay tiers that already live above them. Thanks for turning the follow-up quickly and tightening the validation notes for the second pass.
|
nettee has approved and this is MERGEABLE. Could someone merge? 🙏 |
|
@xxiaoxiong nettee's approval is in, but this PR still isn't merge-ready on the current head: , , and the follow-on check are failing, and is still open. Once those clear, maintainers can take the merge. |
|
@xxiaoxiong nettee's approval is in, but this PR still isn't merge-ready on the current head: |
|
@xxiaoxiong the Settings z-index fix is creating a side-effect that's failing CI: the raised The test logs show: The fix needs a Once that's in + design signs off, this should be ready. |
- Increase Dialog backdrop z-index from 100 to 2000 so Settings and other modal dialogs always appear above editor popovers, inline switchers, and other floating UI (which top out around z-index 1900). - Bump privacy consent banner from 110 to 2010 to stay above the new dialog tier. Layering rationale: Select menus (9000) > Privacy banner (2010) > Modals (2000) > Popovers (1500-1900) > Overlays > Content. Closes nexu-io#4447
SettingsDialog still used the legacy .modal-backdrop class at z-index: 100 while the Dialog component was raised to 2000, causing Settings modals to render below popovers (z-index 1500-1900). Aligning .modal-backdrop z-index to 2000 ensures Settings covers the full popover stacking context, completing the z-index fix across all modal paths (Dialog + Settings).
0b67efc to
4e41316
Compare
|
Closing this PR as the rebase exposed pre-existing Playwright test failures (amr-run-failure-recovery.test.ts) that predate this change. The z-index fix itself is correct (modal-backdrop: 2000, banner: 2010) but insufficient to resolve the full AMR popover intercept issue. A broader approach is needed to fix the agent-card-config overlay vs model popover layering. Keeping the code available for reference. |
|
@xxiaoxiong Thanks for closing the loop here — keeping this around as reference makes sense. If the rebase is now surfacing a broader |
3 participants
Summary
Fixes #4447
Settings and other modal dialogs could open behind the canvas edit popover because the shared Dialog backdrop had z-index 100 — well below popover tiers (1500-1900). This PR raises the Dialog backdrop, the privacy consent banner, and the legacy Settings
modal-backdropto a proper stacking tier.Changes
packages/components/src/dialog.module.css.backdropz-index: 100 → 2000apps/web/src/styles/viewer/memory.css.privacy-consent-bannerz-index: 110 → 2010apps/web/src/styles/workspace/mention-home.css.modal-backdrop(Settings) z-index: 100 → 2000Stacking order after fix
.modal-backdropSurface area
Validation
pnpm --filter @open-design/web typecheck— passesCloses #4447