[EuiFlyout] Resolve disparity between push flyouts and overlay flyouts #7065

cee-chen · 2023-08-10T21:13:12Z

Summary

This is my last PR for #7041.

This relates to the discussion brought up in #6576, but does not resolve it per the issue. It instead takes a completely different direction of specifically marking push flyouts not as dialogs, and removing all dialog-related props (primarily role and tabIndex).

I've opted to go with this approach because of the direction that the new beta collapsible nav has gone in, which will eventually be present on every page of Kibana.

QA

gh pr checkout 7065
yarn storybook
Go to http://localhost:6006/?path=/story/euicollapsiblenavbeta--kibana-example (ensure the nav is on a wide enough screen that it's pushing the page as opposed to being an overlay)
Click the toggle button
Confirm that only one tab press is needed to get to first item/link in the nav
Confirm that screen readers announce/inherit the navigation context/information
Regression testing
- Make your screen small enough so that the nav becomes an overlay
- Click the toggle button
- Confirm the flyout has a label and the normal screen reader dialog text reads out

General checklist

Added documentation
Added or updated jest ~~and cypress tests~~
Checked for accessibility including keyboard-only and screenreader modes
A changelog entry exists and is marked appropriately
- Note: I'd prefer not to mark this as a breaking change personally, as I doubt it will affect any consumers and will have minimal effect on end-users (if anything, it will remove an extra tab stop and unnecessary role announcement for SR users).

…fore rest - these props are already extracted out and don't exist in `...rest`, so there's nothing to override

Push flyouts are already not receiving screen reader specific text, and in the case of the new beta collapsible nav, the push flyout truly isn't a dialog

- only used if the focus trap is enabled, and it's not, so we should just lose it

…IDs to flyouts

kibanamachine · 2023-08-10T21:39:00Z

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

+ misc fragment cleanup

src-docs/src/views/flyout/flyout_example.js

kibanamachine · 2023-08-10T22:35:58Z

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

1Copenut

👍 This is working really well in VO + Safari and VO + Firefox. I have a couple of suggestions for documentation and a default accessible label. LMK what you think and I'll mark approved.

src-docs/src/views/flyout/flyout_example.js

1Copenut · 2023-08-11T15:16:55Z

src/components/collapsible_nav_beta/collapsible_nav_beta.tsx

@@ -183,6 +188,7 @@ export const EuiCollapsibleNavBeta: FunctionComponent<
  // Wait for any fixed headers to be queried before rendering (prevents position jumping)
  const flyout = fixedHeadersCount !== false && (
    <EuiFlyout
+      aria-label={isOverlay ? dialogAriaLabel : undefined} // Overlay flyouts are dialogs and need a label. Push flyouts are not dialogs.


I'd like to think about a default value for the push navigation on wider screens. When I was testing I ran through the Landmarks rotor and it just announced as "navigation" because there was no accessible label.

That's not a problem but could be a UX rough edge when we drop this into a full page template. There may be 2-3 <nav> elements on the page (guessing) and each would just announce as "navigation" in the rotor.

This is a judgment call, but having a label like <aria-label="Site menu"> would announce "Site menu, navigation" in the rotor and when users are traversing the DOM.

I'm good with a default aria label of Site menu. I originally had it as Site navigation which was what led to me trying to conditionally render it - for non-dialogs, VO would read out "Site navigation, navigation" which felt unnecessarily repetitive. Site menu makes a whole lot more sense.

One extra note of caution - while this is unlikely something we have to be concerned about for Kibana at least, it's possible other consumers could use the new EuiCollapsibleNav for non site navigation? 🤷 I might add some prop docs to highlight this at least.

Co-authored-by: Trevor Pierce <1Copenut@users.noreply.github.com>

kibanamachine · 2023-08-11T19:43:54Z

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

elasticmachine · 2023-08-11T19:58:26Z

💚 Build Succeeded

Buildkite Build
Commit: c9e6634

History

💚 Build #279 succeeded 6b30b77
💚 Build #276 succeeded 0d7df8a

1Copenut

👍 LGTM! Let's 🚢 it.

1Copenut

Okay, I'll try this again. Approving for real this time.

`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 4 commits August 10, 2023 13:51

[misc cleanup] move props that don't need to be after ...rest to be…

1653966

…fore rest - these props are already extracted out and don't exist in `...rest`, so there's nothing to override

Remove dialog role + tabIndex from push flyouts

bf1e24d

Push flyouts are already not receiving screen reader specific text, and in the case of the new beta collapsible nav, the push flyout truly isn't a dialog

Remove unnecessary data-autofocus prop from push flyouts

d84f0c6

- only used if the focus trap is enabled, and it's not, so we should just lose it

[misc enhancement] Allow consumers to pass custom aria-describedby …

0d7df8a

…IDs to flyouts

cee-chen mentioned this pull request Aug 10, 2023

[EuiCollapsibleNavBeta] A11y improvements #7041

Closed

3 tasks

[EuiCollapsibleNavBeta] Add an aria-label for overlay mode only

39b5bf3

cee-chen mentioned this pull request Aug 10, 2023

[EuiFlyout] Accessibility issues with type="push" and ownFocus={false} #6576

Closed

cee-chen added 2 commits August 10, 2023 15:07

[Docs] Add warning about push flyout accessibility

7a6a831

+ misc fragment cleanup

changelog

6b30b77

cee-chen force-pushed the collapsible-nav-beta-a11y-3 branch from 5628b2f to 6b30b77 Compare August 10, 2023 22:10

cee-chen added the accessibility label Aug 10, 2023

cee-chen requested review from a team and 1Copenut August 10, 2023 22:13

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

cee-chen commented Aug 10, 2023

View reviewed changes

src-docs/src/views/flyout/flyout_example.js Outdated Show resolved Hide resolved

1Copenut reviewed Aug 11, 2023

View reviewed changes

cee-chen and others added 2 commits August 11, 2023 12:09

[PR feedback] copy

3b1292d

Co-authored-by: Trevor Pierce <1Copenut@users.noreply.github.com>

[PR feedback] Site menu aria-label

c9e6634

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

1Copenut reviewed Aug 11, 2023

View reviewed changes

1Copenut approved these changes Aug 11, 2023

View reviewed changes

cee-chen merged commit 1a10ca9 into elastic:main Aug 11, 2023
1 check passed

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

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

[EuiFlyout] Resolve disparity between push flyouts and overlay flyouts #7065

[EuiFlyout] Resolve disparity between push flyouts and overlay flyouts #7065

cee-chen commented Aug 10, 2023 •

edited

Loading

kibanamachine commented Aug 10, 2023

kibanamachine commented Aug 10, 2023

1Copenut left a comment

1Copenut Aug 11, 2023

cee-chen Aug 11, 2023

cee-chen Aug 11, 2023

kibanamachine commented Aug 11, 2023

elasticmachine commented Aug 11, 2023

1Copenut left a comment

1Copenut left a comment

[EuiFlyout] Resolve disparity between push flyouts and overlay flyouts #7065

[EuiFlyout] Resolve disparity between push flyouts and overlay flyouts #7065

Conversation

cee-chen commented Aug 10, 2023 • edited Loading

Summary

QA

General checklist

kibanamachine commented Aug 10, 2023

kibanamachine commented Aug 10, 2023

1Copenut left a comment

Choose a reason for hiding this comment

1Copenut Aug 11, 2023

Choose a reason for hiding this comment

cee-chen Aug 11, 2023

Choose a reason for hiding this comment

cee-chen Aug 11, 2023

Choose a reason for hiding this comment

kibanamachine commented Aug 11, 2023

elasticmachine commented Aug 11, 2023

💚 Build Succeeded

History

1Copenut left a comment

Choose a reason for hiding this comment

1Copenut left a comment

Choose a reason for hiding this comment

cee-chen commented Aug 10, 2023 •

edited

Loading