fix(Dropdown): keep selected value inside input for searchable single select#3405
Open
rivka-ungar wants to merge 14 commits into
Open
fix(Dropdown): keep selected value inside input for searchable single select#3405rivka-ungar wants to merge 14 commits into
rivka-ungar wants to merge 14 commits into
Conversation
… select The searchable single-select combobox previously forced the input value to null after selection and rendered the selected label as a visual overlay on top of an empty input. Screen readers read the input, not the overlay, so a selected combobox announced as "blank" — failing WCAG 2.1 SC 4.1.2 (Name, Role, Value, Level A). Stop overriding Downshift's default so the selected item's label lives inside the input and is exposed to assistive technologies. Supporting changes keep the component's existing behavior intact: - Seed initialInputValue with the selected label so a defaultValue/value is visible on mount (previously handled by the overlay). - Filter the list only on real user typing (InputChange); ignore the label Downshift writes into the input on selection/blur. - Reset the filter when the menu closes so reopening shows the full list, matching the prior behavior and the documented combobox pattern. The overlay in SingleSelectTrigger now naturally renders only for non-searchable single select (where inputValue stays null), so that path is unaffected. Multi-select uses a separate hook and is untouched. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
Code Review by Qodo
1. Stale textInput summary
|
Contributor
PR Summary by QodoDropdown: keep selected label in input for searchable single-select (WCAG 4.1.2) WalkthroughsDescription• Keep searchable single-select value inside the input so screen readers announce the selection (WCAG 4.1.2). • Add multi-select accessibility modes: text summary in input or keyboard-navigable interactive chips. • Expand Storybook docs and update tests to match the new accessible behaviors. Diagramgraph TD
A["Dropdown (core)"] --> B["SingleSelectTrigger"] --> C("useDropdownCombobox")
A["Dropdown (core)"] --> D["MultiSelectTrigger"] --> E("useDropdownMultiCombobox") --> F["MultiSelectedValues"]
D["MultiSelectTrigger"] --> G["DropdownInput"]
A["Dropdown (core)"] --> H["Docs & tests"]
High-Level AssessmentThe following are alternative approaches to this PR: 1. Keep overlay UI but mirror value via aria-valuetext/hidden input
2. Multi-select: keep chips, add offscreen live summary for AT
3. Adopt a higher-level combobox implementation (e.g., Zag.js/Ark UI patterns)
Recommendation: Current approach is the best trade-off for single-select: rely on Downshift’s default of writing the selected label into the input, which directly satisfies WCAG 4.1.2 and matches common combobox guidance. For multi-select, offering explicit modes (textInput for value announcement; interactiveChips for keyboard-operable chip removal) is a pragmatic, opt-in path without breaking existing chip layouts. File ChangesEnhancement (7)
Bug fix (2)
Refactor (1)
Tests (1)
Documentation (4)
|
Contributor
|
📦 Bundle Size Analysis ✅ No bundle size changes detected. Unchanged Components
📊 Summary:
|
Comprehensive story page covering all searchable single-select variants: overview, sizes, states, default/controlled value, icons & avatars, groups, tooltips, clearable/max-height, and custom filter / empty message. The default-value story demonstrates the selected value living inside the input. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
|
Code review by qodo was updated up to the latest commit 6edf9ba |
Single-page accessibility reference covering the selected-value-in-input behavior, WCAG criteria, keyboard interaction, screen reader output, and the accessibility-relevant props (naming, state, feedback). Excludes layout props like size that do not affect accessibility. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
|
Code review by qodo was updated up to the latest commit 3417b82 |
Convert the plain .md (which Storybook does not pick up) into an .mdx docs page for the Searchable single select group, with the live examples embedded. Trim to accessibility essentials: core selected-value-in-input behavior, WCAG 4.1.2, and the accessibility-relevant props. Dropped the generic keyboard and screen-reader sections and the broader WCAG list. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
|
Code review by qodo was updated up to the latest commit 11b7f86 |
…single select The .selectedItem overlay was previously hidden for searchable mode only via the `!inputValue` guard, so it reappeared and coexisted with the input value whenever the input text was cleared while a selection remained. Gate the overlay on `!searchable` instead — for searchable single select the value lives inside the input, so the overlay must never render. Removes the now-dead `faded`/`hasSelected` classes and their styles. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
|
Code review by qodo was updated up to the latest commit 127c22d |
…-off examples Add a clear "What changed" before/after section to the searchable single select accessibility page, and stories that demonstrate the text-only collapsed selected value: preselected start elements (icons/avatars), end elements (trailing icon, suffix/hint), and a custom valueRenderer that is not applied to the searchable selected display. Remove the Do/Don't section. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
|
Code review by qodo was updated up to the latest commit d0ea70b |
MDX parses {curly braces} as JS expressions; "{selected label}" is not valid
JS and broke the Storybook/Chromatic preview build with an acorn parse error.
Replace it with plain text.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
Looking for bugs?Check back in a few minutes. An AI review agent is analyzing this pull request. |
textInput: selected items are shown as a comma-separated summary in the input (WCAG 4.1.2 — exposes value to assistive technologies on focus). interactiveChips: chips stay visible alongside the input; keyboard nav via getSelectedItemProps — ArrowLeft/Right moves between chips, Backspace/Delete removes the focused chip, ArrowLeft from an empty input navigates to the last chip or the +N overflow badge. Chips overflow uses the existing useItemsOverflow hook for the +N count badge. Both modes keep the menu open on selection and support toggle (re-click to deselect). Default chip mode behavior is unchanged. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Storybook MDX page and stories for the textInput and interactiveChips props explaining how each addresses the WCAG 4.1.2 gap in default multi-select (empty input announced as blank by screen readers). Covers: - Side-by-side comparison of all three modes - textInput: comma-separated value in the input, exposed on mount, controlled usage, trade-offs - interactiveChips: keyboard navigation table, overflow (+N badge), trade-offs - Decision guide for choosing between modes - Common a11y props table (label, clearAriaLabel, error, etc.) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
|
Code review by qodo was updated up to the latest commit 4fcb1dc |
Contributor
|
Code review by qodo was updated up to the latest commit 1769c1d |
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Comment on lines
+46
to
+57
| <div | ||
| className={styles.multiWrapper} | ||
| onKeyDown={e => { | ||
| if ( | ||
| e.key === "ArrowLeft" && | ||
| e.target instanceof HTMLInputElement && | ||
| !e.target.value && | ||
| overflowBadgeRef.current | ||
| ) { | ||
| overflowBadgeRef.current.focus(); | ||
| } | ||
| }} |
Contributor
There was a problem hiding this comment.
3. Chip focus fails without overflow 🐞 Bug ≡ Correctness
In interactiveChips mode, ArrowLeft from the input only focuses the overflow (+N) badge; when there is no overflow badge (all chips visible), ArrowLeft does nothing so keyboard users can’t reach chips via the documented navigation. This contradicts the interactiveChips prop contract that ArrowLeft/Backspace from the input moves focus to the last chip.
Agent Prompt
### Issue description
`interactiveChips` documents that ArrowLeft/Backspace from the input moves focus to the last chip, but the current implementation only moves focus to the overflow badge when it exists. When `hiddenCount===0` (no overflow), there is no focus target and the keyboard flow breaks.
### Issue Context
- The ArrowLeft handler in `MultiSelectTrigger` only focuses `overflowBadgeRef`.
- The ArrowLeft-to-chip behavior in `MultiSelectedValues` is implemented only on the overflow wrapper, which only exists when `hiddenCount > 0`.
### Fix Focus Areas
- packages/core/src/components/Dropdown/components/Trigger/MultiSelectTrigger.tsx[41-71]
- packages/core/src/components/Dropdown/components/MultiSelectedValues/MultiSelectedValues.tsx[129-173]
- packages/core/src/components/Dropdown/Dropdown.types.ts[29-34]
### Implementation guidance
- Add a ref to the last *visible* chip container and focus it when ArrowLeft is pressed from an empty input and there is no overflow badge.
- Example approach:
- In `MultiSelectTrigger`, create `const lastChipRef = useRef<HTMLDivElement>(null)`.
- Pass it via `getChipContainerProps` for the last selected chip (only when you know there is no overflow) and ensure the chip container is focusable.
- Update the ArrowLeft handler:
- If `overflowBadgeRef.current` exists, focus it (current behavior).
- Else `lastChipRef.current?.focus()`.
- Consider also handling `Backspace` similarly if that’s part of the intended keyboard contract for `interactiveChips`.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+23
to
+26
| id?: string, | ||
| onOptionRemove?: (option: T) => void, | ||
| textInput?: boolean, | ||
| interactiveChips?: boolean |
Contributor
There was a problem hiding this comment.
1. Unused hook parameter 🐞 Bug ⚙ Maintainability
useDropdownMultiCombobox adds an interactiveChips parameter but never uses it, which violates the core package’s TypeScript unused-vars ESLint rule and can fail CI/linting. This also indicates the hook signature doesn’t match its implemented behavior.
Agent Prompt
### Issue description
`useDropdownMultiCombobox` declares `interactiveChips?: boolean` but never references it. In `packages/core`, `@typescript-eslint/no-unused-vars` is configured as an error for TS/TSX files, so this can break lint/CI.
### Issue Context
`interactiveChips` is already consumed in UI (Trigger) and context; it does not appear to be needed inside this hook. The cleanest fix is to remove the argument from the hook signature and from the call site(s). If you want to keep it for future work, rename it to `_interactiveChips` to satisfy the lint rule.
### Fix Focus Areas
- packages/core/src/components/Dropdown/hooks/useDropdownMultiCombobox.ts[7-36]
- packages/core/src/components/Dropdown/modes/DropdownMultiComboboxController.tsx[47-84]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
…ipleSelection onStateChange UseMultipleSelectionStateChange has no selectedItem property — diff old vs new selectedItems array instead. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
Code Review by QodoNew Review StartedThis review has been superseded by a new analysis |
Contributor
Code Review by QodoNew Review StartedThis review has been superseded by a new analysis |
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
|
Code review by qodo was updated up to the latest commit f279286 |
Contributor
|
A new prerelease version of this PR has been published! 🎉 |
Comment on lines
+87
to
88
| initialInputValue: inputValueProp ?? (textInput ? currentSelectedItems.map(i => i.label).join(", ") : ""), | ||
| id, |
Contributor
There was a problem hiding this comment.
2. Stale textinput summary 🐞 Bug ≡ Correctness
In multi-select textInput mode, the input’s displayed summary is only seeded via initialInputValue and updated on internal Downshift events; when value is controlled and the parent changes value externally, there is no synchronization path to update Downshift’s inputValue, so the input can display a stale summary that no longer matches the actual selection.
Agent Prompt
### Issue description
For multi-select `textInput`, the input shows a comma-separated summary derived from selected items. But `useCombobox` is only initialized with that summary via `initialInputValue`, and there is no effect or controlled `inputValue` prop to keep it in sync when `value` (controlled selected items) changes outside of Downshift interactions.
### Issue Context
This causes UI divergence in common flows like form reset, server-driven updates, or parent state changes: `selectedItems` in context updates (because it uses `value`), but the input continues to show the old summary.
### Fix Focus Areas
- packages/core/src/components/Dropdown/hooks/useDropdownMultiCombobox.ts[28-118]
- packages/core/src/components/Dropdown/hooks/useDropdownMultiCombobox.ts[140-182]
### Suggested fix
Implement synchronization in `textInput` mode, for example:
- Maintain a dedicated `displayInputValue` state in the hook and pass it to `useCombobox` as `inputValue` (controlled), updating it on:
- user typing (InputChange)
- selection changes (when `currentSelectedItems` changes)
- close/blur (restore summary)
- Or, if available in your Downshift hook return, call `setInputValue(summary)` in a `useEffect` when `textInput` is true and `currentSelectedItems` changes, guarded so it doesn’t overwrite active user typing (e.g., only when menu is closed / input not focused).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
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.
Problem
The searchable single-select combobox forced the input value to
nullafter every selection and rendered the selected label as a visual overlay on top of an empty input.Screen readers read what's inside the input — not the overlay. So a combobox with an active selection announced as "Edit combo, collapsed, blank" (JAWS). The user had no way to know what they'd selected.
This fails WCAG 2.1 SC 4.1.2 — Name, Role, Value (Level A), which requires the current value of a form control to be programmatically determinable.
Fix
Stop overriding Downshift's default so the selected item's label lives inside the input, where assistive technologies can read it. The selected value is no longer a separate visual layer.
Supporting changes keep the component's existing behavior intact:
initialInputValueseeded with the selected label, so adefaultValue/controlledvalueis visible on mount (previously the overlay handled this).onInputValueChangefilters the list only on real user typing (InputChange), ignoring the label Downshift writes into the input on selection/blur.onIsOpenChangeresets the filter when the menu closes, so reopening shows the full option list — matching the component's prior behavior and the documented combobox pattern (same approach Chakra v3 / Ark UI recommend: reset the filter on open).The overlay in
SingleSelectTriggernow renders only for non-searchable single select (whereinputValuestaysnull), so that path is unaffected. Multi-select uses a separate hook and is untouched.Behavior parity
Aligned with Chakra v3 (Ark UI / Zag.js):
aria-selected+ activedescendant on selected option at openreset())Storybook
Adds a dedicated Components/Dropdown/Searchable single select page documenting all searchable single-select variants: overview, sizes, states, default/controlled value, icons & avatars, groups (sticky titles & dividers), tooltips, clearable / max-height, and custom filter / empty message. The default-value story demonstrates the selected value living inside the input.
Tests
searchable: false, since the indent-stripping overlay logic now applies only to non-searchable single select.Test plan
defaultValue/ controlledvalueshows the label in the input on mount🤖 Generated with Claude Code