[EuiSelectable] Disable arrow key navigation on non-search/listbox ch…

…ildren (#6631) * [REVERT ME] Test custom interactive child within EuiSelectable * Prevent all keys (not just enter/esc) from working if focus is not on the search or listbox * Smooth out focus/active option behavior when focusing/blurring to non-search/listbox children - active highlight should go away since focus is no longer on the search/listbox - blurring fix/workaround should still remain intact (goal is to DRY checks, not regress behavior) * changelog * Revert "[REVERT ME] Test custom interactive child within EuiSelectable" This reverts commit 419843c.
elastic · Mar 6, 2023 · e9e1200 · e9e1200
1 parent bca603f
commit e9e1200
Show file tree

Hide file tree

Showing 5 changed files with 109 additions and 31 deletions.
diff --git a/src/components/selectable/__snapshots__/selectable.test.tsx.snap b/src/components/selectable/__snapshots__/selectable.test.tsx.snap
@@ -425,7 +425,7 @@ exports[`EuiSelectable search value supports inheriting initialSearchValue from
 </div>
 `;
 
-exports[`EuiSelectable should not reset the activeOptionIndex nor isFocused when EuiSelectable is blurred in favour of its popover 1`] = `
+exports[`EuiSelectable should not reset the activeOptionIndex nor isFocused when EuiSelectable is blurred in favour of its search/listbox 1`] = `
 Object {
   "activeOptionIndex": 0,
   "isFocused": true,
@@ -445,7 +445,7 @@ Object {
 }
 `;
 
-exports[`EuiSelectable should not reset the activeOptionIndex nor isFocused when EuiSelectable is blurred in favour of its popover 2`] = `
+exports[`EuiSelectable should not reset the activeOptionIndex nor isFocused when EuiSelectable is blurred in favour of its search/listbox 2`] = `
 Object {
   "activeOptionIndex": 0,
   "isFocused": true,

diff --git a/src/components/selectable/selectable.spec.tsx b/src/components/selectable/selectable.spec.tsx
@@ -93,7 +93,7 @@ describe('EuiSelectable', () => {
     it('can clear the input', () => {
       cy.realMount(<EuiSelectableWithSearchInput />);
 
-      // Focus the second option
+      // Search/filter down to the second option
       cy.get('input')
         .realClick()
         .realType('enc')
@@ -103,7 +103,7 @@ describe('EuiSelectable', () => {
             .should('have.attr', 'title', 'Enceladus');
         });
 
-      // Using ENTER
+      // Clear search using ENTER
       cy.get('[data-test-subj="clearSearchButton"]')
         .focus()
         .realPress('{enter}')
@@ -113,7 +113,7 @@ describe('EuiSelectable', () => {
             .should('have.attr', 'title', 'Titan');
         });
 
-      // Focus the second option
+      // Search/filter again
       cy.get('input')
         .realClick()
         .realType('enc')
@@ -123,7 +123,7 @@ describe('EuiSelectable', () => {
             .should('have.attr', 'title', 'Enceladus');
         });
 
-      // Using SPACE
+      // Clear search using SPACE
       cy.get('[data-test-subj="clearSearchButton"]')
         .focus()
         .realPress('Space')
@@ -132,6 +132,30 @@ describe('EuiSelectable', () => {
             .first()
             .should('have.attr', 'title', 'Titan');
         });
+
+      // Ensure the clear button does not respond to up/down arrow keys
+      cy.get('input')
+        .realClick()
+        .realType('titan')
+        .then(() => {
+          cy.get('li[role=option]')
+            .first()
+            .should('have.attr', 'title', 'Titan');
+        });
+      cy.get('[data-test-subj="clearSearchButton"]')
+        .focus()
+        .realPress('ArrowDown')
+        .then(() => {
+          cy.get('li[role=option]')
+            .first()
+            .should('have.attr', 'title', 'Titan');
+        })
+        .realPress('ArrowUp')
+        .then(() => {
+          cy.get('li[role=option]')
+            .first()
+            .should('have.attr', 'title', 'Titan');
+        });
     });
 
     it('allows pressing the Enter key to select an item', () => {

diff --git a/src/components/selectable/selectable.test.tsx b/src/components/selectable/selectable.test.tsx
@@ -49,7 +49,7 @@ describe('EuiSelectable', () => {
     expect(component).toMatchSnapshot();
   });
 
-  test('should not reset the activeOptionIndex nor isFocused when EuiSelectable is blurred in favour of its popover', () => {
+  test('should not reset the activeOptionIndex nor isFocused when EuiSelectable is blurred in favour of its search/listbox', () => {
     const component = mount(
       <EuiSelectable options={options} searchable>
         {(list, search) => (
@@ -67,9 +67,10 @@ describe('EuiSelectable', () => {
     });
     expect(component.state()).toMatchSnapshot();
 
-    component.find('.euiSelectable').simulate('blur', {
-      relatedTarget: { firstChild: { id: 'generated-id_listbox' } },
-    });
+    const listBox = component.find('div.euiSelectableList__list').getDOMNode();
+    component
+      .find('.euiSelectable')
+      .simulate('blur', { relatedTarget: listBox });
     component.update();
     expect(component.state()).toMatchSnapshot();
   });
@@ -383,6 +384,7 @@ describe('EuiSelectable', () => {
           {(list) => list}
         </EuiSelectable>
       );
+      const target = component.find('div.euiSelectableList__list').getDOMNode();
 
       component.find('[role="option"]').first().simulate('click');
       expect(onChange).toHaveBeenCalledTimes(1);
@@ -393,7 +395,7 @@ describe('EuiSelectable', () => {
         checked: 'on',
       });
 
-      component.simulate('keydown', { key: 'Enter', shiftKey: true });
+      component.simulate('keydown', { key: 'Enter', target });
       expect(onChange).toHaveBeenCalledTimes(2);
       expect(onChange.mock.calls[1][0][0].checked).toEqual('on');
       expect(onChange.mock.calls[1][1].type).toEqual('keydown');
@@ -402,6 +404,24 @@ describe('EuiSelectable', () => {
         checked: 'on',
       });
     });
+
+    it('does not call onChange on keydown when focus is not on the search/listbox', () => {
+      const onChange = jest.fn();
+      const component = mount(
+        <EuiSelectable options={options} onChange={onChange}>
+          {(list) => (
+            <>
+              <button id="test">test</button>
+              {list}
+            </>
+          )}
+        </EuiSelectable>
+      );
+      const target = component.find('#test').getDOMNode();
+
+      component.simulate('keydown', { key: 'Enter', target });
+      expect(onChange).toHaveBeenCalledTimes(0);
+    });
   });
 
   describe('onActiveOptionChange', () => {
@@ -412,14 +432,34 @@ describe('EuiSelectable', () => {
           {(list) => list}
         </EuiSelectable>
       );
+      const target = component.find('div.euiSelectableList__list').getDOMNode();
 
-      component.simulate('keydown', { key: 'ArrowDown' });
+      component.simulate('keydown', { key: 'ArrowDown', target });
       expect(callback).toHaveBeenCalledWith(options[0]);
 
-      component.simulate('keydown', { key: 'ArrowUp' });
+      component.simulate('keydown', { key: 'ArrowUp', target });
       expect(callback).toHaveBeenCalledWith(options[2]);
     });
 
+    it('does not change internal activeOptionIndex state on keydown when focus is not on the search/listbox', () => {
+      const callback = jest.fn();
+      const component = mount(
+        <EuiSelectable options={options} onActiveOptionChange={callback}>
+          {(list) => (
+            <>
+              <button id="test">test</button>
+              {list}
+            </>
+          )}
+        </EuiSelectable>
+      );
+      const target = component.find('#test').getDOMNode();
+
+      component.simulate('keydown', { key: 'ArrowDown', target });
+      component.simulate('keydown', { key: 'ArrowUp', target });
+      expect(callback).toHaveBeenCalledTimes(0);
+    });
+
     it('handles the active option changing due to searching', () => {
       const callback = jest.fn();
       const component = mount(
@@ -437,8 +477,9 @@ describe('EuiSelectable', () => {
           )}
         </EuiSelectable>
       );
+      const target = component.find('input[type="search"]').getDOMNode();
 
-      component.simulate('keydown', { key: 'ArrowDown' });
+      component.simulate('keydown', { key: 'ArrowDown', target });
       expect(callback).toHaveBeenCalledWith(options[2]); // Pandora
     });
   });

diff --git a/src/components/selectable/selectable.tsx b/src/components/selectable/selectable.tsx
@@ -14,6 +14,7 @@ import React, {
   ReactElement,
   KeyboardEvent,
   MouseEvent,
+  FocusEvent,
 } from 'react';
 import classNames from 'classnames';
 import { CommonProps, ExclusiveUnion } from '../common';
@@ -280,14 +281,25 @@ export class EuiSelectable<T = {}> extends Component<
     }
   }
 
+  isFocusOnSearchOrListBox = (target: EventTarget | null) => {
+    const searchHasFocus = this.props.searchable && target === this.inputRef;
+
+    const listBox = this.optionsListRef.current?.listBoxRef?.parentElement;
+    const listBoxContainsFocus =
+      target instanceof Node && listBox?.contains(target);
+    const listBoxHasFocus = target === listBox || listBoxContainsFocus;
+
+    return searchHasFocus || listBoxHasFocus;
+  };
+
   onMouseDown = () => {
     // Bypass onFocus when a click event originates from this.containerRef.
     // Prevents onFocus from scrolling away from a clicked option and negating the selection event.
     // https://github.com/elastic/eui/issues/4147
     this.preventOnFocus = true;
   };
 
-  onFocus = () => {
+  onFocus = (event?: FocusEvent) => {
     if (this.preventOnFocus) {
       this.preventOnFocus = false;
       return;
@@ -300,6 +312,10 @@ export class EuiSelectable<T = {}> extends Component<
       return;
     }
 
+    if (event && !this.isFocusOnSearchOrListBox(event.target)) {
+      return;
+    }
+
     const firstSelected = this.state.visibleOptions.findIndex(
       (option) => option.checked && !option.disabled && !option.isGroupLabel
     );
@@ -319,6 +335,15 @@ export class EuiSelectable<T = {}> extends Component<
   onKeyDown = (event: KeyboardEvent<HTMLDivElement>) => {
     const optionsList = this.optionsListRef.current;
 
+    // Check if the user is interacting with something other than the
+    // searchbox or selection list. If so, the user may be attempting to
+    // interact with the search clear button or a totally custom button,
+    // and listbox keyboard navigation/selection should not be triggered.
+    if (!this.isFocusOnSearchOrListBox(event.target)) {
+      this.setState({ activeOptionIndex: undefined, isFocused: false });
+      return;
+    }
+
     switch (event.key) {
       case keys.ARROW_UP:
         event.preventDefault();
@@ -343,19 +368,6 @@ export class EuiSelectable<T = {}> extends Component<
           if (event.target === this.inputRef && event.key === keys.SPACE) {
             return;
           }
-          // Check if the user is interacting with something other than the
-          // searchbox or selection list. If not, the user is attempting to
-          // interact with an internal button such as the clear button,
-          // and the event should not be altered.
-          if (
-            !(
-              event.target === this.inputRef ||
-              event.target ===
-                this.optionsListRef.current?.listBoxRef?.parentElement
-            )
-          ) {
-            return;
-          }
         }
         event.preventDefault();
         event.stopPropagation();
@@ -443,9 +455,7 @@ export class EuiSelectable<T = {}> extends Component<
 
   onContainerBlur = (e: React.FocusEvent) => {
     // Ignore blur events when moving from search to option to avoid activeOptionIndex conflicts
-    if (
-      ((e.relatedTarget as Node)?.firstChild as HTMLElement)?.id === this.listId
-    ) {
+    if (this.isFocusOnSearchOrListBox(e.relatedTarget)) {
       return;
     }
 

diff --git a/upcoming_changelogs/6631.md b/upcoming_changelogs/6631.md
@@ -0,0 +1,3 @@
+**Bug fixes**
+
+- Fixed `EuiSelectable` to no longer show active selection state or respond to the Up/Down arrow keys when focus is inside the selectable container, but not on the searchbox or listbox.
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1,3 @@
		Bug fixes

		- Fixed `EuiSelectable` to no longer show active selection state or respond to the Up/Down arrow keys when focus is inside the selectable container, but not on the searchbox or listbox.