-
Notifications
You must be signed in to change notification settings - Fork 842
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
[EuiFlyout] Include all fixed EuiHeader
s in flyout focus traps + improve screen reader accessibility
#6566
Conversation
Preview documentation changes for this PR: https://eui.elastic.co/pr_6566/ |
6e0c98c
to
5589a1a
Compare
ref
s; Add docs example of passing headers to flyout focusTrapProps.shards
to prevent focus fightingEuiHeader
s in flyout focus traps
Preview documentation changes for this PR: https://eui.elastic.co/pr_6566/ |
1bc425e
to
ed980cf
Compare
NOT YET WORKING: autoFocus on flyout open and returnFocus
+ a11y improvement: mirroring popovers & modals, make the flyout wrapper focusable via `tabIndex={0}`, so that screen readers take out a beat to read out the `labelledby` and (upcoming `describedby`) instead of jumping to the close button or the first focusable child (if no close button exists)
- allow for custom `role`s that consumers pass in that aren't `dialog`s (likely very rare, may need more testing)
019a9b1
to
abaa538
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@1Copenut I need to head out here in a bit for an appt but wanted to get this in front of you first for your thoughts & QA. This PR doesn't solve all issues but is an OK start, in my opinion (see PR description).
src/components/flyout/flyout.tsx
Outdated
<EuiI18n | ||
token="euiFlyout.closeAriaLabel" | ||
default="Close this {role}" | ||
values={{ role: role || Element }} | ||
> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@1Copenut Would appreciate your thoughts on this change. I'm struggling with the fact that we allow consumers to set a custom role
on EuiFlyout's that isn't necessarily dialog
. That makes screen reader instructions incredibly hard to predict, so I tried to compromise on having the screen reader read out loud either 1. the custom role
passed, or 2. the custom as
passed. Am I way off on that?
TBH I believe 99% of consumers leave the default role="dialog"
on. The main use case this probably affects is EuiCollapsibleNav
, which passes role={null} as="nav"
. This reads out to screen readers: "Close this nav", "You are in a nav", etc. Do you think that's okay?
The absolute worst case scenario would be if someone passed role={null}
and no meaningful as
... the screen reader then reads out "Close this div", "You are in a div"
:/ I'm not really sure how else to write that though, I mean, we can only do so much with what we're given if they're removed the actual role="dialog"
property.
Other idea: Should we just change this copy to a generic "Close this flyout", "You are in a flyout"? Does the word "flyout" have any meaning to screen reader users?...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've been re-reading the MDN ARIA: dialog role entry, and came across this interesting tidbit that might be our way forward:
Dialogs can be either non-modal (it's still possible to interact with content outside of the dialog) or modal (only the content in the dialog can be interacted with).
EuiFlyout
should always have a role dialog
, but its modality can change. If we have the smoke overlay and users can't interact with the page "behind" the dialog, that's a modal dialog. If users can interact with the page because we pass ownFocus={false}
that's a non-modal dialog.
Norman Nielsen Group wrote about this way back in 2017—which I just learned about today. Modal & Nonmodal Dialogs: When (& When Not) to Use Them
I'm proposing we drive the close button label using ownFocus
as our pivot. The default should be "Close this modal dialog". If users pass ownFocus={false|
it should say "Close this non-modal dialog." I'd like the label to include the flyout title, but we're not requiring a title, so it's going to be somewhat generic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, that's exactly the kind of context I was hoping for!
Going to address your other comment here because it's more related to this discussion:
I'm struggling with the fact that we allow consumers to set a custom
role
on EuiFlyout's that isn't necessarilydialog
.
I don't think we should allow users to override the
role
if we can help it. These flyouts are dialogs through and through. Allowing users to change the role triggers different behavior in screen readers and might be more confusing.
I mean personally I agree, but it looks like Caroline expanded the option to pass role={null}
as part of EuiCollapsibleNav work in #4713.
I'd want to make sure that we test that use case first before making a breaking API change on this. Is the issue that <nav role="dialog">
throws axe errors? Is that why that change was made in the first place?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW: I'm not getting either axe issues with <nav role="dialog">
or SR issues with Safari+VO (VO reads out both "Main navigation, web dialog, X items") perfectly. I strongly suspect this was simply an unnecessary change that we should roll back.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pushed up the role
breaking change mostly to test on staging: ade9175
TBH, this looks good to me - lmk what you think once staging finishes updating! New screen reader text will need a copy pass as well
src/components/flyout/flyout.tsx
Outdated
<EuiScreenReaderOnly> | ||
<p id={descriptionId}> | ||
<EuiI18n | ||
token="euiFlyout.screenReaderEscapeToClose" | ||
default="You are in a {role}. To close this {role}, press Escape." | ||
values={{ role: role || Element }} | ||
/>{' '} | ||
{hasOverlayMask && ( | ||
<EuiI18n | ||
token="euiFlyout.screenReaderTapToClose" | ||
default="Or tap/click outside the {role} on the shadowed overlay to close." | ||
values={{ role: role || Element }} | ||
/> | ||
)}{' '} | ||
{fixedHeaders.length > 0 && ( | ||
<EuiI18n | ||
token="euiFlyout.screenReaderFixedHeaders" | ||
default="You can still continue tabbing through the page headers in addition to the {role}." | ||
values={{ role: role || Element }} | ||
/> | ||
)} | ||
</p> | ||
</EuiScreenReaderOnly> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would love feedback on this SR text, if you have any! It's definitely tricky to write logic for as EuiFlyout is way more customizable in many ways than either EuiModal or EuiPopover 😬
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the SR text you have is clear and works really well for modal dialogs that have our overlay smoke. It makes sense to keep the text as is and update it when we tackle non-modal dialogs where the underlying page is still available to be interacted with. I don't think we should allow users to override the role
if we can help it. These flyouts are dialogs through and through. Allowing users to change the role triggers different behavior in screen readers and might be more confusing.
Your use case about the collapsible navigation was on point as the exception to prove the rule. I think we can do things with recommended titling and the proper <nav>
inside the dialog to let users know this is a non-modal dialog that contains a list of navigation links.
EuiHeader
s in flyout focus trapsEuiHeader
s in flyout focus traps + improve screen reader accessibility
Preview documentation changes for this PR: https://eui.elastic.co/pr_6566/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cee-chen I did a lot of reading and testing. Two comments for you regarding role and modal vs. non-modal dialogs. I'll add more what I'm thinking in the thread b/c it's out of scope for this PR.
src/components/flyout/flyout.tsx
Outdated
<EuiI18n | ||
token="euiFlyout.closeAriaLabel" | ||
default="Close this {role}" | ||
values={{ role: role || Element }} | ||
> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've been re-reading the MDN ARIA: dialog role entry, and came across this interesting tidbit that might be our way forward:
Dialogs can be either non-modal (it's still possible to interact with content outside of the dialog) or modal (only the content in the dialog can be interacted with).
EuiFlyout
should always have a role dialog
, but its modality can change. If we have the smoke overlay and users can't interact with the page "behind" the dialog, that's a modal dialog. If users can interact with the page because we pass ownFocus={false}
that's a non-modal dialog.
Norman Nielsen Group wrote about this way back in 2017—which I just learned about today. Modal & Nonmodal Dialogs: When (& When Not) to Use Them
I'm proposing we drive the close button label using ownFocus
as our pivot. The default should be "Close this modal dialog". If users pass ownFocus={false|
it should say "Close this non-modal dialog." I'd like the label to include the flyout title, but we're not requiring a title, so it's going to be somewhat generic.
src/components/flyout/flyout.tsx
Outdated
<EuiScreenReaderOnly> | ||
<p id={descriptionId}> | ||
<EuiI18n | ||
token="euiFlyout.screenReaderEscapeToClose" | ||
default="You are in a {role}. To close this {role}, press Escape." | ||
values={{ role: role || Element }} | ||
/>{' '} | ||
{hasOverlayMask && ( | ||
<EuiI18n | ||
token="euiFlyout.screenReaderTapToClose" | ||
default="Or tap/click outside the {role} on the shadowed overlay to close." | ||
values={{ role: role || Element }} | ||
/> | ||
)}{' '} | ||
{fixedHeaders.length > 0 && ( | ||
<EuiI18n | ||
token="euiFlyout.screenReaderFixedHeaders" | ||
default="You can still continue tabbing through the page headers in addition to the {role}." | ||
values={{ role: role || Element }} | ||
/> | ||
)} | ||
</p> | ||
</EuiScreenReaderOnly> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the SR text you have is clear and works really well for modal dialogs that have our overlay smoke. It makes sense to keep the text as is and update it when we tackle non-modal dialogs where the underlying page is still available to be interacted with. I don't think we should allow users to override the role
if we can help it. These flyouts are dialogs through and through. Allowing users to change the role triggers different behavior in screen readers and might be more confusing.
Your use case about the collapsible navigation was on point as the exception to prove the rule. I think we can do things with recommended titling and the proper <nav>
inside the dialog to let users know this is a non-modal dialog that contains a list of navigation links.
ade9175
to
c27ebc2
Compare
Preview documentation changes for this PR: https://eui.elastic.co/pr_6566/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 I took another pass after pulling all your latest changes in. The announcement for modal vs. non-modal dialogs feels natural and clear. Having the role pinned to "dialog" feels like the right approach.
I tested the collapsible navigation example for any quirks and didn't hear any presented.
This is a well-defined and scoped change. The updates drive well and sound good.
Thanks a million Trevor! I just did another quick QA pass in Safari/FF/Edge and am super satisfied with the incremental improvement in this PR. Hopefully we get a chance to pay down #6576 soon, and that this work will help serve as a helpful blueprint or mental model for that discussion! |
## Summary `[email protected]` ⏩ `[email protected]` ___ ## [`75.0.0`](https://github.com/elastic/eui/tree/v75.0.0) - `EuiFlyout`s now automatically shard all fixed `EuiHeader`s on the page. This means that interactions (mouse & keyboard) with items inside `EuiHeader`s when flyouts are open will no longer trigger focus fighting ([#6566](elastic/eui#6566)) - `EuiFlyout`s now read out detailed screen reader dialog instructions and hints on open ([#6566](elastic/eui#6566)) **Bug fixes** - Fixed `EuiSelectable` options with incorrect `aria-posinset` indices when rendered with group labels not at the start of the array ([#6571](elastic/eui#6571)) - Fixed a bug with `EuiSearchBar` where filters with `multiSelect: false` were not able to select a new option when an option was already selected ([#6577](elastic/eui#6577)) **Breaking changes** - Removed the ability to customize the `role` prop of `EuiFlyout`s. `EuiFlyout`s should always be dialog roles for screen reader consistency. ([#6566](elastic/eui#6566)) - Removed `closeButtonAriaLabel` prop from `EuiFlyout` - use `closeButtonProps['aria-label']` instead ([#6566](elastic/eui#6566)) --------- Co-authored-by: Kibana Machine <[email protected]>
## Summary `[email protected]` ⏩ `[email protected]` ___ ## [`75.0.0`](https://github.com/elastic/eui/tree/v75.0.0) - `EuiFlyout`s now automatically shard all fixed `EuiHeader`s on the page. This means that interactions (mouse & keyboard) with items inside `EuiHeader`s when flyouts are open will no longer trigger focus fighting ([elastic#6566](elastic/eui#6566)) - `EuiFlyout`s now read out detailed screen reader dialog instructions and hints on open ([elastic#6566](elastic/eui#6566)) **Bug fixes** - Fixed `EuiSelectable` options with incorrect `aria-posinset` indices when rendered with group labels not at the start of the array ([elastic#6571](elastic/eui#6571)) - Fixed a bug with `EuiSearchBar` where filters with `multiSelect: false` were not able to select a new option when an option was already selected ([elastic#6577](elastic/eui#6577)) **Breaking changes** - Removed the ability to customize the `role` prop of `EuiFlyout`s. `EuiFlyout`s should always be dialog roles for screen reader consistency. ([elastic#6566](elastic/eui#6566)) - Removed `closeButtonAriaLabel` prop from `EuiFlyout` - use `closeButtonProps['aria-label']` instead ([elastic#6566](elastic/eui#6566)) --------- Co-authored-by: Kibana Machine <[email protected]>
Steps to view problem * install sample data set * Open lens visualization * Open inspector. Notice console errors <img width="300" alt="Screen Shot 2023-05-05 at 11 03 25 AM" src="https://user-images.githubusercontent.com/373691/236521366-d8fb9302-e93b-4047-a0bf-d7c09dcc3ffb.png"> elastic/eui#6566 removed `closeButtonAriaLabel` prop from [EuiFlyout](https://elastic.github.io/eui/#/layout/flyout) EUI 75.0.0 (Effecting 8.8 and 8.9). FlyoutService spreads options into `EuiFlyout`, resulting in `closeButtonAriaLabel` getting added to dom and causing error. `OverlayFlyoutOpenOptions` type added by #37894. I replaced `OverlayFlyoutOpenOptions` with `EuiFlyoutProps` to make it more clear what props are accepted and provide stronger typing that stays in sync with EUI typings --------- Co-authored-by: kibanamachine <[email protected]>
Steps to view problem * install sample data set * Open lens visualization * Open inspector. Notice console errors <img width="300" alt="Screen Shot 2023-05-05 at 11 03 25 AM" src="https://user-images.githubusercontent.com/373691/236521366-d8fb9302-e93b-4047-a0bf-d7c09dcc3ffb.png"> elastic/eui#6566 removed `closeButtonAriaLabel` prop from [EuiFlyout](https://elastic.github.io/eui/#/layout/flyout) EUI 75.0.0 (Effecting 8.8 and 8.9). FlyoutService spreads options into `EuiFlyout`, resulting in `closeButtonAriaLabel` getting added to dom and causing error. `OverlayFlyoutOpenOptions` type added by elastic#37894. I replaced `OverlayFlyoutOpenOptions` with `EuiFlyoutProps` to make it more clear what props are accepted and provide stronger typing that stays in sync with EUI typings --------- Co-authored-by: kibanamachine <[email protected]> (cherry picked from commit b803ba9)
Steps to view problem * install sample data set * Open lens visualization * Open inspector. Notice console errors <img width="300" alt="Screen Shot 2023-05-05 at 11 03 25 AM" src="https://user-images.githubusercontent.com/373691/236521366-d8fb9302-e93b-4047-a0bf-d7c09dcc3ffb.png"> elastic/eui#6566 removed `closeButtonAriaLabel` prop from [EuiFlyout](https://elastic.github.io/eui/#/layout/flyout) EUI 75.0.0 (Effecting 8.8 and 8.9). FlyoutService spreads options into `EuiFlyout`, resulting in `closeButtonAriaLabel` getting added to dom and causing error. `OverlayFlyoutOpenOptions` type added by #37894. I replaced `OverlayFlyoutOpenOptions` with `EuiFlyoutProps` to make it more clear what props are accepted and provide stronger typing that stays in sync with EUI typings --------- Co-authored-by: kibanamachine <[email protected]>
Summary
closes #5206
Fixed
EuiHeader
s that sit next toEuiFlyout
s and can still be interacted with at the same time asEuiFlyout
s need to be automatically added to the flyout'sfocusTrapProps.shards
API in order for the flyout to not consider the header "outside of the flyout" and trigger focus trap close.What this PR does:
includeFixedHeadersInFocusTrap
prop.EuiPopover
andEuiModal
- as opposed to the close button (or the first focusable item if the close button has been removed))EuiPopover
screen reader descriptions)role="dialog"
closeButtonAriaLabel
- it's unnecessary ascloseButtonProps: { 'aria-label': 'something' }
does the same thingWhat this PR DOES NOT do:
push
type EuiFlyoutsownFocus={false}
type="push"
andownFocus={false}
#6576QA
The focus fighting issue was already afflicting our "Elastic navigation pattern" demo on prod. This PR can be compared directly against prod behavior by opening a flyout or nav and then clicking the sitewide search input.
Main navigation, web dialog
General checklist
@default
if default values are missing) and playground toggles- [ ] Checked in both light and dark modes- [ ] Checked in mobile- [ ] Added documentation- [ ] Checked Code Sandbox works for any docs examples- [ ] Updated the Figma library counterpart