fix(ui): raise floating layers above modals so selects open in front - #2173
Open
jgt87 wants to merge 1 commit into
Open
fix(ui): raise floating layers above modals so selects open in front#2173jgt87 wants to merge 1 commit into
jgt87 wants to merge 1 commit into
Conversation
The Wiki style select in the Add Repository dialog opened behind the dialog. Radix portals SelectContent to document.body, so inside a Dialog it is a sibling of the dialog portal rather than a child of it; the DOM nesting that would normally settle paint order is gone and the raw z-index values decide it instead. --z-dropdown was 20 against --z-modal's 40, so the listbox painted under the overlay, and the overlay's backdrop-blur-sm containing block made the occlusion total rather than merely dimmed. The failure is quiet in a way that matters. Radix still gives the hidden listbox focus, so arrow keys and typeahead change the wiki style under a dropdown nobody can see. Fixed on the scale rather than at the call site. --z-dropdown moves to 55: above every surface it can be anchored inside, below --z-toast. A className override on the single SelectContent in add-repo-wizard.tsx would have fixed the reported symptom with a smaller diff, but Popover and Tooltip carry the same --z-dropdown and would fail identically the first time either is rendered inside a Dialog, so the token block now says which layers must outrank which and why. 55 rather than 45 because the command palette hardcodes calc(var(--z-modal) + 1) instead of reading --z-command; a dropdown that only cleared --z-modal would still open behind the palette. No render test could have caught this. jsdom has no paint and no stacking contexts, and the wizard's existing test renders the Select happily today. So the new guard asserts the ordering of the scale itself, and pins the components to the tokens it sorts, since an ordered scale means nothing once a portalled layer is switched to a bare z-50. Fixes repowise-dev#2172
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
--z-dropdownabove the surface tokens (20 → 55) so portalled floating layers paint in front of the surfaces they can be anchored inside. Fixes the Wiki styleSelectin the Add Repository dialog opening behind the dialog overlay.globals.css: which layers must outrank which, and why the portalling makes DOM nesting irrelevant here.packages/ui/__tests__/z-layering.test.ts, an ordering guard for the--z-*scale in the style of the existingtoken-drift.test.ts.Related Issues
Fixes #2172
The bug
Radix portals
SelectContenttodocument.body, so inside aDialogit is a sibling of the dialog portal rather than a child of it. The DOM nesting that would normally settle paint order is gone, and the raw z-index values decide:DialogOverlay—packages/ui/src/ui/dialog.tsx:20z-[var(--z-modal)]DialogContent—packages/ui/src/ui/dialog.tsx:41z-[var(--z-modal)]SelectContent—packages/ui/src/ui/select.tsx:68z-[var(--z-dropdown)]The overlay's
backdrop-blur-smalso establishes a containing block, which makes the occlusion total rather than merely dimmed.It fails quietly in a way worth noting: Radix still gives the hidden listbox focus, so arrow keys and typeahead change the wiki style under a dropdown the user cannot see.
Why the token change rather than a call-site override
Passing
className="z-[var(--z-modal)]"to the one<SelectContent>inadd-repo-wizard.tsxis a smaller diff and fixes the reported symptom. I went the other way becausePopover(popover.tsx:21) andTooltip(tooltip.tsx:20) carry the same--z-dropdownand would fail identically the first time either is rendered inside aDialog— the point-fix leaves that armed.55 rather than 45, because the command palette hardcodes
z-[calc(var(--z-modal) + 1)](packages/web/src/components/search/command-palette.tsx:105) instead of reading--z-command. A dropdown that only cleared--z-modalwould still open behind the palette.Happy to swap to the narrow fix if you'd prefer to keep the scale as-is.
On the test
No render test can catch this: jsdom has no paint and no stacking contexts, and the wizard's existing test renders the
Selecthappily today. So the guard asserts the ordering of the scale itself, and additionally pins the components to the tokens it sorts — an ordered scale means nothing once a portalled layer is switched to a barez-50.Verified it fails before the fix:
Out of scope, but noticed
--z-base,--z-elevated,--z-sidebarand--z-commandcurrently have no call sites — the palette offsets--z-modaldirectly instead of using--z-command. Left alone here; happy to fold a cleanup into a separate PR if useful.Test Plan
npm run type-check, exit 0npm run lint, exit 0 (one pre-existing warning incoverage/freshness-table-wrapper.tsx, untouched here)npm run build, exit 0 (Next.js build + VS Code extension bundle)uv run ruff check ., "All checks passed!"token-drift,brandandadd-repo-wizardtests pass alongside itpytest— not completed locally (still running when this was opened). This change touches no Python; deferring to CI's 3.11/3.12/3.13 jobs.One note on the
packages/uivitest suite: it is flaky on my machine independently of this change — 14 failures on a cleanmaincheckout, then 12 and 13 across two runs with this patch applied, with the failing sets differing between runs. Total test count moves 1671 → 1676, exactly the five added here. All tests this change could plausibly affect pass in isolation. Flagging in case that is known; I have not investigated it and it is not addressed by this PR.Checklist