[EuiAccordion] Add configurable `role` prop and change default role to `group` #7326

cee-chen · 2023-10-27T20:55:37Z

Summary

closes #7316 - please see the link for more discussion/reasoning as to why EUI defaults to the less SR-intrusive group role over region (TL;DR, since we cannot know what content our consumers are using in accordions, we should allow them to determine for themselves whether their accordion content is significant enough to be a landmark region).

This PR also (in a separate commit) updates the beta EuiCollapsibleNavGroup components to follow a similar role="group" and aria-labelledby markup.

QA

Open a screen reader and test:
https://eui.elastic.co/pr_7326/#/layout/accordion
https://eui.elastic.co/pr_7326/storybook/?path=/story/euicollapsiblenavbeta-group--playground

General checklist

Browser QA
- Checked for accessibility including keyboard-only and screenreader modes
Docs site QA
- Added documentation
- Props have proper autodocs (using @default if default values are missing)~ and playground toggles~
Code quality checklist
- Added or updated jest ~~and cypress tests~~
Release checklist
- A changelog entry exists and is marked appropriately.
Designer checklist - N/A

… `region`

…="group"` following the same DOM pattern as accordions, but not customizable (since the role is literally in the component name)

cee-chen · 2023-10-27T21:04:56Z

src/components/accordion/accordion.tsx

+    /**
+     * Defaults to the [group role](https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/group_role).
+     *
+     * If your accordion contains significant enough content to be a document
+     * [landmark role](https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/region_role#accessibility_concerns), consider using the `region` role instead.
+     * @default group
+     */
+    role?: HTMLAttributes<HTMLElement>['role'];


@1Copenut Do you feel like this needs its own documentation section in our Accordion docs? Or is the prop documentation sufficient?

To be honest, I could be way off here, but I strongly suspect that 90%+ of accordion use cases do not fit a 'landmark' role, and I'd rather have people under rather than overindexing on that 🤷

@cee-chen I agree with your assessment here, especially now that I've had a chance to drive it with multiple screen readers. You mentioned that you felt landmarks were designated waypoints on the page last week. I started considering landmarks like points on a map and yeah, those shouldn't be ephemeral. They should be present and predictable. Accordions—and by proxy their content—can be expanded and collapsed, so the content is not always present and is not always predictable.

Let's keep the documentation just in props for now. If folks really need to upgrade a content block's status they'll have a means to do so. But my hunch is a good content strategy would recommend making content worthy of a role="region" an always-on-screen block.

kibanamachine · 2023-10-27T22:49:32Z

Preview staging links for this PR:

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

elasticmachine · 2023-10-27T22:49:35Z

💚 Build Succeeded

Buildkite Build
Commit: bb4f0aa

History

💚 Build #926 succeeded 6e2599c

1Copenut

👍 I tested the QA links in the big three screen readers + preferred browsers. They drove really well and having the aria-labelledby attributes by default struck the right balance between general groups and overly prominent regions.

1Copenut · 2023-10-30T15:46:13Z

src/components/accordion/accordion.tsx

+    /**
+     * Defaults to the [group role](https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/group_role).
+     *
+     * If your accordion contains significant enough content to be a document
+     * [landmark role](https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/region_role#accessibility_concerns), consider using the `region` role instead.
+     * @default group
+     */
+    role?: HTMLAttributes<HTMLElement>['role'];


@cee-chen I agree with your assessment here, especially now that I've had a chance to drive it with multiple screen readers. You mentioned that you felt landmarks were designated waypoints on the page last week. I started considering landmarks like points on a map and yeah, those shouldn't be ephemeral. They should be present and predictable. Accordions—and by proxy their content—can be expanded and collapsed, so the content is not always present and is not always predictable.

Let's keep the documentation just in props for now. If folks really need to upgrade a content block's status they'll have a means to do so. But my hunch is a good content strategy would recommend making content worthy of a role="region" an always-on-screen block.

1Copenut · 2023-10-30T15:47:06Z

src/components/collapsible_nav_beta/collapsible_nav_item/collapsible_nav_group.tsx

  return (
    <div className={classes} {...cssStyles} {...rest}>
      <EuiCollapsibleNavLink
        href={href}
+        id={labelledById}


This is a solid improvement for screen readers. Hearing the button read out as an identifying reference for the content group felt good and helped frame what I would be hearing more about.

`v90.0.0`⏩`v91.0.0-backport.0` ⚠️ While this upgrade pings many teams and has a large code diff, **the majority of the changes are snapshots or tests-related** and do not touch source code, so should theoretically only need a code review and not dedicated QA. The changes in EUI that required a large swathe of these updates are: - **EuiPopover** removed an extra unnecessary `<div>` wrapper on its anchors, which affected many snapshots and a few CSS overrides, which should have been updated - **EuiButtonGroup** now renders `<button>` elements instead of `<input type="radio">` elements for single selection, which affected both snapshots and E2E tests - **EuiSuperDatePicker**'s absolute date input now requires an `Enter` keypress when parsing dates (affected E2E tests) - **EuiComboBox**, when rendered with `singleSelection={{ plainText: 'true' }}`, no longer renders a pill (i.e. text). This combobox type now behaves more like an `EuiFieldText`, where the selection is rendered via input `value` instead. This affected a high amount of E2E tests (both FTR and Cypress), both in terms of updating assertions and changing selections, but should **not** significantly affect user experience - see elastic/eui#7332 for more. --- ## [`v91.0.0-backport.0`](https://github.com/elastic/eui/tree/v91.0.0-backport.0) **This is a backport release only intended for use by Kibana.** - Added `esqlVis`, `pipeBreaks`, and `pipeNoBreaks` icon glyphs. - `EuiSelectable` now allows configurable text truncation via `listProps.truncationProps` ([#7388](elastic/eui#7388)) - `EuiTextTruncate` now supports a new `calculationDelayMs` prop for working around font loading or layout shifting scenarios ([#7388](elastic/eui#7388)) **Bug fixes** - Fixed a bug with `EuiSelectable`s with custom `truncationProps`, where scrollbar widths were not being accounted for ([#7392](elastic/eui#7392)) ## [`91.0.0`](https://github.com/elastic/eui/tree/v91.0.0) - Updated the background color of `EuiPopover`s in dark mode to increase visibility & contrast against other page/panel backgrounds ([#7310](elastic/eui#7310)) - Memoized `EuiDataGrid` to prevent unneeded re-renders ([#7324](elastic/eui#7324)) - Added a configurable `role` prop to `EuiAccordion` ([#7326](elastic/eui#7326)) - Added a configurable `role` prop to `EuiGlobalToastList` ([#7328](elastic/eui#7328)) - For greater flexibility, `EuiSuperDatePicker` now allows users to paste ISO 8601, RFC 2822, and Unix timestamps in the `Absolute` tab input, in addition to timestamps in the `dateFormat` prop ([#7331](elastic/eui#7331)) - Plain text `EuiComboBox`es now behave more like a normal text field/input. Backspacing will no longer delete the entire value, and selected values can now be double clicked and copied. ([#7332](elastic/eui#7332)) - `EuiDataGrid`'s display settings popover now allows users to clear the "Lines per row" input before typing in a new number ([#7338](elastic/eui#7338)) - Improved the UX of `EuiSuperDatePicker`'s Absolute tab for users manually typing in timestamps ([#7341](elastic/eui#7341)) - Updated `EuiI18n`s with multiple `tokens` to accept dynamic `values` ([#7341](elastic/eui#7341)) **Bug fixes** - Fixed `EuiComboBox`'s `onSearchChange` callback to pass the correct `hasMatchingOptions` value ([#7334](elastic/eui#7334)) - Fixed an `EuiSelectableTemplateSitewide` bug where the `popoverButton` behavior would break if passed a non-DOM React wrapper ([#7339](elastic/eui#7339)) **Deprecations** - `EuiPopover`: deprecated `anchorClassName`. Use `className` instead ([#7311](elastic/eui#7311)) - `EuiPopover`: deprecated `buttonRef`. Use `popoverRef` instead ([#7311](elastic/eui#7311)) - `EuiPopover`: removed extra `.euiPopover__anchor` div wrapper. Target `.euiPopover` instead if necessary ([#7311](elastic/eui#7311)) - Deprecated `EuiButtonGroup`'s `name` prop. This can safely be removed. ([#7325](elastic/eui#7325)) **Breaking changes** - Removed deprecated `euiPaletteComplimentary` - use `euiPaletteComplementary` Instead ([#7333](elastic/eui#7333)) **Accessibility** - Updated `type="single"` `EuiButtonGroup`s to render standard buttons instead of radio buttons under the hood, per recent a11y recommendations ([#7325](elastic/eui#7325)) - `EuiAccordion` now defaults to a less screenreader-noisy `group` role instead of `region`. If your accordion contains significant enough content to be a document landmark role, you may re-configure it back to `region`. ([#7326](elastic/eui#7326)) - Reduced screen reader noisiness when sorting `EuiDataGrid` columns via toolbar ([#7327](elastic/eui#7327)) - `EuiGlobalToastList` now defaults to a `log` role. If your toasts will always require immediate user action, consider (with caution) using the `alert` role instead. ([#7328](elastic/eui#7328)) **CSS-in-JS conversions** - Updated `$euiFontFamily` and `$euiCodeFontFamily` to match Emotion fonts ([#7332](elastic/eui#7332)) --------- Co-authored-by: Cee Chen <constance.chen@elastic.co> Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Cee Chen <549407+cee-chen@users.noreply.github.com> Co-authored-by: Stratoula Kalafateli <efstratia.kalafateli@elastic.co>

cee-chen added 4 commits October 27, 2023 13:37

[EuiAccordion] Add role prop and default to group (was previously…

dbb4fd7

… `region`

Update tests/snapshots

ac093b0

Update downstream snapshots

d894a5b

[EuiCollapsibleNavBeta] Update static group components to set a `role…

d34ac51

…="group"` following the same DOM pattern as accordions, but not customizable (since the role is literally in the component name)

cee-chen added the accessibility label Oct 27, 2023

changelog

6e2599c

cee-chen commented Oct 27, 2023

View reviewed changes

Fix props table not showing role prop

bb4f0aa

cee-chen marked this pull request as ready for review October 27, 2023 23:03

cee-chen requested a review from a team as a code owner October 27, 2023 23:03

1Copenut approved these changes Oct 30, 2023

View reviewed changes

cee-chen merged commit 31f1a48 into elastic:main Oct 30, 2023
2 checks passed

cee-chen deleted the accordion/role branch October 30, 2023 20:13

tkajtoch mentioned this pull request Nov 7, 2023

Upgrade EUI to v91.0.0 (with backports) elastic/kibana#170716

Merged

cee-chen mentioned this pull request Nov 27, 2023

WIP elastic/kibana#172021

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[EuiAccordion] Add configurable `role` prop and change default role to `group` #7326

[EuiAccordion] Add configurable `role` prop and change default role to `group` #7326

cee-chen commented Oct 27, 2023 •

edited by 1Copenut

Loading

cee-chen Oct 27, 2023

1Copenut Oct 30, 2023

kibanamachine commented Oct 27, 2023

elasticmachine commented Oct 27, 2023

1Copenut left a comment

1Copenut Oct 30, 2023

1Copenut Oct 30, 2023

[EuiAccordion] Add configurable role prop and change default role to group #7326

[EuiAccordion] Add configurable role prop and change default role to group #7326

Conversation

cee-chen commented Oct 27, 2023 • edited by 1Copenut Loading

Summary

QA

General checklist

cee-chen Oct 27, 2023

Choose a reason for hiding this comment

1Copenut Oct 30, 2023

Choose a reason for hiding this comment

kibanamachine commented Oct 27, 2023

elasticmachine commented Oct 27, 2023

💚 Build Succeeded

History

1Copenut left a comment

Choose a reason for hiding this comment

1Copenut Oct 30, 2023

Choose a reason for hiding this comment

1Copenut Oct 30, 2023

Choose a reason for hiding this comment

[EuiAccordion] Add configurable `role` prop and change default role to `group` #7326

[EuiAccordion] Add configurable `role` prop and change default role to `group` #7326

cee-chen commented Oct 27, 2023 •

edited by 1Copenut

Loading