Fix EuiComboBox focus trap. Make clear button keyboard-accessible.

- Redesign EuiFormControlLayout props to use icon and clear configuration objects.
elastic · May 23, 2018 · fecf0be · fecf0be
1 parent 3cea448
commit fecf0be
Show file tree

Hide file tree

Showing 15 changed files with 314 additions and 82 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,7 +1,12 @@
 ## [`master`](https://github.com/elastic/eui/tree/master)
 
+**Breaking changes**
+
+- `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 and the clear button is now keyboard-accessible ([#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))

diff --git a/src/components/combo_box/__snapshots__/combo_box.test.js.snap b/src/components/combo_box/__snapshots__/combo_box.test.js.snap
@@ -10,6 +10,7 @@ exports[`EuiComboBox is rendered 1`] = `
 >
   <EuiComboBoxInput
     autoSizeInputRef={[Function]}
+    clearButtonRef={[Function]}
     hasSelectedOptions={false}
     inputRef={[Function]}
     isListOpen={false}
@@ -38,6 +39,7 @@ exports[`props isClearable is false with selectedOptions 1`] = `
 >
   <EuiComboBoxInput
     autoSizeInputRef={[Function]}
+    clearButtonRef={[Function]}
     hasSelectedOptions={true}
     inputRef={[Function]}
     isListOpen={false}
@@ -74,6 +76,7 @@ exports[`props isClearable is false without selectedOptions 1`] = `
 >
   <EuiComboBoxInput
     autoSizeInputRef={[Function]}
+    clearButtonRef={[Function]}
     hasSelectedOptions={false}
     inputRef={[Function]}
     isListOpen={false}
@@ -101,6 +104,7 @@ exports[`props isDisabled 1`] = `
 >
   <EuiComboBoxInput
     autoSizeInputRef={[Function]}
+    clearButtonRef={[Function]}
     hasSelectedOptions={true}
     inputRef={[Function]}
     isDisabled={true}
@@ -135,6 +139,7 @@ exports[`props options 1`] = `
 >
   <EuiComboBoxInput
     autoSizeInputRef={[Function]}
+    clearButtonRef={[Function]}
     hasSelectedOptions={false}
     inputRef={[Function]}
     isListOpen={false}
@@ -163,6 +168,7 @@ exports[`props selectedOptions 1`] = `
 >
   <EuiComboBoxInput
     autoSizeInputRef={[Function]}
+    clearButtonRef={[Function]}
     hasSelectedOptions={true}
     inputRef={[Function]}
     isListOpen={false}
@@ -200,6 +206,7 @@ exports[`props singleSelection 1`] = `
 >
   <EuiComboBoxInput
     autoSizeInputRef={[Function]}
+    clearButtonRef={[Function]}
     hasSelectedOptions={true}
     inputRef={[Function]}
     isListOpen={false}

diff --git a/src/components/combo_box/combo_box.js b/src/components/combo_box/combo_box.js
@@ -138,25 +138,37 @@ export class EuiComboBox extends Component {
 
   tabAway = 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 < 0) {
+        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 > 0) {
+        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 +302,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 +317,7 @@ export class EuiComboBox extends Component {
       || this.optionsList === event.target
       || this.optionsList && this.optionsList.contains(event.target)
     ) {
+      this.onFocus();
       return;
     }
 
@@ -454,8 +463,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(() => {
@@ -593,6 +608,7 @@ export class EuiComboBox extends Component {
           onOpenListClick={this.onOpenListClick}
           singleSelection={singleSelection}
           isDisabled={isDisabled}
+          clearButtonRef={this.clearButtonRef}
         />
 
         {optionsList}

diff --git a/src/components/combo_box/combo_box_input/combo_box_input.js b/src/components/combo_box/combo_box_input/combo_box_input.js
@@ -29,6 +29,7 @@ export class EuiComboBoxInput extends Component {
     onOpenListClick: PropTypes.func.isRequired,
     singleSelection: PropTypes.bool,
     isDisabled: PropTypes.bool,
+    clearButtonRef: PropTypes.func,
   }
 
   constructor(props) {
@@ -88,6 +89,7 @@ export class EuiComboBoxInput extends Component {
       onOpenListClick,
       singleSelection,
       isDisabled,
+      clearButtonRef,
     } = this.props;
 
     const pills = selectedOptions.map((option) => {
@@ -147,14 +149,24 @@ export class EuiComboBoxInput extends Component {
     const clickProps = {};
 
     if (!isDisabled) {
-      clickProps.onClear = hasSelectedOptions ? onClear : undefined;
-      clickProps.onIconClick = isListOpen ? undefined : onOpenListClick;
+      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

diff --git a/src/components/date_picker/__snapshots__/date_picker.test.js.snap b/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>

diff --git a/src/components/form/field_number/field_number.js b/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,

diff --git a/src/components/form/field_number/field_number.test.js b/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', () => {

diff --git a/src/components/form/field_text/field_text.js b/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,

diff --git a/src/components/form/field_text/field_text.test.js b/src/components/form/field_text/field_text.test.js
@@ -4,7 +4,13 @@ import { requiredProps } from '../../../test/required_props';
 
 import { EuiFieldText } from './field_text';
 
-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('EuiFieldText', () => {

diff --git a/src/components/form/form_control_layout/__snapshots__/form_control_layout.test.js.snap b/src/components/form/form_control_layout/__snapshots__/form_control_layout.test.js.snap
@@ -2,19 +2,52 @@
 
 exports[`EuiFormControlLayout is rendered 1`] = `
 <div
-  class="euiFormControlLayout"
+  aria-label="aria-label"
+  class="euiFormControlLayout testClass1 testClass2"
+  data-test-subj="test subject string"
 >
   <input />
 </div>
 `;
 
+exports[`EuiFormControlLayout props clear onClick is rendered 1`] = `
+<div
+  class="euiFormControlLayout"
+>
+  <button
+    class="euiFormControlLayout__clear customClass"
+    data-test-subj="clearButton"
+  >
+    <svg
+      class="euiIcon euiIcon--medium euiFormControlLayout__clearIcon"
+      height="16"
+      viewBox="0 0 16 16"
+      width="16"
+      xlink="http://www.w3.org/1999/xlink"
+      xmlns="http://www.w3.org/2000/svg"
+    >
+      <defs>
+        <path
+          d="M7.293 8l-4.147 4.146a.5.5 0 0 0 .708.708L8 8.707l4.146 4.147a.5.5 0 0 0 .708-.708L8.707 8l4.147-4.146a.5.5 0 0 0-.708-.708L8 7.293 3.854 3.146a.5.5 0 1 0-.708.708L7.293 8z"
+          id="cross-a"
+        />
+      </defs>
+      <use
+        fill-rule="nonzero"
+        href="#cross-a"
+      />
+    </svg>
+  </button>
+</div>
+`;
+
 exports[`EuiFormControlLayout props fullWidth is rendered 1`] = `
 <div
   class="euiFormControlLayout euiFormControlLayout--fullWidth"
 />
 `;
 
-exports[`EuiFormControlLayout props icon is rendered 1`] = `
+exports[`EuiFormControlLayout props icon is rendered as a string 1`] = `
 <div
   class="euiFormControlLayout"
 >
@@ -40,7 +73,34 @@ exports[`EuiFormControlLayout props icon is rendered 1`] = `
 </div>
 `;
 
-exports[`EuiFormControlLayout props iconSide left is rendered 1`] = `
+exports[`EuiFormControlLayout props icon is rendered as an object 1`] = `
+<div
+  class="euiFormControlLayout"
+>
+  <svg
+    aria-hidden="true"
+    class="euiIcon euiIcon--medium euiFormControlLayout__icon customClass"
+    data-test-subj="myIcon"
+    height="16"
+    viewBox="0 0 16 16"
+    width="16"
+    xmlns="http://www.w3.org/2000/svg"
+  >
+    <g
+      fill-rule="evenodd"
+    >
+      <path
+        d="M7.5 2.236L1.618 14h11.764L7.5 2.236zm.894-.447l5.882 11.764A1 1 0 0 1 13.382 15H1.618a1 1 0 0 1-.894-1.447L6.606 1.789a1 1 0 0 1 1.788 0z"
+      />
+      <path
+        d="M7 6h1v5H7zM7 12h1v1H7z"
+      />
+    </g>
+  </svg>
+</div>
+`;
+
+exports[`EuiFormControlLayout props icon side left is rendered 1`] = `
 <div
   class="euiFormControlLayout"
 >
@@ -66,7 +126,7 @@ exports[`EuiFormControlLayout props iconSide left is rendered 1`] = `
 </div>
 `;
 
-exports[`EuiFormControlLayout props iconSide right is rendered 1`] = `
+exports[`EuiFormControlLayout props icon side right is rendered 1`] = `
 <div
   class="euiFormControlLayout"
 >