Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 4 additions & 0 deletions docs/reference/ui-primitives.md
Original file line number Diff line number Diff line change
Expand Up @@ -319,6 +319,8 @@ import { Dialog } from '@ui/components/Dialog'

`Dialog` traps focus, restores it on close, escape-closes, click-outside-closes (configurable). Modal by default. Replaces native `alert()` / `confirm()` (which are banned by `no-native-browser-dialogs.test.ts`).

**Modal shells must bind Escape on `document`, not as a React `onKeyDown` on the dialog element.** React's delegated handler only fires while focus sits inside the dialog subtree, and a click on any non-focusable spot in a panel blurs to `<body>` — outside that subtree — leaving Escape silently dead while backdrop click still works. Pair it with `tabIndex={-1}` on the panel so such clicks land on the panel instead of `<body>` and the Tab trap holds.

---

## `Tooltip`
Expand All @@ -333,6 +335,8 @@ import { Tooltip } from '@ui/components/Tooltip'

Replaces native `title="..."` (gated by `no-native-title-tooltips.test.ts`). Works on disabled buttons because `mouseenter` fires on disabled `<button>` elements.

Escape hides the tooltip but is **never consumed** — it keeps propagating to whatever surface owns the keystroke. Tooltips open on plain hover, so swallowing the key would kill Escape app-wide whenever the pointer happened to rest on a trigger.

`Button` accepts a `tooltip` prop and wraps itself — prefer that over composing `<Tooltip><Button .../></Tooltip>` for buttons.

`CursorTooltip` lives in the same module for canvas/editor chrome that must follow a pointer coordinate instead of anchoring to a trigger element.
Expand Down
68 changes: 61 additions & 7 deletions src/__tests__/settings/settingsModal.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -354,22 +354,76 @@ describe('SettingsModal — close behaviours', () => {
// ---------------------------------------------------------------------------

describe('SettingsModal — focus trap keyboard logic', () => {
it('dialog has onKeyDown handler attached (Escape closes)', () => {
it('pressing Escape on a child element inside the dialog closes the modal', () => {
openModal()
render(<SettingsModal />)
const dialog = screen.getByRole('dialog')
fireEvent.keyDown(dialog, { key: 'Escape' })
const nav = screen.getByRole('navigation', { name: /settings sections/i })
const firstBtn = nav.querySelector('button')!
fireEvent.keyDown(firstBtn, { key: 'Escape' })
expect(useEditorStore.getState().isSettingsOpen).toBe(false)
})

it('pressing Escape on a child element inside the dialog closes the modal', () => {
// Regression: Escape used to be a React `onKeyDown` on the dialog element,
// so it only fired while focus sat inside the dialog subtree. Clicking any
// non-focusable spot in the panel (a label, the section header, empty space)
// blurs to `<body>` — outside that subtree — and Escape went dead for the
// rest of the session while backdrop click kept working.
it('pressing Escape closes the modal when focus has left the dialog', () => {
openModal()
render(<SettingsModal />)
const nav = screen.getByRole('navigation', { name: /settings sections/i })
const firstBtn = nav.querySelector('button')!
fireEvent.keyDown(firstBtn, { key: 'Escape' })

// Simulate the blur-to-body a click on non-focusable panel chrome causes.
;(document.activeElement as HTMLElement | null)?.blur()
expect(document.activeElement).toBe(document.body)

fireEvent.keyDown(document.body, { key: 'Escape' })
expect(useEditorStore.getState().isSettingsOpen).toBe(false)
})

// A document-level Escape listener must not collapse the whole modal stack:
// surfaces opened from inside settings (media picker, confirm dialogs) own
// the keystroke until they close.
it('ignores Escape while another dialog is stacked above it', () => {
openModal()
render(<SettingsModal />)

const nested = document.createElement('div')
nested.setAttribute('role', 'dialog')
document.body.appendChild(nested)

fireEvent.keyDown(document.body, { key: 'Escape' })
expect(useEditorStore.getState().isSettingsOpen).toBe(true)

// Once the nested dialog closes, Escape reaches settings again.
nested.remove()
fireEvent.keyDown(document.body, { key: 'Escape' })
expect(useEditorStore.getState().isSettingsOpen).toBe(false)
})

it('keeps the panel focusable so a click on dead space cannot blur to body', () => {
openModal()
const { container } = render(<SettingsModal />)
const panel = container.querySelector('[data-accent]')
expect(panel?.getAttribute('tabindex')).toBe('-1')
})

// Guideline #225 / WCAG 2.4.3. The restore used to sit in an `open === false`
// branch that never ran — all three layouts unmount the modal on close, so
// `open` never flips to false while mounted. Assert the behaviour, not the
// implementation shape.
it('returns focus to the opening trigger when the modal unmounts', () => {
const trigger = document.createElement('button')
document.body.appendChild(trigger)
trigger.focus()
expect(document.activeElement).toBe(trigger)

openModal()
const { unmount } = render(<SettingsModal />)
unmount()

expect(document.activeElement).toBe(trigger)
trigger.remove()
})
})

// ---------------------------------------------------------------------------
Expand Down
32 changes: 13 additions & 19 deletions src/__tests__/toolbar/toolbar.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -753,38 +753,32 @@ describe('ModulePicker — ArrowDown keyboard bridge (WCAG SC 2.1.1)', () => {
// ---------------------------------------------------------------------------

describe('SettingsModal — WCAG 2.4.3 focus-return on close (Guideline #225)', () => {
it('declares a triggerRef to capture the element that opened the modal', () => {
it('captures the opening element and restores focus in the effect cleanup', () => {
const { readFileSync } = require('fs')
const src = readFileSync(
new URL('../../admin/modals/Settings/SettingsModal.tsx', import.meta.url).pathname,
'utf-8',
) as string
// The ref must be a nullable HTMLElement ref (so .focus() is available)
expect(src).toContain('triggerRef')
expect(src).toMatch(/useRef<HTMLElement\s*\|\s*null>/)
})

it('captures document.activeElement into triggerRef when modal opens', () => {
const { readFileSync } = require('fs')
const src = readFileSync(
new URL('../../admin/modals/Settings/SettingsModal.tsx', import.meta.url).pathname,
'utf-8',
) as string
// Must guard with instanceof before assigning (avoids assigning non-focusable elements)
// Must guard with instanceof before capturing (never stash a non-focusable node)
expect(src).toMatch(/document\.activeElement\s+instanceof\s+HTMLElement/)
expect(src).toContain('triggerRef.current = document.activeElement')
// The restore must live in the effect CLEANUP, not an `open === false`
// branch: all three layouts unmount the modal on close, so `open` never
// transitions to false while mounted and such a branch is dead code.
expect(src).toMatch(/return\s*\(\)\s*=>\s*\{[^}]*trigger\?\.focus\(\)/)
})

it('restores focus to trigger when modal closes (Guideline #225)', () => {
it('binds Escape on document, not as a React onKeyDown on the dialog', () => {
const { readFileSync } = require('fs')
const src = readFileSync(
new URL('../../admin/modals/Settings/SettingsModal.tsx', import.meta.url).pathname,
'utf-8',
) as string
// The else branch (open → false) must focus the captured trigger
expect(src).toContain('triggerRef.current?.focus()')
// And must clear the ref to avoid a stale reference
expect(src).toContain('triggerRef.current = null')
// A React onKeyDown only fires while focus is inside the dialog subtree;
// clicking non-focusable panel chrome blurs to <body> and killed Escape.
expect(src).toContain("document.addEventListener('keydown'")
expect(src).toContain("document.removeEventListener('keydown'")
// The remaining React onKeyDown is the Tab trap only.
expect(src).not.toMatch(/onKeyDown[\s\S]{0,400}?e\.key === 'Escape'/)
})

it('does not regress to a 36px touch target anywhere', () => {
Expand Down
12 changes: 9 additions & 3 deletions src/__tests__/ui/tooltip.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,12 @@ describe('Tooltip', () => {
expect(screen.queryByRole('tooltip')).toBeNull()
})

it('consumes Escape before a parent panel handler can close', () => {
// Regression: the tooltip used to `preventDefault()` + `stopPropagation()`
// on Escape. Tooltips open on plain hover, so that swallowed Escape app-wide
// — no modal, context menu, or spotlight could close whenever the pointer
// happened to rest on a tooltip trigger. Escape now dismisses the tooltip
// AND reaches whatever surface owns the keystroke.
it('hides on Escape without consuming it from other handlers', () => {
let parentEscapeCount = 0
const onKeyDown = (event: KeyboardEvent) => {
if (event.key === 'Escape') parentEscapeCount += 1
Expand All @@ -162,10 +167,11 @@ describe('Tooltip', () => {
const trigger = screen.getByRole('button', { name: 'Trigger' })
act(() => trigger.focus())

fireEvent.keyDown(trigger, { key: 'Escape' })
const delivered = fireEvent.keyDown(trigger, { key: 'Escape' })

expect(screen.queryByRole('tooltip')).toBeNull()
expect(parentEscapeCount).toBe(0)
expect(parentEscapeCount).toBe(1)
expect(delivered).toBe(true) // not defaultPrevented
expect(document.activeElement).toBe(trigger)
} finally {
document.removeEventListener('keydown', onKeyDown)
Expand Down
85 changes: 60 additions & 25 deletions src/admin/modals/Settings/SettingsModal.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
*
* data-testid="settings-modal" for Playwright (Guideline #221)
*/
import { useEffect, useRef } from 'react'
import { useEffect, useEffectEvent, useRef } from 'react'
import { cn } from '@ui/cn'
import { useEditorStore } from '@site/store/store'
import { useAdminUi } from '@admin/state/adminUi'
Expand Down Expand Up @@ -68,29 +68,64 @@ export function SettingsModal() {
const activeItem = NAV_ITEMS.find((n) => n.id === activeSection) ?? NAV_ITEMS[0]
const dialogRef = useRef<HTMLDivElement>(null)
const navRef = useRef<HTMLElement>(null)
const triggerRef = useRef<HTMLElement | null>(null)

// Focus management: capture trigger on open, restore on close (Guideline #225)
useEffect(() => {
if (open) {
if (document.activeElement instanceof HTMLElement) {
triggerRef.current = document.activeElement
}
requestAnimationFrame(() => {
navRef.current?.querySelector<HTMLButtonElement>('button')?.focus()
})
} else {
triggerRef.current?.focus()
triggerRef.current = null
}
}, [open])

// Close routes through adminUi — the editor store's `isSettingsOpen`
// gets cleared by the bridge in `settingsSlice.ts`.
const handleClose = () => {
closeAdminUi()
}

// Escape closes the modal. Document-level listener, matching `Dialog` and
// every other modal shell: a React `onKeyDown` on the dialog only fires
// while focus sits inside the dialog subtree, and a click on any
// non-focusable spot in the panel (a label, the section header, empty
// space) drops focus to `<body>` — outside the subtree — which left Esc
// silently dead for the rest of the session.
const handleCloseEvent = useEffectEvent(() => handleClose())

useEffect(() => {
if (!open) return undefined
function onKeyDown(event: globalThis.KeyboardEvent) {
if (event.key !== 'Escape') return

// Only the topmost layer reacts. Surfaces opened from inside settings
// (the media picker behind "Browse library…", confirm dialogs) either
// portal to `document.body` or nest in this subtree — both come after
// this dialog in document order, and both own Escape until they close.
// Without this, one Escape would collapse the whole stack at once.
const self = dialogRef.current
if (!self) return
const stackedAbove = Array.from(document.querySelectorAll('[role="dialog"]')).some(
(el) =>
el !== self &&
Boolean(self.compareDocumentPosition(el) & Node.DOCUMENT_POSITION_FOLLOWING),
)
if (stackedAbove) return

event.preventDefault()
event.stopPropagation()
handleCloseEvent()
}
document.addEventListener('keydown', onKeyDown)
return () => document.removeEventListener('keydown', onKeyDown)
}, [open])

// Focus management: capture trigger on open, restore on teardown
// (Guideline #225). Restoring in the cleanup rather than an `else` branch
// is what makes it actually run — all three layouts unmount this modal on
// close, so `open` never transitions to `false` while mounted.
useEffect(() => {
if (!open) return undefined
const trigger = document.activeElement instanceof HTMLElement ? document.activeElement : null
const raf = requestAnimationFrame(() => {
navRef.current?.querySelector<HTMLButtonElement>('button')?.focus()
})
return () => {
cancelAnimationFrame(raf)
trigger?.focus()
}
}, [open])

// Update section in BOTH stores. adminUi for the modal's own selection,
// editor's settingsSlice for downstream readers (spotlight commands).
const openAdminUi = useAdminUi((state) => state.openSettings)
Expand All @@ -99,14 +134,9 @@ export function SettingsModal() {
openAdminUi(id)
}

// Focus trap + Esc handler
// Focus trap. Esc is handled by the document-level listener above, not
// here — this only cycles Tab / Shift+Tab within the dialog.
const handleKeyDown = (e: React.KeyboardEvent<HTMLDivElement>) => {
if (e.key === 'Escape') {
e.preventDefault()
handleClose()
return
}

if (e.key !== 'Tab') return

const dialog = dialogRef.current
Expand Down Expand Up @@ -158,7 +188,12 @@ export function SettingsModal() {
onKeyDown={handleKeyDown}
className={s.dialogWrapper}
>
<div className={s.panel} data-accent={activeItem.accent}>
{/* tabIndex={-1} keeps the Tab trap honest: clicking a non-focusable
spot in the panel (a label, the section header, empty space) would
otherwise blur to `<body>` and let the next Tab walk the page
behind the modal. Excluded from the focusable query above by its
own `:not([tabindex="-1"])` clause. */}
<div className={s.panel} tabIndex={-1} data-accent={activeItem.accent}>
{/* Screen-reader description */}
<p id="settings-modal-desc" className={s.srOnly}>
Site-level configuration. Press Escape to close.
Expand Down
10 changes: 6 additions & 4 deletions src/ui/components/Tooltip/Tooltip.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -146,17 +146,19 @@ function TooltipInner({
const onScroll = () => hide()
const onKeyDown = (e: KeyboardEvent) => {
if (e.key !== 'Escape') return
e.preventDefault()
e.stopPropagation()
hide()
}
const onPointerDown = (e: PointerEvent) => {
if (!triggerEl.contains(e.target as Node)) hide()
}

window.addEventListener('scroll', onScroll, { capture: true, passive: true })
// Capture lets the tooltip consume Escape before a parent panel's document
// handler sees it and closes the whole surface.
// Capture so the tooltip still hides even if a downstream handler stops
// propagation. It must NOT consume the key: a tooltip shows on plain
// hover, so swallowing Escape here (preventDefault + stopPropagation)
// silently killed Escape app-wide — no modal, context menu, or spotlight
// could close whenever the pointer happened to rest on a tooltip trigger.
// Escape dismisses the tooltip AND whatever surface owns the keystroke.
window.addEventListener('keydown', onKeyDown, { capture: true })
window.addEventListener('pointerdown', onPointerDown)

Expand Down
Loading