Move Sheet focus only to controls that can take keyboard focus
CI / ci (push) Successful in 1m30s
CI / ci (pull_request) Successful in 1m29s

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>
This commit is contained in:
2026-10-03 18:14:15 -04:00
co-authored by Claude Opus 5.5
parent 1dd97b123c
commit 0fa9cfe611
2 changed files with 86 additions and 3 deletions
+59
View File
@@ -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()
+27 -3
View File
@@ -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')