[EuiSelectable] Fix focus ring on non-searchable listboxes #6637

cee-chen · 2023-03-07T23:11:22Z

Summary

This PR attempts to fix the odd focus ring behavior for EuiSelectables that are listboxes only (i.e., do not have the searchable prop).

Unfortunately, due to limitations of :focus-within and :focus-visible (WICG/focus-visible#151), we can only support clear focus rings for for browsers that support the :has selector (i.e. all browsers except FF, which is working on :has support).

Before (Chrome)

After (Chrome) - keyboard focus only

Before (FF)

After (FF) - both keyboard & mouse focus

QA

Go to https://eui.elastic.co/pr_6637/#/forms/selectable#the-basics
In Chrome/Safari/Edge, confirm that keyboard tabbing to the listbox shows a non-wonky-looking focus ring and allows all normal navigation/selection key behavior
In Chrome/Safari/Edge, confirm that clicking on options still works as before and does not show a focus ring

General checklist

Checked in both light and dark modes
Checked in Chrome, Safari, Edge, and Firefox
Checked for accessibility including keyboard-only and screenreader modes
A changelog entry exists and is marked appropriately

cee-chen · 2023-03-07T23:15:33Z

src/components/selectable/selectable_list/_selectable_list.scss

-  &:focus-within {
-    @include euiFocusRing;
+  // NOTE: This is currently only visible on supported browsers (all except FF)
+  // which is (unfortunately) better than always displaying the focus ring to mouse users
+  &:has(:focus-visible) {


See WICG/focus-visible#151 for more discussion - :focus-within unfortunately grabs all focus including mouse clicks, when we likely only want keyboard focus. :focus-within-visible was discarded by WICG as a spec, with :has() being the recommended way forward.

cee-chen · 2023-03-07T23:16:15Z

src/themes/amsterdam/global_styling/mixins/_states.scss

+    @if (unitless($offset)) {
+      outline-offset: #{$offset}px;
+    } @else {
+      outline-offset: #{$offset};
+    }


This fixes a bug where output would be 1pxpx if $euiFocusRingSize / 2 was passed in to the $offset param.

Also offset needs to be pretty exact here because otherwise the horizontal border between selectable list options cuts into the outline on Chrome & Edge:

cee-chen · 2023-03-07T23:20:22Z

src/themes/amsterdam/global_styling/mixins/_states.scss

+  @if ($focusVisibleSelectors) {
+    // Chrome
+    &:focus-visible {
+      outline-style: auto;
+    }

-  &:not(:focus-visible) {
-    outline: none;
+    &:not(:focus-visible) {
+      outline: none;
+    }
+  } @else {
+    outline-style: auto;


This extra $focusVisibleSelectors param/conditional rendering needed to be added for the pseudo focus ring styles to work correctly on an item that didn't actually contain focus, only focus within.

the :not(:focus-visible) in particular was what causing an outline: none and for the previous mixin include on .euiSelectableList to not work.

However, the outline-style: auto is still needed for outline styles in Safari and FF to inherit their default browser focus ring styles (Safari's focus ring looks particularly ugly w/o auto - no border-radius, etc).

kibanamachine · 2023-03-07T23:39:01Z

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

cee-chen · 2023-03-09T22:00:34Z

@daveyholler Any chance you can review this PR today? 🙏 It should hopefully be fairly quick.

daveyholler

I peeped the PR preview. This looks good!

## 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 2 commits March 7, 2023 14:45

Remove unwanted inner focus ring

7ddb172

[Optional] Fix non-working focus ring on parent .euiSelectable

c03a120

cee-chen requested a review from elizabetdev March 7, 2023 23:11

changelog

07347d9

cee-chen commented Mar 7, 2023

View reviewed changes

cee-chen requested a review from daveyholler March 9, 2023 22:00

daveyholler approved these changes Mar 9, 2023

View reviewed changes

cee-chen merged commit 319bd99 into elastic:main Mar 9, 2023

cee-chen deleted the selectable-focus branch March 9, 2023 23:13

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[EuiSelectable] Fix focus ring on non-searchable listboxes #6637

[EuiSelectable] Fix focus ring on non-searchable listboxes #6637

cee-chen commented Mar 7, 2023 •

edited

Loading

cee-chen Mar 7, 2023

cee-chen Mar 7, 2023 •

edited

Loading

cee-chen Mar 7, 2023

kibanamachine commented Mar 7, 2023

cee-chen commented Mar 9, 2023

daveyholler left a comment

[EuiSelectable] Fix focus ring on non-searchable listboxes #6637

[EuiSelectable] Fix focus ring on non-searchable listboxes #6637

Conversation

cee-chen commented Mar 7, 2023 • edited Loading

Summary

Before (Chrome)

After (Chrome) - keyboard focus only

Before (FF)

After (FF) - both keyboard & mouse focus

QA

General checklist

cee-chen Mar 7, 2023

Choose a reason for hiding this comment

cee-chen Mar 7, 2023 • edited Loading

Choose a reason for hiding this comment

cee-chen Mar 7, 2023

Choose a reason for hiding this comment

kibanamachine commented Mar 7, 2023

cee-chen commented Mar 9, 2023

daveyholler left a comment

Choose a reason for hiding this comment

cee-chen commented Mar 7, 2023 •

edited

Loading

cee-chen Mar 7, 2023 •

edited

Loading