Fix EuiComboBox focus trap

CHANGELOG.md

@@ -4,9 +4,11 @@
 
 - `EuiSearchBar` no longer has an `onParse` callback, and now passes an object to `onChange` with the shape `{ query, queryText, error }` ([#863](https://github.com/elastic/eui/pull/863))
 - `EuiInMemoryTable`'s `search.onChange` callback now passes an object with `{ query, queryText, error }` instead of only the query ([#863](https://github.com/elastic/eui/pull/863))
+- `EuiFormControlLayout` no longer has `onClear`, `iconSide`, or `onIconClick` props. Instead of `onClear` it now accepts a `clear` object of the shape `{ onClick }`. Instead of the icon props, it now accepts a single `icon` prop which be either a string or an object of the shape `{ type, side, onClick }`. ([#866](https://github.com/elastic/eui/pull/866))
 
 **Bug fixes**
 
+- `EuiComboBox` is no longer a focus trap, the clear button is now keyboard-accessible, and the virtualized list no longer interferes with the tab order ([#866](https://github.com/elastic/eui/pull/866))
 - `EuiButton`, `EuiButtonEmpty`, and `EuiButtonIcon` now look and behave disabled when `isDisabled={true}` ([#862](https://github.com/elastic/eui/pull/862))
 - `EuiGlobalToastList` no longer triggers `Uncaught TypeError: _this.callback is not a function`  ([#865](https://github.com/elastic/eui/pull/865))
 - `EuiGlobalToastList` checks to see if it has dismissed a toast before re-dismissing it ([#868](https://github.com/elastic/eui/pull/868))

src/components/combo_box/__snapshots__/combo_box.test.js.snap

@@ -10,15 +10,15 @@ exports[`EuiComboBox is rendered 1`] = `
 >
   <EuiComboBoxInput
     autoSizeInputRef={[Function]}
+    clearButtonRef={[Function]}
     hasSelectedOptions={false}
     inputRef={[Function]}
     isListOpen={false}
     onChange={[Function]}
     onClear={[Function]}
     onClick={[Function]}
-    onClose={[Function]}
     onFocus={[Function]}
-    onOpen={[Function]}
+    onOpenListClick={[Function]}
     onRemoveOption={[Function]}
     searchValue=""
     selectedOptions={Array []}
@@ -39,14 +39,14 @@ exports[`props isClearable is false with selectedOptions 1`] = `
 >
   <EuiComboBoxInput
     autoSizeInputRef={[Function]}
+    clearButtonRef={[Function]}
     hasSelectedOptions={true}
     inputRef={[Function]}
     isListOpen={false}
     onChange={[Function]}
     onClick={[Function]}
-    onClose={[Function]}
     onFocus={[Function]}
-    onOpen={[Function]}
+    onOpenListClick={[Function]}
     onRemoveOption={[Function]}
     searchValue=""
     selectedOptions={
@@ -76,14 +76,14 @@ exports[`props isClearable is false without selectedOptions 1`] = `
 >
   <EuiComboBoxInput
     autoSizeInputRef={[Function]}
+    clearButtonRef={[Function]}
     hasSelectedOptions={false}
     inputRef={[Function]}
     isListOpen={false}
     onChange={[Function]}
     onClick={[Function]}
-    onClose={[Function]}
     onFocus={[Function]}
-    onOpen={[Function]}
+    onOpenListClick={[Function]}
     onRemoveOption={[Function]}
     searchValue=""
     selectedOptions={Array []}
@@ -104,15 +104,15 @@ exports[`props isDisabled 1`] = `
 >
   <EuiComboBoxInput
     autoSizeInputRef={[Function]}
+    clearButtonRef={[Function]}
     hasSelectedOptions={true}
     inputRef={[Function]}
     isDisabled={true}
     isListOpen={false}
     onChange={[Function]}
     onClick={[Function]}
-    onClose={[Function]}
     onFocus={[Function]}
-    onOpen={[Function]}
+    onOpenListClick={[Function]}
     onRemoveOption={[Function]}
     searchValue=""
     selectedOptions={
@@ -139,15 +139,15 @@ exports[`props options 1`] = `
 >
   <EuiComboBoxInput
     autoSizeInputRef={[Function]}
+    clearButtonRef={[Function]}
     hasSelectedOptions={false}
     inputRef={[Function]}
     isListOpen={false}
     onChange={[Function]}
     onClear={[Function]}
     onClick={[Function]}
-    onClose={[Function]}
     onFocus={[Function]}
-    onOpen={[Function]}
+    onOpenListClick={[Function]}
     onRemoveOption={[Function]}
     searchValue=""
     selectedOptions={Array []}
@@ -168,15 +168,15 @@ exports[`props selectedOptions 1`] = `
 >
   <EuiComboBoxInput
     autoSizeInputRef={[Function]}
+    clearButtonRef={[Function]}
     hasSelectedOptions={true}
     inputRef={[Function]}
     isListOpen={false}
     onChange={[Function]}
     onClear={[Function]}
     onClick={[Function]}
-    onClose={[Function]}
     onFocus={[Function]}
-    onOpen={[Function]}
+    onOpenListClick={[Function]}
     onRemoveOption={[Function]}
     searchValue=""
     selectedOptions={
@@ -206,15 +206,15 @@ exports[`props singleSelection 1`] = `
 >
   <EuiComboBoxInput
     autoSizeInputRef={[Function]}
+    clearButtonRef={[Function]}
     hasSelectedOptions={true}
     inputRef={[Function]}
     isListOpen={false}
     onChange={[Function]}
     onClear={[Function]}
     onClick={[Function]}
-    onClose={[Function]}
     onFocus={[Function]}
-    onOpen={[Function]}
+    onOpenListClick={[Function]}
     onRemoveOption={[Function]}
     searchValue=""
     selectedOptions={

src/components/combo_box/combo_box.js

@@ -137,26 +137,42 @@ export class EuiComboBox extends Component {
   };
 
   tabAway = amount => {
+    if (![-1, 1].includes(amount)) {
+      throw new Error(`tabAway expects amount to be -1 or 1, but received ${amount}`);
+    }
+
     const tabbableItems = tabbable(document);
-    const comboBoxIndex = tabbableItems.indexOf(this.searchInput);
 
-    // Wrap to last tabbable if tabbing backwards.
-    if (amount < 0) {
-      if (comboBoxIndex === 0) {
-        tabbableItems[tabbableItems.length - 1].focus();
-        return;
+    if (document.activeElement === this.searchInput) {
+      const searchInputIndex = tabbableItems.indexOf(this.searchInput);
+
+      // Wrap to last tabbable if tabbing backwards.
+      if (amount === -1) {
+        if (searchInputIndex === 0) {
+          tabbableItems[tabbableItems.length - 1].focus();
+          return;
+        }
       }
+
+      // Otherwise tab to the next adjacent item.
+      tabbableItems[searchInputIndex + amount].focus();
+      return;
     }
 
-    // Wrap to first tabbable if tabbing forwards.
-    if (amount > 0) {
-      if (comboBoxIndex === tabbableItems.length - 1) {
-        tabbableItems[0].focus();
-        return;
+    if (document.activeElement === this.clearButton) {
+      const clearButtonIndex = tabbableItems.indexOf(this.clearButton);
+
+      // Wrap to first tabbable if tabbing forwards.
+      if (amount === 1) {
+        if (clearButtonIndex === tabbableItems.length - 1) {
+          tabbableItems[0].focus();
+          return;
+        }
       }
-    }
 
-    tabbableItems[comboBoxIndex + amount].focus();
+      // Otherwise tab to the next adjacent item.
+      tabbableItems[clearButtonIndex + amount].focus();
+    }
   };
 
   incrementActiveOptionIndex = throttle(amount => {
@@ -290,14 +306,10 @@ export class EuiComboBox extends Component {
   };
 
   onFocus = () => {
-    document.addEventListener('click', this.onDocumentFocusChange);
-    document.addEventListener('focusin', this.onDocumentFocusChange);
     this.openList();
   }
 
   onBlur = () => {
-    document.removeEventListener('click', this.onDocumentFocusChange);
-    document.removeEventListener('focusin', this.onDocumentFocusChange);
     this.closeList();
   }
 
@@ -309,6 +321,7 @@ export class EuiComboBox extends Component {
       || this.optionsList === event.target
       || this.optionsList && this.optionsList.contains(event.target)
     ) {
+      this.onFocus();
       return;
     }
 
@@ -417,6 +430,10 @@ export class EuiComboBox extends Component {
     }
   };
 
+  onOpenListClick = () => {
+    this.searchInput.focus();
+  };
+
   onSearchChange = (searchValue) => {
     if (this.props.onSearchChange) {
       this.props.onSearchChange(searchValue);
@@ -450,8 +467,14 @@ export class EuiComboBox extends Component {
     this.options[index] = node;
   };
 
+  clearButtonRef = node => {
+    this.clearButton = node;
+  };
+
   componentDidMount() {
     this._isMounted = true;
+    document.addEventListener('click', this.onDocumentFocusChange);
+    document.addEventListener('focusin', this.onDocumentFocusChange);
 
     // TODO: This will need to be called once the actual stylesheet loads.
     setTimeout(() => {
@@ -586,10 +609,10 @@ export class EuiComboBox extends Component {
           onClear={isClearable && !isDisabled ? this.clearSelectedOptions : undefined}
           hasSelectedOptions={selectedOptions.length > 0}
           isListOpen={isListOpen}
-          onOpen={this.openList}
-          onClose={this.closeList}
+          onOpenListClick={this.onOpenListClick}
           singleSelection={singleSelection}
           isDisabled={isDisabled}
+          clearButtonRef={this.clearButtonRef}
         />
 
         {optionsList}

src/components/combo_box/combo_box_input/combo_box_input.js

@@ -26,10 +26,10 @@ export class EuiComboBoxInput extends Component {
     onClear: PropTypes.func,
     hasSelectedOptions: PropTypes.bool.isRequired,
     isListOpen: PropTypes.bool.isRequired,
-    onOpen: PropTypes.func.isRequired,
-    onClose: PropTypes.func.isRequired,
+    onOpenListClick: PropTypes.func.isRequired,
     singleSelection: PropTypes.bool,
     isDisabled: PropTypes.bool,
+    clearButtonRef: PropTypes.func,
   }
 
   constructor(props) {
@@ -86,10 +86,10 @@ export class EuiComboBoxInput extends Component {
       onClear,
       hasSelectedOptions,
       isListOpen,
-      onOpen,
-      onClose,
+      onOpenListClick,
       singleSelection,
       isDisabled,
+      clearButtonRef,
     } = this.props;
 
     const pills = selectedOptions.map((option) => {
@@ -149,14 +149,24 @@ export class EuiComboBoxInput extends Component {
     const clickProps = {};
 
     if (!isDisabled) {
-      clickProps.onClear = hasSelectedOptions ? onClear : undefined;
-      clickProps.onIconClick = isListOpen ? onClose : onOpen;
+      clickProps.clear = {
+        onClick: hasSelectedOptions ? onClear : undefined,
+        ref: clearButtonRef,
+      };
     }
 
+    const icon = {
+      type: 'arrowDown',
+      side: 'right',
+      onClick: isListOpen && !isDisabled ? undefined : onOpenListClick,
+      // We want to remove this from the tab order because you can open the combo box by tabbing
+      // to it already.
+      tabIndex: '-1',
+    };
+
     return (
       <EuiFormControlLayout
-        icon="arrowDown"
-        iconSide="right"
+        icon={icon}
         {...clickProps}
       >
         <div

src/components/combo_box/combo_box_options_list/combo_box_options_list.js

@@ -161,6 +161,7 @@ export class EuiComboBoxOptionsList extends Component {
 
     const optionsList = (
       <List
+        tabIndex={-1}
         width={width}
         height={height}
         rowCount={matchingOptions.length}

src/components/date_picker/__snapshots__/date_picker.test.js.snap

@@ -9,7 +9,6 @@ exports[`EuiDatePicker is rendered 1`] = `
       compressed={false}
       fullWidth={false}
       icon="calendar"
-      iconSide="left"
       isLoading={false}
     >
       <EuiValidatableControl>

src/components/form/field_number/field_number.js

@@ -3,6 +3,7 @@ import PropTypes from 'prop-types';
 import classNames from 'classnames';
 
 import {
+  ICON_SIDES,
   EuiFormControlLayout,
 } from '../form_control_layout';
 
@@ -81,7 +82,14 @@ EuiFieldNumber.propTypes = {
   max: PropTypes.number,
   step: PropTypes.number,
   value: numberOrEmptyString,
-  icon: PropTypes.string,
+  icon: PropTypes.oneOfType([
+    PropTypes.string,
+    PropTypes.shape({
+      type: PropTypes.string,
+      side: PropTypes.oneOf(ICON_SIDES),
+      onClick: PropTypes.func,
+    }),
+  ]),
   isInvalid: PropTypes.bool,
   fullWidth: PropTypes.bool,
   isLoading: PropTypes.bool,

src/components/form/field_number/field_number.test.js

@@ -4,7 +4,13 @@ import { requiredProps } from '../../../test/required_props';
 
 import { EuiFieldNumber } from './field_number';
 
-jest.mock('../form_control_layout', () => ({ EuiFormControlLayout: 'eui-form-control-layout' }));
+jest.mock('../form_control_layout', () => {
+  const formControlLayout = require.requireActual('../form_control_layout')
+  return {
+    ...formControlLayout,
+    EuiFormControlLayout: 'eui-form-control-layout',
+  }
+});
 jest.mock('../validatable_control', () => ({ EuiValidatableControl: 'eui-validatable-control' }));
 
 describe('EuiFieldNumber', () => {

src/components/form/field_text/field_text.js

@@ -3,6 +3,7 @@ import PropTypes from 'prop-types';
 import classNames from 'classnames';
 
 import {
+  ICON_SIDES,
   EuiFormControlLayout,
 } from '../form_control_layout';
 
@@ -61,7 +62,14 @@ EuiFieldText.propTypes = {
   id: PropTypes.string,
   placeholder: PropTypes.string,
   value: PropTypes.string,
-  icon: PropTypes.string,
+  icon: PropTypes.oneOfType([
+    PropTypes.string,
+    PropTypes.shape({
+      type: PropTypes.string,
+      side: PropTypes.oneOf(ICON_SIDES),
+      onClick: PropTypes.func,
+    }),
+  ]),
   isInvalid: PropTypes.bool,
   inputRef: PropTypes.func,
   fullWidth: PropTypes.bool,