react icon indicating copy to clipboard operation
react copied to clipboard

focus gets stuck in TabNav focuszone within ActionMenu.Overlay

Open jdrush89 opened this issue 3 years ago • 1 comments

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 avatar Sep 15 '22 17:09 jdrush89

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.

davekenn avatar Sep 20 '22 14:09 davekenn

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

Is it possible this is a duplicate of https://github.com/primer/react/issues/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 avatar Sep 23 '22 00:09 lesliecdubs

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).

iansan5653 avatar Sep 23 '22 14:09 iansan5653

👋🏻 @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?

lesliecdubs avatar Sep 26 '22 23:09 lesliecdubs

Any updates?

davekenn avatar Oct 20 '22 19:10 davekenn

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.

lesliecdubs avatar Oct 21 '22 02:10 lesliecdubs

@lesliecdubs definitely! I'll take a look 👀

joshblack avatar Oct 21 '22 16:10 joshblack

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?

joshblack avatar Oct 21 '22 16:10 joshblack

Just a quick update on this, have an interim fix over at: https://github.com/primer/react/pull/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!

joshblack avatar Oct 24 '22 18:10 joshblack