From 44cc596c39169d2cfcdfc02eb6f3d86290479306 Mon Sep 17 00:00:00 2001 From: Maurice Date: Thu, 1 Oct 2026 09:50:31 +0200 Subject: [PATCH] feat(dialogs): the focus starts inside a dialog and Tab stays there Every dialog, the shared frame and the older modal alike, moves the focus in when it opens: on a desktop into the first text field, on a phone onto the panel so no keyboard pops up. Tab wraps around inside instead of running through the page behind, and closing hands the focus back to what opened it. Fields that open a list on focus are skipped, and the older modal's close button got a label and a tooltip. (#1302) --- .../src/components/Planner/AddressInput.tsx | 2 + .../src/components/Planner/AirportSelect.tsx | 2 + .../src/components/Planner/LocationSelect.tsx | 2 + .../src/components/Planner/PlaceFormModal.tsx | 4 +- .../components/Planner/TransitSearchPanel.tsx | 2 + .../Roadtrip/RoadtripStayModal.test.tsx | 5 +- client/src/components/shared/DialogShell.tsx | 7 +- client/src/components/shared/Modal.tsx | 51 +++++-- .../components/shared/dialogFocus.test.tsx | 141 ++++++++++++++++++ client/src/components/shared/dialogFocus.ts | 81 ++++++++++ 10 files changed, 278 insertions(+), 19 deletions(-) create mode 100644 client/src/components/shared/dialogFocus.test.tsx create mode 100644 client/src/components/shared/dialogFocus.ts diff --git a/client/src/components/Planner/AddressInput.tsx b/client/src/components/Planner/AddressInput.tsx index 332fca8d1..ea415beb3 100644 --- a/client/src/components/Planner/AddressInput.tsx +++ b/client/src/components/Planner/AddressInput.tsx @@ -89,6 +89,8 @@ export default function AddressInput({ value, onChange, placeholder, className } value={value} placeholder={placeholder} onChange={e => { onChange(e.target.value); setOpen(true); search(e.target.value) }} + // Opens its list on focus, so a dialog must not focus it by itself (#1302). + data-no-autofocus onFocus={() => setOpen(true)} onKeyDown={onKey} className={className} diff --git a/client/src/components/Planner/AirportSelect.tsx b/client/src/components/Planner/AirportSelect.tsx index 42ea3c9b8..caec9ff69 100644 --- a/client/src/components/Planner/AirportSelect.tsx +++ b/client/src/components/Planner/AirportSelect.tsx @@ -113,6 +113,8 @@ export default function AirportSelect({ value, onChange, placeholder, style }: P value={query} placeholder={placeholder ?? t('airport.searchPlaceholder')} onChange={(e) => { setQuery(e.target.value); setOpen(true); if (value) onChange(null) }} + // Opens its list on focus, so a dialog must not focus it by itself (#1302). + data-no-autofocus onFocus={() => setOpen(true)} onKeyDown={onKey} className="bg-transparent text-content" diff --git a/client/src/components/Planner/LocationSelect.tsx b/client/src/components/Planner/LocationSelect.tsx index 9c45ff763..3d8dde4e5 100644 --- a/client/src/components/Planner/LocationSelect.tsx +++ b/client/src/components/Planner/LocationSelect.tsx @@ -115,6 +115,8 @@ export default function LocationSelect({ value, onChange, placeholder, style, pl value={query} placeholder={placeholder ?? t('reservations.searchLocation')} onChange={(e) => { setQuery(e.target.value); setOpen(true); setHighlight(-1); if (value) onChange(null) }} + // Opens its list on focus, so a dialog must not focus it by itself (#1302). + data-no-autofocus onFocus={() => setOpen(true)} onKeyDown={onKey} className="bg-transparent text-content" diff --git a/client/src/components/Planner/PlaceFormModal.tsx b/client/src/components/Planner/PlaceFormModal.tsx index 5914d557d..2b137b916 100644 --- a/client/src/components/Planner/PlaceFormModal.tsx +++ b/client/src/components/Planner/PlaceFormModal.tsx @@ -939,7 +939,9 @@ export default function PlaceFormModal(props: PlaceFormModalProps) { labelId={titleId} onClose={onClose} eyebrow={title} - // Never focused on opening: a new place starts at the search, an edit at the dialog. + // Not autoFocused itself: a new place starts at the search, and otherwise the + // dialog's own first-field focus lands here on a desktop (#1302), so a place + // brought in from the map or opened to edit is ready for its name. titleInput={{ value: form.name, onChange: value => handleChange('name', value), diff --git a/client/src/components/Planner/TransitSearchPanel.tsx b/client/src/components/Planner/TransitSearchPanel.tsx index b8ddbf7b2..86d9dc821 100644 --- a/client/src/components/Planner/TransitSearchPanel.tsx +++ b/client/src/components/Planner/TransitSearchPanel.tsx @@ -256,6 +256,8 @@ function StopPicker({ label, value, onPick, quickPicks, near, placeholder }: { search(e.target.value)} + // Opens its list on focus, so a dialog must not focus it by itself (#1302). + data-no-autofocus onFocus={() => setOpen(true)} placeholder={placeholder} className="text-content" diff --git a/client/src/components/Roadtrip/RoadtripStayModal.test.tsx b/client/src/components/Roadtrip/RoadtripStayModal.test.tsx index 8b456192b..da6bca46a 100644 --- a/client/src/components/Roadtrip/RoadtripStayModal.test.tsx +++ b/client/src/components/Roadtrip/RoadtripStayModal.test.tsx @@ -217,7 +217,10 @@ describe('RoadtripStayModal for a stop left at a set time', () => { it('FE-STAYMODAL-019: closing changes nothing', () => { const { onClose, onSave } = open(leaving) - fireEvent.click(screen.getByRole('button', { name: /^close$/i })) + // The footer's Close; the header's X answers to the same name and closes the same way. + const closeButtons = screen.getAllByRole('button', { name: /^close$/i }) + expect(closeButtons).toHaveLength(2) + fireEvent.click(closeButtons[1]) expect(onClose).toHaveBeenCalled() expect(onSave).not.toHaveBeenCalled() expect(setAssignmentTimes).not.toHaveBeenCalled() diff --git a/client/src/components/shared/DialogShell.tsx b/client/src/components/shared/DialogShell.tsx index 7daae2735..4f1ec48bc 100644 --- a/client/src/components/shared/DialogShell.tsx +++ b/client/src/components/shared/DialogShell.tsx @@ -4,6 +4,7 @@ import { Trash2, X } from 'lucide-react' import { useTranslation } from '../../i18n' import { lockBodyScroll } from '../../utils/bodyScrollLock' import { Tooltip } from './Tooltip' +import { focusDialog, trapTab } from './dialogFocus' /** A font size that follows the user's text size setting for its tier. */ export const fs = (px: number, tier: 'caption' | 'body' | 'subtitle' = 'caption'): CSSProperties => ({ fontSize: `calc(${px}px * var(--fs-scale-${tier}, 1))` }) @@ -61,13 +62,14 @@ function DialogFrame({ onClose, labelledBy, width = 'detail', align = 'center', return () => document.removeEventListener('keydown', onKey) }, [blocked, onClose]) - // The page stays put behind the dialog, and whatever opened it gets the focus back. + // The page stays put behind the dialog, the focus moves into it (the first text + // field on a desktop, #1302), and whatever opened it gets the focus back. useEffect(() => { const panel = panelRef.current const focusedInside = !!panel && panel.contains(document.activeElement) const opener = focusedInside ? focusedBefore : document.activeElement const release = lockBodyScroll() - if (!focusedInside) panel?.focus() + if (!focusedInside && panel) focusDialog(panel) return () => { release() if (opener instanceof HTMLElement) opener.focus() @@ -100,6 +102,7 @@ function DialogFrame({ onClose, labelledBy, width = 'detail', align = 'center', aria-labelledby={labelledBy} tabIndex={-1} onPaste={onPaste} + onKeyDown={e => { if (panelRef.current) trapTab(e, panelRef.current) }} className={`trek-modal-enter flex max-h-full w-full ${PANEL_WIDTH[width]} flex-col overflow-hidden rounded-[22px] bg-surface-card shadow-2xl outline-none`} > {header} diff --git a/client/src/components/shared/Modal.tsx b/client/src/components/shared/Modal.tsx index 57f0bb65c..4814166db 100644 --- a/client/src/components/shared/Modal.tsx +++ b/client/src/components/shared/Modal.tsx @@ -1,7 +1,10 @@ -import React, { useEffect, useCallback, useRef } from 'react' +import React, { useEffect, useCallback, useRef, useState } from 'react' import { createPortal } from 'react-dom' import { X } from 'lucide-react' import { lockBodyScroll } from '../../utils/bodyScrollLock' +import { useTranslation } from '../../i18n' +import { Tooltip } from './Tooltip' +import { focusDialog, trapTab } from './dialogFocus' const sizeClasses: Record = { sm: 'max-w-sm', @@ -32,8 +35,11 @@ interface ModalProps { align?: 'center' | 'top' } -export default function Modal({ - isOpen, +export default function Modal({ isOpen, ...frame }: ModalProps) { + return isOpen ? : null +} + +function ModalFrame({ onClose, title, children, @@ -41,28 +47,37 @@ export default function Modal({ footer, hideCloseButton = false, align = 'center', -}: ModalProps) { +}: Omit) { + const { t } = useTranslation() + const panelRef = useRef(null) + const mouseDownTarget = useRef(null) + // Read while rendering, before a field inside can take the focus with autoFocus. + const [focusedBefore] = useState(() => document.activeElement) + const handleEsc = useCallback((e: KeyboardEvent) => { if (e.key === 'Escape') onClose() }, [onClose]) useEffect(() => { - if (!isOpen) return document.addEventListener('keydown', handleEsc) return () => document.removeEventListener('keydown', handleEsc) - }, [isOpen, handleEsc]) + }, [handleEsc]) // Separate from the key listener so a new onClose identity does not release // and re-take the lock on every render. The shared lock is ref-counted: this // modal must not clear a lock another overlay is still holding (#1809). + useEffect(() => lockBodyScroll(), []) + + // The focus moves into the dialog when it opens (the first text field on a + // desktop) and back to whatever opened it when it closes, the way DialogShell + // does it (#1302). Without this Tab ran through the page behind first. useEffect(() => { - if (!isOpen) return - return lockBodyScroll() - }, [isOpen]) - - const mouseDownTarget = useRef(null) - - if (!isOpen) return null + const panel = panelRef.current + if (panel) focusDialog(panel) + return () => { + if (focusedBefore instanceof HTMLElement && focusedBefore.isConnected) focusedBefore.focus() + } + }, [focusedBefore]) return createPortal(
{ if (panelRef.current) trapTab(e, panelRef.current) }} className={` - trek-modal-enter + trek-modal-enter outline-none rounded-2xl overflow-hidden shadow-2xl w-full ${sizeClasses[size] || sizeClasses.md} flex flex-col max-h-[calc(100dvh-var(--bottom-nav-h)-90px)] sm:max-h-[calc(100dvh-90px)] @@ -93,12 +111,15 @@ export default function Modal({

{title}

{!hideCloseButton && ( + + )}
diff --git a/client/src/components/shared/dialogFocus.test.tsx b/client/src/components/shared/dialogFocus.test.tsx new file mode 100644 index 000000000..55e70178e --- /dev/null +++ b/client/src/components/shared/dialogFocus.test.tsx @@ -0,0 +1,141 @@ +// FE-SHARED-DIALOGFOCUS-001 to FE-SHARED-DIALOGFOCUS-009 +import { afterEach, describe, expect, it, vi } from 'vitest' +import { useState } from 'react' +import { render, screen, fireEvent } from '../../../tests/helpers/render' +import { DialogShell } from './DialogShell' +import Modal from './Modal' +import { firstTextField } from './dialogFocus' + +/** A desktop with a mouse: the only case the first field takes the focus. */ +function asDesktop() { + const original = window.matchMedia + window.matchMedia = vi.fn().mockImplementation((query: string) => ({ + matches: query === '(pointer: fine)', + media: query, + addEventListener: vi.fn(), + removeEventListener: vi.fn(), + addListener: vi.fn(), + removeListener: vi.fn(), + onchange: null, + dispatchEvent: vi.fn(), + })) as unknown as typeof window.matchMedia + return () => { window.matchMedia = original } +} + +let restore: (() => void) | null = null +afterEach(() => { restore?.(); restore = null }) + +function Shell({ children }: { children: React.ReactNode }) { + return ( + {}} labelledBy="t" header={

Title

}> + {children} + + ) +} + +describe('firstTextField', () => { + it('FE-SHARED-DIALOGFOCUS-001: skips buttons, read-only, disabled and list-opening fields', () => { + const panel = document.createElement('div') + panel.innerHTML = ` + + + + + + + + + ` + expect(firstTextField(panel)?.id).toBe('target') + }) + + it('FE-SHARED-DIALOGFOCUS-002: a dialog with no field has none', () => { + const panel = document.createElement('div') + panel.innerHTML = '' + expect(firstTextField(panel)).toBeNull() + }) +}) + +describe('DialogShell focus (#1302)', () => { + it('FE-SHARED-DIALOGFOCUS-003: on a desktop the first text field is ready to type into', () => { + restore = asDesktop() + render() + expect(screen.getByLabelText('Name')).toHaveFocus() + }) + + it('FE-SHARED-DIALOGFOCUS-004: on a touch screen the panel takes the focus, so no keyboard pops up', () => { + render() + expect(screen.getByRole('dialog')).toHaveFocus() + }) + + it('FE-SHARED-DIALOGFOCUS-005: a field that took the focus itself keeps it', () => { + restore = asDesktop() + render() + expect(screen.getByLabelText('Search')).toHaveFocus() + }) + + it('FE-SHARED-DIALOGFOCUS-006: Tab wraps from the last control to the first and back', () => { + render() + const one = screen.getByRole('button', { name: 'One' }) + const two = screen.getByRole('button', { name: 'Two' }) + + two.focus() + fireEvent.keyDown(two, { key: 'Tab' }) + expect(one).toHaveFocus() + + fireEvent.keyDown(one, { key: 'Tab', shiftKey: true }) + expect(two).toHaveFocus() + + // From the panel itself, Shift+Tab goes to the last control rather than the page. + const panel = screen.getByRole('dialog') + panel.focus() + fireEvent.keyDown(panel, { key: 'Tab', shiftKey: true }) + expect(two).toHaveFocus() + }) + + it('FE-SHARED-DIALOGFOCUS-007: Tab in the middle is left to the browser', () => { + render() + const two = screen.getByRole('button', { name: 'Two' }) + two.focus() + const event = new KeyboardEvent('keydown', { key: 'Tab', bubbles: true, cancelable: true }) + two.dispatchEvent(event) + expect(event.defaultPrevented).toBe(false) + }) +}) + +describe('Modal focus (#1302)', () => { + function Harness() { + const [open, setOpen] = useState(false) + return ( + <> + + setOpen(false)} title="Edit"> + + + + + ) + } + + it('FE-SHARED-DIALOGFOCUS-008: the focus moves in on open and back to the opener on close', () => { + restore = asDesktop() + render() + const opener = screen.getByRole('button', { name: 'Open' }) + opener.focus() + fireEvent.click(opener) + expect(screen.getByLabelText('Title')).toHaveFocus() + + fireEvent.click(screen.getByRole('button', { name: 'Close' })) + expect(screen.queryByLabelText('Title')).toBeNull() + expect(opener).toHaveFocus() + }) + + it('FE-SHARED-DIALOGFOCUS-009: Tab stays inside the modal', () => { + render() + fireEvent.click(screen.getByRole('button', { name: 'Open' })) + const save = screen.getByRole('button', { name: 'Save' }) + save.focus() + fireEvent.keyDown(save, { key: 'Tab' }) + expect(screen.getByRole('button', { name: 'Close' })).toHaveFocus() + }) +}) diff --git a/client/src/components/shared/dialogFocus.ts b/client/src/components/shared/dialogFocus.ts new file mode 100644 index 000000000..0d45fc1a0 --- /dev/null +++ b/client/src/components/shared/dialogFocus.ts @@ -0,0 +1,81 @@ +import type { KeyboardEvent as ReactKeyboardEvent } from 'react' + +/** + * Keyboard focus for the app's dialogs (#1302): the first Tab lands in the dialog + * instead of running through the page behind it, Tab wraps around inside it, and + * on a desktop the first text field is ready to type into. + * + * Fields that open a list as soon as they get the focus (an address or airport + * search) carry role="combobox" and are skipped, so a dialog never opens with a + * dropdown already hanging off it. A field can opt out with data-no-autofocus. + */ +const TEXT_FIELD = [ + 'input:not([type]),input[type="text"],input[type="search"],input[type="email"],input[type="url"],input[type="tel"],input[type="number"],input[type="password"]', + 'textarea', +].join(',') + +const TABBABLE = [ + 'a[href]', 'button', 'input:not([type="hidden"])', 'select', 'textarea', + '[tabindex]', '[contenteditable="true"]', +].join(',') + +function usable(el: HTMLElement): boolean { + if ((el as HTMLInputElement).disabled) return false + if (el.closest('[hidden],[aria-hidden="true"],[inert]')) return false + return el.getAttribute('tabindex') !== '-1' +} + +/** The first field a user would type into, or null when the dialog has none. */ +export function firstTextField(panel: HTMLElement): HTMLElement | null { + for (const el of panel.querySelectorAll(TEXT_FIELD)) { + if (!usable(el)) continue + if ((el as HTMLInputElement).readOnly) continue + if (el.getAttribute('role') === 'combobox' || el.hasAttribute('aria-haspopup') || el.hasAttribute('data-no-autofocus')) continue + return el + } + return null +} + +/** Only with a mouse or trackpad: on a phone a focused field would throw the keyboard up. */ +function typingDevice(): boolean { + if (typeof window.matchMedia !== 'function') return false + return window.matchMedia('(pointer: fine)').matches && !window.matchMedia('(max-width: 767px)').matches +} + +/** + * Puts the focus into a dialog that just opened, unless something inside already + * took it (a field with autoFocus, a dialog's own choice). The panel itself is + * the fallback, so the next Tab starts inside the dialog either way. + */ +export function focusDialog(panel: HTMLElement): void { + if (panel.contains(document.activeElement)) return + const field = typingDevice() ? firstTextField(panel) : null + ;(field ?? panel).focus({ preventScroll: true }) +} + +/** + * Keeps Tab inside the dialog: past the last control it wraps to the first, and + * Shift+Tab from the first (or from the panel) to the last. A key pressed in a + * picker list the dialog opened in a portal on the body is left alone, because + * that list is not inside the panel and handles its own keys. + */ +export function trapTab(e: ReactKeyboardEvent, panel: HTMLElement): void { + if (e.key !== 'Tab' || e.defaultPrevented || e.altKey || e.ctrlKey || e.metaKey) return + const target = e.target as Node + if (target !== panel && !panel.contains(target)) return + const items = Array.from(panel.querySelectorAll(TABBABLE)).filter(usable) + if (items.length === 0) { + e.preventDefault() + panel.focus({ preventScroll: true }) + return + } + const first = items[0] + const last = items[items.length - 1] + if (e.shiftKey && (target === first || target === panel)) { + e.preventDefault() + last.focus() + } else if (!e.shiftKey && target === last) { + e.preventDefault() + first.focus() + } +}