Make the canvas keyboard-reachable + trap focus in dialogs #92
@@ -1,5 +1,12 @@
|
||||
import { useEffect, useRef, type ReactNode } from 'react'
|
||||
|
||||
// Tabbable controls inside the dialog, in DOM order. type="hidden" inputs are
|
||||
// excluded — they'd match `input:not([disabled])` and, sitting at a boundary,
|
||||
// break the wrap math. Hoisted out of the handler so it isn't rebuilt per Tab.
|
||||
const FOCUSABLE_SELECTOR =
|
||||
'a[href], button:not([disabled]), textarea:not([disabled]), ' +
|
||||
'input:not([disabled]):not([type="hidden"]), select:not([disabled]), [tabindex]:not([tabindex="-1"])'
|
||||
|
||||
/**
|
||||
* A centered modal dialog over a dimmed backdrop. Closes on Escape or a backdrop
|
||||
* click, unless `busy` (a mutation is in flight) — then it stays put so the
|
||||
@@ -27,12 +34,57 @@ export function Modal({
|
||||
busyRef.current = busy
|
||||
|
||||
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()
|
||||
|
||||
const focusable = () =>
|
||||
Array.from(card?.querySelectorAll<HTMLElement>(FOCUSABLE_SELECTOR) ?? [])
|
||||
|
||||
function onKey(e: KeyboardEvent) {
|
||||
if (e.key === 'Escape' && !busyRef.current) onCloseRef.current()
|
||||
if (e.key === 'Escape' && !busyRef.current) {
|
||||
onCloseRef.current()
|
||||
return
|
||||
|
gitea-actions
commented
🟠 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
🪰 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>
|
||||
}
|
||||
if (e.key !== 'Tab') return
|
||||
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
|
||||
// If focus is NOT inside the dialog, pull it back in rather than let Tab
|
||||
|
gitea-actions
commented
🟡 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
🪰 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>
|
||||
// escape. This is the robust case that covers focus having fallen to
|
||||
// <body> — a control that was removed (ShareGardenModal's remove-share
|
||||
// button) or disabled while busy — as well as any externally-stolen focus.
|
||||
if (!card || !card.contains(active)) {
|
||||
e.preventDefault()
|
||||
;(e.shiftKey ? last : first).focus()
|
||||
return
|
||||
}
|
||||
if (e.shiftKey && (active === first || active === card)) {
|
||||
e.preventDefault()
|
||||
last.focus()
|
||||
} else if (!e.shiftKey && active === last) {
|
||||
|
gitea-actions
commented
🟠 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
🪰 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>
|
||||
e.preventDefault()
|
||||
first.focus()
|
||||
}
|
||||
}
|
||||
document.addEventListener('keydown', onKey)
|
||||
return () => document.removeEventListener('keydown', onKey)
|
||||
return () => {
|
||||
document.removeEventListener('keydown', onKey)
|
||||
// Restore focus to the opener only if it's still in the document — the
|
||||
// delete/clear flows this trap targets often remove the element that
|
||||
// opened the dialog (a garden card, a plop row). A disconnected node's
|
||||
// focus() silently no-ops and leaves focus on <body>, so fall through to
|
||||
// that case explicitly rather than pretend it worked.
|
||||
if (opener && opener.isConnected) opener.focus()
|
||||
}
|
||||
}, [])
|
||||
|
||||
return (
|
||||
|
||||
@@ -235,7 +235,13 @@ export function GardenCanvas({
|
||||
className="h-full w-full select-none"
|
||||
style={{ touchAction: 'none' }}
|
||||
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})`}>
|
||||
{drawnGridCm != null && (
|
||||
<>
|
||||
|
||||
@@ -1,5 +1,6 @@
|
||||
import { memo, type PointerEvent } from 'react'
|
||||
import { memo, type KeyboardEvent, type PointerEvent } from 'react'
|
||||
import { objectTransform } from './shared'
|
||||
import { kindDef, objectDisplayName } from './kinds'
|
||||
import type { EditorObject } from './types'
|
||||
|
||||
const DEFAULT_FILL = '#8a8a8a'
|
||||
@@ -57,11 +58,50 @@ export const ObjectShape = memo(function ObjectShape({
|
||||
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 strokeWidth = selected ? 2 : 1
|
||||
|
||||
// A concise accessible name: the object's label plus its kind's canonical
|
||||
// label, e.g. "North Bed, In-ground" — reusing kindDef so it never diverges
|
||||
// from what the UI shows (an ad-hoc kind.replace() gave "in ground"). The
|
||||
// dimensions aren't included; they need the garden's unit context this
|
||||
|
gitea-actions
commented
🟠 Ad-hoc kind string formatting duplicates existing kindDef labels correctness, maintainability · flagged by 4 models
🪰 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>
|
||||
// component doesn't hold, so they're a follow-up.
|
||||
const kindLabel = kindDef(object.kind)?.label ?? object.kind
|
||||
const label = `${objectDisplayName(object)}, ${kindLabel}`
|
||||
|
||||
// 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 (
|
||||
<g transform={objectTransform(object)} onPointerDown={handleDown} style={{ cursor: 'pointer' }}>
|
||||
<g
|
||||
className="object-shape"
|
||||
transform={objectTransform(object)}
|
||||
onPointerDown={handleDown}
|
||||
onKeyDown={handleKey}
|
||||
role="button"
|
||||
|
gitea-actions
commented
🟠 aria-pressed misused on non-toggle role="button"; object selection is not a toggle action correctness · flagged by 2 models
🪰 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>
|
||||
tabIndex={0}
|
||||
aria-label={label}
|
||||
// aria-current, not aria-pressed: selecting an object isn't a toggle (a
|
||||
// toggle is what aria-pressed means). aria-current marks it as the active
|
||||
// item among the objects. Omitted, not "false", when unselected.
|
||||
aria-current={selected || undefined}
|
||||
style={{ cursor: 'pointer' }}
|
||||
>
|
||||
{object.shape === 'circle' ? (
|
||||
<ellipse
|
||||
cx={0}
|
||||
|
||||
@@ -28,6 +28,19 @@
|
||||
font-family: var(--font-sans);
|
||||
-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. */
|
||||
|
||||
🟠 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— Thefocusable()selector usesinput:not([disabled]), which includesinput[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