fix(#439): make anchored-dialog Tab navigation deterministic on Safari/WebKit - #442
Merged
Merged
Conversation
…i/WebKit openAnchoredDialog's focus trap only wrapped at the first/last declared focusable element and delegated every middle-of-list Tab to native browser traversal. WebKit/Safari's default "Tab highlights every item" preference is off, so that native traversal skipped checkboxes/buttons in the multi-select and time-range popovers, stranding keyboard users before Clear/Cancel/Apply. The trap now owns every Tab/Shift+Tab transition via modular index arithmetic over the freshly recomputed eligible set, verified end-to-end in Chromium, Firefox, and WebKit. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KyBqpFXWKxkVrScz3N1E3v
Review finding: disabling the currently-focused control natively blurs it out of the dialog entirely (to <body>), past the shared Tab trap's dialog-scoped reach — a plain Tab press right after never re-enters the modal. The multi-select's busy state (a Filter source loading while the popover stays open) disables every row but Cancel, so a focused Search/ option/Clear/Apply could escape. Adds `handle.reclaimFocus()` to openAnchoredDialog: recomputes the eligible set and, if the active element is no longer in it, moves focus to the first eligible element. Multi-select calls it the moment busy state lands. Verified the regression reproduces (and is fixed) in Chromium, Firefox, and WebKit. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KyBqpFXWKxkVrScz3N1E3v
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What & why
openAnchoredDialog's shared focus trap (src/ui/popover.ts) only handled wrapping at the first/last declared focusable element and delegated every middle-of-list Tab press to native browser sequential-focus. WebKit/Safari's default "Tab highlights every item on a webpage" preference is off, so native traversal there could skip checkboxes and buttons — in the multi-select and time-range popovers this meant keyboard focus could never reach Clear/Cancel/Apply, and the modal trap could let focus escape rather than wrapping.The trap now owns every Tab/Shift+Tab transition: the freshly recomputed eligible set's current index (or -1 if focus is outside the set) plus modular arithmetic decides the next stop, in both directions, with wrap. Disabled/hidden/removed elements drop out immediately; restored ones rejoin without reopening the dialog.
Follow-up finding from review: disabling the currently-focused control natively blurs it out of the dialog entirely (to
<body>), past the trap's dialog-scoped reach — a plain Tab press right after never re-enters the modal. The multi-select's busy state (a Filter source loading while the popover stays open) disables every row but Cancel, so a focused Search/option/Clear/Apply could escape undetected; the new unit tests couldn't catch this because they dispatch synthetic keydowns directly on the dialog, bypassing the real event-path gap. Addedhandle.reclaimFocus()to the shared primitive — recomputes the eligible set and, if the active element is no longer in it, moves focus to the first eligible element — and multi-select now calls it the moment busy state lands. Verified the regression reproduces (and is fixed) with a real Playwright test across Chromium, Firefox, and WebKit (confirmed by temporarily reverting the fix and watching the new test fail in all three engines, then re-applying it).Closes #439.
Checklist
npm testpasses (the per-file coverage gate is non-negotiable)npm run buildsucceeds (single-filedist/sql.html)src/core/, network insrc/net/(injected fetch), DOM insrc/ui/CHANGELOG.md([Unreleased]) updatedVerification
🤖 Generated with Claude Code
https://claude.ai/code/session_01KyBqpFXWKxkVrScz3N1E3v