Add keyboard navigation to Dropdown menus#320
Open
jonatankruszewski wants to merge 1 commit into
Open
Conversation
The shared Dropdown never moved focus into its menu, so once open it was unreachable by keyboard: arrow keys were unhandled, no item was ever highlighted, and because the menu is portaled to the end of <body>, Tab couldn't reach it either. Only Enter/Space on the trigger and Escape were wired up, which made it feel keyboard-accessible right up until you tried to pick something. Opening now seeds an active item -- the selected one, so the theme menu opens on the active theme -- and moves focus to it. ArrowUp/ArrowDown move the highlight, wrapping at the ends and skipping disabled items; Enter/Space selects; Escape and Tab close. Closing returns focus to the trigger whenever focus is still inside the menu, so mouse users don't lose their place in the tab order either. Hovering syncs the highlight so the pointer and keyboard can't disagree about which item is active. Menus driven by selectedId are single-select, so their items are now exposed as menuitemradio/aria-checked rather than plain menuitem, and the trigger carries aria-haspopup/aria-expanded. The index math (wrapping, skipping disabled entries, seeding from the current selection) lives in dropdownNavigation.ts and is unit-tested; a Playwright spec drives the real theme picker end to end. Closes dcouple#319
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.
Description
Closes #319.
The shared
Dropdownnever moved focus into its menu, so once open it was unreachable by keyboard: arrow keys were unhandled, no item was ever highlighted, and because the menu is portaled to the end of<body>, Tab couldn't reach it either. Only Enter/Space on the trigger and Escape were wired up, which made it feel keyboard-accessible right up until you tried to pick something. It's the shared component, so this affected every menu in the app — the Theme picker is just the easiest place to see it.Opening now seeds an active item (the selected one, so the theme menu opens on the active theme) and moves focus to it:
selectedIdare single-select, so their items are exposed asmenuitemradio/aria-checkedrather than plainmenuitem; the trigger carriesaria-haspopup/aria-expandedThe index math (wrapping, skipping disabled entries, seeding from the current selection) is pulled out into
dropdownNavigation.tsso it can be unit-tested without React.Type of Change
Checklist
pnpm typecheckandpnpm lintlocallypnpm electron-devCritical Areas Modified
None of the listed areas. The change is contained to
frontend/src/components/ui/Dropdown.tsxplus a new pure helper module and its tests.Additional Notes
Tests: 12 unit tests for the navigation helpers, and a Playwright spec that drives the real Theme picker end to end (arrow keys move focus, Enter applies the theme, Escape closes without committing a change).
Two things I deliberately left out of scope, happy to fold either in if you'd prefer:
Dropdownwraps its trigger in a<div tabIndex={0}>and callers pass a real<button>, so one control gets two tab stops. That predates this PR, and fixing it properly means removing the wrapper, which touches everyDropdowncall site. I tried addingrole="button"to the wrapper and it made things worse — the wrapper and the inner button both appear as buttons with the same accessible name (it brokegetByRole('button', ...)in the existing smoke test), so the ARIA can't be fixed without removing the wrapper.CommandPalette,InterceptorDropdown, and here). A shared hook would be nice, but that felt like a bigger refactor than this fix warrants.