[EuiAccordion] Improve keyboard accessibility of tabbable children#7064
[EuiAccordion] Improve keyboard accessibility of tabbable children#7064cee-chen merged 6 commits intoelastic:mainfrom
Conversation
- this visually broke when I removed the `eui-yScroll` child div - fixing it now
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_7064_buildkite/ |
💚 Build Succeeded
|
breehall
left a comment
There was a problem hiding this comment.
This is a solid solution Cee and it's going to make navigating the nav much better. I don't have any questions or critiques of the code and its structure. Seeing the getter used in accordion.tsx was a good reminder (I haven't played with class components in a while).
In Storybook, everything looks good. The keyboard navigation doesn't skip when tabbing through accordions with nested children.
| // If the element has an existing `tabIndex` set, make sure we can restore it | ||
| const originalTabIndex = element.getAttribute('tabIndex'); | ||
| if (originalTabIndex) { | ||
| element.setAttribute('data-original-tabindex', originalTabIndex); | ||
| } |
There was a problem hiding this comment.
This is good! I'm not sure I would have thought to retain the original tab index.
There was a problem hiding this comment.
The current EuiDataGridCell usage doesn't do this although it probably should 😂 but I figured I'd add that extra flexibility I'm in here, since accordion content is much more variable than datagrid cell content.
| if (forceState) { | ||
| this.props.onToggle && | ||
| this.props.onToggle(forceState === 'open' ? false : true); | ||
| this.props.onToggle?.(forceState === 'open' ? false : true); |
There was a problem hiding this comment.
[attaching to a random comment for threading]
Seeing the getter used in
accordion.tsxwas a good reminder (I haven't played with class components in a while).
Just mentioning for completeness, getters can be used inside any JS object, not just inside classes! https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/get
`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>
Summary
closes #6980, part of #7041
EuiAccordions children cannot be hidden due to accordions needing to detect their height. This means that interactive/tabbable children still remain the keyboard tab order when accordions are closed when they should not be. Example production behavior: https://codesandbox.io/s/withered-moon-hc3ft6?file=/demo.js (notice several tabs are required to reach the 2nd visible button).The solution to this is that on close,
EuiAccordionneeds to evaluate its tabbable children and manually set them totabIndex={-1}to remove them from tab order. On open, their original tabindex behavior should be restored.There's some misc syntax cleanup in this PR, so as always, follow along by commit!
Extra note
An example of similar behavior/logic in EUI also exists in EuiDataGridCell.
QA
yarn storybookAccordion with nested accordionsitem and confirm that pressing tab jumps you immediately to the next top-level accordion itemGeneral checklist
and cypress tests- I skipped Cypress test for now as the Jest tests felt adequate to me, but feel free to shout if you think E2E tests would feel better