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

[EuiFlyoutBody] Add scrollableTabIndex prop #7061

Merged
merged 4 commits into from
Aug 10, 2023

Conversation

cee-chen
Copy link
Contributor

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

Summary

This PR is part of #7041 (several a11y improvements/enhancements for the new EuiCollapsibleNavBeta work).

This PR only addresses the extra tab stop caused by EuiFlyoutBody - it does not address any issues inherent to push flyouts (#6576). I'll be looking into that as a separate PR.

QA

General checklist

  • Props have proper autodocs (using @default if default values are missing) and playground toggles
    - [ ] Added documentation - Opting for just props documentation, I don't think this needs its own docs example
  • Added or updated jest and cypress tests
  • Checked for accessibility including keyboard-only and screenreader modes
  • A changelog entry exists and is marked appropriately

- the children of each are already focusable and enable keyboard scrolling, so removing the extra tab stop makes sense and is a keyboard a11y improvement
@cee-chen cee-chen requested review from 1Copenut and breehall August 9, 2023 17:17
@cee-chen
Copy link
Contributor Author

cee-chen commented Aug 9, 2023

@1Copenut I know you're busy with other work this week (Kibana upgrade, buildkite CI) so I'm mostly just tagging you for info. If you see any issues with this implementation or with the docs, please feel to shout.

@breehall I'm hoping you can grab the main review/QA of this PR, since you've had some experience QAing this component and noticed the focus/tab UX previosuly.

@cee-chen cee-chen marked this pull request as ready for review August 9, 2023 17:20
@kibanamachine
Copy link

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

@cee-chen
Copy link
Contributor Author

cee-chen commented Aug 9, 2023

buildkite test this

@kibanamachine
Copy link

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

@cee-chen
Copy link
Contributor Author

cee-chen commented Aug 9, 2023

buildkite test this

@kibanamachine
Copy link

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

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 great accessibility improvement. When tabbing in CollapsibleNavBeta in Storybook, there's only one tab stop as expected and there are no Axe errors on the page.

[Question]
What do you think about adding a small blurb to this in our docs? This is really great info and could be very useful for devs to know.

[Follow up]
This prop also the following examples for EuiFlyout in our docs. The same behavior is occurring where there is an actionable and focusable item (like a button or button group) and there's a skip in focus. I applied the new prop locally and it fixes them - I can create a separate docs PR to update these!

@cee-chen
Copy link
Contributor Author

cee-chen commented Aug 9, 2023

I would strongly rather not update our EuiFlyout docs examples with this prop. As the prop documentation specifies, unsetting tabIndex={0} is only optional if you know your flyout children has focusable content. In the case of EuiCollapsibleNav, we can be pretty damn sure it'll contain focusable children, since it's intended to contain links. So keyboard scrolling is not an issue for this specific usage of EuiFlyoutBody.

In the case of other flyouts - we have absolutely no idea whether that's true or not. Consumers could have an entire essay in there of just text without any interactive children, which means that EuiFlyoutBody needs to have tabIndex={0} in order for sighted keyboard users to be able to scroll the region with arrow keys.

As we've mentioned before in the past, consumers tend to blindly copy and paste what's in our docs examples without giving too much thought of whether they need to change it or not. In those scenarios, we need to default the scrollable region having a tabIndex. The a11y experience of a user not being able to scroll via keyboard is breaking vs having an extra tab stop (a mild annoyance) - we should absolutely not be defaulting to the "breaking" experience.

@cee-chen
Copy link
Contributor Author

cee-chen commented Aug 9, 2023

What do you think about adding a small blurb to this in our docs? This is really great info and could be very useful for devs to know.

Personally, I think the documentation added for the prop fully covers the information needed, and like I mentioned above, I'd rather not have consumers who have no idea what they're doing (in terms of a11y) customize this prop. It's there if devs know enough to even look for it, otherwise I'd rather have it working as a sensible default in the background without devs needing to configure it unless they know what they're doing.

@kibanamachine
Copy link

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

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

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.

Thanks for your thorough explanation. To confirm, we won't be moving forward with updating our flyout docs or creating additional documentation

@cee-chen
Copy link
Contributor Author

Yep! Thanks Bree. Going to go ahead and merge, but Trevor, feel free to chime in or follow up on Slack if you disagree - we can circle back to this later if needed.

@cee-chen cee-chen merged commit 76e7889 into elastic:main Aug 10, 2023
@cee-chen cee-chen deleted the collapsible-nav-beta-ally-2 branch August 10, 2023 15:09
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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants