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..b0e611b4 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,37 @@ 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); + }); +}); diff --git a/frontend/src/components/ui/modal.tsx b/frontend/src/components/ui/modal.tsx index 5d7f9135..4e2e13bc 100644 --- a/frontend/src/components/ui/modal.tsx +++ b/frontend/src/components/ui/modal.tsx @@ -12,6 +12,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 +64,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 +118,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. diff --git a/frontend/src/components/ui/sheet.test.tsx b/frontend/src/components/ui/sheet.test.tsx index f109115d..248c1714 100644 --- a/frontend/src/components/ui/sheet.test.tsx +++ b/frontend/src/components/ui/sheet.test.tsx @@ -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,66 @@ 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( + + 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()); + }); +}); diff --git a/frontend/src/components/ui/sheet.tsx b/frontend/src/components/ui/sheet.tsx index 8869a495..04df1079 100644 --- a/frontend/src/components/ui/sheet.tsx +++ b/frontend/src/components/ui/sheet.tsx @@ -5,6 +5,7 @@ import { cva } from 'class-variance-authority'; 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 +70,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 +96,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 +109,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,6 +118,7 @@ function SheetContent({ data-side={side} className={cn(sheetContentVariants({ side }), className)} {...props} + {...focusReturn} > {title ? {title} : null} {handle && } 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/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" >