🐛 fix: stop the Select reopening when a Drawer or Modal returns focus - #686
Merged
Conversation
Wrapping the Modal and Drawer panels in react-focus-lock (0.1.2-alpha.104) made the Select unusable inside them. Clicking an option focuses the `<li>`, the list then unmounts, and the focus lock returns focus to the combobox — which fired the `focusin` listener that opened the list whenever the target matched `:focus-visible`. The list reopened immediately and covered every field below it, so the form could not be filled in at all. Opening on focus was wrong regardless of the focus lock: per the ARIA select-only combobox pattern, focus alone must not expand the listbox. It is replaced with explicit keyboard intent on the combobox — ArrowDown opens and, once open, moves to the first option; Enter and Space toggle it, but only while the combobox itself holds focus, so both keys still reach the search input of a searchable Select. Both imperative listeners in `useSelect` are consolidated into a React `onKeyDown`, where `isOpen` can never be stale. jsdom reports `false` for `:focus-visible` on every element, which is why the existing "closes after selecting an option in modal" test stayed green while the behaviour was broken in real browsers. The new tests cover the keyboard open paths and the drawer focus-return.
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
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
Stops the
Selectfrom reopening its listbox when aDrawerorModalreturns focus to the combobox, and moves opening to explicit keyboard intent.useSelectno longer opens the list from afocusinwhose target matches:focus-visible.onKeyDown:ArrowDownopens and, once open, moves focus to the first option;Enter/Spacetoggle it, but only while the combobox itself holds focus.addEventListenercalls inuseSelectare consolidated into that React handler, whereisOpencan never be stale.Why
0.1.2-alpha.104wrapped theModalandDrawerpanels inreact-focus-lock(#672). That made everySelectinside them unusable:<li>.toggleOpen(false)unmounts the list, so the focused element disappears.div[role="combobox"].focusinlistener which opened the list whenever the target matched:focus-visible.The list reopened immediately and covered every field below it, so the form could not be filled in at all. Verified in a real browser (see below).
Opening on focus was wrong regardless of the focus lock: per the ARIA select-only combobox pattern, focus alone must not expand the listbox —
ArrowDown,EnterandSpaceare the open keys. KeepingEnter/Spacescoped toevent.target === event.currentTargetmeans both keys still reach the search input of a searchableSelect.Testing
vitest run— 574/574 pass.npm run check:types,npm run lint,prettier --check— clean.Select.test.tsx. The 4 keyboard-open ones fail onmainand pass here (confirmed by stashing only the source changes), plus one asserting focus alone does not expand and one for aSelectinside aDrawer.civo/dashboard/dns-micro-frontend(whoseadd-recordCypress spec is what surfaced the bug): e2e 13/13 (was 12/13) and visual regression 8/8, including thedomain-records-after-createsnapshot.Worth noting: jsdom returns
falseformatches(':focus-visible')on every element, so thefocusinbranch was dead code under unit tests. That is why the existing "should close select list after selecting an option in modal" test stayed green while the behaviour was broken in real browsers.