Repository navigation
fix(multi-select): Correct selected pill accessibility - #4192
minwookshin wants to merge 2 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Accessible names and selected-pill semantics modules/preview-react/multi-select/lib/MultiSelectInput.tsx, modules/preview-react/multi-select/lib/MultiSelectedList.tsx, modules/preview-react/multi-select/lib/MultiSelectedItem.tsx, modules/preview-react/multi-select/spec/MultiSelectInput.spec.tsx, cypress/component/MultiSelect.spec.tsx |
Both input variants pass accessible names to their inputs and selected-pill groups. The selected-pill container uses grid and row semantics, and pills use gridcell semantics without aria-selected. Tests check accessible names, attributes, and accessibility scans. |
Pill removal and focus behavior modules/preview-react/multi-select/lib/useMultiSelectModel.ts, modules/preview-react/multi-select/lib/MultiSelectInput.tsx, modules/preview-react/multi-select/lib/useMultiSelectItemRemove.ts, cypress/component/MultiSelect.spec.tsx |
The model exposes a search-input ref and derives the selected-pill items for the list model. Pill focus updates the selected cursor. Keyboard and click removal focus the search input when no next item remains. Cypress tests cover keyboard removal and form submission behavior. |
Search clear-button accessibility modules/react/text-input/lib/InputGroup.tsx, cypress/component/MultiSelect.spec.tsx |
The clear button uses aria-hidden instead of role="presentation". Cypress tests check its accessibility and tab-order behavior, and verify that clearing search leaves focus on the combobox. |
Priority:
Estimated code review effort: 3 (Moderate) | ~20 minutes
Change: Bug fix · Severity of issue fixed: Medium
Possibly related issues
- Workday/canvas-kit#4212: The changed selected-pill grid and gridcell structure relates to the issue’s VoiceOver position-count reports.
Possibly related PRs
- Workday/canvas-kit#4215: This PR documents MultiSelect and Pill accessibility behavior, which overlaps with the selected-pill semantics and keyboard focus changes here.
Suggested reviewers: mannycarrera4, williamjstanton
Merge Risk: 
2b056
-
The inspected accessibility and removal flows show no actionable issue; the change is ready to merge after normal checks.
Security architecture risk: fd5b1
-
The reviewed changes adjust accessibility and focus behavior without expanding privileges or data access. Submitted selections remain separate from search text, and removal buttons explicitly avoid submitting the surrounding form.
Retained concerns
No architecture-level concerns identified.Security review details
-
Security Blast Radius
- inferred — The reviewed exposure is browser-side accessibility, collection interaction, and form behavior. Shared clear-button consumers inherit the accessibility change, but the inspected modifications do not add a privileged sink or expand authority beyond existing component interactions.
Trust Boundaries and Controls
- observed — Caller-supplied accessible names are forwarded as React attributes rather than selection values. The default selected-item container is a span, and its inner removal control now explicitly uses type="button", preventing implicit surrounding-form submission through that control.
-
Pre-merge checks: 
-
Check name Status Explanation Title check The title clearly and concisely describes the main change: correcting accessibility behavior for selected pills in the multi-select component. Linked Issues check Issue [ #3919] requires no multiselect-related axe issues before or after selection. The PR hides the redundant clear button from assistive technology, replaces the selected-pill listbox with a named g…Out of Scope Changes check The focus cursor updates, search-input ref, focus return, default-action prevention, and form-submission tests support the selected-pill removal flow. The InputGroup clear-button change directly addre… Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Full details: Linked Issues check
-
Explanation
Issue [
#3919] requires no multiselect-related axe issues before or after selection. The PR hides the redundant clear button from assistive technology, replaces the selected-pill listbox with a named grid containing a row and gridcells, and removes the unsupported aria-selected state from removal buttons. The removal buttons use type="button". Cypress and unit tests cover axe checks, accessible names, roles, keyboard removal, focus behavior, and form submission.
Full details: Out of Scope Changes check
-
Explanation
The focus cursor updates, search-input ref, focus return, default-action prevention, and form-submission tests support the selected-pill removal flow. The InputGroup clear-button change directly addresses the clear-button findings in [
#3919]. No unrelated changes are identified.
-
Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
minwookshin
changed the title
Workday/canvas-kit
 |
||||||||||||||||||||||||||||||||||||||||
| Project |
Workday/canvas-kit
|
| Branch Review |
fix/multiselect-selected-accessibility
|
| Run status |
|
| Run duration | 02m 37s |
| Commit |
|
| Committer | Minwook Shin |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
 |
0
|
 |
0
|
 |
17
|
 |
0
|
 |
873
|
| View all changes introduced in this branch ↗︎ | |
UI Coverage
14.57%
|
|
|---|---|
|
|
2501
|
|
|
424
|
Accessibility
99.48%
|
|
|---|---|
|
|
3 critical
5 serious
3 moderate
1 minor
|
|
|
69
|
|
Nice work on grouping and labeling the pills. Since this set of selected pills depends on using the arrow keys to move across them, I think we need to investigate an alternative to |
|
Changed the pills to a single-row layout grid, with gridcells around the native removal buttons. The roving tab stop, arrows, Home/End, and deletion focus behavior are covered by tests; all 9 Chrome Cypress cases and 113 unit tests pass. I couldn't verify JAWS/NVDA on this Mac. Could you check arrow-key passthrough on Windows with the updated preview? The current CI run also needs maintainer approval. Could a maintainer approve it and add |
I'm glad that you experimented with this, because it was one of the options I was considering. Functionally, this is going to work but, and this is opinionated, I find it really annoying that screen readers will describe the selected pills as a "table with 1 row and {X} columns" every time. That's not valuable information for this selected pills pattern. I found a way to make the ARIA <Pill as={Element} disabled={disabled} variant="removable">
<Pill.Label aria-hidden={true}>{children}</Pill.Label>
<Pill.IconButton role="option" aria-label={removeLabel} ref={ref} {...(elemProps as any)} />
</Pill>In conclusion, I think changing the |
|
Switched the selected pills to a named toolbar with native removal buttons and horizontal roving focus in |
Oh thank you, I think I'm really happy with this toolbar idea. I spoke to @mannycarrera4 earlier today, and bug fixes like this need to be merged onto the Thanks for working on this and I apologize for any inconvenience! |
6f7a880 to
77bdbed
Compare
|
Moved the fix onto |
williamjstanton
left a comment
williamjstanton
left a comment
There was a problem hiding this comment.
I think it looks good from a11y.
|
Requesting an additional code review. |
Summary
Fixes #3919.
Fixes #4212.
Based on
support(v15), as requested.Expose selected pills as a named toolbar of native removal buttons, and hide the redundant, untabbable clear button from assistive technology.
The removal flow also keeps the focus cursor in sync, returns focus to the search input after the last removal, and avoids submitting the surrounding form. Search text remains independent of the submitted selection value.
Release Category
Components
Checklist
ready for reviewhas been added to PRFor the Reviewer
Where Should the Reviewer Start?
MultiSelectedList.tsx,MultiSelectedItem.tsx, anduseMultiSelectItemRemove.ts. Both input variants pass their accessible name to the selected-item toolbar. Horizontal roving focus provides one tab stop, Left/Right movement with wrapping, and Home/End navigation. Focus remains on the native removal buttons. The search input needs a separate focus ref because its text is not the constrained form value.Testing Manually
In the Searching story, focus the input, press ArrowDown to open the popup, then type
apple, select two results, and press Escape. Tab to a removal button, move with Left/Right or Home/End, then remove items with Backspace/Delete, Enter, or Space. Removing the last item returns focus to the input; clicking remove does not submit the form. Clearing search retains input focus.Verification on
support(October 27): all 9 Chrome Cypress cases pass, covering axe, the single tab stop, wrapping Left/Right and Home/End navigation, Enter/Space activation, deletion/focus return, search clearing, and form submission behavior. All 112 focused unit tests, all typecheck targets, full lint, and changed-file ESLint/formatting checks pass. The support tests use the existing ArrowDown popup-opening behavior.JAWS/NVDA behavior on Windows and VoiceOver speech output have not been verified. Please confirm automatic arrow-key passthrough and announcements with the toolbar before merging. Earlier cross-browser checks of the removal flow predate this role change.
Summary by CodeRabbit