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

cee-chen · 2023-03-03T01:16:22Z

Summary

In #5613 and #5693, an early return was added to the space/enter keydown listeners to ensure the search clear button still worked without accidentally selecting an option.

It turns out we should have been more aggressive about this early return and put it in front of the entire listener so that the up/down arrow keys also did not function when on the clear button (or any other wildcard interactive content passed by consumers). The arrow keys and enter/space keys should only function when consumers are focused either on the searchbox, or on the listbox.

Before

After

Custom interactive child content within <EuiSelectable> (should remove focus/active state):

QA

Go to https://eui.elastic.co/pr_6631/#/forms/selectable#rendering-the-options (scroll all the way down)
Tab into the search box, arrow up/down, confirm active option selection works
Tab to the fake "Filters" button
Confirm that the active selection goes away
Confirm that using the up/down arrow keys does nothing on the button
Tab backwards to the search box and confirm active option selection/navigation works as expected

General checklist

Revert [REVERT ME] commit
Checked in Chrome, Safari, Edge, and Firefox
Added or updated jest and cypress tests
Checked for accessibility including keyboard-only and screenreader modes
A changelog entry exists and is marked appropriately

~~- [ ] Checked in both light and dark modes~~
- [ ] Checked in mobile
- [ ] Props have proper autodocs (using @default if default values are missing) and playground toggles
- [ ] Added documentation
~~- [ ] Checked Code Sandbox works for any docs examples~~
~~- [ ] Checked for breaking changes and labeled appropriately~~
~~- [ ] Updated the Figma library counterpart~~

… the search or listbox

…-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)

cee-chen · 2023-03-03T01:39:14Z

src/components/selectable/selectable.tsx

-    if (
-      ((e.relatedTarget as Node)?.firstChild as HTMLElement)?.id === this.listId
-    ) {
+    if (this.isFocusOnSearchOrListBox(e.relatedTarget)) {


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.

cee-chen · 2023-03-03T01:40:50Z

src/components/selectable/selectable.tsx

  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) => {


[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.

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

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

cee-chen · 2023-03-03T01:41:33Z

src-docs/src/views/selectable/selectable_custom_render.tsx

+                onClick={() => {
+                  inputRef?.focus();
+                }}


@Heenawter FYI - it looks like the searchProps.inputRef has to be a function (e.g. a state setter) and not a useRef, but you should definitely be able to auto-focus back to the searchbox once users are done with your custom filter component.

kibanamachine · 2023-03-03T01:44:29Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_6631/

elizabetdev

Thanks, @cee-chen!

Tested the provided QA link in Chrome, Safari, and Firefox. It works well.

I also tested the searchable example and I confirm that it works as expected.

This reverts commit 419843c.

kibanamachine · 2023-03-06T18:22:39Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_6631/

## Summary eui@76.0.2 ⏩ eui@76.3.0 ## [`76.3.0`](https://github.com/elastic/eui/tree/v76.3.0) - Updated `EuiSkipLink`'s `fallbackDestination` prop to support an array of query selector strings ([#6646](elastic/eui#6646)) **Bug fixes** - Fixed `EuiFlyout` to preserve body scrollbar width on open ([#6645](elastic/eui#6645)) - Fixed `EuiImage`'s full screen mode to not scroll jump & to preserve body scrollbar width on open ([#6645](elastic/eui#6645)) - Fixed `EuiCodeBlock`'s full screen mode to not scroll jump & to preserve body scrollbar width on open ([#6645](elastic/eui#6645)) ## [`76.2.0`](https://github.com/elastic/eui/tree/v76.2.0) - Added new `renderCustomGridBody` escape hatch rendering prop to `EuiDataGrid` ([#6624](elastic/eui#6624)) **Bug fixes** - Fixed visual listbox focus ring bug on non-searchable `EuiSelectable`s ([#6637](elastic/eui#6637)) - Added a legacy `alert` alias for the `warning` `EuiIcon` type ([#6640](elastic/eui#6640)) - Fixed a type definition incorrectly coming from a dev dependency, which was causing issues for some consuming projects ([#6643](elastic/eui#6643)) ## [`76.1.0`](https://github.com/elastic/eui/tree/v76.1.0) - Added more detailed screen reader instructions to `EuiSelectable`, `EuiSuggest`, `EuiSelectableTemplateSitewide`, `EuiRange`, and `EuiDualRange`. ([#6589](elastic/eui#6589)) - Added new `placeholder` prop to `EuiSuperSelect` ([#6630](elastic/eui#6630)) - Added new `setCellPopoverProps` parameter callback to `EuiDataGrid`'s `renderCellPopover` prop ([#6632](elastic/eui#6632)) **Bug fixes** - Fixed an ARIA attribute in `EuiSelectableList` ([#6589](elastic/eui#6589)) - 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. ([#6631](elastic/eui#6631)) --------- Co-authored-by: Tiago Costa <tiago.costa@elastic.co>

## Summary eui@76.0.2 ⏩ eui@76.3.0 ## [`76.3.0`](https://github.com/elastic/eui/tree/v76.3.0) - Updated `EuiSkipLink`'s `fallbackDestination` prop to support an array of query selector strings ([elastic#6646](elastic/eui#6646)) **Bug fixes** - Fixed `EuiFlyout` to preserve body scrollbar width on open ([elastic#6645](elastic/eui#6645)) - Fixed `EuiImage`'s full screen mode to not scroll jump & to preserve body scrollbar width on open ([elastic#6645](elastic/eui#6645)) - Fixed `EuiCodeBlock`'s full screen mode to not scroll jump & to preserve body scrollbar width on open ([elastic#6645](elastic/eui#6645)) ## [`76.2.0`](https://github.com/elastic/eui/tree/v76.2.0) - Added new `renderCustomGridBody` escape hatch rendering prop to `EuiDataGrid` ([elastic#6624](elastic/eui#6624)) **Bug fixes** - Fixed visual listbox focus ring bug on non-searchable `EuiSelectable`s ([elastic#6637](elastic/eui#6637)) - Added a legacy `alert` alias for the `warning` `EuiIcon` type ([elastic#6640](elastic/eui#6640)) - Fixed a type definition incorrectly coming from a dev dependency, which was causing issues for some consuming projects ([elastic#6643](elastic/eui#6643)) ## [`76.1.0`](https://github.com/elastic/eui/tree/v76.1.0) - Added more detailed screen reader instructions to `EuiSelectable`, `EuiSuggest`, `EuiSelectableTemplateSitewide`, `EuiRange`, and `EuiDualRange`. ([elastic#6589](elastic/eui#6589)) - Added new `placeholder` prop to `EuiSuperSelect` ([elastic#6630](elastic/eui#6630)) - Added new `setCellPopoverProps` parameter callback to `EuiDataGrid`'s `renderCellPopover` prop ([elastic#6632](elastic/eui#6632)) **Bug fixes** - Fixed an ARIA attribute in `EuiSelectableList` ([elastic#6589](elastic/eui#6589)) - 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. ([elastic#6631](elastic/eui#6631)) --------- Co-authored-by: Tiago Costa <tiago.costa@elastic.co>

cee-chen added 3 commits March 2, 2023 16:11

[REVERT ME] Test custom interactive child within EuiSelectable

419843c

Prevent all keys (not just enter/esc) from working if focus is not on…

480dbb9

… the search or listbox

Smooth out focus/active option behavior when focusing/blurring to non…

1413d9e

…-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)

cee-chen changed the title ~~[EuiSelectable] Disable arrow key navigation on children not inside focus children~~ [EuiSelectable] Disable arrow key navigation on non-search/listbox children Mar 3, 2023

changelog

d9ef9e7

cee-chen commented Mar 3, 2023

View reviewed changes

cee-chen marked this pull request as ready for review March 3, 2023 01:41

cee-chen requested a review from elizabetdev March 3, 2023 02:00

elizabetdev approved these changes Mar 6, 2023

View reviewed changes

cee-chen mentioned this pull request Mar 6, 2023

[EuiSelectable] Non-searchable listboxes' focus rings #6633

Closed

Revert "[REVERT ME] Test custom interactive child within EuiSelectable"

80e0402

This reverts commit 419843c.

cee-chen enabled auto-merge (squash) March 6, 2023 17:58

cee-chen merged commit e9e1200 into elastic:main Mar 6, 2023

cee-chen deleted the selectable-focus-children branch March 6, 2023 18:36

This was referenced Mar 20, 2023

Upgrade EUI to latest v76.2.0 elastic/kibana#153302

Closed

Upgrade EUI to v76.3.0 elastic/kibana#153341

Merged

cee-chen mentioned this pull request Oct 17, 2023

[EuiComboBox] Broken keyboard interaction in x clear button #7289

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

cee-chen commented Mar 3, 2023 •

edited

Loading

cee-chen Mar 3, 2023 •

edited

Loading

cee-chen Mar 3, 2023 •

edited

Loading

elizabetdev Mar 6, 2023

cee-chen Mar 6, 2023

cee-chen Mar 3, 2023

kibanamachine commented Mar 3, 2023

elizabetdev left a comment

kibanamachine commented Mar 6, 2023

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

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

Conversation

cee-chen commented Mar 3, 2023 • edited Loading

Summary

Before

After

QA

General checklist

cee-chen Mar 3, 2023 • edited Loading

Choose a reason for hiding this comment

cee-chen Mar 3, 2023 • edited Loading

Choose a reason for hiding this comment

elizabetdev Mar 6, 2023

Choose a reason for hiding this comment

cee-chen Mar 6, 2023

Choose a reason for hiding this comment

cee-chen Mar 3, 2023

Choose a reason for hiding this comment

kibanamachine commented Mar 3, 2023

elizabetdev left a comment

Choose a reason for hiding this comment

kibanamachine commented Mar 6, 2023

cee-chen commented Mar 3, 2023 •

edited

Loading

cee-chen Mar 3, 2023 •

edited

Loading

cee-chen Mar 3, 2023 •

edited

Loading