#490 - Select: multiple mode closes the dropdown after every selection - #491
Conversation
Selecting an option in multiple mode no longer closes the dropdown, and clicking an already-selected option deselects it. Escape and outside click (useDismiss) still close the list; single mode still closes on select. Updated MultiSelect and InlineSearchMulti story play tests to assert the keep-open and toggle behavior.
Flipping `multiple` to true while a single value was already selected crashed the component: `SelectItem`'s `multipleChecked` called `.some()` on a non-array value and `handleMultiSelect` spread it, so opening the dropdown or picking a second option threw a TypeError. Add a `toValuesArray()` helper and route every multi-value path through it (badge rendering, toggle/close handlers, placeholder checks, inline search backspace). In single mode a non-empty array now falls back to its first entry so `render()`/children never receive an array; an empty array is passed through unchanged to preserve existing output. Also add the missing `multiple`/`render`/`children`/`by` memo dependencies that kept stale single-mode closures alive after a mode flip. Adds a ToggleMultipleAtRuntime story covering the flip in both directions.
ravindrakele
left a comment
ravindrakele
left a comment
There was a problem hiding this comment.
Core fix reads right. One blocker on the inline-search path, rest are small.
Not inline (outside the diff hunks):
- Badge
key={ index }inrenderSelectedis more exposed now that mid-list deselect is possible. Key on thebyvalue. - Description skips
toValuesArray, the single-mode array fallback andToggleMultipleAtRuntime. Those fix a separate runtimemultipleflip crash, not #490. Call it out or split it. - Base is still
component-improvements/editor-inputand #489 is merged, so retarget tomainbefore merge. CODE_REVIEWandclaude-reviewboth failed at ~15m, which reads as a timeout rather than a finding. Worth confirming.
| refs.reference.current ) as HTMLElement | null | ||
| )?.focus(); | ||
| setIsOpen( false ); | ||
| setSearchKeyword( '' ); |
There was a problem hiding this comment.
Blocker: dropping the refocus breaks inlineSearch + multiple. Options get no tabIndex there and the branch has no FloatingFocusManager, so clicking one sends focus to document.body and type-to-filter, arrow nav and Backspace-removes-badge all stop working. Restore refs.domReference.current?.focus() behind if ( inlineSearch ), and assert typing still filters right after a pick.
There was a problem hiding this comment.
Restored behind inlineSearch guard, typing-after-pick asserted. 73564a8
| // Escape and outside click (useDismiss) still close it. | ||
| if ( valueIndex !== -1 ) { | ||
| return; | ||
| selectedValues.splice( valueIndex, 1 ); |
There was a problem hiding this comment.
setSelectedIndex is not updated on deselect, so useListNavigation reopens highlighting an option that is no longer selected.
There was a problem hiding this comment.
Cleared when deselected option was highlighted. 73564a8
| @@ -197,13 +206,6 @@ export const SelectButton = forwardRef<HTMLElement, SelectButtonProps>( | |||
| ) { | |||
| const childProps = { | |||
| value: selectedValue as SelectOptionValue, | |||
There was a problem hiding this comment.
Correct removal since the multiple branch returns earlier, but SelectFunctionChildren in select-types.ts still declares onClose. Drop it there too so the type matches.
There was a problem hiding this comment.
Dropped from SelectFunctionChildren. 73564a8
|
|
||
| return indx === selectedIndex; | ||
| }, [ multipleChecked, selectedIndex, selected ] ); | ||
| }, [ multipleChecked, selectedIndex, selected, multiple ] ); |
There was a problem hiding this comment.
Deps gained multiple but still omit indx, which shifts under combobox filtering. Worth adding while you are in here.
|
|
||
| // Click on the first option | ||
| // Select two options — the dropdown stays open in multiple mode | ||
| const allOptions = await screen.findAllByRole( 'option' ); |
There was a problem hiding this comment.
These node refs are reused across re-renders. Fine today, but re-querying after each click is less brittle.
There was a problem hiding this comment.
Re-queried by name after each click now. 73564a8
|
@jaieds - Can you check @ravindrakele's above comments once, approving for now. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
- restore trigger refocus behind inlineSearch so type-to-filter, arrow nav and Backspace-removes-badge survive a pick - clear selectedIndex when the deselected option was highlighted - drop onClose from SelectFunctionChildren type - add indx to isChecked deps - stories: re-query options after each click, badge key by `by` value, assert typing filters right after a pick in inlineSearch
ravindrakele
left a comment
ravindrakele
left a comment
There was a problem hiding this comment.
All five review points are addressed cleanly in 73564a86.
- The
inlineSearchrefocus is scoped correctly, so the button-trigger path keeps focus on the clicked option and inline search returns focus to the input. The newInlineSearchMultiassertion (typingoranwith no re-click) genuinely proves it rather than just asserting the listbox is still mounted. selectedIndexonly clears when the deselected index is the tracked one, so deselecting a non-last option leaves the highlight on something still selected.onClose, theisCheckeddeps and the reused option node refs are all done as suggested.
Full check run is green. LGTM.
Two non-blocking notes for later:
- Badge
keyfalls back toString( valueItem[ by ] ), so objects missing thebykey all key to"undefined". Same shape already breaks the dedup inhandleMultiSelect, so it is broken usage either way, just noting it. - This changes multi-select behavior for every existing consumer and there is no opt-out prop. Intended per #490, but worth a deliberate call on whether it ships as
1.8.1or a minor.
3 participants
Closes #490
What
multiplemode: dropdown stays open after an option is clicked or toggled via Enter/Space, so several options can be picked without reopening.useDismiss(unchanged); focus returns to the trigger throughFloatingFocusManager.inlineSearchmode focus returns to the trigger input after each pick, so type-to-filter, arrow navigation and Backspace-removes-badge keep working while the list stays open.Also in this PR (separate from #490)
multipleflip crash:toValuesArray()inutils.tsnormalizes a single value into an array wherever multi-select code paths assumed an array (renderSelected,handleOnCloseItem,handleMultiSelect,SelectItem), and single mode falls back to the first entry when the value is still an array. Covered by the newToggleMultipleAtRuntimestory. This can be split out if preferred.How
handleMultiSelectinselect.tsxno longer callssetIsOpen(false); the early-return for already-selected values is replaced with a splice (deselect), andselectedIndexis cleared when the deselected option was the highlighted one souseListNavigationdoes not reopen on a stale highlight. The trigger refocus is kept only forinlineSearch(options there have notabIndexand noFloatingFocusManager, so focus would drop todocument.body). Checkmarks/badges update live since option checked state derives fromgetValues(); badges are keyed by theirbyvalue so mid-list deselects reconcile correctly.Tests
MultiSelectandInlineSearchMultiplay tests: keep-open assertions after each pick (options re-queried after every click), toggle-deselect without close, typing filters immediately after a pick ininlineSearch, Escape closes.Notes
stagingafter #2600 - Support smart tags in Custom JSON-LD string values #489 merged.select-atom.stories.tsxhas pre-existing CRLF line endings in HEAD; left untouched to keep the diff clean.