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

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": "[email protected]",
+  "commit": "1c107f4a4a4650bd46798a3e058b1f5a8e771610",
+  "date": "2019-12-11T22:23:30.882Z"
+}

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)

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)
     };
   }
 );

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,

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(
+      <ComboBox
+        options={DEFAULT_OPTIONS}
+        autoComplete="off"
+        allowFreeform={true}
+        // tslint:disable-next-line:jsx-no-lambda
+        onChange={(event: React.FormEvent<IComboBox>, 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<any>();
     wrapper = mount(<ComboBox multiSelect options={DEFAULT_OPTIONS} componentRef={comboBoxRef} />);
@@ -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<any>();
+    wrapper = mount(
+      <ComboBox
+        multiSelect
+        options={DEFAULT_OPTIONS}
+        componentRef={comboBoxRef}
+        selectedKey={[DEFAULT_OPTIONS[0].key as string, DEFAULT_OPTIONS[2].key as string]}
+      />
+    );
+
+    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<any>();
+    wrapper = mount(<ComboBox multiSelect options={DEFAULT_OPTIONS} componentRef={comboBoxRef} />);
+
+    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(<ComboBox multiSelect options={DEFAULT_OPTIONS} onItemClick={onItemClickMock} />);

packages/office-ui-fabric-react/src/components/ComboBox/ComboBox.tsx

@@ -334,19 +334,33 @@ export class ComboBox extends BaseComponent<IComboBoxProps, IComboBoxState> {
       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<React.HTMLAttributes<HTMLDivElement>>(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<IComboBoxProps, IComboBoxState> {
         {label && (
           <Label id={id + '-label'} disabled={disabled} required={required} className={this._classNames.label}>
             {label}
+            {multiselectAccessibleText && <span className={this._classNames.screenReaderText}>{multiselectAccessibleText}</span>}
           </Label>
         )}
         <KeytipData keytipProps={keytipProps} disabled={disabled}>
@@ -504,10 +519,8 @@ export class ComboBox extends BaseComponent<IComboBoxProps, IComboBoxState> {
 
     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,30 +563,16 @@ export class ComboBox extends BaseComponent<IComboBoxProps, IComboBoxState> {
       return text;
     }
 
-    // Values to display in the BaseAutoFill area
-    const displayValues = [];
-
     if (this.props.multiSelect) {
       // Multi-select
       if (focused) {
         let index = -1;
         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<IComboBoxProps, IComboBoxState> {
 
         // 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<IComboBoxProps, IComboBoxState> {
           // 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<IComboBoxProps, IComboBoxState> {
       displayString += displayValues[idx];
     }
     return displayString;
-  };
+  }
 
   /**
    * Is the index within the bounds of the array?
@@ -662,7 +676,6 @@ export class ComboBox extends BaseComponent<IComboBoxProps, IComboBoxState> {
    */
   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<IComboBoxProps, IComboBoxState> {
   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<IComboBoxProps, IComboBoxState> {
     }
 
     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<IComboBoxProps, IComboBoxState> {
   private _normalizeToString(value?: string): string {
     return value || '';
   }
-
-  private _removeZeroWidthSpaces(value: string): string {
-    // remove any zero width space characters
-    return value.replace(RegExp('​', 'g'), '');
-  }
 }

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;
 }
 
 /**