Uh oh!
There was an error while loading. Please reload this page.
fix(ui): route Select keyboard and ARIA through floating-ui hooks - #9179
Conversation
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: 311ca4a The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Repository UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthrough
ChangesSelect navigation and accessibility
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SelectButton
participant Select
participant useListNavigation
participant SelectOptionList
participant SelectOption
SelectButton->>Select: ArrowUp or ArrowDown
Select->>useListNavigation: Update activeIndex
useListNavigation->>SelectOptionList: Expose active option
SelectOptionList->>SelectOption: Apply active-descendant and item props
SelectOptionList->>Select: Enter selects active option
Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
bfd8210 to
cbaf0bcComparecbaf0bc to
e317f53Compare@clerk/astro@clerk/backend@clerk/chrome-extension@clerk/clerk-js@clerk/electron@clerk/electron-passkeys@clerk/eslint-plugin@clerk/expo@clerk/expo-passkeys@clerk/express@clerk/fastify@clerk/hono@clerk/localizations@clerk/nextjs@clerk/nuxt@clerk/react@clerk/react-router@clerk/shared@clerk/tanstack-react-start@clerk/testing@clerk/ui@clerk/upgrade@clerk/vuecommit: |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/ui/src/elements/Select.tsx (1)
507-521: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPass consumer handlers through
getReferenceProps.{...rest}can overwrite Floating UI’s composedonClick/onKeyDown, so consumer-supplied handlers break trigger toggling and keyboard opening. Move the remaining props intogetReferencePropsinstead of spreading them afterward.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ui/src/elements/Select.tsx` around lines 507 - 521, Update the trigger prop composition around getReferenceProps so consumer props from rest are passed into getReferenceProps and its composed handlers take precedence. Remove the trailing {...rest} spread, preserving aria attributes and styling while ensuring consumer onClick/onKeyDown handlers do not overwrite Floating UI trigger behavior.Sources: Coding guidelines, Path instructions
🧹 Nitpick comments (1)
packages/ui/src/elements/Select.tsx (1)
201-214: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPreserve the option generic instead of erasing it with
any.
SelectrenderOptionProps<any>discardsOacrossoption,renderOption, andhandleSelect. Keep these fields typed asOand preserve the generic throughReact.memo.As per coding guidelines, “No
anytypes without justification in code review.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ui/src/elements/Select.tsx` around lines 201 - 214, Update SelectrenderOptionProps and SelectRenderOption to preserve the option generic O: type option and handleSelect with O, use RenderOption<O>, and remove the any instantiation. Preserve O through the React.memo-wrapped component rather than erasing it at the component boundary.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/ui/src/elements/Select.tsx`:
- Around line 507-521: Update the trigger prop composition around
getReferenceProps so consumer props from rest are passed into getReferenceProps
and its composed handlers take precedence. Remove the trailing {...rest} spread,
preserving aria attributes and styling while ensuring consumer onClick/onKeyDown
handlers do not overwrite Floating UI trigger behavior.
---
Nitpick comments:
In `@packages/ui/src/elements/Select.tsx`:
- Around line 201-214: Update SelectrenderOptionProps and SelectRenderOption to
preserve the option generic O: type option and handleSelect with O, use
RenderOption<O>, and remove the any instantiation. Preserve O through the
React.memo-wrapped component rather than erasing it at the component boundary.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Repository UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: a183e7aa-821b-451e-b50b-e46045e34ab8
📒 Files selected for processing (3)
.changeset/drawer-select-escape.mdpackages/ui/src/elements/Select.tsxpackages/ui/src/elements/__tests__/Select.test.tsx
API Changes Report
Summary
No API Changes DetectedAll packages have stable APIs with no detected changes. Report generated by Break Check Last ran on |
e317f53 to
311ca4aCompare
maxyinger
left a comment
There was a problem hiding this comment.
TIL about aria-activedescendant. very cool. less useEffect logic too. nice 😎
alexcarpenter
commented
Jul 16, 2026
yeah, currently navigating the Phone Input options via keyboard while the input is focused doesn't announce which option you are on, BEFORE before.movAFTER after.mov |
Description
Stacked on top of #9176 (
esc-closes-checkout-drawer). ReworksSelectso its keyboard handling goes through floating-ui'suseListNavigation/useInteractionsinstead of a hand-rolledonKeyDown: pressingArrowUp/ArrowDownon a focused, closedSelectnow opens the listbox, and navigation is a single source of truth. Options are wired up withgetItemProps(so hover follows the keyboard highlight), and the active option is announced viaaria-activedescendant. ARIA is now correct per mode — the plain variant keeps its listbox semantics, and the searchable variant (e.g. thePhoneInputcountry picker) exposes a proper combobox: the input isrole="combobox"witharia-controls,aria-autocomplete="list", andaria-activedescendant.To test: focus a
Selecttrigger and press Arrow keys to open it; navigate with the arrows and confirm the highlighted option is exposed viaaria-activedescendant(on the listbox for a plain select, on the search input for the searchablePhoneInputpicker).Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change
Summary by CodeRabbit
Selectkeyboard navigation: pressing Arrow Up/Down on a focused trigger now opens the listbox, and Enter selects the active option.aria-activedescendant.Selectvariants, updated combobox semantics (including correct ARIA attributes) so assistive technologies read the input/listbox state properly.