Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[EuiAccordion] Improve keyboard accessibility of tabbable children #7064

Merged
merged 6 commits into from
Aug 10, 2023

Conversation

cee-chen
Copy link
Member

@cee-chen cee-chen commented Aug 9, 2023

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, EuiAccordion needs to evaluate its tabbable children and manually set them to tabIndex={-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 storybook
  • Go to http://localhost:6006/?path=/story/euicollapsiblenavitem--edge-case-testing
  • Close the Accordion with nested accordions item and confirm that pressing tab jumps you immediately to the next top-level accordion item
  • Re-open the accordion and confirm its children are tabbable
  • Repeat the above with as many other accordions/nested accordions as you want to see if you can catch any unexpected tab behavior

General checklist

  • Checked in Chrome, Safari, Edge, and Firefox
  • Added or updated jest 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
  • Checked for accessibility including keyboard-only and screenreader modes
  • A changelog entry exists and is marked appropriately

@cee-chen cee-chen requested review from breehall and a team August 9, 2023 19:45
- this visually broke when I removed the `eui-yScroll` child div - fixing it now
@cee-chen cee-chen marked this pull request as ready for review August 9, 2023 19:57
@kibanamachine
Copy link

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

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

Copy link
Contributor

@breehall breehall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +210 to +214
// 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);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is good! I'm not sure I would have thought to retain the original tab index.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -159,8 +159,7 @@ export class EuiAccordionClass extends Component<
onToggle = () => {
const { forceState } = this.props;
if (forceState) {
this.props.onToggle &&
this.props.onToggle(forceState === 'open' ? false : true);
this.props.onToggle?.(forceState === 'open' ? false : true);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice clean up!

Copy link
Member Author

@cee-chen cee-chen Aug 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[attaching to a random comment for threading]

Seeing the getter used in accordion.tsx was 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

@cee-chen cee-chen merged commit 5d1d301 into elastic:main Aug 10, 2023
@cee-chen cee-chen deleted the accordion-a11y-tabbable branch August 10, 2023 19:51
cee-chen added a commit to elastic/kibana that referenced this pull request Aug 21, 2023
`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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[EuiAccordion] Children are still present in keyboard tab order when accordions are closed
4 participants