Skip to content

fix(layout): keep confirmation dialogs above map-control panels (#451) #461

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
giswqs merged 2 commits into from
Jun 18, 2026
Merged

fix(layout): keep confirmation dialogs above map-control panels (#451) #461

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0f2b676
fix(layout): keep confirmation dialogs above map-control panels (#451)
giswqs Jun 18, 2026
c4a872b
Address Claude review feedback
giswqs Jun 18, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion apps/geolibre-desktop/src/components/layout/DesktopShell.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1190,7 +1190,8 @@ export function DesktopShell({
</SectionErrorBoundary>
) : 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.

@github-actions github-actions Bot Jun 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

Suggested change
// `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.

className={`relative isolate min-w-0 flex-1 overflow-hidden ${

@github-actions github-actions Bot Jun 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@giswqs giswqs Jun 18, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

layoutOptions.compact ? "min-h-0" : "min-h-72 md:min-h-0"
}`}
>
Expand Down
Loading