diff --git a/frontend/DESIGN.md b/frontend/DESIGN.md index 9ae44f23..3584eab5 100644 --- a/frontend/DESIGN.md +++ b/frontend/DESIGN.md @@ -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. diff --git a/frontend/eslint.config.js b/frontend/eslint.config.js index a0b01e8b..ade63a0a 100644 --- a/frontend/eslint.config.js +++ b/frontend/eslint.config.js @@ -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. diff --git a/frontend/src/components/ui/bar-tint-reset.ts b/frontend/src/components/ui/bar-tint-reset.ts new file mode 100644 index 00000000..dc25462e --- /dev/null +++ b/frontend/src/components/ui/bar-tint-reset.ts @@ -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 }; diff --git a/frontend/src/components/ui/dialog.test.tsx b/frontend/src/components/ui/dialog.test.tsx index da9f4a46..2dd97b4b 100644 --- a/frontend/src/components/ui/dialog.test.tsx +++ b/frontend/src/components/ui/dialog.test.tsx @@ -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('[data-testid="opener"]')!; + + const renderDialog = (open: boolean) => + render( + <> + + + + Search + + + , + ); + + 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()); + }); +}); diff --git a/frontend/src/components/ui/dialog.tsx b/frontend/src/components/ui/dialog.tsx index bd3cade0..7c6c5670 100644 --- a/frontend/src/components/ui/dialog.tsx +++ b/frontend/src/components/ui/dialog.tsx @@ -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 & { showCloseButton?: boolean; }) { + const focusReturn = useFocusReturn(onOpenAutoFocus, onCloseAutoFocus); return ( @@ -63,6 +67,7 @@ function DialogContent({ className, )} {...props} + {...focusReturn} > {children} {showCloseButton && ( diff --git a/frontend/src/components/ui/modal.test.tsx b/frontend/src/components/ui/modal.test.tsx index 7ba01da9..ca85d0df 100644 --- a/frontend/src/components/ui/modal.test.tsx +++ b/frontend/src/components/ui/modal.test.tsx @@ -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('[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('[data-testid="opener"]')!; + + const renderModal = (open: boolean) => + render( + <> + + undefined} title="Upload"> + Body + + , + ); + + 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('[data-slot="bottom-tint-reset"]'); + + const renderModal = (open: boolean) => + render( + undefined} + title="Upload" + mobileVariant="sheet" + > + Body + , + ); + + 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(); + }); +}); diff --git a/frontend/src/components/ui/modal.tsx b/frontend/src/components/ui/modal.tsx index 5d7f9135..5d2b818c 100644 --- a/frontend/src/components/ui/modal.tsx +++ b/frontend/src/components/ui/modal.tsx @@ -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(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(function Modal( data-mobile-sheet={isMobileSheet ? '' : undefined} onPointerDownOutside={blockOutsideInteractions} onInteractOutside={blockOutsideInteractions} - onOpenAutoFocus={ - isMobileSheet ? (event) => event.preventDefault() : undefined - } + {...focusReturn} // Radix portals this to 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(function Modal( ), )} > + {isMobileSheet && } {isMobileSheet && } {headerNode}
{ 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( + + Tools + {/* Like the phone pickers: no autofocus, so the keyboard stays down. */} + event.preventDefault()} + > + + + , + ); + + const trigger = () => + document.querySelector('[data-testid="trigger"]')!; + + it('never draws the browser outline around the panel', async () => { + await render( + + + , + ); + 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('[data-slot="bottom-tint-reset"]'); + + const renderSheet = (open: boolean, side: 'bottom' | 'right') => + render( + + + , + ); + + 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(); + }); +}); diff --git a/frontend/src/components/ui/sheet.tsx b/frontend/src/components/ui/sheet.tsx index 8869a495..eda968bc 100644 --- a/frontend/src/components/ui/sheet.tsx +++ b/frontend/src/components/ui/sheet.tsx @@ -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) { return ; @@ -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 & { 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 ( @@ -114,8 +119,10 @@ function SheetContent({ data-side={side} className={cn(sheetContentVariants({ side }), className)} {...props} + {...focusReturn} > {title ? {title} : null} + {side === 'bottom' && } {handle && } {children} {showCloseButton && ( diff --git a/frontend/src/components/ui/use-focus-return.ts b/frontend/src/components/ui/use-focus-return.ts new file mode 100644 index 00000000..ec879c77 --- /dev/null +++ b/frontend/src/components/ui/use-focus-return.ts @@ -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 . + * + * 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(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 }; diff --git a/frontend/src/hooks/index.ts b/frontend/src/hooks/index.ts index eab5fabe..04911355 100644 --- a/frontend/src/hooks/index.ts +++ b/frontend/src/hooks/index.ts @@ -1,5 +1,7 @@ import { useCallback, useEffect, useRef, useState, RefObject } from 'react'; +import { resetBottomTint, resetTopTint } from '@/components/ui/bar-tint-reset'; + export function useOutsideAlerter( ref: RefObject, handler: () => void, @@ -118,6 +120,8 @@ export function useDarkTheme() { const [isDarkTheme, setIsDarkTheme] = useState(getInitialTheme()); const [componentMounted, setComponentMounted] = useState(false); + // The theme applied by this hook's last run; null before the first one. + const appliedTheme = useRef(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]); diff --git a/frontend/src/hooks/useDarkTheme.test.tsx b/frontend/src/hooks/useDarkTheme.test.tsx index 0866b625..3d482fe8 100644 --- a/frontend/src/hooks/useDarkTheme.test.tsx +++ b/frontend/src/hooks/useDarkTheme.test.tsx @@ -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( + <> + + + , + ); + }); + 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()); + }); }); diff --git a/frontend/src/settings/traces/TraceSheet.tsx b/frontend/src/settings/traces/TraceSheet.tsx index a9886d03..9ca67214 100644 --- a/frontend/src/settings/traces/TraceSheet.tsx +++ b/frontend/src/settings/traces/TraceSheet.tsx @@ -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(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" >