[select] Skip disabled items in typeahead and fix multiple-mode serialization#5025
Open
atomiks wants to merge 1 commit into
Open
[select] Skip disabled items in typeahead and fix multiple-mode serialization#5025atomiks wants to merge 1 commit into
atomiks wants to merge 1 commit into
Conversation
commit: |
Bundle size
PerformanceTotal duration: 1,352.76 ms -101.49 ms(-7.0%) | Renders: 50 (+0) | Paint: 2,057.49 ms -126.90 ms(-5.8%)
11 tests within noise — details Check out the code infra dashboard for more information about this PR. |
✅ Deploy Preview for base-ui ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
2d54c1b to
7e3f5fc
Compare
7e3f5fc to
691843e
Compare
…lization Bugs: typeahead now skips disabled items while matching, so a single keypress advances to the next selectable item (like native <select> and arrow-key navigation) in both open and closed states — via a new opt-in 'isIndexDisabled' predicate on useTypeahead (Menu/Combobox don't pass it, so they're unchanged); multiple mode no longer passes the whole value array to itemToStringValue (the shared input is nameless); maxHeight:'auto' (invalid no-op) -> 'none' in align mode; trigger callback refs no longer fire twice; empty multiple array compares as undefined (not raw []) for custom isItemEqualToValue. Cleanup: remove write-only keyboardActiveRef and the unconsumed events context field; drop the stray useStore arg in SelectArrow and the unused scroll-arrow refs; use ownerWindow(el).getComputedStyle per repo convention; comment fixes (SelectBackdrop description + docs, parseFloat fallback, outside-press); remove a shipped XXX note. Tests: typeahead skips a disabled item and commits the next match; commits nothing when the only match is disabled; multiple-mode itemToStringValue isn't invoked with the array.
691843e to
78d89a5
Compare
There was a problem hiding this comment.
Pull request overview
This PR fixes several Select correctness issues, centered on typeahead behavior and multiple-mode form serialization, and adds regression tests to prevent future regressions.
Changes:
- Extend
useTypeaheadwith an optionalisIndexDisabledpredicate and use it in Select to skip disabled items during matching (including closed-trigger typeahead). - Fix Select multiple-mode serialization so the shared hidden input doesn’t attempt to stringify the entire selected-value array.
- Misc. Select cleanup (remove unused context fields/refs, realm-safe
getComputedStyle, ref wiring) plus regression tests and doc/comment corrections.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| packages/react/src/select/trigger/SelectTrigger.tsx | Ref wiring cleanup to prevent duplicate ref-callback firing; clarifies outside-press mouseup handling. |
| packages/react/src/select/root/SelectRootContext.ts | Removes unused context fields (events, keyboardActiveRef) and unused import. |
| packages/react/src/select/root/SelectRoot.tsx | Multiple-mode serialization fix; integrates disabled-skipping into typeahead via new hook predicate; removes unconsumed events plumbing. |
| packages/react/src/select/root/SelectRoot.test.tsx | Adds regression tests for multiple-mode serialization and disabled-skipping typeahead on closed trigger. |
| packages/react/src/select/popup/SelectPopup.tsx | Uses realm-safe ownerWindow(...).getComputedStyle(...); fixes maxHeight assignment; removes unused refs. |
| packages/react/src/select/item/SelectItem.tsx | Ensures custom equality function isn’t called with the raw empty array in multiple mode. |
| packages/react/src/select/backdrop/SelectBackdrop.tsx | Corrects component doc comment (“select popup”). |
| packages/react/src/select/arrow/SelectArrow.tsx | Removes a stray unused argument passed to useStore. |
| packages/react/src/floating-ui-react/hooks/useTypeahead.ts | Adds isIndexDisabled option and skips disabled items while matching. |
| docs/src/app/(docs)/react/components/select/types.md | Updates generated docs text to match Select terminology (“select popup”). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
Summary
Fixes a few Select correctness issues plus adjacent cleanup, with regression tests.
Bugs
useTypeaheadonly skipped invisible elements, so typeahead could stop on a disabled item (and, on a closed trigger, commit it). It now skips disabled items while matching, so a single keypress advances to the next selectable item — matching native<select>and arrow-key navigation — in both open and closed states. This is done via a new opt-inisIndexDisabledpredicate onuseTypeahead, decoupled from theelementsRef/visibility filter so Select's mounted-but-hidden closed items still match. Menu and Combobox don't pass it, so their behavior is unchanged.itemToStringValue. The shared hidden input is nameless in multiple mode (per-value<input>s carry the data), soserializedValueis now''; previously a useritemToStringValuewritten for a single item would throw or yield''.positionerElement.style.maxHeight = 'auto'(invalid, silently ignored) →'none', so the explicit height governs in align mode and isn't clamped by user CSS.useRenderElementref array).isItemEqualToValueis no longer handed the raw empty array as the "selected value" in multiple mode (undefinedwhen nothing is selected).Cleanup
keyboardActiveRefand the unconsumedeventscontext field.ownerWindow(el).getComputedStyle(el)inSelectPopup(repo convention for realm-sensitive lookups).useStoreargument inSelectArrowand unused scroll-arrow refs.parseFloatfallback, outside-press); remove a shippedXXXnote.Tests
itemToStringValueisn't invoked with the array.Typecheck, eslint, prettier, and the Select / Menu / Combobox / useTypeahead suites pass. Based on current
master.