Merge pull request #2834 from arc53/iphone-modal-fixes

Iphone modal fixes
This commit is contained in:
Alex authored and GitHub committed 2026-09-27 10:42:34 +01:00
commit d37e0bd1ad
13 files changed
+449 -26

No files matched your search

+28 -3
View File
@@ -869,6 +869,20 @@ drag; pass `showCloseButton` to keep an X).
and `max-h-sheet` are the `index.css` utilities for the safe-area insets; never
spell `env()` in a class.
Closing a bottom sheet resets iOS Safari's bottom bar. Safari 26 takes the bar's
colour from a fixed element that appears on the bottom edge, so an open sheet
turns it `bg-card`, and it never changes back when the sheet goes away or the
page changes colour. `SheetContent side="bottom"` and `Modal`'s phone sheet
mount `BottomTintReset` (`ui/bar-tint-reset.ts`), which shows a 6px
`bg-background` strip on the edge for one painted frame after the sheet
unmounts, and Safari samples the page colour again. `useDarkTheme` calls
`resetBottomTint()` and `resetTopTint()` (a 16px strip on the top edge) when the
theme changes. Anything else fixed to the bottom edge that can go away (a
drawer, a bottom banner) calls `resetBottomTint()` when it is removed. The sizes
and the one-frame life were measured on an iPhone: 4px at the bottom, 6px at
the top and a 16ms timer all failed, and a strip behind the content,
transparent or at `opacity-0` isn't sampled, so don't shrink or hide it.
**Open question: side panel or right sheet for chat content.** Chat has two
ways to show something beside an answer. Notes, todos and files open in
`components/ArtifactSidebar`, a panel that takes a column and leaves the chat
@@ -1051,6 +1065,17 @@ shows.
so plain elements should become components rather than copy the classes.
Inside `ui/`, spell it with the `focusRing` constant from `lib/utils.ts`
(with `invalidState` and `fieldFrame` for fields) rather than retyping it.
- **Focus return**: Modal, Sheet and DialogContent give focus back on close
only to the element that had it when they opened (`ui/use-focus-return.ts`).
A tap on iOS, and a click in Safari, doesn't focus the trigger, so Radix's
default `trigger.focus()` lit it with a ring nobody asked for; after a
keyboard open the trigger did have focus and still gets it back. Don't pass
`onCloseAutoFocus` from app code to remember focus yourself (ESLint rejects
it); the primitive already does. `onOpenAutoFocus` with `preventDefault()`
is still how a phone picker keeps the keyboard down (`MultiSelectPopover`).
A dialog panel is a `tabIndex=-1` focus target that Radix can focus, so it
carries `outline-none`: Safari draws its own blue `outline: auto` there and
ignores our `outline-color`. Any new focusable container needs the same.
- **Close buttons** on Modal, Sheet and DialogContent are a `ghost-muted`
`size="icon-sm"` Button (32px, accent square on hover) at `top-2 right-2`.
- **Disabled**: buttons (Button, Accordion, Tabs) use
@@ -1077,9 +1102,9 @@ shadow-lg` in both themes: the knob is white on any track, and a white knob
an `absolute` div with a shadow and a hand-picked `z-*`.
- **Stacking**: `z-10` sticky headers and table heads inside a page;
`z-20` in-page floating chrome (banners, scroll-to-bottom, drag
handles); `z-50` overlays, modals, sheets and toasts; `z-200` every
portalled floating list (popover, menu, select, tooltip), so it opens above
a Modal without an override. Do not invent values in between. The app
handles); `z-50` overlays, modals, sheets, toasts and the one-frame
bar-tint strips; `z-200` every portalled floating list (popover, menu,
select, tooltip), so it opens above a Modal without an override. Do not invent values in between. The app
shell uses the low layers too: the phone top bar is `z-10`, and the
sidebar and its phone backdrop are `z-20`, so an open sidebar dims the bar.
+8
View File
@@ -105,6 +105,14 @@ export default [
message:
"Tailwind's screen heights (h-/min-h-/max-h-screen) are the toolbar-hidden viewport on iOS Safari, taller than the visible screen. Use h-dvh / min-h-dvh, or svh for a fixed panel. See DESIGN.md.",
})),
// Modal, Sheet and DialogContent return focus to what had it on open
// (ui/use-focus-return.ts). A hand-rolled return focuses the trigger
// even after a tap, which lights its ring on iOS.
{
selector: 'JSXAttribute[name.name="onCloseAutoFocus"]',
message:
'Modal, Sheet and DialogContent already return focus to what had it on open (ui/use-focus-return.ts). Don\'t hand-roll onCloseAutoFocus. See DESIGN.md "Focus return".',
},
],
// Design-system rules (@shadcn/lint). Tokens, variants and the
// approved exceptions are documented in DESIGN.md.
@@ -0,0 +1,66 @@
import * as React from 'react';
// Tested on iPhone (Safari 26, Low Power Mode too). Bottom edge: in the app
// 6px works and 4px doesn't (4px was enough on a bare test page, 2px never).
// Top edge: 16px works, 6px doesn't. One painted frame is enough; a 16ms
// timer isn't, because it can fire before a paint (Low Power Mode drops the
// refresh rate), so each strip lives for one frame.
/**
* Shows a page-coloured strip on one edge for a single painted frame. At most
* one strip per edge at a time: every useDarkTheme consumer applies a theme
* change, and one strip is enough.
*/
function flashStrip(edge: 'top' | 'bottom', className: string): void {
const slot = `${edge}-tint-reset`;
if (document.querySelector(`[data-slot="${slot}"]`)) return;
const strip = document.createElement('div');
strip.dataset.slot = slot;
strip.setAttribute('aria-hidden', 'true');
strip.className = className;
document.body.appendChild(strip);
// The first frame paints the strip; remove it on the next.
requestAnimationFrame(() => requestAnimationFrame(() => strip.remove()));
}
/**
* Makes iOS Safari re-colour its bottom bar from the page.
*
* Safari 26 takes the bottom bar's colour from a fixed element that appears on
* the bottom edge (a bottom sheet turns it `bg-card`) and doesn't change it
* back when that element goes away, or when the page changes colour (a theme
* switch). A page-coloured strip that appears briefly on the edge makes Safari
* sample again. The strip is 6px, no taller than the gap under the composer,
* so it covers only background of its own colour. It has to be on top: a
* strip behind the content, transparent or at `opacity-0` isn't sampled.
*/
function resetBottomTint(): void {
flashStrip(
'bottom',
'bg-background pointer-events-none fixed inset-x-0 bottom-0 z-50 h-1.5',
);
}
/**
* Makes iOS Safari re-colour its top bar from the page, the same way as
* `resetBottomTint`. Needed after a theme switch, when the top bar keeps the
* old theme's colour. The strip is 16px, inside the phone top bar, which has
* the same background.
*/
function resetTopTint(): void {
flashStrip(
'top',
'bg-background pointer-events-none fixed inset-x-0 top-0 z-50 h-4',
);
}
/**
* Renders nothing; resets Safari's bottom bar when it unmounts. Put it inside
* a bottom sheet's Radix Content, which unmounts after its exit animation.
*/
function BottomTintReset(): null {
React.useEffect(() => resetBottomTint, []);
return null;
}
export { BottomTintReset, resetBottomTint, resetTopTint };
@@ -26,6 +26,11 @@ afterEach(async () => {
container.remove();
});
const settle = () =>
act(async () => {
await new Promise((resolve) => setTimeout(resolve, 20));
});
const render = async (element: React.ReactElement) => {
await act(async () => root.render(element));
};
@@ -127,3 +132,31 @@ describe('DialogContent layout', () => {
expect(header).not.toContain('text-center');
});
});
describe('DialogContent focus return', () => {
const opener = () =>
document.querySelector<HTMLButtonElement>('[data-testid="opener"]')!;
const renderDialog = (open: boolean) =>
render(
<>
<button type="button" data-testid="opener">
Search
</button>
<Dialog open={open}>
<DialogContent aria-describedby={undefined}>
<DialogTitle>Search</DialogTitle>
</DialogContent>
</Dialog>
</>,
);
it('returns focus to the element that had it on open', async () => {
await renderDialog(false);
opener().focus();
await renderDialog(true);
await renderDialog(false);
await settle();
expect(document.activeElement).toBe(opener());
});
});
+5
View File
@@ -2,6 +2,7 @@ import { XIcon } from 'lucide-react';
import * as React from 'react';
import { Button } from '@/components/ui/button';
import { useFocusReturn } from '@/components/ui/use-focus-return';
import { cn, overlayScrim } from '@/lib/utils';
import { Dialog as DialogPrimitive } from 'radix-ui';
@@ -49,10 +50,13 @@ function DialogContent({
className,
children,
showCloseButton = true,
onOpenAutoFocus,
onCloseAutoFocus,
...props
}: React.ComponentProps<typeof DialogPrimitive.Content> & {
showCloseButton?: boolean;
}) {
const focusReturn = useFocusReturn(onOpenAutoFocus, onCloseAutoFocus);
return (
<DialogPortal data-slot="dialog-portal">
<DialogOverlay />
@@ -63,6 +67,7 @@ function DialogContent({
className,
)}
{...props}
{...focusReturn}
>
{children}
{showCloseButton && (
+81
View File
@@ -30,6 +30,11 @@ const render = async (element: React.ReactElement) => {
await act(async () => root.render(element));
};
const settle = () =>
act(async () => {
await new Promise((resolve) => setTimeout(resolve, 20));
});
const content = () =>
document.querySelector<HTMLElement>('[data-slot="modal-content"]')!;
@@ -251,3 +256,79 @@ describe('Modal mobile sheet', () => {
expect(content().querySelector('[data-slot="sheet-handle"]')).toBeNull();
});
});
describe('Modal focus return', () => {
const opener = () =>
document.querySelector<HTMLButtonElement>('[data-testid="opener"]')!;
const renderModal = (open: boolean) =>
render(
<>
<button type="button" data-testid="opener">
Open
</button>
<Modal open={open} onOpenChange={() => undefined} title="Upload">
Body
</Modal>
</>,
);
it('returns focus to the element that had it on open', async () => {
await renderModal(false);
opener().focus();
await renderModal(true);
await renderModal(false);
await settle();
expect(document.activeElement).toBe(opener());
});
it('moves focus nowhere when nothing had it on open', async () => {
await renderModal(false);
await renderModal(true);
await renderModal(false);
await settle();
expect(document.activeElement).toBe(document.body);
});
});
describe('Modal bottom-bar reset', () => {
// Earlier tests close bottom sheets on unmount, which leaves strips behind.
beforeEach(() => {
document
.querySelectorAll('[data-slot="bottom-tint-reset"]')
.forEach((node) => node.remove());
});
afterEach(() => {
media.isMobile = false;
});
const strip = () =>
document.querySelector<HTMLElement>('[data-slot="bottom-tint-reset"]');
const renderModal = (open: boolean) =>
render(
<Modal
open={open}
onOpenChange={() => undefined}
title="Upload"
mobileVariant="sheet"
>
Body
</Modal>,
);
it('resets the bottom bar when the phone sheet closes', async () => {
media.isMobile = true;
await renderModal(true);
await renderModal(false);
expect(strip()).not.toBeNull();
});
it('leaves the bar alone for the centred dialog', async () => {
await renderModal(true);
await renderModal(false);
await settle();
expect(strip()).toBeNull();
});
});
+8 -3
View File
@@ -2,6 +2,7 @@ import { XIcon } from 'lucide-react';
import { Dialog as DialogPrimitive, VisuallyHidden } from 'radix-ui';
import * as React from 'react';
import { BottomTintReset } from '@/components/ui/bar-tint-reset';
import { Button } from '@/components/ui/button';
import {
Dialog,
@@ -12,6 +13,7 @@ import {
DialogTitle,
} from '@/components/ui/dialog';
import { SheetHandle, sheetBottomShape } from '@/components/ui/sheet';
import { useFocusReturn } from '@/components/ui/use-focus-return';
import { useMediaQuery } from '@/hooks';
import { cn } from '@/lib/utils';
@@ -63,6 +65,10 @@ const Modal = React.forwardRef<HTMLDivElement, ModalProps>(function Modal(
const { isMobile } = useMediaQuery();
const isMobileSheet = mobileVariant === 'sheet' && isMobile;
const shouldShowCloseButton = showCloseButton && !isPerformingTask;
// The phone sheet opens without autofocus so the keyboard stays down.
const focusReturn = useFocusReturn(
isMobileSheet ? (event) => event.preventDefault() : undefined,
);
// When a task is performing, block click-outside / pointer-outside to
// mirror the legacy WrapperModal lock. Esc remains enabled (Radix default).
@@ -113,9 +119,7 @@ const Modal = React.forwardRef<HTMLDivElement, ModalProps>(function Modal(
data-mobile-sheet={isMobileSheet ? '' : undefined}
onPointerDownOutside={blockOutsideInteractions}
onInteractOutside={blockOutsideInteractions}
onOpenAutoFocus={
isMobileSheet ? (event) => event.preventDefault() : undefined
}
{...focusReturn}
// Radix portals this to <body> in the DOM, but React still bubbles
// synthetic events through the JSX tree. Stop the bubble at the
// modal boundary so consumers mounted inside clickable cards (e.g.
@@ -135,6 +139,7 @@ const Modal = React.forwardRef<HTMLDivElement, ModalProps>(function Modal(
),
)}
>
{isMobileSheet && <BottomTintReset />}
{isMobileSheet && <SheetHandle />}
{headerNode}
<div
+107 -1
View File
@@ -2,7 +2,7 @@ import { act } from 'react';
import { createRoot, type Root } from 'react-dom/client';
import { afterEach, beforeEach, describe, expect, it } from 'vitest';
import { Sheet, SheetContent, SheetTitle } from './sheet';
import { Sheet, SheetContent, SheetTitle, SheetTrigger } from './sheet';
Object.assign(globalThis, { IS_REACT_ACT_ENVIRONMENT: true });
@@ -153,3 +153,109 @@ describe('SheetOverlay', () => {
expect(title?.className).toContain('text-xl leading-tight font-semibold');
});
});
describe('SheetContent focus', () => {
const pressEscape = async () => {
await act(async () => {
document.dispatchEvent(
new KeyboardEvent('keydown', { key: 'Escape', bubbles: true }),
);
});
// Radix moves focus once the closed content has unmounted.
await act(async () => {
await new Promise((resolve) => setTimeout(resolve, 20));
});
};
const renderWithTrigger = () =>
render(
<Sheet>
<SheetTrigger data-testid="trigger">Tools</SheetTrigger>
{/* Like the phone pickers: no autofocus, so the keyboard stays down. */}
<SheetContent
side="bottom"
title="Tools"
aria-describedby={undefined}
onOpenAutoFocus={(event) => event.preventDefault()}
>
<button type="button">Inside</button>
</SheetContent>
</Sheet>,
);
const trigger = () =>
document.querySelector<HTMLButtonElement>('[data-testid="trigger"]')!;
it('never draws the browser outline around the panel', async () => {
await render(
<Sheet open>
<SheetContent
side="bottom"
title="Tools"
aria-describedby={undefined}
/>
</Sheet>,
);
expect(content().className.split(' ')).toContain('outline-none');
});
it('leaves the trigger unfocused when a tap opened the sheet', async () => {
await renderWithTrigger();
// iOS doesn't focus a tapped button: the click lands with body focused.
await act(async () => trigger().click());
expect(content()).not.toBeNull();
await pressEscape();
expect(document.activeElement).not.toBe(trigger());
});
it('returns focus to the trigger when it had focus on open', async () => {
await renderWithTrigger();
trigger().focus();
await act(async () => trigger().click());
await pressEscape();
expect(document.activeElement).toBe(trigger());
});
});
describe('SheetContent bottom-bar reset', () => {
// Earlier tests close bottom sheets on unmount, which leaves strips behind.
beforeEach(() => {
document
.querySelectorAll('[data-slot="bottom-tint-reset"]')
.forEach((node) => node.remove());
});
const wait = (ms: number) =>
act(async () => {
await new Promise((resolve) => setTimeout(resolve, ms));
});
const strip = () =>
document.querySelector<HTMLElement>('[data-slot="bottom-tint-reset"]');
const renderSheet = (open: boolean, side: 'bottom' | 'right') =>
render(
<Sheet open={open}>
<SheetContent side={side} title="Tools" aria-describedby={undefined} />
</Sheet>,
);
it('shows a 6px page-coloured strip on the bottom edge for one frame after a bottom sheet closes', async () => {
await renderSheet(true, 'bottom');
expect(strip()).toBeNull();
await renderSheet(false, 'bottom');
const classes = strip()!.className.split(' ');
expect(classes).toEqual(
expect.arrayContaining(['bg-background', 'fixed', 'bottom-0', 'h-1.5']),
);
expect(strip()!.getAttribute('aria-hidden')).toBe('true');
await wait(120);
expect(strip()).toBeNull();
});
it('adds no strip for a side sheet', async () => {
await renderSheet(true, 'right');
await renderSheet(false, 'right');
await wait(20);
expect(strip()).toBeNull();
});
});
+8 -1
View File
@@ -3,8 +3,10 @@ import { Dialog as SheetPrimitive } from 'radix-ui';
import { XIcon } from 'lucide-react';
import { cva } from 'class-variance-authority';
import { BottomTintReset } from '@/components/ui/bar-tint-reset';
import { Button } from '@/components/ui/button';
import { cn, overlayScrim } from '@/lib/utils';
import { useFocusReturn } from '@/components/ui/use-focus-return';
function Sheet({ ...props }: React.ComponentProps<typeof SheetPrimitive.Root>) {
return <SheetPrimitive.Root data-slot="sheet" {...props} />;
@@ -69,7 +71,7 @@ function SheetHandle({ className, ...props }: React.ComponentProps<'div'>) {
}
const sheetContentVariants = cva(
'bg-background data-[state=open]:animate-in data-[state=closed]:animate-out fixed z-50 flex flex-col gap-4 shadow-lg transition ease-in-out data-[state=closed]:duration-300 data-[state=open]:duration-500',
'bg-background data-[state=open]:animate-in data-[state=closed]:animate-out fixed z-50 flex flex-col gap-4 shadow-lg outline-none transition ease-in-out data-[state=closed]:duration-300 data-[state=open]:duration-500',
{
variants: {
side: {
@@ -95,6 +97,8 @@ function SheetContent({
// handle is only a cue; it doesn't drag).
showCloseButton = !handle,
title,
onOpenAutoFocus,
onCloseAutoFocus,
...props
}: React.ComponentProps<typeof SheetPrimitive.Content> & {
side?: 'top' | 'right' | 'bottom' | 'left';
@@ -106,6 +110,7 @@ function SheetContent({
// heading of its own (omit it if the children already render a SheetTitle).
title?: string;
}) {
const focusReturn = useFocusReturn(onOpenAutoFocus, onCloseAutoFocus);
return (
<SheetPortal>
<SheetOverlay />
@@ -114,8 +119,10 @@ function SheetContent({
data-side={side}
className={cn(sheetContentVariants({ side }), className)}
{...props}
{...focusReturn}
>
{title ? <SheetTitle className="sr-only">{title}</SheetTitle> : null}
{side === 'bottom' && <BottomTintReset />}
{handle && <SheetHandle />}
{children}
{showCloseButton && (
@@ -0,0 +1,58 @@
import * as React from 'react';
type AutoFocusHandler = (event: Event) => void;
/**
* Focus return for Radix dialogs (Sheet, Modal, DialogContent): on close,
* focus goes back to whatever had it when the dialog opened, and nowhere if
* nothing did.
*
* Radix focuses the trigger on close even when it never had focus. A tap on
* iOS (and a click in Safari) doesn't focus a button, so after a phone sheet
* the trigger would light up with a focus-visible ring out of nowhere. After a
* keyboard open the trigger did have focus and gets it back as before, and a
* dialog opened without a trigger (a Modal from a menu row) now returns focus
* to where the user was instead of dropping it on <body>.
*
* Returns the two handlers to pass to the Radix Content. The caller's own
* handlers run first; a caller that calls `preventDefault()` on close keeps
* control of focus.
*/
function useFocusReturn(
onOpenAutoFocus?: AutoFocusHandler,
onCloseAutoFocus?: AutoFocusHandler,
) {
const returnTo = React.useRef<HTMLElement | null>(null);
const handleOpenAutoFocus = React.useCallback(
(event: Event) => {
// Still the element that had focus before the dialog opened.
const active = document.activeElement;
returnTo.current =
active instanceof HTMLElement && active !== document.body
? active
: null;
onOpenAutoFocus?.(event);
},
[onOpenAutoFocus],
);
const handleCloseAutoFocus = React.useCallback(
(event: Event) => {
onCloseAutoFocus?.(event);
const target = returnTo.current;
returnTo.current = null;
if (event.defaultPrevented) return;
event.preventDefault();
if (target?.isConnected) target.focus();
},
[onCloseAutoFocus],
);
return {
onOpenAutoFocus: handleOpenAutoFocus,
onCloseAutoFocus: handleCloseAutoFocus,
};
}
export { useFocusReturn };
+11
View File
@@ -1,5 +1,7 @@
import { useCallback, useEffect, useRef, useState, RefObject } from 'react';
import { resetBottomTint, resetTopTint } from '@/components/ui/bar-tint-reset';
export function useOutsideAlerter<T extends HTMLElement>(
ref: RefObject<T | null>,
handler: () => void,
@@ -118,6 +120,8 @@ export function useDarkTheme() {
const [isDarkTheme, setIsDarkTheme] = useState<boolean>(getInitialTheme());
const [componentMounted, setComponentMounted] = useState(false);
// The theme applied by this hook's last run; null before the first one.
const appliedTheme = useRef<boolean | null>(null);
useEffect(() => {
const mediaQuery = window.matchMedia('(prefers-color-scheme: dark)');
@@ -157,6 +161,13 @@ export function useDarkTheme() {
m.removeAttribute('media');
m.setAttribute('content', color);
});
// Safari keeps both bars in the old theme's colour until a fixed element
// appears on their edge. Only on a change: on load they already match.
if (appliedTheme.current !== null && appliedTheme.current !== isDarkTheme) {
resetBottomTint();
resetTopTint();
}
appliedTheme.current = isDarkTheme;
setComponentMounted(true);
}, [isDarkTheme]);
+35
View File
@@ -60,4 +60,39 @@ describe('useDarkTheme', () => {
await act(async () => root.unmount());
});
it('resets both Safari bars once per theme change, not on load', async () => {
const strips = (edge: 'top' | 'bottom') =>
document.querySelectorAll(`[data-slot="${edge}-tint-reset"]`).length;
let toggle: (() => void) | undefined;
function Settings() {
const [, toggleTheme] = useDarkTheme();
toggle = toggleTheme;
return null;
}
function Logo() {
useDarkTheme();
return null;
}
const root = createRoot(container);
await act(async () => {
root.render(
<>
<Settings />
<Logo />
</>,
);
});
expect(strips('bottom')).toBe(0);
expect(strips('top')).toBe(0);
// Both consumers apply the new theme; one strip is enough.
await act(async () => toggle?.());
expect(strips('bottom')).toBe(1);
expect(strips('top')).toBe(1);
await act(async () => root.unmount());
});
});
+1 -18
View File
@@ -1,4 +1,4 @@
import React, { useEffect, useRef, useState } from 'react';
import React, { useEffect, useState } from 'react';
import { useTranslation } from 'react-i18next';
import { useSelector } from 'react-redux';
@@ -47,9 +47,6 @@ export default function TraceSheet({
const [failed, setFailed] = useState(false);
// Bumped by Retry to re-run the fetch for the same trace.
const [reloadKey, setReloadKey] = useState(0);
// The sheet is opened from a Logs row, not a SheetTrigger, so Radix has no
// trigger to return focus to on close; remember what had focus instead.
const returnFocusRef = useRef<HTMLElement | null>(null);
useEffect(() => {
if (!traceRef) return;
@@ -87,20 +84,6 @@ export default function TraceSheet({
side="right"
title={t('settings.logs.trace.title')}
aria-describedby={undefined}
onOpenAutoFocus={() => {
returnFocusRef.current =
document.activeElement instanceof HTMLElement
? document.activeElement
: null;
}}
onCloseAutoFocus={(event) => {
const target = returnFocusRef.current;
returnFocusRef.current = null;
if (target?.isConnected) {
event.preventDefault();
target.focus();
}
}}
className="w-full overflow-y-auto sm:max-w-3xl"
>
<div className="flex flex-col gap-6 px-4 pt-4 pb-6">