diff --git a/src/components/selectable/__snapshots__/selectable.test.tsx.snap b/src/components/selectable/__snapshots__/selectable.test.tsx.snap index f63895402a1..9c5ac047667 100644 --- 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 `; -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 index d04ab874e08..18f4a762836 100644 --- 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(); - // 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 index a1d6c33114b..12fccd2c6a4 100644 --- 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( {(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} ); + 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( + + {(list) => ( + <> + + {list} + + )} + + ); + 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} ); + 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( + + {(list) => ( + <> + + {list} + + )} + + ); + 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', () => { )} ); + 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 index cd40838d23f..439449d15d9 100644 --- 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,6 +281,17 @@ export class EuiSelectable 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. @@ -287,7 +299,7 @@ export class EuiSelectable extends Component< this.preventOnFocus = true; }; - onFocus = () => { + onFocus = (event?: FocusEvent) => { if (this.preventOnFocus) { this.preventOnFocus = false; return; @@ -300,6 +312,10 @@ export class EuiSelectable 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 extends Component< onKeyDown = (event: KeyboardEvent) => { 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 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 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 new file mode 100644 index 00000000000..d522d7e1d04 --- /dev/null +++ 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.