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

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

Closed
cee-chen opened this issue Feb 2, 2023 · 9 comments
Closed

Comments

@cee-chen
Copy link
Contributor

cee-chen commented Feb 2, 2023

CC @1Copenut for thoughts

Push flyouts (link)

These flyouts have the most severe keyboard/screen reader accessibility issues that may or may not require addressing depending on what the intention of them are. Current issues:

  1. Focus traps on them are completely disabled. Keyboard users are not automatically focused into the flyout. Flyouts do not trap focus (users can tab in/out at all). Depending on where in the DOM the consumer places the push flyout, accessing the flyout can either be a huge headache or visually not make sense with the rest of the page.

  2. Focus is not automatically returned to the flyout toggle on close (focus stranding)

  3. Push flyouts do not close on Escape keypress

At first glance, all of these issues seem extremely problematic. However, if the goal of these "flyouts" is really more to "conditionally render some content within the existing flow of the page but visually to the side", then maybe they're not really dialogs at all, and we can get away with simply removing role if type === 'push' and call it a day? 😕

Or, should we prefer to say "no, these are definitely dialogs, treat them like ownFocus={false} flyouts?

ownFocus={false} flyouts (link)

While not nearly problematic or difficult for keyboard/SR users to use, the main problem with ownFocus={false} flyouts is that they deliberately advertise a functionality available to mouse users that is not available to keyboard/SR users:

Mouse users are allowed to interact with items outside the flyout, keyboard/SR users are not. While this is certainly a design/UX decision, it feels like a slightly unfair/against best practices one, since usually the recommendation for accessibility parity is that keyboard users should be able to access everything mouse users can.

That being said, in this case it's not a hugely debilitating UX loss (unless some team in Kibana is literally relying on outside-flyout changes to happen at the same time as inside-flyout UI/UX), so I think we would have a case for keeping this as-is, but I would want to make sure it's an extremely conscious and a11y-vetted decision.

@1Copenut
Copy link
Contributor

1Copenut commented Feb 6, 2023

ownFocus={false} flyout a11y

This flyout doesn't meet SC 2.1.1 Keyboard (Level A) because we're confining keyboard users to the flyout, where mouse users can still interact with the rest of the page. Based on my new understanding of modal vs. non-modal dialogs, this flyout would be non-modal. When we announce this as a "non-modal dialog", screen reader users could have an expectation the page is still active and reachable. Trapping users could be a violation of SC 1.3.1 Info and Relationships (Level A).

Tasks

  • Remove the focus trap
  • Update the SR-only help text to identify this as a non-modal dialog, and let users know they can interact with the page

@1Copenut
Copy link
Contributor

1Copenut commented Feb 6, 2023

Push flyouts

Looking at our documentation and the component, this does feel like a variation on the ownFocus={false} non-modal dialog. If this was only used for navigation, I'd approach it differently, but this looks like a multiple use flyout that could contain FAQs, meta data, navigation links, etc. Given this fact, I feel we should give this the same keyboard behaviors as the ownFocus={false} non-modal dialog with the slightly different CSS.

@cee-chen
Copy link
Contributor Author

cee-chen commented Feb 6, 2023

I'm fine removing the focus trap on the flyout for ownFocus={false}, but I think it's problematic that keyboard/SR users no longer auto-focus the flyout at that point (nor is focus returned to the flyout toggle when the flyout closes).

Rather than removing the focus trap entirely, I think we should consider adding the entire body component as a shard, thus including the entire page in the focus trap so that it remains tabbable. This (theoretically) preserves react-focus-on's auto focus and return focus behavior instead of us having to code it in programmatically. (I say theoretically because it's possible this doesn't work and we need to programmatically handle flyout focus anyway 💀)

I also think we need to come to a decision as to whether Escape keypresses should close push flyouts. I'm confused as to why they wouldn't.

@1Copenut
Copy link
Contributor

1Copenut commented Feb 6, 2023

I think Escape should close the push flyouts as well. We're moving toward consistency, so let's include that as expected behavior.

@cee-chen
Copy link
Contributor Author

cee-chen commented Feb 6, 2023

OK, in that case the updated task list of items are:

  • Remove logic preventing escape keypress from closing push flyout
  • Ensure that when all flyouts open, focus is on the flyout wrapper
  • Ensure that when all flyouts close, focus is on the button that toggled the flyout
  • Either add shards or remove the focus trap on ownFocus={false} flyouts, so that the entire page is accessible for keyboard users
  • Update the SR-only help text to identify this as a non-modal dialog, and let users know they can interact with the page

Does that look right to you, or is there anything missing?

@1Copenut
Copy link
Contributor

1Copenut commented Feb 7, 2023

I'm looking at this again today, and questioning my prior conclusion. Blaming it on the cold I'm working through. I've put it on the discussion agenda for today because there might be more nuance.

@1Copenut 1Copenut closed this as completed Feb 7, 2023
@1Copenut 1Copenut reopened this Feb 7, 2023
@cee-chen
Copy link
Contributor Author

cee-chen commented Feb 7, 2023

I don't disagree with any of your conclusions or see any major UX issues - what are you concerned about?

@1Copenut
Copy link
Contributor

1Copenut commented Feb 7, 2023

I briefly entertained the idea our push flyout might need to have a role other than dialog but after discussing in the EUI team meeting, I agree with you it is a dialog, and should have the behaviors outlined in your comment above.

@cee-chen
Copy link
Contributor Author

Closing this issue as a won't fix. Unfortunately, the new beta collapsible nav used by the new serverless design utilizes a push flyout that never disappears, and thus is not a dialog. To serve that use case, I've instead opted to take a different approach: #7065

If, in the future, someone else can reconcile the new collapsible nav's requirements with actual dynamic push flyouts, please feel free to re-open this issue or create a new one.

@cee-chen cee-chen closed this as not planned Won't fix, can't repro, duplicate, stale Aug 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants