From ccedc92343d198d921b9d0f80cf23b52ae107971 Mon Sep 17 00:00:00 2001 From: Jon Schectman Date: Wed, 18 Dec 2019 13:48:08 -0800 Subject: [PATCH] Combobox: Fixes multiselect selected values not being read (#11436) * fix combobox * fix aria-label * add some more tests, minor refactoring * Change files * minor comment tweak * add missing letter in string' * update api * update snapshots * fix comments * minor comment fixes * update snapshots * Add comment * Update office-ui-fabric-react-2019-12-11-14-23-30-combobox-access.json * fix bad variable name --- ...t-2019-12-11-14-23-30-combobox-access.json | 8 ++ .../etc/office-ui-fabric-react.api.md | 1 + .../ComboBox/ComboBox.classNames.ts | 4 +- .../components/ComboBox/ComboBox.styles.ts | 4 +- .../src/components/ComboBox/ComboBox.test.tsx | 86 ++++++++++++++- .../src/components/ComboBox/ComboBox.tsx | 101 ++++++++++-------- .../src/components/ComboBox/ComboBox.types.ts | 5 + .../ComboBox.Basic.Example.tsx.shot | 21 ++++ .../ComboBox.Controlled.Example.tsx.shot | 1 + 9 files changed, 178 insertions(+), 53 deletions(-) create mode 100644 change/office-ui-fabric-react-2019-12-11-14-23-30-combobox-access.json diff --git a/change/office-ui-fabric-react-2019-12-11-14-23-30-combobox-access.json b/change/office-ui-fabric-react-2019-12-11-14-23-30-combobox-access.json new file mode 100644 index 00000000000000..1dba8b0fde5f1a --- /dev/null +++ b/change/office-ui-fabric-react-2019-12-11-14-23-30-combobox-access.json @@ -0,0 +1,8 @@ +{ + "type": "minor", + "comment": "Combobox: Fix multiselect options not being read by screen readers.", + "packageName": "office-ui-fabric-react", + "email": "joschect@microsoft.com", + "commit": "1c107f4a4a4650bd46798a3e058b1f5a8e771610", + "date": "2019-12-11T22:23:30.882Z" +} diff --git a/packages/office-ui-fabric-react/etc/office-ui-fabric-react.api.md b/packages/office-ui-fabric-react/etc/office-ui-fabric-react.api.md index ab439010087e14..a12664161f680c 100644 --- a/packages/office-ui-fabric-react/etc/office-ui-fabric-react.api.md +++ b/packages/office-ui-fabric-react/etc/office-ui-fabric-react.api.md @@ -2826,6 +2826,7 @@ export interface IComboBoxStyles { rootFocused: IStyle; rootHovered: IStyle; rootPressed: IStyle; + screenReaderText: IStyle; } // @public (undocumented) diff --git a/packages/office-ui-fabric-react/src/components/ComboBox/ComboBox.classNames.ts b/packages/office-ui-fabric-react/src/components/ComboBox/ComboBox.classNames.ts index 74362c8d0a5f66..f33ab52fbcf7d6 100644 --- a/packages/office-ui-fabric-react/src/components/ComboBox/ComboBox.classNames.ts +++ b/packages/office-ui-fabric-react/src/components/ComboBox/ComboBox.classNames.ts @@ -13,6 +13,7 @@ export interface IComboBoxClassNames { header: string; divider: string; optionsContainerWrapper: string; + screenReaderText: string; } export interface IComboBoxOptionClassNames { @@ -57,7 +58,8 @@ export const getClassNames = memoizeFunction( optionsContainerWrapper: mergeStyles('ms-ComboBox-optionsContainerWrapper', styles.optionsContainerWrapper), optionsContainer: mergeStyles('ms-ComboBox-optionsContainer', styles.optionsContainer), header: mergeStyles('ms-ComboBox-header', styles.header), - divider: mergeStyles('ms-ComboBox-divider', styles.divider) + divider: mergeStyles('ms-ComboBox-divider', styles.divider), + screenReaderText: mergeStyles(styles.screenReaderText) }; } ); diff --git a/packages/office-ui-fabric-react/src/components/ComboBox/ComboBox.styles.ts b/packages/office-ui-fabric-react/src/components/ComboBox/ComboBox.styles.ts index 386c146e6c8677..b60e7dae87666d 100644 --- a/packages/office-ui-fabric-react/src/components/ComboBox/ComboBox.styles.ts +++ b/packages/office-ui-fabric-react/src/components/ComboBox/ComboBox.styles.ts @@ -6,7 +6,8 @@ import { getFocusStyle, HighContrastSelector, IStyle, - getPlaceholderStyles + getPlaceholderStyles, + hiddenContentStyle } from '../../Styling'; import { IComboBoxOptionStyles, IComboBoxStyles } from './ComboBox.types'; @@ -465,6 +466,7 @@ export const getStyles = memoizeFunction( optionsContainer: { display: 'block' }, + screenReaderText: hiddenContentStyle, header: [ fonts.medium, diff --git a/packages/office-ui-fabric-react/src/components/ComboBox/ComboBox.test.tsx b/packages/office-ui-fabric-react/src/components/ComboBox/ComboBox.test.tsx index fe87bdbd3cd131..9d628d9e5dd3e4 100644 --- a/packages/office-ui-fabric-react/src/components/ComboBox/ComboBox.test.tsx +++ b/packages/office-ui-fabric-react/src/components/ComboBox/ComboBox.test.tsx @@ -129,9 +129,7 @@ describe('ComboBox', () => { wrapper.setProps({ selectedKey: null }); - // \u200B is a zero width space. - // See https://github.com/OfficeDev/office-ui-fabric-react/blob/d4e9b6d28b25a3e123b2d47c0a03f18113fbee60/packages/office-ui-fabric-react/src/components/ComboBox/ComboBox.tsx#L481. - expect(wrapper.find('input').props().value).toEqual('\u200B'); + expect(wrapper.find('input').props().value).toEqual(''); }); it('Renders a placeholder', () => { @@ -482,7 +480,7 @@ describe('ComboBox', () => { // SelectedKey is set to null wrapper.setProps({ selectedKey: null }); - expect(wrapper.find('input').props().value).toEqual('\u200B'); + expect(wrapper.find('input').props().value).toEqual(''); const suggestedDisplay = (componentRef.current as ComboBox).state.suggestedDisplayValue; expect(suggestedDisplay).toEqual(undefined); @@ -556,6 +554,37 @@ describe('ComboBox', () => { expect(updatedText).toEqual(''); }); + it('Can clear text in controlled case with autoComplete off and allowFreeform on', () => { + let updatedText; + wrapper = mount( + , option?: IComboBoxOption, index?: number, value?: string) => { + updatedText = value; + }} + /> + ); + + const input = wrapper.find('input'); + (input.instance() as any).value = 'ab'; + input.simulate('input', { target: { value: 'ab' } }); + input.simulate('keydown', { which: KeyCodes.backspace }); + input.simulate('input', { target: { value: 'a' } }); + input.simulate('keydown', { which: KeyCodes.backspace }); + wrapper.update(); + + (input.instance() as any).value = ''; + input.simulate('input', { target: { value: '' } }); + wrapper.update(); + expect((input.instance() as any).value).toEqual(''); + input.simulate('keydown', { which: KeyCodes.enter }); + + expect(updatedText).toEqual(''); + }); + it('in multiSelect mode, selectedIndices are correct after performing multiple selections using mouse click', () => { const comboBoxRef = React.createRef(); wrapper = mount(); @@ -572,6 +601,55 @@ describe('ComboBox', () => { expect((comboBoxRef.current as ComboBox).state.selectedIndices).toEqual([0, 2, 1]); }); + it('in multiSelect mode, defaultselected keys produce correct display input', () => { + const comboBoxRef = React.createRef(); + wrapper = mount( + + ); + + const comboBoxRoot = wrapper.find('.ms-ComboBox'); + const inputElement = comboBoxRoot.find('input'); + inputElement.simulate('keydown', { which: KeyCodes.enter }); + const buttons = document.querySelectorAll('.ms-ComboBox-option > input'); + + ReactTestUtils.Simulate.change(buttons[2]); + ReactTestUtils.Simulate.change(buttons[0]); + const compare = [DEFAULT_OPTIONS[0], DEFAULT_OPTIONS[2]].reduce((previous: string, current: IComboBoxOption) => { + if (previous !== '') { + return previous + ', ' + current.text; + } + return current.text; + }, ''); + + expect((inputElement.instance() as any).value).toEqual(compare); + }); + + it('in multiSelect mode, input has correct value', () => { + const comboBoxRef = React.createRef(); + wrapper = mount(); + + const comboBoxRoot = wrapper.find('.ms-ComboBox'); + const inputElement = comboBoxRoot.find('input'); + inputElement.simulate('keydown', { which: KeyCodes.enter }); + const buttons = document.querySelectorAll('.ms-ComboBox-option > input'); + + ReactTestUtils.Simulate.change(buttons[0]); + ReactTestUtils.Simulate.change(buttons[2]); + const compare = [DEFAULT_OPTIONS[0], DEFAULT_OPTIONS[2]].reduce((previous: string, current: IComboBoxOption) => { + if (previous !== '') { + return previous + ', ' + current.text; + } + return current.text; + }, ''); + + expect((inputElement.instance() as any).value).toEqual(compare); + }); + it('in multiSelect mode, optional onItemClick callback invoked per option select', () => { const onItemClickMock = jest.fn(); wrapper = mount(); diff --git a/packages/office-ui-fabric-react/src/components/ComboBox/ComboBox.tsx b/packages/office-ui-fabric-react/src/components/ComboBox/ComboBox.tsx index 1fae4857bd55b7..2dcfe934a5dbbc 100644 --- a/packages/office-ui-fabric-react/src/components/ComboBox/ComboBox.tsx +++ b/packages/office-ui-fabric-react/src/components/ComboBox/ComboBox.tsx @@ -334,19 +334,33 @@ export class ComboBox extends BaseComponent { theme, title, keytipProps, - placeholder, + placeholder: placeholderProp, tabIndex, autofill, persistMenu, - iconButtonProps + iconButtonProps, + multiSelect } = this.props; const { isOpen, focused, suggestedDisplayValue } = this.state; this._currentVisibleValue = this._getVisibleValue(); + // Single select is already accessible since the whole text is selected + // when focus enters the input. Since multiselect appears to clear the input + // it needs special accessible text + const multiselectAccessibleText = multiSelect + ? this._getMultiselectDisplayString(this.state.selectedIndices, this.state.currentOptions, suggestedDisplayValue) + : undefined; + const divProps = getNativeProps>(this.props, divProperties, ['onChange', 'value']); const hasErrorMessage = errorMessage && errorMessage.length > 0 ? true : false; + // If the combobox has focus, is multiselect, and has a display string, then use that placeholder + // so that the selected items don't appear to vanish. This is not ideal but it's the only reasonable way + // to correct the behavior where the input is cleared so the user can type. If a full refactor is done, then this + // should be removed and the multiselect combobox should behave like a picker. + const placeholder = focused && this.props.multiSelect && multiselectAccessibleText ? multiselectAccessibleText : placeholderProp; + this._classNames = this.props.getClassNames ? this.props.getClassNames(theme!, !!isOpen, !!disabled, !!required, !!focused, !!allowFreeform, !!hasErrorMessage, className) : getClassNames( @@ -365,6 +379,7 @@ export class ComboBox extends BaseComponent { {label && ( )} @@ -504,10 +519,8 @@ export class ComboBox extends BaseComponent { const visibleValue = this._normalizeToString(this._currentVisibleValue); if (comboBox.value !== visibleValue) { - // If visibleValue is empty, make it a zero width space. - // If we did not do that, the empty string would not get used - // potentially resulting in an unexpected value being used - return visibleValue || '​'; + // If visibleValue is empty, ensure that the empty string is used + return visibleValue || ''; } return comboBox.value; @@ -550,9 +563,6 @@ export class ComboBox extends BaseComponent { return text; } - // Values to display in the BaseAutoFill area - const displayValues = []; - if (this.props.multiSelect) { // Multi-select if (focused) { @@ -560,20 +570,9 @@ export class ComboBox extends BaseComponent { if (autoComplete === 'on' && currentPendingIndexValid) { index = currentPendingValueValidIndex; } - displayValues.push( - currentPendingValue !== null && currentPendingValue !== undefined - ? currentPendingValue - : this._indexWithinBounds(currentOptions, index) - ? currentOptions[index].text - : '' - ); + return this._getPendingString(currentPendingValue, currentOptions, index); } else { - for (let idx = 0; selectedIndices && idx < selectedIndices.length; idx++) { - const index: number = selectedIndices[idx]; - displayValues.push( - this._indexWithinBounds(currentOptions, index) ? currentOptions[index].text : this._normalizeToString(suggestedDisplayValue) - ); - } + return this._getMultiselectDisplayString(selectedIndices, currentOptions, suggestedDisplayValue); } } else { // Single-select @@ -588,13 +587,7 @@ export class ComboBox extends BaseComponent { // Since we are allowing freeform, if there is currently a pending value, use that // otherwise use the index determined above (falling back to '' if we did not get a valid index) - displayValues.push( - currentPendingValue !== null && currentPendingValue !== undefined - ? currentPendingValue - : this._indexWithinBounds(currentOptions, index) - ? currentOptions[index].text - : '' - ); + return this._getPendingString(currentPendingValue, currentOptions, index); } else { // If we are not allowing freeform and have a // valid index that matches the pending value, @@ -604,21 +597,42 @@ export class ComboBox extends BaseComponent { // raw pending value, otherwise remember // the matched option's index index = currentPendingValueValidIndex; - displayValues.push(this._normalizeToString(currentPendingValue)); + return this._normalizeToString(currentPendingValue); } else if (!this.state.isOpen && currentPendingValue) { - displayValues.push( - this._indexWithinBounds(currentOptions, index) ? currentPendingValue : this._normalizeToString(suggestedDisplayValue) - ); + return this._indexWithinBounds(currentOptions, index) ? currentPendingValue : this._normalizeToString(suggestedDisplayValue); } else { - displayValues.push( - this._indexWithinBounds(currentOptions, index) ? currentOptions[index].text : this._normalizeToString(suggestedDisplayValue) - ); + return this._indexWithinBounds(currentOptions, index) + ? currentOptions[index].text + : this._normalizeToString(suggestedDisplayValue); } } } + }; - // If we have a valid index then return the text value of that option, - // otherwise return the suggestedDisplayValue + private _getPendingString(currentPendingValue: string | null | undefined, currentOptions: IComboBoxOption[], index: number) { + return currentPendingValue !== null && currentPendingValue !== undefined + ? currentPendingValue + : this._indexWithinBounds(currentOptions, index) + ? currentOptions[index].text + : ''; + } + + /** + * Returns a string that concatenates all of the selected values + * for multiselect combobox. + */ + private _getMultiselectDisplayString( + selectedIndices: number[] | undefined, + currentOptions: IComboBoxOption[], + suggestedDisplayValue: string | undefined + ) { + const displayValues = []; + for (let idx = 0; selectedIndices && idx < selectedIndices.length; idx++) { + const index: number = selectedIndices[idx]; + displayValues.push( + this._indexWithinBounds(currentOptions, index) ? currentOptions[index].text : this._normalizeToString(suggestedDisplayValue) + ); + } let displayString = ''; for (let idx = 0; idx < displayValues.length; idx++) { if (idx > 0) { @@ -627,7 +641,7 @@ export class ComboBox extends BaseComponent { displayString += displayValues[idx]; } return displayString; - }; + } /** * Is the index within the bounds of the array? @@ -662,7 +676,6 @@ export class ComboBox extends BaseComponent { */ private _processInputChangeWithFreeform(updatedValue: string): void { const { currentOptions } = this.state; - updatedValue = this._removeZeroWidthSpaces(updatedValue); let newCurrentPendingValueValidIndex = -1; // if the new value is empty, see if we have an exact match @@ -748,7 +761,6 @@ export class ComboBox extends BaseComponent { private _processInputChangeWithoutFreeform(updatedValue: string): void { const { currentPendingValue, currentPendingValueValidIndex, currentOptions } = this.state; - updatedValue = this._removeZeroWidthSpaces(updatedValue); if (this.props.autoComplete === 'on') { // If autoComplete is on while allow freeform is off, // we will remember the keypresses and build up a string to attempt to match @@ -1568,7 +1580,7 @@ export class ComboBox extends BaseComponent { } this.setState({ - currentPendingValue: currentPendingValue && this._removeZeroWidthSpaces(currentPendingValue), + currentPendingValue: this._normalizeToString(currentPendingValue), currentPendingValueValidIndex: currentPendingValueValidIndex, suggestedDisplayValue: suggestedDisplayValue, currentPendingValueValidIndexOnHover: HoverStatus.default @@ -2117,9 +2129,4 @@ export class ComboBox extends BaseComponent { private _normalizeToString(value?: string): string { return value || ''; } - - private _removeZeroWidthSpaces(value: string): string { - // remove any zero width space characters - return value.replace(RegExp('​', 'g'), ''); - } } diff --git a/packages/office-ui-fabric-react/src/components/ComboBox/ComboBox.types.ts b/packages/office-ui-fabric-react/src/components/ComboBox/ComboBox.types.ts index 0d0db285e300c9..14654203622abb 100644 --- a/packages/office-ui-fabric-react/src/components/ComboBox/ComboBox.types.ts +++ b/packages/office-ui-fabric-react/src/components/ComboBox/ComboBox.types.ts @@ -347,6 +347,11 @@ export interface IComboBoxStyles { * Styles for a divider in the options. */ divider: IStyle; + + /** + * Styles for hidden screen reader text. + */ + screenReaderText: IStyle; } /** diff --git a/packages/office-ui-fabric-react/src/components/__snapshots__/ComboBox.Basic.Example.tsx.shot b/packages/office-ui-fabric-react/src/components/__snapshots__/ComboBox.Basic.Example.tsx.shot index b8430a7dcd9e8c..e478574c7e59e9 100644 --- a/packages/office-ui-fabric-react/src/components/__snapshots__/ComboBox.Basic.Example.tsx.shot +++ b/packages/office-ui-fabric-react/src/components/__snapshots__/ComboBox.Basic.Example.tsx.shot @@ -595,6 +595,27 @@ exports[`Component Examples renders ComboBox.Basic.Example.tsx correctly 1`] = ` id="ComboBox7-label" > Multi-select ComboBox (uncontrolled) + + Option C, Option E +
Controlled multi-select ComboBox (allowFreeform: T) +