[EuiSuperSelect] Fix various focus and accessibility issues: The Sequel, Part Two #7650

cee-chen · 2024-04-04T23:47:15Z

Summary

The year was 2021, and a younger and much more foolish Cee thought they were squashing bugs and taking names in #5097.

But as it turns out, EuiSuperSelect is kind of an a11y tire fire and needed much more than that!

Fixes in this PR:

EuiSuperSelect now announces parent EuiFormRow labels, the same way an EuiSelect component would (thank you @cleydyr for pointing this out to us!)
EuiSuperSelect no longer strands keyboard focus on popover close 😬
EuiSuperSelect's popover can now also be opened on Space keypress (same as EuiSelect)
EuiSuperSelect's options can now be selected via Tab keypress (same as EuiSelect)
EuiSuperSelect no longer cycles through its options when pressing arrow up or arrow down at the top or bottom of the options list (same as EuiSelect)
EuiSuperSelect now correctly keyboard navigates up/down past disabled options in the middle of the options list (we didn't notice this bug in our docs examples previously because the disabled options were at the start or end of the options list 🤦)
Various regression tests have been added for all of the above.

QA

Go to https://eui.elastic.co/pr_7650/#/forms/super-select
Confirm that the first demo correctly announces the form row label alongside the currently selected option, the same way that a native <select> would
Confirm you can use Space to open the dropdown list and Tab to select an option
Confirm that your keyboard focus doesn't get stranded on dropdown close

General checklist

Browser QA
- Checked in Chrome, Safari, Edge, and Firefox
- Checked for accessibility including keyboard-only and screenreader modes
  ~~- [ ] Checked in both light and dark modes~~
  - [ ] Checked in mobile
Docs site QA - N/A
Code quality checklist
- Added or updated jest and cypress tests
Release checklist
- A changelog entry exists and is marked appropriately.
  ~~- [ ] If applicable, added the breaking change issue label (and filled out the breaking change checklist)~~
Designer checklist - N/A

…label - mimicking default `<select>` component

- requires disabling the default focus trap, since this component handles so much of its own focus manually

- open/close keyboard a11y (+ refocus class check) - tab to select, escape should not select - form row focus, which was fixed 2 ish years ago

…options` array - would previously stop / not skip past disabled options like native selects should

cee-chen · 2024-04-05T00:03:38Z

src/components/form/super_select/super_select.tsx

+      // Note: this component purposely does not cycle arrow key navigation
+      // to match native <select> elements
      if (direction === ShiftDirection.BACK) {
-        targetElementIndex =
-          currentIndex === 0 ? this.itemNodes.length - 1 : currentIndex - 1;
+        targetElementIndex = currentIndex - 1;
      } else {
-        targetElementIndex =
-          currentIndex === this.itemNodes.length - 1 ? 0 : currentIndex + 1;


Open to feedback on this opinionated change - obviously our native <select> component doesn't support cycling/looping between the list of options, but our EuiComboBox and EuiSelectable components do.

I leaned towards removing the behavior so that EuiSuperSelect more closely matches a native <select> (but with JSX rendering) and also to reduce the needed logic within the component, but feel free to shout if you feel strongly otherwise!

I think this is a reasonable change if we want it to be closer related to EuiSelect 👍
And in doubt we can still iterate on it in the future if the need arises for cycling the options.

+1 to @mgadewoll on this item.

kibanamachine · 2024-04-05T00:16:57Z

Preview staging links for this PR:

Docs site: https://eui.elastic.co/pr_7650/
Storybook: https://eui.elastic.co/pr_7650/storybook

elasticmachine · 2024-04-05T00:16:59Z

💚 Build Succeeded

Buildkite Build
Commit: 7d64171

cc @cee-chen

mgadewoll

🚢 🐈‍⬛ Awesome changes to ensure a more accessible component! 🎉

1Copenut

Summary

One change request for you @cee-chen.

In testing I noticed some unpredictability with Safari + VO on MacOS Ventura. This wasn't a regression from the prior production code, just a flaky handling of focus. VO acts like it's re-rendering and recycles itself. I did not experience this with any other pairing.

Screen reader test pairings

MacOS + Safari + VO
Win10 + Firefox + NVDA
Win10 + Chrome + NVDA
Win10 + Chrome + JAWS
iOS + Safari + VO

1Copenut · 2024-04-05T15:58:23Z

src/components/form/super_select/super_select.tsx

+      // Note: this component purposely does not cycle arrow key navigation
+      // to match native <select> elements
      if (direction === ShiftDirection.BACK) {
-        targetElementIndex =
-          currentIndex === 0 ? this.itemNodes.length - 1 : currentIndex - 1;
+        targetElementIndex = currentIndex - 1;
      } else {
-        targetElementIndex =
-          currentIndex === this.itemNodes.length - 1 ? 0 : currentIndex + 1;


+1 to @mgadewoll on this item.

1Copenut · 2024-04-05T17:07:10Z

src/components/form/super_select/super_select.tsx

+    // Refocus back to the toggling control button on popover close
+    requestAnimationFrame(() => {
+      this.controlButtonRef.current?.focus();
+    });
+


Safari + VO is flaky on this function. It acts like there's a full refresh happening, because the introductory message "Welcome to VoiceOver" is being announced. I have a feeling this might be another event handler confounding the focus behavior. I did not witness this behavior with NVDA, JAWS or iOS Safari + VoiceOver, just the desktop.

This doesn't introduce regression that I can say definitively, but we'll want to keep an ear for feedback from the community.

That's super weird, I didn't experience that during my testing (not to be the "it works on my machine" developer 🙈). Just to clarify, you're OK with leaving this for now and keeping an eye on feedback to see if we need to fix it later?

Yes I'm okay leaving as is, and listening for feedback. I had a thought my phantom issues could be Chrome being open in the background and causing just enough CPU load to adversely affect VO.

src/components/form/super_select/super_select.tsx

1Copenut · 2024-04-05T17:11:54Z

src/components/form/super_select/super_select_control.tsx

+
+  const buttonId = hasFormLabel ? `${id}-button` : undefined;
+  const ariaLabelledBy = hasFormLabel
+    ? `${buttonId} ${formLabelId}`


Is it possible to reliably have the form label first, then the button label when we're building the aria-labelledby attribute? I think it makes more sense to hear "Status, Minor" because that feels like the key: value relationship order.

If it can be swapped, let's go for it.

I kept it in this order to match how native <select>s work (at least for Mac+VO) - see https://eui.elastic.co/#/forms/form-controls#select.

Is the order different on Windows/JAWS/NVDA etc? If so I'm down to swap it since those are more used screen readers. What I'm going for is 1:1 screen reader experience with EuiSelect.

That's good enough reason for me. I'm much for consistency.

1Copenut

I responded in thread to two questions, I have no issue approving as is.

cee-chen · 2024-04-05T18:32:33Z

Thanks y'all!!

`v93.5.2` ⏩ `v93.6.0` --- ## [`v93.6.0`](https://github.com/elastic/eui/releases/v93.6.0) - Updated `EuiBreadcrumb` styles to improve visual distinction of clickable breadcrumbs ([#7615](elastic/eui#7615)) **Deprecations** - Deprecated `color` prop on `EuiBreadcrumb` ([#7615](elastic/eui#7615)) **Bug fixes** - Fixed `EuiComboBox` to correctly select full matches within groups via the `Enter` key ([#7658](elastic/eui#7658)) **Accessibility** - Updated `EuiHeaderBreadcrumb` styles to ensure min. required color contrast ([#7643](elastic/eui#7643)) - `EuiSuperSelect` now correctly reads out parent `EuiFormRow` labels to screen readers ([#7650](elastic/eui#7650)) - `EuiSuperSelect` now more closely mimics native `<select>` behavior in its keyboard behavior and navigation ([#7650](elastic/eui#7650)) - `EuiSuperSelect` no longer strands keyboard focus on close ([#7650](elastic/eui#7650)) - `EuiSuperSelect` now correctly allows keyboard navigating past disabled options in the middle of the options list ([#7650](elastic/eui#7650))

…test (#180457) closes #180424 The SuperSelect EUI component started showing an extra `<span/>` when it displayed the selected value ([change](https://github.com/elastic/eui/blob/d08f49cf1f0ee6a31301ade914a2d06143d4a3d4/src/components/form/super_select/super_select_control.tsx#L195-L201) from elastic/eui#7650) . This caused a problem in a test that checks the agent policy name strictly. I changed the test to see if the text `contains` the string instead of `has` it. I checked the flow and found it doesn't affect the data sent to the API. Also, the `euiScreenReaderOnly` property hides this extra span in the regular UI so it is not visible to users. ![Screenshot 2024-04-10 at 12 47 31](https://github.com/elastic/kibana/assets/29123534/b7ff5b0e-5792-4bab-ac3a-61ba622c2c07)

cee-chen added 7 commits April 4, 2024 11:51

Update usage within EuiFormRow to read out both option followed by …

ba7ad61

…label - mimicking default `<select>` component

Add hidden , to force SRs to pause between current option and label

610a85b

Mimic more native <select> keyboard behavior

8964b48

Fix focus stranding issues on popover close

eceb751

- requires disabling the default focus trap, since this component handles so much of its own focus manually

Add Cypress E2E regression tests for several implemented fixes

27abc92

- open/close keyboard a11y (+ refocus class check) - tab to select, escape should not select - form row focus, which was fixed 2 ish years ago

[opinionated] Remove option keyboard cycling to match native selects

091e4b1

Fix keyboard navigation past disabled elements in the middle of the `…

60bf338

…options` array - would previously stop / not skip past disabled options like native selects should

cee-chen added the accessibility label Apr 4, 2024

cee-chen added 2 commits April 4, 2024 16:49

first

a21a655

changelog

7d64171

cee-chen requested a review from 1Copenut April 5, 2024 00:01

cee-chen marked this pull request as ready for review April 5, 2024 00:01

cee-chen requested a review from a team as a code owner April 5, 2024 00:01

cee-chen commented Apr 5, 2024

View reviewed changes

cee-chen mentioned this pull request Apr 5, 2024

[EuiSuperSelect] Refactor to use EuiSelectable #2708

Closed

cee-chen self-assigned this Apr 5, 2024

mgadewoll approved these changes Apr 5, 2024

View reviewed changes

1Copenut requested changes Apr 5, 2024

View reviewed changes

1Copenut approved these changes Apr 5, 2024

View reviewed changes

cee-chen merged commit d08f49c into elastic:main Apr 5, 2024
8 checks passed

cee-chen deleted the super-select/a11y-fixes branch April 5, 2024 18:32

cee-chen mentioned this pull request Apr 8, 2024

Upgrade EUI to v93.6.0 elastic/kibana#180316

Merged

szwarckonrad mentioned this pull request Apr 10, 2024

[EDR Workflows][Osquery][E2E] Unskip osquery packs_integration·cy·ts test elastic/kibana#180457

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[EuiSuperSelect] Fix various focus and accessibility issues: The Sequel, Part Two #7650

[EuiSuperSelect] Fix various focus and accessibility issues: The Sequel, Part Two #7650

cee-chen commented Apr 4, 2024 •

edited

cee-chen Apr 5, 2024

mgadewoll Apr 5, 2024

1Copenut Apr 5, 2024

kibanamachine commented Apr 5, 2024

elasticmachine commented Apr 5, 2024

mgadewoll left a comment

1Copenut left a comment

1Copenut Apr 5, 2024

1Copenut Apr 5, 2024

cee-chen Apr 5, 2024

1Copenut Apr 5, 2024

1Copenut Apr 5, 2024

cee-chen Apr 5, 2024 •

edited

1Copenut Apr 5, 2024

1Copenut left a comment

cee-chen commented Apr 5, 2024

[EuiSuperSelect] Fix various focus and accessibility issues: The Sequel, Part Two #7650

[EuiSuperSelect] Fix various focus and accessibility issues: The Sequel, Part Two #7650

Conversation

cee-chen commented Apr 4, 2024 • edited

Summary

QA

General checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibanamachine commented Apr 5, 2024

elasticmachine commented Apr 5, 2024

💚 Build Succeeded

mgadewoll left a comment

Choose a reason for hiding this comment

1Copenut left a comment

Choose a reason for hiding this comment

Summary

Screen reader test pairings

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cee-chen Apr 5, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1Copenut left a comment

Choose a reason for hiding this comment

cee-chen commented Apr 5, 2024

cee-chen commented Apr 4, 2024 •

edited

cee-chen Apr 5, 2024 •

edited