Make the canvas keyboard-reachable + trap focus in dialogs #92

Merged
steve merged 2 commits from feat/canvas-a11y into main 2026-07-22 03:34:08 +00:00
4 changed files with 97 additions and 5 deletions
Showing only changes of commit bfc5d9a871 - Show all commits
+42 -3
View File
@@ -27,12 +27,51 @@ export function Modal({
busyRef.current = busy busyRef.current = busy
useEffect(() => { useEffect(() => {
cardRef.current?.focus() const card = cardRef.current
// Remember who opened the dialog so focus can return there on close —
// otherwise it lands on <body> and a keyboard user loses their place.
const opener = document.activeElement as HTMLElement | null
card?.focus()
// The dialog's own focusable controls, in DOM order, skipping disabled ones.
const focusable = () =>
Array.from(
card?.querySelectorAll<HTMLElement>(
'a[href], button:not([disabled]), textarea:not([disabled]), input:not([disabled]), select:not([disabled]), [tabindex]:not([tabindex="-1"])',
Review

🟠 focusable() selector includes input[type="hidden"], breaking tab trap boundary when hidden inputs are at modal edges

correctness, maintainability · flagged by 2 models

  • web/src/components/ui/Modal.tsx:40 — The focusable() selector uses input:not([disabled]), which includes input[type="hidden"]. Hidden inputs are not keyboard-focusable. If a modal ever places a hidden input at the start or end of its content (e.g., a form with a CSRF token), the focus trap’s boundary checks (active === first / active === last) will compare against the hidden input instead of the real last tabbable control. Tab (or Shift+Tab) from the actual last visible button…

🪰 Gadfly · advisory

🟠 **focusable() selector includes input[type="hidden"], breaking tab trap boundary when hidden inputs are at modal edges** _correctness, maintainability · flagged by 2 models_ - **`web/src/components/ui/Modal.tsx:40`** — The `focusable()` selector uses `input:not([disabled])`, which includes `input[type="hidden"]`. Hidden inputs are not keyboard-focusable. If a modal ever places a hidden input at the start or end of its content (e.g., a form with a CSRF token), the focus trap’s boundary checks (`active === first` / `active === last`) will compare against the hidden input instead of the real last tabbable control. Tab (or Shift+Tab) from the actual last visible button… <sub>🪰 Gadfly · advisory</sub>
) ?? [],
)
function onKey(e: KeyboardEvent) { function onKey(e: KeyboardEvent) {
if (e.key === 'Escape' && !busyRef.current) onCloseRef.current() if (e.key === 'Escape' && !busyRef.current) {
onCloseRef.current()
return
}
if (e.key !== 'Tab') return
Review

🟠 Focus trap doesn't handle the focused element being removed from the DOM (e.g. ShareGardenModal's remove-share button), letting Tab escape the dialog

correctness · flagged by 1 model

  • web/src/components/ui/Modal.tsx:49 — The Tab-trap checks active === first/last/card but not the case where the previously-focused element was removed from the DOM (focus reverts to <body> per browser default). ShareGardenModal.tsx:117-127's per-share "✕ Remove" button lives inside the trapped dialog; activating it via keyboard removes it from the list, so the next Tab press finds document.activeElement === body, which matches none of the checked branches — no preventDefault() fires…

🪰 Gadfly · advisory

🟠 **Focus trap doesn't handle the focused element being removed from the DOM (e.g. ShareGardenModal's remove-share button), letting Tab escape the dialog** _correctness · flagged by 1 model_ - `web/src/components/ui/Modal.tsx:49` — The Tab-trap checks `active === first/last/card` but not the case where the previously-focused element was removed from the DOM (focus reverts to `<body>` per browser default). `ShareGardenModal.tsx:117-127`'s per-share "✕ Remove" button lives inside the trapped dialog; activating it via keyboard removes it from the list, so the next Tab press finds `document.activeElement === body`, which matches none of the checked branches — no `preventDefault()` fires… <sub>🪰 Gadfly · advisory</sub>
// Trap Tab inside the dialog: wrap at the ends, and pull a stray focus
// (e.g. starting from the card itself) back to a real control. Without
// this, Tab walks straight out into the page behind the backdrop.
const items = focusable()
if (items.length === 0) {
e.preventDefault()
card?.focus()
return
}
const first = items[0]
const last = items[items.length - 1]
const active = document.activeElement
Outdated
Review

🟡 Focus trap falls through (Tab escapes) when activeElement is outside {first, last, card} — external/stolen focus isn't pulled back in

correctness, error-handling · flagged by 2 models

  • web/src/components/ui/Modal.tsx:61-68 — the Tab-wrap logic only fires when active === first (Shift), active === card (Shift), or active === last (non-Shift). If document.activeElement is none of those — e.g. focus escaped to <body> or an element behind the backdrop because something stole focus after mount, or focus landed on a [tabindex="-1"] child that isn't the card — neither branch executes, preventDefault is not called, and Tab walks out of the dialog. The comment claims i…

🪰 Gadfly · advisory

🟡 **Focus trap falls through (Tab escapes) when activeElement is outside {first, last, card} — external/stolen focus isn't pulled back in** _correctness, error-handling · flagged by 2 models_ - `web/src/components/ui/Modal.tsx:61-68` — the Tab-wrap logic only fires when `active === first` (Shift), `active === card` (Shift), or `active === last` (non-Shift). If `document.activeElement` is none of those — e.g. focus escaped to `<body>` or an element behind the backdrop because something stole focus after mount, or focus landed on a `[tabindex="-1"]` child that isn't the card — neither branch executes, `preventDefault` is not called, and Tab walks out of the dialog. The comment claims i… <sub>🪰 Gadfly · advisory</sub>
if (e.shiftKey && (active === first || active === card)) {
e.preventDefault()
last.focus()
} else if (!e.shiftKey && active === last) {
e.preventDefault()
first.focus()
}
} }
document.addEventListener('keydown', onKey) document.addEventListener('keydown', onKey)
return () => document.removeEventListener('keydown', onKey) return () => {
document.removeEventListener('keydown', onKey)
opener?.focus?.()
Review

🟠 Focus restore on modal close doesn't check if the opener element is still in the DOM, silently failing for the exact delete/clear flows the PR targets

error-handling · flagged by 2 models

  • web/src/components/ui/Modal.tsx:73opener is captured once at mount (line 33) as document.activeElement and the cleanup calls opener?.focus?.(). The ref is never re-resolved, and there is no isConnected guard or fallback target. If the opener was removed from the DOM by the time the dialog closes (e.g. the trigger's parent conditionally re-rendered, or a list item behind the backdrop was swapped out), .focus() on a detached node is a silent no-op and focus falls to <body> — exa…

🪰 Gadfly · advisory

🟠 **Focus restore on modal close doesn't check if the opener element is still in the DOM, silently failing for the exact delete/clear flows the PR targets** _error-handling · flagged by 2 models_ - `web/src/components/ui/Modal.tsx:73` — `opener` is captured once at mount (line 33) as `document.activeElement` and the cleanup calls `opener?.focus?.()`. The ref is never re-resolved, and there is no `isConnected` guard or fallback target. If the opener was removed from the DOM by the time the dialog closes (e.g. the trigger's parent conditionally re-rendered, or a list item behind the backdrop was swapped out), `.focus()` on a detached node is a silent no-op and focus falls to `<body>` — exa… <sub>🪰 Gadfly · advisory</sub>
}
}, []) }, [])
return ( return (
+6
View File
@@ -235,7 +235,13 @@ export function GardenCanvas({
className="h-full w-full select-none" className="h-full w-full select-none"
style={{ touchAction: 'none' }} style={{ touchAction: 'none' }}
onPointerDown={onCanvasPointerDown} onPointerDown={onCanvasPointerDown}
// role="application" tells a screen reader this is an interactive canvas
// to operate, not a document to read linearly. The <title> names it, and
// objects inside are individually focusable buttons (see ObjectShape).
role="application"
aria-label={`${garden.name} — garden layout. Tab between objects; Enter selects; arrow keys nudge a selection.`}
> >
<title>{garden.name} garden layout</title>
<g transform={`translate(${viewport.tx} ${viewport.ty}) scale(${viewport.scale})`}> <g transform={`translate(${viewport.tx} ${viewport.ty}) scale(${viewport.scale})`}>
{drawnGridCm != null && ( {drawnGridCm != null && (
<> <>
+36 -2
View File
@@ -1,5 +1,6 @@
import { memo, type PointerEvent } from 'react' import { memo, type KeyboardEvent, type PointerEvent } from 'react'
import { objectTransform } from './shared' import { objectTransform } from './shared'
import { objectDisplayName } from './kinds'
import type { EditorObject } from './types' import type { EditorObject } from './types'
const DEFAULT_FILL = '#8a8a8a' const DEFAULT_FILL = '#8a8a8a'
@@ -57,11 +58,44 @@ export const ObjectShape = memo(function ObjectShape({
onSelect(object.id) onSelect(object.id)
} }
// Keyboard path into selection (#84): the arrow-key nudge handler already
// exists but only ever acted on a pointer selection, so it was unreachable
// without a mouse. Enter/Space on a focused object selects it, which is the
// step that was missing.
function handleKey(e: KeyboardEvent) {
if (e.key === 'Enter' || e.key === ' ') {
e.preventDefault()
e.stopPropagation()
onSelect(object.id)
}
}
const stroke = selected ? '#2f7a3e' : '#00000033' const stroke = selected ? '#2f7a3e' : '#00000033'
const strokeWidth = selected ? 2 : 1 const strokeWidth = selected ? 2 : 1
// A concise accessible name: the object's label plus its kind, e.g.
// "North Bed, raised bed". The dimensions aren't included — they need the
// garden's unit context this component doesn't hold — so they're a follow-up.
const label = `${objectDisplayName(object)}, ${object.kind.replace(/_/g, ' ')}`
Outdated
Review

🟠 Ad-hoc kind string formatting duplicates existing kindDef labels

correctness, maintainability · flagged by 4 models

  • web/src/editor/ObjectShape.tsx:79 — The accessible label builds the kind display string with object.kind.replace(/_/g, ' '), duplicating a concern that kindDef() in ./kinds.ts already solves. kindDef returns curated, Title Case labels (e.g. "Grow bag", "In-ground") while the regex produces lowercase, unhyphenated output ("grow bag", "in ground"). This creates an inconsistency with the rest of the UI (e.g. JournalPanel uses objectDisplayName, which respects kindDef l…

🪰 Gadfly · advisory

🟠 **Ad-hoc kind string formatting duplicates existing kindDef labels** _correctness, maintainability · flagged by 4 models_ - **`web/src/editor/ObjectShape.tsx:79`** — The accessible label builds the kind display string with `object.kind.replace(/_/g, ' ')`, duplicating a concern that `kindDef()` in `./kinds.ts` already solves. `kindDef` returns curated, Title Case labels (e.g. `"Grow bag"`, `"In-ground"`) while the regex produces lowercase, unhyphenated output (`"grow bag"`, `"in ground"`). This creates an inconsistency with the rest of the UI (e.g. `JournalPanel` uses `objectDisplayName`, which respects `kindDef` l… <sub>🪰 Gadfly · advisory</sub>
// Keyboard focus needs to be VISIBLE — that's the point of making the canvas
// keyboard-reachable. The `object-shape` class carries a :focus-visible rule
// (styles/index.css) that draws a dashed ring; :focus-visible means it shows
// for keyboard focus but NOT a mouse click, which is exactly what we want. CSS
// rather than React state because onFocus on an SVG <g> is unreliable and a
// presentation attribute is overridden by any CSS rule.
return ( return (
<g transform={objectTransform(object)} onPointerDown={handleDown} style={{ cursor: 'pointer' }}> <g
className="object-shape"
transform={objectTransform(object)}
onPointerDown={handleDown}
onKeyDown={handleKey}
role="button"
tabIndex={0}
aria-label={label}
aria-pressed={selected}
Review

🟠 aria-pressed misused on non-toggle role="button"; object selection is not a toggle action

correctness · flagged by 2 models

  • web/src/editor/ObjectShape.tsx:96aria-pressed={selected} on role="button" advertises a toggle button to screen readers (pressed / not pressed). The keyboard action (handleKey) always calls onSelect(object.id); there is no path that deselects the object when Enter/Space is pressed again on an already-selected item, so the toggle semantics are misleading. A screen reader user will expect the state to flip on second activation, which it does not. Fix: remove aria-pressed. I…

🪰 Gadfly · advisory

🟠 **aria-pressed misused on non-toggle role="button"; object selection is not a toggle action** _correctness · flagged by 2 models_ - **`web/src/editor/ObjectShape.tsx:96`** — `aria-pressed={selected}` on `role="button"` advertises a toggle button to screen readers (pressed / not pressed). The keyboard action (`handleKey`) always calls `onSelect(object.id)`; there is no path that deselects the object when Enter/Space is pressed again on an already-selected item, so the toggle semantics are misleading. A screen reader user will expect the state to flip on second activation, which it does not. **Fix:** remove `aria-pressed`. I… <sub>🪰 Gadfly · advisory</sub>
style={{ cursor: 'pointer' }}
>
{object.shape === 'circle' ? ( {object.shape === 'circle' ? (
<ellipse <ellipse
cx={0} cx={0}
+13
View File
@@ -28,6 +28,19 @@
font-family: var(--font-sans); font-family: var(--font-sans);
-webkit-font-smoothing: antialiased; -webkit-font-smoothing: antialiased;
} }
/* Keyboard focus on a canvas object (#84). :focus-visible shows the ring for
keyboard focus but not a mouse click; the dashed accent ring distinguishes
"focused" from the solid ring that marks "selected". A CSS rule overrides
the shape's inline stroke presentation attributes. */
.object-shape {
outline: none;
}
.object-shape:focus-visible :is(rect, ellipse) {
stroke: var(--color-accent-strong);
stroke-width: 2;
stroke-dasharray: 5 4;
}
} }
/* Dark theme: override the same tokens so utilities recolor automatically. */ /* Dark theme: override the same tokens so utilities recolor automatically. */