From 0fa9cfe611a9dcdcc76890a04dc1a8820126076f Mon Sep 17 00:00:00 2001
From: dev_agent <dev_agent@cubecraftlabs.com>
Date: Sat, 3 Oct 2026 18:14:15 -0400
Subject: [PATCH] Move Sheet focus only to controls that can take keyboard
 focus

Review finding MQ-36-01: a hidden first button was chosen as the initial
focus target and as a Tab trap stop, so focus never entered the dialog.
Filter out controls with a negative tabIndex, a hidden/aria-hidden/inert
ancestor, display: none or visibility: hidden; on open, skip any control
whose focus() does not take, falling back to the dialog panel.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
---
 web/src/ui/Sheet.test.tsx | 59 +++++++++++++++++++++++++++++++++++++++
 web/src/ui/Sheet.tsx      | 30 ++++++++++++++++++--
 2 files changed, 86 insertions(+), 3 deletions(-)

diff --git a/web/src/ui/Sheet.test.tsx b/web/src/ui/Sheet.test.tsx
index 14564b9..fb672d2 100644
--- a/web/src/ui/Sheet.test.tsx
+++ b/web/src/ui/Sheet.test.tsx
@@ -78,6 +78,65 @@ describe('Sheet', () => {
     expect(dialog).toHaveFocus()
   })
 
+  it('skips controls that cannot take keyboard focus when moving focus in', () => {
+    render(
+      <Sheet aria-label="Quest log" open onClose={() => {}}>
+        <button type="button" hidden>
+          Hidden
+        </button>
+        <button type="button" style={{ display: 'none' }}>
+          Not displayed
+        </button>
+        <button type="button" style={{ visibility: 'hidden' }}>
+          Invisible
+        </button>
+        <button type="button" tabIndex={-1}>
+          Not tabbable
+        </button>
+        <div aria-hidden="true">
+          <button type="button">Aria hidden</button>
+        </div>
+        <button type="button">Try it</button>
+      </Sheet>,
+    )
+    expect(screen.getByRole('button', { name: 'Try it' })).toHaveFocus()
+  })
+
+  it('focuses the dialog itself when every control is hidden', () => {
+    render(
+      <Sheet aria-label="Quest log" open onClose={() => {}}>
+        <button type="button" hidden>
+          Hidden
+        </button>
+      </Sheet>,
+    )
+    expect(screen.getByRole('dialog', { name: 'Quest log' })).toHaveFocus()
+  })
+
+  it('does not stop the Tab trap on a hidden control', () => {
+    render(
+      <Sheet aria-label="Quest log" open onClose={() => {}}>
+        <button type="button" hidden>
+          Hidden first
+        </button>
+        <button type="button">Try it</button>
+        <button type="button">Pass</button>
+        <button type="button" hidden>
+          Hidden last
+        </button>
+      </Sheet>,
+    )
+    const first = screen.getByRole('button', { name: 'Try it' })
+    const last = screen.getByRole('button', { name: 'Pass' })
+
+    last.focus()
+    fireEvent.keyDown(last, { key: 'Tab' })
+    expect(first).toHaveFocus()
+
+    fireEvent.keyDown(first, { key: 'Tab', shiftKey: true })
+    expect(last).toHaveFocus()
+  })
+
   it('wraps Tab and Shift+Tab inside the sheet', () => {
     render(<Harness />)
     openSheet()
diff --git a/web/src/ui/Sheet.tsx b/web/src/ui/Sheet.tsx
index d1db410..18303b5 100644
--- a/web/src/ui/Sheet.tsx
+++ b/web/src/ui/Sheet.tsx
@@ -19,11 +19,35 @@ const focusableSelector = [
   'input:not([disabled])',
   'select:not([disabled])',
   'textarea:not([disabled])',
-  '[tabindex]:not([tabindex="-1"])',
+  '[tabindex]',
 ].join(',')
 
+// Whether an element can take keyboard focus: a negative tabIndex, a hidden,
+// aria-hidden or inert ancestor (or self), display: none on the way up to the
+// panel, or visibility: hidden all rule it out.
+function isKeyboardFocusable(element: HTMLElement, panel: HTMLElement): boolean {
+  if (element.tabIndex < 0) return false
+  if (element.closest('[hidden], [aria-hidden="true"], [inert]')) return false
+  if (getComputedStyle(element).visibility === 'hidden') return false
+  for (let node: HTMLElement | null = element; node && node !== panel; node = node.parentElement) {
+    if (getComputedStyle(node).display === 'none') return false
+  }
+  return true
+}
+
 function focusables(panel: HTMLElement): HTMLElement[] {
-  return Array.from(panel.querySelectorAll<HTMLElement>(focusableSelector))
+  return Array.from(panel.querySelectorAll<HTMLElement>(focusableSelector)).filter((element) =>
+    isKeyboardFocusable(element, panel),
+  )
+}
+
+// Focuses the first control that actually takes focus, or the panel if none does.
+function focusFirst(panel: HTMLElement) {
+  for (const element of focusables(panel)) {
+    element.focus()
+    if (document.activeElement === element) return
+  }
+  panel.focus()
 }
 
 // Modal bottom sheet. Portalled to <body> so everything else there can be made
@@ -43,7 +67,7 @@ export function Sheet({ open, onClose, children, ...name }: SheetProps) {
       (element) => element !== root && !element.hasAttribute('inert'),
     )
     for (const element of background) element.setAttribute('inert', '')
-    ;(focusables(panel)[0] ?? panel).focus()
+    focusFirst(panel)
 
     return () => {
       for (const element of background) element.removeAttribute('inert')