Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[EuiSelectable] Disable arrow key navigation on non-search/listbox children #6631

Merged
merged 5 commits into from
Mar 6, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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,
Expand Down
32 changes: 28 additions & 4 deletions src/components/selectable/selectable.spec.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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')
Expand All @@ -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}')
Expand All @@ -113,7 +113,7 @@ describe('EuiSelectable', () => {
.should('have.attr', 'title', 'Titan');
});

// Focus the second option
// Search/filter again
cy.get('input')
.realClick()
.realType('enc')
Expand All @@ -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')
Expand All @@ -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', () => {
Expand Down
57 changes: 49 additions & 8 deletions src/components/selectable/selectable.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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) => (
Expand All @@ -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();
});
Expand Down Expand Up @@ -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);
Expand All @@ -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');
Expand All @@ -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', () => {
Expand All @@ -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(
Expand All @@ -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
});
});
Expand Down
44 changes: 27 additions & 17 deletions src/components/selectable/selectable.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import React, {
ReactElement,
KeyboardEvent,
MouseEvent,
FocusEvent,
} from 'react';
import classNames from 'classnames';
import { CommonProps, ExclusiveUnion } from '../common';
Expand Down Expand Up @@ -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) => {
Copy link
Member Author

@cee-chen cee-chen Mar 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Attaching to a random line for threading]

@miukimiu, @1Copenut - should non-searchable EuiSelectable's have this slightly-but-not-really visible focus ring around the listbox? (Note: this is happening on production, not just this PR)

Above screenshot is from FF, but it's happening on Chrome/Edge/Safari as well.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested in Chrome. I think it shouldn't have the visible focus ring around the listbox. It doesn't look good.

Screenshot 2023-03-06 at 17 46 18

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I'll open up a follow-up issue for this since it's already happening in production: #6633

if (this.preventOnFocus) {
this.preventOnFocus = false;
return;
Expand All @@ -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
);
Expand All @@ -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();
Expand All @@ -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();
Expand Down Expand Up @@ -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)) {
Comment on lines -446 to +458
Copy link
Member Author

@cee-chen cee-chen Mar 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The goal of this change was just DRYness, I tested the 'double click' bug fixed/reported in #5021 fairly thoroughly in Chrome in EuiSelectable in a popover and EuiSelectableTemplateSitewide and am fairly sure there are no regressions.

return;
}

Expand Down
3 changes: 3 additions & 0 deletions upcoming_changelogs/6631.md
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.