From 77bdbedcdf02411c0cae20f3fa850ebc01cef800 Mon Sep 17 00:00:00 2001 From: Minwook Shin <163576506+minwookshin@users.noreply.github.com> Date: Tue, 27 Oct 2026 10:49:18 -0400 Subject: [PATCH] fix(multi-select): use a labeled toolbar for selected items --- cypress/component/MultiSelect.spec.tsx | 124 ++++++++++++++++++ .../multi-select/lib/MultiSelectInput.tsx | 24 +++- .../multi-select/lib/MultiSelectedItem.tsx | 8 +- .../multi-select/lib/MultiSelectedList.tsx | 10 +- .../lib/useMultiSelectItemRemove.ts | 8 +- .../multi-select/lib/useMultiSelectModel.ts | 8 +- .../spec/MultiSelectInput.spec.tsx | 30 +++++ modules/react/text-input/lib/InputGroup.tsx | 2 +- 8 files changed, 195 insertions(+), 19 deletions(-) create mode 100644 cypress/component/MultiSelect.spec.tsx create mode 100644 modules/preview-react/multi-select/spec/MultiSelectInput.spec.tsx diff --git a/cypress/component/MultiSelect.spec.tsx b/cypress/component/MultiSelect.spec.tsx new file mode 100644 index 0000000000..0373e94b24 --- /dev/null +++ b/cypress/component/MultiSelect.spec.tsx @@ -0,0 +1,124 @@ +import {Searching} from '../../modules/preview-react/multi-select/stories/examples/Searching'; + +describe('MultiSelect', () => { + context('given the Searching story is rendered', () => { + beforeEach(() => { + cy.mount(); + }); + + // Cypress adds its own main landmark; scan the controls without the story's nested main. + it('should not have any axe errors before interaction', () => { + cy.checkA11y({include: ['form > main > *']}); + }); + + context('when options have been selected', () => { + beforeEach(() => { + cy.findByRole('combobox', {name: 'Fruits'}).focus().realPress('ArrowDown'); + cy.findByRole('option', {name: 'Red Apple 1'}).should('be.visible'); + cy.findByRole('combobox', {name: 'Fruits'}).realType('apple'); + cy.findByRole('option', {name: 'Red Apple 1'}).click(); + cy.findByRole('option', {name: 'Blue Apple 7'}).click(); + cy.findByRole('combobox', {name: 'Fruits'}).focus().realPress('Escape'); + }); + + it('should expose removal buttons in a named toolbar without axe errors', () => { + cy.findByRole('toolbar', {name: 'Fruits'}).within(() => { + cy.findAllByRole('button').should('have.length', 2); + cy.findByRole('button', {name: 'Remove Red Apple 1'}).should($button => { + expect($button).not.to.have.attr('aria-selected'); + expect($button).to.have.attr('type', 'button'); + }); + cy.findByRole('button', {name: 'Remove Blue Apple 7'}).should('be.visible'); + }); + cy.checkA11y({include: ['form > main > *']}); + }); + + it('should navigate and remove pills with the keyboard, then return focus to the input', () => { + cy.findByRole('button', {name: 'Remove Red Apple 1'}).focus().realPress('ArrowRight'); + cy.findByRole('button', {name: 'Remove Blue Apple 7'}) + .should('be.focused') + .realPress('Backspace'); + cy.findByRole('button', {name: 'Remove Blue Apple 7'}).should('not.exist'); + cy.findByRole('button', {name: 'Remove Red Apple 1'}) + .should('be.focused') + .realPress('Delete'); + cy.findByRole('toolbar', {name: 'Fruits'}).should('not.exist'); + cy.findByRole('combobox', {name: 'Fruits'}).should('be.focused'); + }); + + it('should use one tab stop and navigate the toolbar with Home, End, and arrow keys', () => { + cy.findByRole('combobox', {name: 'Fruits'}).focus().realPress('Tab'); + cy.findByRole('button', {name: 'Remove Red Apple 1'}).should('be.focused'); + cy.findByRole('toolbar', {name: 'Fruits'}) + .find('button[tabindex="0"]') + .should('have.length', 1); + cy.realPress('End'); + cy.findByRole('button', {name: 'Remove Blue Apple 7'}).should('be.focused'); + cy.realPress('ArrowRight'); + cy.findByRole('button', {name: 'Remove Red Apple 1'}).should('be.focused'); + cy.realPress('ArrowLeft'); + cy.findByRole('button', {name: 'Remove Blue Apple 7'}).should('be.focused'); + cy.realPress('ArrowLeft'); + cy.findByRole('button', {name: 'Remove Red Apple 1'}).should('be.focused'); + cy.realPress('ArrowRight'); + cy.findByRole('button', {name: 'Remove Blue Apple 7'}).should('be.focused'); + cy.realPress('Home'); + cy.findByRole('button', {name: 'Remove Red Apple 1'}).should('be.focused'); + cy.realPress('ArrowUp'); + cy.realPress('ArrowDown'); + cy.findByRole('button', {name: 'Remove Red Apple 1'}).should('be.focused'); + cy.realPress(['Shift', 'Tab']); + cy.findByRole('combobox', {name: 'Fruits'}).should('be.focused'); + cy.checkA11y({include: ['form > main > *']}); + }); + + it('should remove a pill without submitting the form', () => { + const onSubmit = cy.stub().as('submit'); + cy.get('form').then($form => $form[0].addEventListener('submit', onSubmit)); + cy.findByRole('button', {name: 'Remove Red Apple 1'}).click(); + cy.findByRole('button', {name: 'Remove Red Apple 1'}).should('not.exist'); + cy.get('@submit').should('not.have.been.called'); + }); + + it('should activate removal buttons with Enter and Space without submitting the form', () => { + const onSubmit = cy.stub().as('submit'); + cy.get('form').then($form => $form[0].addEventListener('submit', onSubmit)); + cy.findByRole('button', {name: 'Remove Red Apple 1'}).focus().realPress('Enter'); + cy.findByRole('button', {name: 'Remove Red Apple 1'}).should('not.exist'); + cy.findByRole('button', {name: 'Remove Blue Apple 7'}) + .should('be.focused') + .realPress('Space'); + cy.findByRole('toolbar', {name: 'Fruits'}).should('not.exist'); + cy.findByRole('combobox', {name: 'Fruits'}).should('be.focused'); + cy.get('@submit').should('not.have.been.called'); + cy.checkA11y({include: ['form > main > *']}); + }); + }); + + context('when search text is entered using the keyboard', () => { + beforeEach(() => { + cy.findByRole('combobox', {name: 'Fruits'}).focus().realPress('ArrowDown'); + cy.findByRole('option', {name: 'Red Apple 1'}).should('be.visible'); + cy.findByRole('combobox', {name: 'Fruits'}).realType('apple'); + }); + + it('should not have any axe errors', () => { + cy.findByRole('option', {name: 'Pink Apple 25'}).should('be.visible'); + cy.checkA11y({include: ['form > main > *', '[role="listbox"]']}); + }); + + it('should open the listbox with the filtered result', () => { + cy.findByRole('combobox', {name: 'Fruits'}).should('have.attr', 'aria-expanded', 'true'); + cy.findByRole('option', {name: 'Pink Apple 25'}).should('be.visible'); + }); + + it('should clear the search without moving focus or exposing a redundant button', () => { + cy.get('[data-part="input-group-clear-button"]') + .should('have.attr', 'aria-hidden', 'true') + .and('have.attr', 'tabindex', '-1') + .click(); + cy.findByRole('combobox', {name: 'Fruits'}).should('have.value', '').and('be.focused'); + }); + }); + }); +}); diff --git a/modules/preview-react/multi-select/lib/MultiSelectInput.tsx b/modules/preview-react/multi-select/lib/MultiSelectInput.tsx index d6b974cb0a..b90e5a613f 100644 --- a/modules/preview-react/multi-select/lib/MultiSelectInput.tsx +++ b/modules/preview-react/multi-select/lib/MultiSelectInput.tsx @@ -187,7 +187,7 @@ export interface MultiSelectInputProps extends CSProps, Pick< React.InputHTMLAttributes, - 'disabled' | 'className' | 'style' | 'aria-labelledby' + 'disabled' | 'className' | 'style' | 'aria-label' | 'aria-labelledby' >, Pick { error?: ErrorType; @@ -203,6 +203,7 @@ export const MultiSelectInput = createSubcomponent(TextInput)({ cs, style, error, + 'aria-label': ariaLabel, 'aria-labelledby': ariaLabelledBy, removeLabel, disabled, @@ -218,6 +219,7 @@ export const MultiSelectInput = createSubcomponent(TextInput)({ - + ); } @@ -243,6 +250,7 @@ export const MultiSelectSearchInput = createSubcomponent(TextInput)({ className, cs, style, + 'aria-label': ariaLabel, 'aria-labelledby': ariaLabelledBy, removeLabel, formInputProps, @@ -251,7 +259,8 @@ export const MultiSelectSearchInput = createSubcomponent(TextInput)({ error, ...elemProps }, - Element + Element, + model ) => { return (
@@ -267,9 +276,11 @@ export const MultiSelectSearchInput = createSubcomponent(TextInput)({ @@ -279,7 +290,12 @@ export const MultiSelectSearchInput = createSubcomponent(TextInput)({ - +
); } diff --git a/modules/preview-react/multi-select/lib/MultiSelectedItem.tsx b/modules/preview-react/multi-select/lib/MultiSelectedItem.tsx index 20c1019fb3..7fdcc684c3 100644 --- a/modules/preview-react/multi-select/lib/MultiSelectedItem.tsx +++ b/modules/preview-react/multi-select/lib/MultiSelectedItem.tsx @@ -1,7 +1,6 @@ import {useListItemRegister, useListItemRovingFocus} from '@workday/canvas-kit-react/collection'; import { composeHooks, - createElemPropsHook, createSubModelElemPropsHook, createSubcomponent, } from '@workday/canvas-kit-react/common'; @@ -25,11 +24,6 @@ export interface MultiSelectedItemProps { } export const useMultiSelectedItem = composeHooks( - createElemPropsHook(useMultiSelectModel)(model => { - return { - 'aria-selected': true, - }; - }), useMultiSelectItemRemove, createSubModelElemPropsHook(useMultiSelectModel)(m => m.selected, useListItemRovingFocus), createSubModelElemPropsHook(useMultiSelectModel)(m => m.selected, useListItemRegister) @@ -42,7 +36,7 @@ export const MultiSelectedItem = createSubcomponent('span')({ return ( {children} - + ); }); diff --git a/modules/preview-react/multi-select/lib/MultiSelectedList.tsx b/modules/preview-react/multi-select/lib/MultiSelectedList.tsx index b9d9c7fae9..27b02893a8 100644 --- a/modules/preview-react/multi-select/lib/MultiSelectedList.tsx +++ b/modules/preview-react/multi-select/lib/MultiSelectedList.tsx @@ -19,15 +19,19 @@ export interface MultiSelectedListProps export const MultiSelectedList = createSubcomponent('div')({ modelHook: useMultiSelectModel, })( - ({'aria-labelledby': ariaLabelledBy, disabled, removeLabel}, Element, model) => { + ( + {'aria-label': ariaLabel, 'aria-labelledby': ariaLabelledBy, disabled, removeLabel}, + Element, + model + ) => { return model.selected.state.items.length ? ( <>
{item => ( diff --git a/modules/preview-react/multi-select/lib/useMultiSelectItemRemove.ts b/modules/preview-react/multi-select/lib/useMultiSelectItemRemove.ts index af54003119..fdb42aecad 100644 --- a/modules/preview-react/multi-select/lib/useMultiSelectItemRemove.ts +++ b/modules/preview-react/multi-select/lib/useMultiSelectItemRemove.ts @@ -23,15 +23,19 @@ import {useMultiSelectModel} from './useMultiSelectModel'; */ export const useMultiSelectItemRemove = createElemPropsHook(useMultiSelectModel)((model, _ref) => { return { + onFocus(event: React.FocusEvent) { + model.selected.events.goTo({id: event.currentTarget.dataset.id || ''}); + }, onKeyDown(event: React.KeyboardEvent) { if (event.key === 'Backspace' || event.key === 'Delete') { + event.preventDefault(); const id = event.currentTarget.dataset.id || ''; const nextId = listItemRemove(id, model.selected); model.selected.events.remove({id, event}); if (nextId) { focusOnCurrentCursor(model.selected, nextId, event.currentTarget); } else { - model.state.inputRef.current?.focus(); + (model.state.searchInputRef.current || model.state.inputRef.current)?.focus(); } } }, @@ -42,7 +46,7 @@ export const useMultiSelectItemRemove = createElemPropsHook(useMultiSelectModel) if (nextId) { focusOnCurrentCursor(model.selected, nextId, event.currentTarget); } else { - model.state.inputRef.current?.focus(); + (model.state.searchInputRef.current || model.state.inputRef.current)?.focus(); } }, }; diff --git a/modules/preview-react/multi-select/lib/useMultiSelectModel.ts b/modules/preview-react/multi-select/lib/useMultiSelectModel.ts index fda04f0dd5..387032b6bd 100644 --- a/modules/preview-react/multi-select/lib/useMultiSelectModel.ts +++ b/modules/preview-react/multi-select/lib/useMultiSelectModel.ts @@ -30,6 +30,8 @@ export const useMultiSelectModel = createModelHook({ contextOverride: useComboboxModel.Context, })(config => { const cachedSelectedRef = React.useRef[]>([]); + // Search text is independent of the constrained form value, but still needs a focus target. + const searchInputRef = React.useRef(null); const model = useComboboxModel( useComboboxModel.mergeConfig(config, { onHide() { @@ -61,7 +63,8 @@ export const useMultiSelectModel = createModelHook({ ); cachedSelectedRef.current = cachedSelected; - // The `listbox` of pills under the MultiSelect combobox input. + const visibleSelectedItems = model.state.visibility === 'hidden' ? cachedSelected : selectedItems; + const selected = useListModel({ orientation: 'horizontal', onRemove({id}) { @@ -69,7 +72,7 @@ export const useMultiSelectModel = createModelHook({ setSelectedItems(cachedSelected.filter(item => item.id !== id)); }, shouldVirtualize: false, - items: model.state.visibility === 'hidden' ? cachedSelected : selectedItems, + items: visibleSelectedItems, }); return { @@ -87,5 +90,6 @@ export const useMultiSelectModel = createModelHook({ }, }, ...model, + state: {...model.state, searchInputRef}, }; }); diff --git a/modules/preview-react/multi-select/spec/MultiSelectInput.spec.tsx b/modules/preview-react/multi-select/spec/MultiSelectInput.spec.tsx new file mode 100644 index 0000000000..368aa3224f --- /dev/null +++ b/modules/preview-react/multi-select/spec/MultiSelectInput.spec.tsx @@ -0,0 +1,30 @@ +import {render, screen, within} from '@testing-library/react'; + +import {MultiSelect} from '../lib/MultiSelect'; + +describe.each([ + ['Input', MultiSelect.Input], + ['SearchInput', MultiSelect.SearchInput], +] as const)('MultiSelect.%s selected items', (_, Input) => { + it.each(['aria-label', 'aria-labelledby'] as const)( + 'should use %s to label the removal toolbar', + labelProp => { + render( + + Fruits + + + ); + + expect(screen.getByRole('combobox', {name: 'Fruits'})).toBeInTheDocument(); + const toolbar = screen.getByRole('toolbar', {name: 'Fruits'}); + const button = within(toolbar).getByRole('button', {name: 'Remove Apple'}); + expect(button).not.toHaveAttribute('aria-selected'); + expect(button).toHaveAttribute('type', 'button'); + expect(screen.queryByRole('grid')).not.toBeInTheDocument(); + } + ); +}); diff --git a/modules/react/text-input/lib/InputGroup.tsx b/modules/react/text-input/lib/InputGroup.tsx index 2555d7c3c9..6775c12697 100644 --- a/modules/react/text-input/lib/InputGroup.tsx +++ b/modules/react/text-input/lib/InputGroup.tsx @@ -158,7 +158,7 @@ export const useClearButton = createElemPropsHook(useInputGroupModel)(model => { return { // This element does not need to be accessible via screen reader. The user can already clear // an input - role: 'presentation', + 'aria-hidden': true, // A clear input button doesn't need focus. There's already keyboard keys to clear an input tabIndex: -1, icon: xSmallIcon as CanvasSystemIcon,