Skip to content

Improve mobile modal UX with bottom sheet style #901

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

Draft
dadofsambonzuki wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Improve mobile modal UX with bottom sheet style #901

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
6 changes: 4 additions & 2 deletions src/components/Modal.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ export function setTrigger(el: HTMLElement) {
role="dialog"
aria-modal="true"
aria-labelledby={titleId}
class="z-[2000] flex flex-col overflow-auto border border-gray-300 bg-white p-6 shadow-2xl fixed inset-0 w-full h-full dark:border-white/95 dark:bg-dark md:inset-auto md:top-1/2 md:left-1/2 md:w-80 md:max-h-[90vh] md:h-auto md:rounded-xl md:translate-x-[-50%] md:translate-y-[-50%]"
class="z-[2000] flex flex-col overflow-hidden border border-gray-300 bg-white shadow-2xl fixed inset-x-4 bottom-4 max-h-[85dvh] rounded-2xl dark:border-white/95 dark:bg-dark sm:inset-auto sm:top-1/2 sm:left-1/2 sm:w-80 sm:max-h-[90vh] sm:h-auto sm:rounded-xl sm:translate-x-[-50%] sm:translate-y-[-50%] sm:px-6 sm:py-6 px-5 py-5"
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 11, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Add a viewport backdrop/inert layer for true modal behavior.

At Line 50, the dialog is now a floating panel, but there’s no full-screen backdrop layer. With aria-modal="true", background content should not remain interactable; currently, taps outside the sheet can hit underlying page controls.

💡 Suggested structural fix 
 {`#if` open}
-	<OutClick on:outclick={() => (open = false)}>
+	<div class="fixed inset-0 z-[1999] bg-black/40" aria-hidden="true"></div>
+	<OutClick on:outclick={() => (open = false)}>
 		<div
 			bind:this={modalEl}
 			transition:fly={{ y: 200, duration: 300 }}
 			role="dialog"
 			aria-modal="true"
 			aria-labelledby={titleId}
-			class="z-[2000] flex flex-col overflow-hidden border border-gray-300 bg-white shadow-2xl fixed inset-x-4 bottom-4 max-h-[85dvh] rounded-2xl dark:border-white/95 dark:bg-dark sm:inset-auto sm:top-1/2 sm:left-1/2 sm:w-80 sm:max-h-[90vh] sm:h-auto sm:rounded-xl sm:translate-x-[-50%] sm:translate-y-[-50%] sm:px-6 sm:py-6 px-5 py-5"
+			class="z-[2000] fixed inset-x-4 bottom-4 flex flex-col overflow-hidden rounded-2xl border border-gray-300 bg-white shadow-2xl max-h-[85dvh] px-5 py-5 dark:border-white/95 dark:bg-dark sm:inset-auto sm:left-1/2 sm:top-1/2 sm:h-auto sm:w-80 sm:max-h-[90vh] sm:translate-x-[-50%] sm:translate-y-[-50%] sm:rounded-xl sm:px-6 sm:py-6"
 		>
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@src/components/Modal.svelte` at line 50, Add a full-viewport backdrop element
to enforce true modal behavior: insert a fixed inset-0 backdrop div rendered
when the modal is open (placed behind the floating panel whose current class has
z-[2000]) with a semi-opaque bg and a lower z-index (e.g., z-[1000]) that
captures clicks and calls the component's existing close handler (the same
function used to close the modal on other interactions); ensure the backdrop
prevents pointer events to underlying content and mark non-modal UI as
inert/aria-hidden while the modal is open (set document.body.inert = true or
toggle aria-hidden on the app root when showing the modal and revert on close)
so background controls are not interactable even with aria-modal="true".

Copy link
Copy Markdown
Contributor

@qodo-code-review qodo-code-review Bot Apr 11, 2026

Choose a reason for hiding this comment

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

Action required

2. Background click-through actions 🐞 Bug ☼ Reliability

On mobile the modal is no longer full-screen (fixed inset-x-4 bottom-4 ...), but there is no
backdrop element to intercept pointer events; taps above the bottom sheet can activate underlying
links/buttons and then close the modal via OutClick. This can cause unintended navigation or
actions (e.g., footer links behind LanguageModal or app cards behind AppDownloadModal).
Agent Prompt 
## Issue description
After the bottom-sheet change, the modal no longer covers the full viewport on mobile, and there is no backdrop layer. Taps outside the sheet therefore hit underlying page elements (links/buttons) and can trigger navigation/actions, then the modal closes.

## Issue Context
This is especially risky where modals are used over interactive UIs (footer nav links; app cards).

## Fix Focus Areas
- src/components/Modal.svelte[42-64]

## Suggested change
Render a full-screen backdrop behind the dialog when `open` is true, and ensure it intercepts pointer events:
- Add a `div` like `class="fixed inset-0"` (optionally with a translucent background)
- Attach `on:click={() => (open = false)}` to the backdrop
- Ensure clicks inside the dialog don’t bubble to the backdrop (`on:click|stopPropagation` on the dialog container, or structure so backdrop and dialog are siblings)
- This also prevents interaction with underlying content while the modal is open.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

>
<div class="mb-4 flex items-center justify-between">
<h2 id={titleId} class="text-lg font-semibold text-primary dark:text-white">
Expand All @@ -56,7 +56,9 @@ export function setTrigger(el: HTMLElement) {
<CloseButton on:click={() => (open = false)} />
</div>

<slot />
<div class="flex-1 overflow-y-auto -mx-5 px-5">
Copy link

Copilot AI Apr 11, 2026

Choose a reason for hiding this comment

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

The scrollable content wrapper is a flex child (flex-1) inside a column flex container with a constrained height (max-h-*). In this layout, overflow-y-auto commonly won’t work correctly unless the flex child is allowed to shrink; elsewhere in the codebase scrollable flex regions include min-h-0. Add min-h-0 (and keep overflow-y-auto) so the slot content reliably scrolls instead of overflowing the modal.

Suggested change
<div class="flex-1 overflow-y-auto -mx-5 px-5">
<div class="min-h-0 flex-1 overflow-y-auto -mx-5 px-5">

Copilot uses AI. Check for mistakes.
Copy link

Copilot AI Apr 11, 2026

Choose a reason for hiding this comment

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

The inner wrapper uses -mx-5 px-5 to counteract the modal’s horizontal padding, but the modal padding becomes sm:px-6 on larger screens. Since the inner wrapper doesn’t also switch to 6, the content area will be horizontally misaligned relative to the header on sm+. Make the negative margins/padding responsive as well (e.g., match sm:px-6 with sm:-mx-6 sm:px-6) or remove the negative margins approach.

Suggested change
<div class="flex-1 overflow-y-auto -mx-5 px-5">
<div class="flex-1 overflow-y-auto -mx-5 px-5 sm:-mx-6 sm:px-6">

Copilot uses AI. Check for mistakes.
<slot />
</div>
Comment on lines +59 to +61
Copy link
Copy Markdown
Contributor

@qodo-code-review qodo-code-review Bot Apr 11, 2026

Choose a reason for hiding this comment

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

Action required

1. Modal scroll can be clipped 🐞 Bug ≡ Correctness

The new scroll wrapper is a flex child inside a column flex parent with a max-height, but it lacks
min-h-0, so it may not shrink and the parent’s overflow-hidden will clip content instead of
allowing scrolling. Any modal whose slot content exceeds max-h-[85dvh] can become partially
inaccessible.
Agent Prompt 
## Issue description
`Modal.svelte` uses a `flex flex-col` container with `max-h-*` and `overflow-hidden`, and delegates scrolling to an inner `flex-1 overflow-y-auto` wrapper. Without `min-h-0`, the inner flex item may not shrink, so it won’t become scrollable and content gets clipped.

## Issue Context
The modal is used for list-based content (language selection, app store links) where the slot content can exceed the sheet height.

## Fix Focus Areas
- src/components/Modal.svelte[50-61]

## Suggested change
Add `min-h-0` to the scroll container (or to an appropriate flex wrapper) so it can shrink and scroll:
- e.g. `class="min-h-0 flex-1 overflow-y-auto ..."`

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

</div>
</OutClick>
{/if}
Loading