Address Gadfly findings on #84

- Object aria-label uses kindDef().label (the canonical "In-ground") instead of an ad-hoc kind.replace() that produced "in ground" and diverged from the UI. 4+ models flagged this. - aria-current, not aria-pressed, for the selected object — selection isn't a toggle, which is what aria-pressed means; aria-current marks the active item. - Modal focus trap made robust: if focus is NOT inside the dialog (fell to <body> because the focused control was removed — ShareGardenModal's remove-share button — or disabled while busy, or externally stolen), Tab now pulls it back in instead of escaping. The previous branches only handled focus being exactly at a known boundary. - Focus restore checks opener.isConnected before calling focus(): the delete/ clear flows this targets often remove the element that opened the dialog, and a disconnected node's focus() silently no-ops. - Hoisted the focusable-element selector to a module constant, and excluded input[type="hidden"] (it matched input:not([disabled]) and, at a boundary, broke the wrap). Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
2026-07-21 23:33:38 -04:00
parent bfc5d9a871
commit 4b348dcbc0
2 changed files with 35 additions and 16 deletions
@@ -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
@@ -33,13 +40,8 @@ export function Modal({
    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"])',
-        ) ?? [],
-      )
+      Array.from(card?.querySelectorAll<HTMLElement>(FOCUSABLE_SELECTOR) ?? [])

    function onKey(e: KeyboardEvent) {
      if (e.key === 'Escape' && !busyRef.current) {
@@ -47,9 +49,6 @@ export function Modal({
        return
      }
      if (e.key !== 'Tab') return
-      // 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()
@@ -59,6 +58,15 @@ export function Modal({
      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
+      // 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()
@@ -70,7 +78,12 @@ export function Modal({
    document.addEventListener('keydown', onKey)
    return () => {
      document.removeEventListener('keydown', onKey)
-      opener?.focus?.()
+      // 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()
    }
  }, [])