fix(layout): keep confirmation dialogs above map-control panels (#451)#461
Conversation
Map-control panels (e.g. maplibre-gl-basemap-control) append their panel to the map container with `position: absolute; z-index: 1000`. The map area `<main>` was only `position: relative`, so that z-index escaped to the root stacking context and rendered above body-portaled modal dialogs (z-50) such as the layer-removal confirmation, leaving part of the dialog unreadable behind the panel. Make `<main>` an isolating stacking context (`isolation: isolate`) so all map-control panels appended to the map container stay contained below the modal layer. This fixes the reported basemap case and the whole class of map-panel-over-dialog conflicts without touching the shadcn z-index scale (so Radix popovers/selects nested inside dialogs keep working).
✅ Deploy Preview for geolibre-app ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds the ChangesMap Container Stacking Context Fix
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // layer. Without it those z-indexes escape to the root stacking | ||
| // context and cover body-portaled dialogs like the layer-removal | ||
| // confirmation (z-50). See issue #451. | ||
| className={`relative isolate min-w-0 flex-1 overflow-hidden ${ |
There was a problem hiding this comment.
Potential mobile regression for position: fixed control-panel dropdowns (medium confidence).
basemap-style.ts, swipe-style.ts, lidar-style.ts, and geoagent-style.ts all render a position: fixed; z-index: 10000 dropdown menu that is appended directly to map.getContainer() — i.e. inside <main>.
Before this fix those fixed dropdowns had z-10000 in the root stacking context. On mobile, LayerPanel and StylePanel have max-md:z-30 on their root elements, so they were stacking contexts at z-30 in the root — below the dropdowns.
After this fix <main> is an isolated stacking context with z-index: auto in the parent context. The position: fixed elements (layout unchanged — they still position relative to the viewport) now have their z-index evaluated within <main>. When the browser resolves the paint order it compares <main> (z-auto) against LayerPanel/StylePanel (z-30), making those panels paint on top of everything inside <main>, including those fixed dropdowns.
In practice this is low-risk because on mobile those panels cover the whole map area and a user is unlikely to have both a map-control dropdown and a mobile overlay panel open simultaneously. But it is a change in z-order that wasn't called out in the PR description and wasn't covered by the Playwright causal test, so it is worth confirming with a manual smoke-test on a mobile viewport.
There was a problem hiding this comment.
Thanks - this is an accepted, intentional consequence of the containment fix rather than a regression to patch. The whole point of isolate on <main> is to keep the map-control dropdowns (z-index: 10000, appended to the map container) contained below body-portaled dialogs; a side effect is that on a mobile viewport those dropdowns no longer escape above the full-screen LayerPanel/StylePanel (max-md:z-30). Since those panels cover the entire map area on mobile, having both a map-control dropdown and a panel open at once is not a realistic state, which matches your low-risk assessment. Leaving this thread open so a maintainer can confirm with the manual mobile-viewport smoke-test you suggested before resolving.
Code reviewThis is a small, well-reasoned CSS fix. The core mechanism is correct: Bugs
SecurityNothing to flag. PerformanceNothing to flag. Quality
CLAUDE.md
Overall: The fix is mechanically correct and solves the reported issue cleanly. The two actionable items are fixing the |
- Condense the `<main>` isolate comment to a single line and correct the cited map-panel z-index from 1000 to 10000 (the four control-panel style sheets all use z-index: 10000).
| ) : null} | ||
| <main | ||
| className={`relative min-w-0 flex-1 overflow-hidden ${ | ||
| // `isolate` creates a stacking context so map-panel z-indexes (up to 10000) stay below body-portaled dialogs. See #451. |
There was a problem hiding this comment.
Minor nit: the comment says "up to 10000" but the PR description (and the basemap control's actual CSS) cites z-index: 1000. The order of magnitude difference could mislead a future reader. Consider using the documented value or a loose upper-bound phrasing:
| // `isolate` creates a stacking context so map-panel z-indexes (up to 10000) stay below body-portaled dialogs. See #451. | |
| // `isolate` creates a stacking context so map-panel z-indexes (≤ 1000) stay below body-portaled dialogs. See #451. |
Code reviewThe fix is correct and well-reasoned. without altering layout, flex behaviour, overflow clipping, or any other box properties. Radix UI dialogs/popovers/selects all portal todocument.body(outside``), so they remain in the root stacking context and are unaffected. MapLibre markers, popups, and control panels are DOM children of the map container inside ``, so they are correctly contained by the new stacking context. The sibling LayerPanelandStylePanelelements also sit outside``` in the DOM, so they are unaffected.
FindingsQuality
Bugs / Security / Performance / CLAUDE.md Nothing to flag. The one-line change is minimal-blast-radius, the Tailwind |
1 participant
Summary
Fixes #451. When a map-control panel is open (the reporter hit it with the basemap control), the layer-removal confirmation dialog renders behind the panel, leaving part of the text unreadable.
Root cause
Map-control packages (e.g.
maplibre-gl-basemap-control) append their panel to the map container —map.getContainer().appendChild(panel)— withposition: absolute; z-index: 1000. The map area<main>was onlyposition: relative(not an isolating stacking context), so thatz-index: 1000escaped to the root stacking context and beat the body-portaled shadcnDialog(z-50). The.maplibregl-control-container(z-index: 2) does not help here because the panel is appended to the map container directly, not the control container.Fix
Add
isolation: isolateto<main>so it becomes its own stacking context. Every map-control panel appended to the map container is then contained below the modal layer. This fixes the whole class of map-panel-over-dialog conflicts without touching the shadcn z-index scale (so Radix popovers/selects nested inside dialogs keep working).<main>already hasoverflow-hidden, so nothing relied on panels escaping it.Verification
Drove the real running app with Playwright and ran a causal stacking test against the actual
.maplibregl-mapcontainer (the node the basemap control appends to) and the real<main>:mainisolated)main.style.isolation='auto')Confirmed
<main>is now the only stacking context between the map container and<body>. Pre-commit (eslint + full build) passes; app renders with no layout regression.Summary by CodeRabbit