[EuiSuperSelect] Adding generated IDs to create accessible label and description by 1Copenut · Pull Request #5364 · elastic/eui

1Copenut · 2021-11-09T17:24:42Z

Summary

The EuiSuperSelect was flagging an error in axe-core where the div[role="listbox"] was missing an accessible label. I refactored it to include an accessible label and description. This improves screen reader UX by announcing a label and instructions for interacting with a keyboard. This PR closes #5292 and also closes #5367.

Generated class attribute IDs for aria-labelledby and aria-describedby in Super Select
Passing the title attribute to SR-only components in Color Palette Picker
Updated snapshot tests
Adding CHANGELOG entry to main

Probably non-breaking

I thought about adding the breaking change tag, but feel we're safe making this change for a couple of reasons:

The screenReaderId prop is passed down from a class attribute that will always exist
EuiSuperSelect is self-contained except for use in EuiColorPalettePicker, which was updated to include the new prop.
EuiColorPalettePicker ended up needing a title attribute passed to child components so the <button> that's rendered by EuiSuperSelectControl always has an accessible label.

Screen reader testing

Tested with VoiceOver + Safari on MacOS Big Sur for a significant amount of time. Buttons are announcing their accessible label every time, and the listbox is announcing the instructions for use, and whether an option is "selected" or "text", as before. The selected option is announcing correctly when the listbox is closed, and VO identifies the button as having child options.

Checklist

* Generated class attributes for aria-labelledby and aria-describedby * Updated snapshot tests * Adding CHANGELOG entry to main

src/components/form/super_select/super_select.tsx

1Copenut

Added one explanatory comment for reasoning why I removed an attribute.

kibanamachine · 2021-11-09T17:55:57Z

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

kibanamachine · 2021-11-09T18:34:37Z

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

chandlerprall

Thanks for tackling this!

Two AXE errors from CI:

11:33:54 Errors on tp://localhost:9999/#/forms/compressed-forms
11:38:10 [button-name]: Ensures buttons have discernible text
11:38:10   Help: https://dequeuniversity.com/rules/axe/4.1/button-name?application=axe-puppeteer
11:38:10   Elements:
11:38:10     - .euiSuperSelectControl
11:38:10 Errors on tp://localhost:9999/#/elastic-charts/creating-charts
11:39:15 [button-name]: Ensures buttons have discernible text
11:39:15   Help: https://dequeuniversity.com/rules/axe/4.1/button-name?application=axe-puppeteer
11:39:15   Elements:
11:39:15     - .euiSuperSelectControl
11:39:15 2 accessibility errors

CHANGELOG.md

src/components/color_picker/color_palette_picker/color_palette_picker.tsx

src/components/form/super_select/super_select.tsx

* Removed `screenReaderId` from parent, passed directly to child component * Added screen reader text to the Color Palette Picker button * Updated snapshot tests

kibanamachine · 2021-11-10T04:17:10Z

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

CHANGELOG.md

src/components/color_picker/color_palette_picker/color_palette_picker.tsx

* Refactored to add `aria-hidden` to color bars * Added screen reader only text titles * Fixes an axe-core issue with buttons needing discernible text in SuperSelect * Added issue #5367 to CHANGELOG

1Copenut

Last changes finished and tested. PR update incoming shortly.

src/components/color_picker/color_palette_picker/color_palette_picker.tsx

1Copenut

The scope expanded a bit because of Color Palette Picker. I found a way to announce the Super Select listbox options consistently, and always have an accessible label for the button when the listbox is closed.

src/components/color_picker/color_palette_display/color_palette_display_gradient.tsx

1Copenut · 2021-11-10T18:57:16Z

src/components/color_picker/color_palette_display/color_palette_display_fixed.tsx

+        <span>{title}</span>
+      </EuiScreenReaderOnly>
+      <span
+        aria-hidden="true"


This aria-hidden="true" is to ensure the color blocks are ignored by screen readers, and the only accessible text for options is the title string.

kibanamachine · 2021-11-10T19:32:11Z

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

1Copenut · 2021-11-10T20:57:14Z

@cchaos Added requested comment to both components.

kibanamachine · 2021-11-10T21:20:57Z

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

1Copenut · 2021-11-10T21:27:00Z

jenkins test this

kibanamachine · 2021-11-10T21:52:06Z

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

chandlerprall

A changelog note I'd missed earlier, + a small code simplification

CHANGELOG.md

src/components/color_picker/color_palette_display/color_palette_display.tsx

kibanamachine · 2021-11-10T22:51:52Z

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

src/components/color_picker/color_palette_display/color_palette_display_fixed.tsx

src/components/color_picker/color_palette_picker/color_palette_picker.tsx

src/components/form/super_select/super_select_control.tsx

kibanamachine · 2021-11-15T18:29:48Z

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

cchaos

💟 All the a11y PR's!! LGTM, mostly as a code check, I didn't run through a screen-reader to confirm.

1Copenut · 2021-11-15T19:35:04Z

💟 All the a11y PR's!! LGTM, mostly as a code check, I didn't run through a screen-reader to confirm.

@cchaos I ran it through VoiceOver on MacOS. I'll pass a preview URL to Zuhair and see if he's got time to test with NVDA and JAWS for coverage.

chandlerprall

Fantastic, and with such a clean change set! This LGTM, tested functionality out in the PR's canary docs

breehall

This looks good! I pulled the branch and tested with Voiceover (macOS BigSur). Awesome job!

zuhairmahd · 2021-11-17T14:27:36Z

@cchaos and myself went through the URLS with JAWS and NVDA. Both screen readers behaved as expected but we note the following minor issues:

The describedby text was not reading in JAWS but was reading in NVDA
The describedby label tries to be helpful by informing the user of the number of items in the list, but fails to take into account disabled items.

@1Copenut will take out the number of items from the describedby , after which this should be complete. I will research why JAWS is not reading, and will open a future enhancement if a fix is needed on our end.

cchaos · 2021-11-17T14:35:51Z

I did? Was I sleep-walking? 🤣

zuhairmahd · 2021-11-17T14:50:24Z

I did? Was I sleep-walking? 🤣

Thought you did! Maybe I was falling asleep! :-)

By the way, no need to do anything with JAWS since the default verbosity is set to ignore describedby labels.

1Copenut · 2021-11-17T15:29:19Z

Great find @zuhairmahd. I'm surprised the default verbosity ignores describedby labels, but knowing this will help inform future decisions. I wonder if we should start a corpus of defaults, unique behaviors, et al. ?

kibanamachine · 2021-11-17T15:49:11Z

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

Adding generated IDs to create accessible label and description

c55e713

* Generated class attributes for aria-labelledby and aria-describedby * Updated snapshot tests * Adding CHANGELOG entry to main

1Copenut added accessibility accessibility - automated labels Nov 9, 2021

1Copenut requested review from breehall, cee-chen, chandlerprall and thompsongl November 9, 2021 17:24

1Copenut self-assigned this Nov 9, 2021

1Copenut commented Nov 9, 2021

View reviewed changes

src/components/form/super_select/super_select.tsx Show resolved Hide resolved

1Copenut commented Nov 9, 2021

View reviewed changes

Fixed CHANGELOG merge conflict.

5672587

chandlerprall suggested changes Nov 9, 2021

View reviewed changes

cchaos changed the title ~~Adding generated IDs to create accessible label and description~~ [EuiSuperSelect] Adding generated IDs to create accessible label and description Nov 9, 2021

1Copenut added 2 commits November 9, 2021 21:37

Refactoring props for cleaner implementation

bfe5fa6

* Removed `screenReaderId` from parent, passed directly to child component * Added screen reader text to the Color Palette Picker button * Updated snapshot tests

Removing unnecessary prop from EuiColorPalettePicker

e0de6db

chandlerprall suggested changes Nov 10, 2021

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

src/components/color_picker/color_palette_picker/color_palette_picker.tsx Outdated Show resolved Hide resolved

Adding screen reader only titles for Color Palette Picker

356a986

* Refactored to add `aria-hidden` to color bars * Added screen reader only text titles * Fixes an axe-core issue with buttons needing discernible text in SuperSelect * Added issue #5367 to CHANGELOG

1Copenut commented Nov 10, 2021

View reviewed changes

src/components/color_picker/color_palette_picker/color_palette_picker.tsx Outdated Show resolved Hide resolved

Removing extra ternary logic in Color Palette Picker

0bb616b

1Copenut commented Nov 10, 2021

View reviewed changes

src/components/color_picker/color_palette_picker/color_palette_picker.tsx Show resolved Hide resolved

1Copenut commented Nov 10, 2021

View reviewed changes

Editing CHANGELOG message for mis-spelling.

9924f5b

1Copenut requested a review from chandlerprall November 10, 2021 19:02

Adding inline comment to explain a11y intent

dcac44f

chandlerprall suggested changes Nov 10, 2021

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

src/components/color_picker/color_palette_display/color_palette_display.tsx Outdated Show resolved Hide resolved

Refactoring to use spread operator to pass title prop

b69ff5e

1Copenut requested a review from chandlerprall November 10, 2021 22:31

cchaos suggested changes Nov 11, 2021

View reviewed changes

Adding conditional checks to avoid empty SR-only spans.

c4b91da

1Copenut requested a review from cchaos November 15, 2021 18:01

cchaos approved these changes Nov 15, 2021

View reviewed changes

chandlerprall approved these changes Nov 15, 2021

View reviewed changes

breehall approved these changes Nov 16, 2021

View reviewed changes

Revising describedby helper text after screen reader testing.

a676423

1Copenut merged commit cf4f132 into elastic:main Nov 17, 2021

1Copenut deleted the tpierce-eui-5292 branch November 17, 2021 16:32

1Copenut pushed a commit that referenced this pull request Nov 18, 2021

Moving fixes for #5364 under main heading

1c99ab5

1Copenut mentioned this pull request Nov 19, 2021

Bumping EUI to v41.2.2 elastic/kibana#119205

Merged

Conversation

1Copenut commented Nov 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Probably non-breaking

Screen reader testing

Checklist

Uh oh!

Uh oh!

1Copenut left a comment

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Nov 9, 2021

Uh oh!

kibanamachine commented Nov 9, 2021

Uh oh!

chandlerprall left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kibanamachine commented Nov 10, 2021

Uh oh!

Uh oh!

Uh oh!

1Copenut left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

1Copenut left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

1Copenut Nov 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Nov 10, 2021

Uh oh!

1Copenut commented Nov 10, 2021

Uh oh!

kibanamachine commented Nov 10, 2021

Uh oh!

1Copenut commented Nov 10, 2021

Uh oh!

kibanamachine commented Nov 10, 2021

Uh oh!

chandlerprall left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kibanamachine commented Nov 10, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kibanamachine commented Nov 15, 2021

Uh oh!

cchaos left a comment

Choose a reason for hiding this comment

Uh oh!

1Copenut commented Nov 15, 2021

Uh oh!

chandlerprall left a comment

Choose a reason for hiding this comment

Uh oh!

breehall left a comment

Choose a reason for hiding this comment

Uh oh!

zuhairmahd commented Nov 17, 2021

Uh oh!

cchaos commented Nov 17, 2021

Uh oh!

zuhairmahd commented Nov 17, 2021

Uh oh!

1Copenut commented Nov 17, 2021

1Copenut commented Nov 9, 2021 •

edited

Loading

1Copenut Nov 10, 2021 •

edited

Loading