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

focus gets stuck in TabNav focuszone within ActionMenu.Overlay #2340

Closed
jdrush89 opened this issue Sep 15, 2022 · 9 comments · Fixed by #2468
Closed

focus gets stuck in TabNav focuszone within ActionMenu.Overlay #2340

jdrush89 opened this issue Sep 15, 2022 · 9 comments · Fixed by #2468
Assignees
Labels
bug Something isn't working react support Tasks where the team is supporting and helping other teams

Comments

@jdrush89
Copy link
Contributor

Description

If I have a TabNav within an ActionMenu.Overlay and focusable items above and below it, focus can get stuck in the TabNav and I will be unable to navigate to the top item. It seems like there is a top level focuszone for the overlay and another focuszone within the TabNav. When focus moves into the tabnav, the top level focuszone expects it to move to the rightmost tab, but the tabNav moves focus to the selected tab. This seems to cause the top level focuszone to lose track of which element is focused and it stops processing key presses to move the focus up out of the TabNav.

Steps to reproduce

  1. Go to https://codesandbox.io/s/primer-components-ts-testing-forked-7defd0?file=/src/index.tsx
  2. Click to open the menu
  3. Use the down arrow key to nav to the delete file button
  4. Try to use the up arrow key to get back to the top text field
  5. Observe that focus gets trapped on the selected tab.

Version

v35.9.0

Browser

Chrome

@jdrush89 jdrush89 added the bug Something isn't working label Sep 15, 2022
@davekenn
Copy link

davekenn commented Sep 20, 2022

We are hitting this in our updated react version of the code/blob view and navigation, which we will be opening up to customers for Universe.

@lesliecdubs
Copy link
Member

Thanks for filing, and for adding the Universe context @davekenn.

Is it possible this is a duplicate of #2319? If so, there may be a fix in the works there.

Regardless, we'll plan to triage this at our team sync on Monday Sept 26 and get someone assigned to address this if there hasn't already been movement on a solution in #2319.

@lesliecdubs lesliecdubs added needs triage support Tasks where the team is supporting and helping other teams labels Sep 23, 2022
@iansan5653
Copy link
Contributor

This doesn't look like a duplicate of #2319 - that's related to focuseable elements inside tabs, while this is related to tabs inside overlays (if I understand correctly).

@lesliecdubs
Copy link
Member

👋🏻 @siddharthkp at today's PRC sync, we swapped this in the place of another less urgent support issue. Do you think you could pick this up as part of your FR rotation this week since it's needed for Universe?

@davekenn
Copy link

Any updates?

@lesliecdubs
Copy link
Member

Thanks for pinging us again @davekenn.

@joshblack are you willing to dive into this one by Monday since it's related to a Universe ship? Feel free to bring in our Accessibility Design & Engineering friends if you need any second opinions on the focus behavior.

@joshblack
Copy link
Member

@lesliecdubs definitely! I'll take a look 👀

@joshblack joshblack self-assigned this Oct 21, 2022
@joshblack
Copy link
Member

Something that is a little confusing for ActionMenu in this scenario is that it has role="menu" semantics, so descendants should only be of role="menuitem", role="menuitemradio", or role="menuitemcheckbox" (or a group containing these roles)1.

Is there an alternative that is recommended for this situation where it seems like the intent is more of a disclosure pattern?

Footnotes

  1. https://w3c.github.io/aria/#menu

@joshblack
Copy link
Member

Just a quick update on this, have an interim fix over at: #2468

However, it seems like the ideal here is to avoid using ActionMenu due to role="menu" semantics. Due to these semantics, having items other than menu items will be confusing to screen readers. It seems like using AnchoredOverlay to create something custom is preferred at the moment.

I think this kind of pattern will ultimately be captured over in: https://github.com/github/accessibility/issues/1897 but am not 100% sure.

Hope this helps!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working react support Tasks where the team is supporting and helping other teams
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants