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

Spaces menu keyboard and screen reader navigation is broken #130898

Closed
jportner opened this issue Apr 25, 2022 · 8 comments · Fixed by #134454
Closed

Spaces menu keyboard and screen reader navigation is broken #130898

jportner opened this issue Apr 25, 2022 · 8 comments · Fixed by #134454
Assignees
Labels
bug Fixes for quality problems that affect the customer experience Feature:Security/Spaces Platform Security - Spaces feature good first issue low hanging fruit impact:needs-assessment Product and/or Engineering needs to evaluate the impact of the change. Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! usability UX Debt

Comments

@jportner
Copy link
Contributor

jportner commented Apr 25, 2022

Kibana version:

8.3.0 (unreleased)

Describe the bug:

In #130593, we upgraded to EUI 55. As a result, EUI panels no longer auto-focus on the first child element, and the EuiContextMenuPanel component no longer supports a hasFocus property, and the keyboard/screen reader navigation for the SpacesMenu component is broken.

Steps to reproduce:

  1. Start Kibana and create several spaces
  2. Click the Space Selector and press Tab to navigate down to the first element (the search box)
  3. Press the down arrow key; observe that instead of focusing on the second element (the first space), nothing happens
  4. Type a few characters, then press Tab to focus on the "X" icon, and press Enter to clear the characters; observe that you have lost focus on the search box
  5. Press Tab to navigate down to the first space, then press Shift+Tab to navigate back to the search box; observe that you have immediately lost focus on the search box

There might be other buggy behavior but this is what I observed in my short time testing the aforementioned EUI upgrade PR.

Expected behavior:

The spaces menu should be fully usable with keyboard and screen reader navigation.

Screenshots (if relevant):

spaces-menu

Any additional context:

Based on the PR, the Spaces Popover List in the Edit Roles page is probably also impacted. I didn't examine that, but whoever takes on this issue should look at it too.

Edit 2022-05-19: there was another small change in EUI v55.1.3 that might have impact as well, see #132451 (review)

@jportner jportner added bug Fixes for quality problems that affect the customer experience Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! Feature:Security/Spaces Platform Security - Spaces feature labels Apr 25, 2022
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-security (Team:Security)

@exalate-issue-sync exalate-issue-sync bot added the impact:needs-assessment Product and/or Engineering needs to evaluate the impact of the change. label Apr 25, 2022
@cchaos
Copy link
Contributor

cchaos commented Apr 25, 2022

My suggestion would be updating that component to use EuiSelectable with a custom prepend for the options list to show the avatars. EuiSelectable handles all the search matching logic, keyboard commands, and screen-reader hints. We already have an example wired up in the EUI docs: https://elastic.github.io/eui/#/layout/header
Screen Shot 2022-04-25 at 10 33 41 AM

Look for the HeaderSpacesMenu component in the Demo JS tab.

@jeramysoucy
Copy link
Contributor

My suggestion would be updating that component to use EuiSelectable with a custom prepend for the options list to show the avatars. EuiSelectable handles all the search matching logic, keyboard commands, and screen-reader hints. We already have an example wired up in the EUI docs: https://elastic.github.io/eui/#/layout/header

@cchaos The existing behavior allows a user to middle-click or right-click a space in the popover and open it in a new browser tab (using the EuiContextMenuItem href prop). The label prop of EuiSelectableOptionProps is defined as a string, but if I set it to an anchor element with an appropriate href it appears to function in the same way. Is an acceptable/intended use scenario, or is there a more recommended way to achieve this behavior with EuiSelectable?

@cchaos
Copy link
Contributor

cchaos commented Jun 9, 2022

@constancecchen What did we end up implementing for the EuiSelectableSitewideTemplate component to be able to open options like normal links? Is it something that is now available also to the generic EuiSelectable?

@cee-chen
Copy link
Contributor

cee-chen commented Jun 9, 2022

@cchaos elastic/eui#5926 affects the generic EuiSelectable, yes, so now all EuiSelectable will receive either the mouse or keyboard event as the 2nd arg of their onChange callback. This means you can listen for event.shiftKey or event.metaKey to manually call window.open (example).

You could also add middle mouse button checking via event.button === 1, but right clicking likely would not work natively (in the sense that you can right click a link and select a 'open in new tab' browser menu option) as our selectable items are not rendered as links yet - we would need a larger overhaul/PR for that as specified in elastic/eui#5408 (comment). CC @1Copenut

@1Copenut
Copy link
Contributor

1Copenut commented Jun 9, 2022

I stated to answer this question on another Kibana ticket, but didn't have a complete picture. This comment elastic/eui#5408 (comment) I think describes what you're looking for here, and a way to make semantic distinction for listboxes vs. menus for assistive tech users.

@jeramysoucy
Copy link
Contributor

@constancecchen @1Copenut @cchaos Thanks for the feedback! Sounds like currently, the best approach is to treat Spaces in the menu as 'navigation items' as opposed to links, and rely on intercepting keyboard/mouse event options (cmd/ctrl, mouse button, shift, etc.) to achieve the desired behaviors (open in new tab/window).
This would change how users are currently interacting with the Spaces menu, but not in a major way. And if it more closely resembles the behavior of other exiting navigation menus/items, then it's probably the way to go.

@cee-chen
Copy link
Contributor

cee-chen commented Aug 2, 2022

Woohoo! Awesome work on this @jeramysoucy!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Fixes for quality problems that affect the customer experience Feature:Security/Spaces Platform Security - Spaces feature good first issue low hanging fruit impact:needs-assessment Product and/or Engineering needs to evaluate the impact of the change. Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! usability UX Debt
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants