[EuiCollapsibleNavBeta] Improve collapsed button icon screen reader UX #7055

cee-chen · 2023-08-07T21:53:16Z

Summary

Part of [EuiCollapsibleNavBeta] A11y improvements #7041.
⚠️ This PR does not resolve all a11y issues with EuiCollapsibleNavBeta, just one of them (see linked issue for full list).

This PR:

Makes the typing of nav item titles stricter - they now must be strings and not ReactNodes (in order to support aria-label/title type attributes)
Sets an aria-label on all collapsed/docked button icon nav items (example below)

which resolves the many warnings in tests as well as devtools:
However, adding an aria-label to button icons with a tooltip wrapped around them leads to duplicated content, because the aria-label and aria-describedby contain the same exact content. The solution is to modify EuiToolTip to allow configuring disabling the aria-describedby logic if necessary/if tooltip content is duplicative.
- We went back on this decision after more SR testing and per the below GitHub discussion. This PR contains several EuiToolTip cleanup items instead.

QA

Do not check out this PR yet - on main, run yarn storybook
Go to http://localhost:6006/?path=/story/euicollapsiblenavbeta--kibana-example&args=initialIsCollapsed:true (NOTE: Storybook is being odd about not setting initialIsCollapsed properly, if you refresh the page and the button icons aren't present, simply click the collapse nav button in the top left corner)
Turn on VoiceOver and tab to any of the button icons
The tooltip content should be read out after a pause, because it lives in aria-describedby:

Now gh pr checkout TODO (yarn storybook should update automatically)
yarn storybook
Go to http://localhost:6006/?path=/story/euicollapsiblenavbeta--kibana-example&args=initialIsCollapsed:true again (or refresh the page and re-collapse the nav)
Tab to the button icons, and confirm they now read out the label of the button immediately, and do not contain duplicate aria-describedby text:

General checklist

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

- instead of accepting any JSX/react node, *only* accept strings - This was checked with Tim Sullivan and confirmed to match Kibana usage

+ DRY out EuiToolTipAnchorProps types + fix Enzyme test causing false negatives in other tests

…bedby` - this prevents repeat/duplicate information, since `aria-label` and `aria-describedby` are the exact same content

…cribedby`s

- `id` is already destructured, no need to use `this.state`

- Remove className maps - they're not actually being used and we should be using more modern `as const`/`typeof` instead of `keysOf`

cee-chen · 2023-08-07T22:06:46Z

src/components/collapsible_nav_beta/collapsible_nav_item/collapsed/collapsed_nav_button.tsx

@@ -67,6 +67,7 @@ export const EuiCollapsedNavButton: FunctionComponent<
  return (
    <EuiToolTip
      content={title}
+      setAriaDescribedBy={false} // `title` content already set in `aria-label` below


@1Copenut I could use your help testing this and verifying that it's actually doing anything. I originally thought it was necessary to unset aria-describedby if aria-label contained the same text, but now I'm not so sure.

In Safari+VO, VO appears to intelligently detect if they're the same and does not read out the aria-describedby text

In FF+VO, VO reads out the label twice even if no aria-describedby is present, which I think is a bug with FF and should be ignored

I have no idea what JAWS/NVDA do and I'd really appreciate your help figuring that out.

If it turns out that all screen readers successfully de-duplicate labels and descriptions that contain the same content, I'll likely go ahead and revert the part of this PR that adds the setAriaDescribedBy prop entirely.

@cee-chen Can do. I'll get the JAWS license squared away and test it tomorrow morning if that works.

@cee-chen I've started testing this afternoon. I won't be able to test JAWS and NVDA until we have a preview in Buildkite unless I get really lucky and get code compiled and running on the Windows machine.

Doh, I forgot about that, apologies. Can you use this proof-of-concept CodeSandbox in the interim?

https://codesandbox.io/s/strange-surf-xrxfd7?file=/demo.js

The first button has the same aria-label and aria-describedby, the second button has a separate label/description.

@cee-chen Yes, that'll work great! I can test this immediately and provide feedback.

I like this option to allow consumers to disable the aria-describedby. Your comments inline are an excellent descriptor of when users might want to do that. Because we have the aria-label for semantic labeling we shouldn't run afoul of automated checks, and the default experience is be to have aria-describedby enabled.

Sorry, I don't think I made the original intent clear.

I was just asking you to test what happens when a button has both an aria-label and an aria-describedby with the same content. Do screen readers automatically de-duplicate that content, or do they read out both?

The second button was just there as a control for how screen readers behave without duplicated content. It's not an option for the beta collapsible nav.

Okay, took a second run through and here's the results by pairing. I deleted my prior comment because it didn't apply to how I needed to be testing.

It's awesome that screen readers recognize duplicates for a fairly high number of pairings. NVDA on Chrome/Edge do repeat the content, but none of the three I tested on JAWS read back the description if it matched the label.

NVDA

Chrome + NVDA repeats the description whether it's the same or different than the label:

Title button, title

Title button, description

Edge + NVDA repeats the description whether it's the same or different than the label:

Title button, title

Title button, description

Firefox + NVDA does not repeat the description if it matches the label:

Title button

Title button, description

JAWS

Chrome + JAWS 23 does not repeat the description if it matches the label:

Title button. To activate press Enter.

Title button. To activate press Enter. Description.

Edge + JAWS 23 does not repeat the description if it matches the label:

Title button. To activate press Enter.

Title button. To activate press Enter. Description.

Firefox + JAWS 23 does not repeat the description if it matches the label:

Title button. To activate press Enter.

Title button. To activate press Enter. Description. Description.

Here's the results by pairing for second run, using the following test criteria. All tests were run using the same criteria. Start screen reader first, then start browser and navigate to the CodeSandbox page.

I want to test how a button without any aria-describedby compares to a button with the same aria-label/describedby

Button 1 has aria-label and aria-describedby.
Button 2 has aria-label only.

NVDA

Chrome + NVDA

Button 1: Title button. Title.

Button 2: Title button.

Edge + NVDA

Button 1: Title button. Title.

Button 2: Title button.

Firefox + NVDA

Button 1: Title button

Button 2: Title button

JAWS

Chrome + JAWS

Button 1: Title button. To activate press Enter.

Button 2: Title button. To activate press Enter.

Edge + JAWS

Button 1: Title button. To activate press Enter.

Button 2: Title button. To activate press Enter.

Firefox + JAWS

Button 1: Title button. To activate press Enter.

Button 2: Title button. To activate press Enter.

Recap of our findings and discussions:

All screen readers (i.e., JAWS & VO) except for NVDA on Chrome/Edge de-duplicate repeat content between aria-label and aria-describedby

(worth noting: WebAim's latest screen reader survey lists a 16% user base of NVDA+Chrome)

However, since this only affects 1 in 3 screen readers, we've decided to revert the new setAriaDescribedBy prop in this PR. The potential for degraded UX by allowing the aria-describedby to be disabled is greater than the gain of not hearing a snippet of duplicated text.

kibanamachine · 2023-08-07T22:21:51Z

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

cee-chen · 2023-08-08T15:18:28Z

Going to hold off on fixing the failing downstream snapshot tests until we know for sure if removing the aria-describedby is necessary. If it's not, then I'll remove the prop and the snapshot changes won't be needed

1Copenut

These changes look good @cee-chen. The screen readers tested well with your proof of concept code. I logged the pairings and speech read back for each. We won't completely eliminate duplicated text, but the experience is really solid.

I don't have any changes. Will mark this approved after a quick run-through on a build preview.

1Copenut · 2023-08-10T23:33:21Z

src/components/collapsible_nav_beta/collapsible_nav_item/collapsed/collapsed_nav_button.tsx

@@ -67,6 +67,7 @@ export const EuiCollapsedNavButton: FunctionComponent<
  return (
    <EuiToolTip
      content={title}
+      setAriaDescribedBy={false} // `title` content already set in `aria-label` below


I like this option to allow consumers to disable the aria-describedby. Your comments inline are an excellent descriptor of when users might want to do that. Because we have the aria-label for semantic labeling we shouldn't run afoul of automated checks, and the default experience is be to have aria-describedby enabled.

1Copenut · 2023-08-11T00:10:48Z

src/components/collapsible_nav_beta/collapsible_nav_item/collapsed/collapsed_nav_button.tsx

@@ -67,6 +67,7 @@ export const EuiCollapsedNavButton: FunctionComponent<
  return (
    <EuiToolTip
      content={title}
+      setAriaDescribedBy={false} // `title` content already set in `aria-label` below


Okay, took a second run through and here's the results by pairing. I deleted my prior comment because it didn't apply to how I needed to be testing.

It's awesome that screen readers recognize duplicates for a fairly high number of pairings. NVDA on Chrome/Edge do repeat the content, but none of the three I tested on JAWS read back the description if it matched the label.

NVDA

Chrome + NVDA repeats the description whether it's the same or different than the label:

Title button, title

Title button, description

Edge + NVDA repeats the description whether it's the same or different than the label:

Title button, title

Title button, description

Firefox + NVDA does not repeat the description if it matches the label:

Title button

Title button, description

JAWS

Chrome + JAWS 23 does not repeat the description if it matches the label:

Title button. To activate press Enter.

Title button. To activate press Enter. Description.

Edge + JAWS 23 does not repeat the description if it matches the label:

Title button. To activate press Enter.

Title button. To activate press Enter. Description.

Firefox + JAWS 23 does not repeat the description if it matches the label:

Title button. To activate press Enter.

Title button. To activate press Enter. Description. Description.

src/components/collapsible_nav_beta/collapsible_nav_item/collapsible_nav_item.tsx

1Copenut · 2023-08-11T20:02:27Z

src/components/tool_tip/tool_tip.test.tsx

+
+      expect(
+        getByTestSubject('anchor').getAttribute('aria-describedby')
+      ).toEqual('toolTipId customId');


I like this order of concatenation. Screen readers key off IDs in order, so it'll always read our described by text first, then additional.

1Copenut

👍 High five Cee. The VoiceOver tests are working great, announcing buttons as we expected. Axe is reporting 0 errors. This PR is good to 🚢

kibanamachine · 2023-08-11T20:12:00Z

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

elasticmachine · 2023-08-11T20:30:34Z

💚 Build Succeeded

Buildkite Build
Commit: ee868f5

History

💔 Build #245 failed 4ed760b

`v86.0.0`⏩`v87.1.0` ⚠️ The biggest set of type changes in this PR come from the breaking change that makes `pageSize` and `pageSizeOptions` now optional props for `EuiBasicTable.pagination`, `EuiInMemoryTable.pagination` and `EuiDataGrid.pagination`. This caused several other components that were cloning EUI's pagination type to start throwing type warnings about `pageSize` being optional. Where I came across these errors, I modified the extended types to require `pageSize`. These types and their usages may end up changing again in any case once the Shared UX team looks into #56406. --- ## [`87.1.0`](https://github.com/elastic/eui/tree/v87.1.0) - Updated the underlying library powering `EuiAutoSizer`. This primarily affects typing around the `disableHeight` and `disableWidth` props ([#6798](elastic/eui#6798)) - Added new `EuiAutoSize`, `EuiAutoSizeHorizontal`, and `EuiAutoSizeVertical` types to support `EuiAutoSizer`'s now-stricter typing ([#6798](elastic/eui#6798)) - Updated `EuiDatePickerRange` to support `compressed` display ([#7058](elastic/eui#7058)) - Updated `EuiFlyoutBody` with a new `scrollableTabIndex` prop ([#7061](elastic/eui#7061)) - Added a new `panelMinWidth` prop to `EuiInputPopover` ([#7071](elastic/eui#7071)) - Added a new `inputPopoverProps` prop for `EuiRange`s and `EuiDualRange`s with `showInput="inputWithPopover"` set ([#7082](elastic/eui#7082)) **Bug fixes** - Fixed `EuiToolTip` overriding instead of merging its `aria-describedby` tooltip ID with any existing `aria-describedby`s ([#7055](elastic/eui#7055)) - Fixed `EuiSuperDatePicker`'s `compressed` display ([#7058](elastic/eui#7058)) - Fixed `EuiAccordion` to remove tabbable children from sequential keyboard navigation when the accordion is closed ([#7064](elastic/eui#7064)) - Fixed `EuiFlyout`s to accept custom `aria-describedby` IDs ([#7065](elastic/eui#7065)) **Accessibility** - Removed the default `dialog` role and `tabIndex` from push `EuiFlyout`s. Push flyouts, compared to overlay flyouts, require manual accessibility management. ([#7065](elastic/eui#7065)) ## [`87.0.0`](https://github.com/elastic/eui/tree/v87.0.0) - Added beta `componentDefaults` prop to `EuiProvider`, which will allow configuring certain default props globally. This list of components and defaults is still under consideration. ([#6923](elastic/eui#6923)) - `EuiPortal`'s `insert` prop can now be configured globally via `EuiProvider.componentDefaults` ([#6941](elastic/eui#6941)) - `EuiFocusTrap`'s `crossFrame` and `gapMode` props can now be configured globally via `EuiProvider.componentDefaults` ([#6942](elastic/eui#6942)) - `EuiTablePagination`'s `itemsPerPage`, `itemsPerPageOptions`, and `showPerPageOptions` props can now be configured globally via `EuiProvider.componentDefaults` ([#6951](elastic/eui#6951)) - `EuiBasicTable`, `EuiInMemoryTable`, and `EuiDataGrid` now allow `pagination.pageSize` to be undefined. If undefined, `pageSize` defaults to `EuiTablePagination`'s `itemsPerPage` component default. ([#6993](elastic/eui#6993)) - `EuiBasicTable`, `EuiInMemoryTable`, and `EuiDataGrid`'s `pagination.pageSizeOptions` will now fall back to `EuiTablePagination`'s `itemsPerPageOptions` component default. ([#6993](elastic/eui#6993)) - Updated `EuiHeaderLinks`'s `gutterSize` spacings ([#7005](elastic/eui#7005)) - Updated `EuiHeaderAlert`'s stacking styles ([#7005](elastic/eui#7005)) - Added `toolTipProps` to `EuiListGroupItem` that allows customizing item tooltips. ([#7018](elastic/eui#7018)) - Updated `EuiBreadcrumbs` to support breadcrumbs that toggle popovers via `popoverContent` and `popoverProps` ([#7031](elastic/eui#7031)) - Improved the contrast ratio of disabled titles within `EuiSteps` and `EuiStepsHorizontal` to meet WCAG AA guidelines. ([#7032](elastic/eui#7032)) - Updated `EuiSteps` and `EuiStepsHorizontal` to highlight and provide a more clear visual indication of the current step ([#7048](elastic/eui#7048)) **Bug fixes** - Single uses of `<EuiHeaderSectionItem side="right" />` now align right as expected without needing a previous `side="left"` sibling. ([#7005](elastic/eui#7005)) - `EuiPageTemplate` now correctly displays `panelled={true}` ([#7044](elastic/eui#7044)) **Breaking changes** - `EuiTablePagination`'s default `itemsPerPage` is now `10` (was previously `50`). This can be configured through `EuiProvider.componentDefaults`. ([#6993](elastic/eui#6993)) - `EuiTablePagination`'s default `itemsPerPageOptions` is now `[10, 25, 50]` (was previously `[10, 20, 50, 100]`). This can be configured through `EuiProvider.componentDefaults`. ([#6993](elastic/eui#6993)) - Removed `border` prop from `EuiHeaderSectionItem` (unused since Amsterdam theme) ([#7005](elastic/eui#7005)) - Removed `borders` object configuration from `EuiHeader.sections` ([#7005](elastic/eui#7005)) **CSS-in-JS conversions** - Converted `EuiHeaderAlert` to Emotion; Removed unused `.euiHeaderAlert__dismiss` CSS ([#7005](elastic/eui#7005)) - Converted `EuiHeaderSection`, `EuiHeaderSectionItem`, and `EuiHeaderSectionItemButton` to Emotion ([#7005](elastic/eui#7005)) - Converted `EuiHeaderLinks` and `EuiHeaderLink` to Emotion; Removed `$euiHeaderLinksGutterSizes` Sass variables ([#7005](elastic/eui#7005)) - Removed `$euiHeaderBackgroundColor` Sass variable; use `$euiColorEmptyShade` instead ([#7005](elastic/eui#7005)) - Removed `$euiHeaderChildSize` Sass variable; use `$euiSizeXXL` instead ([#7005](elastic/eui#7005)) --------- Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Patryk Kopyciński <contact@patrykkopycinski.com>

cee-chen added 8 commits August 7, 2023 14:06

[breaking change] Make title prop stricter

6f58305

- instead of accepting any JSX/react node, *only* accept strings - This was checked with Tim Sullivan and confirmed to match Kibana usage

Set aria-labels for collapsed desktop button icons

33bd496

[EuiToolTip] Add setAriaDescribedBy prop

e9bbbcb

+ DRY out EuiToolTipAnchorProps types + fix Enzyme test causing false negatives in other tests

Update tooltip-wrapped collapsed button icons to not set `aria-descri…

9bb4ff0

…bedby` - this prevents repeat/duplicate information, since `aria-label` and `aria-describedby` are the exact same content

[EuiToolTip] Fix consumers not being able to pass in custom `aria-des…

ffdda86

…cribedby`s

[EuiToolTip] Misc var cleanup

4d204ee

- `id` is already destructured, no need to use `this.state`

[EuiToolTip] Misc types cleanup

9ac1d5f

- Remove className maps - they're not actually being used and we should be using more modern `as const`/`typeof` instead of `keysOf`

changelog

4ed760b

cee-chen force-pushed the collapsible-nav-beta-a11y branch from ebc851e to 4ed760b Compare August 7, 2023 21:57

cee-chen requested review from a team and 1Copenut August 7, 2023 21:58

cee-chen commented Aug 7, 2023

View reviewed changes

cee-chen marked this pull request as ready for review August 7, 2023 22:07

cee-chen mentioned this pull request Aug 10, 2023

[EuiCollapsibleNavBeta] A11y improvements #7041

Closed

3 tasks

1Copenut reviewed Aug 11, 2023

View reviewed changes

[PR feedback] Remove setAriaDescribedBy prop

917751b

cee-chen changed the title ~~[EuiCollapsibleNavBeta] Improve collapsed button icon screen reader UX; [EuiToolTip] Add setAriaDescribedBy prop~~ [EuiCollapsibleNavBeta] Improve collapsed button icon screen reader UX Aug 11, 2023

cee-chen commented Aug 11, 2023

View reviewed changes

src/components/collapsible_nav_beta/collapsible_nav_item/collapsible_nav_item.tsx Outdated Show resolved Hide resolved

[Pr feedback] copy

3049c7f

cee-chen requested a review from 1Copenut August 11, 2023 19:41

Update snapshot

ee868f5

1Copenut reviewed Aug 11, 2023

View reviewed changes

1Copenut approved these changes Aug 11, 2023

View reviewed changes

cee-chen enabled auto-merge (squash) August 11, 2023 20:11

cee-chen merged commit 4b95e45 into elastic:main Aug 11, 2023

cee-chen deleted the collapsible-nav-beta-a11y branch August 11, 2023 20:30

cee-chen mentioned this pull request Aug 15, 2023

Upgrade EUI to v87.1.0 elastic/kibana#163961

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[EuiCollapsibleNavBeta] Improve collapsed button icon screen reader UX #7055

[EuiCollapsibleNavBeta] Improve collapsed button icon screen reader UX #7055

cee-chen commented Aug 7, 2023 •

edited

Loading

cee-chen Aug 7, 2023 •

edited

Loading

1Copenut Aug 7, 2023

1Copenut Aug 10, 2023

cee-chen Aug 10, 2023

1Copenut Aug 10, 2023

1Copenut Aug 10, 2023

cee-chen Aug 10, 2023 •

edited

Loading

1Copenut Aug 11, 2023

1Copenut Aug 11, 2023

cee-chen Aug 11, 2023

kibanamachine commented Aug 7, 2023

cee-chen commented Aug 8, 2023

1Copenut left a comment

1Copenut Aug 10, 2023

1Copenut Aug 11, 2023

1Copenut Aug 11, 2023

1Copenut left a comment

kibanamachine commented Aug 11, 2023

elasticmachine commented Aug 11, 2023

[EuiCollapsibleNavBeta] Improve collapsed button icon screen reader UX #7055

[EuiCollapsibleNavBeta] Improve collapsed button icon screen reader UX #7055

Conversation

cee-chen commented Aug 7, 2023 • edited Loading

Summary

QA

General checklist

cee-chen Aug 7, 2023 • edited Loading

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

Choose a reason for hiding this comment

cee-chen Aug 10, 2023 • edited Loading

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

kibanamachine commented Aug 7, 2023

cee-chen commented Aug 8, 2023

1Copenut left a 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

1Copenut left a comment

Choose a reason for hiding this comment

kibanamachine commented Aug 11, 2023

elasticmachine commented Aug 11, 2023

💚 Build Succeeded

History

cee-chen commented Aug 7, 2023 •

edited

Loading

cee-chen Aug 7, 2023 •

edited

Loading

cee-chen Aug 10, 2023 •

edited

Loading