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

Tab navigation on OverflowMenu skips the tabbing of upcoming focusable elements #9471

Closed
DavidC123 opened this issue Aug 11, 2021 · 11 comments · Fixed by #12977
Closed

Tab navigation on OverflowMenu skips the tabbing of upcoming focusable elements #9471

DavidC123 opened this issue Aug 11, 2021 · 11 comments · Fixed by #12977

Comments

@DavidC123
Copy link

Brief description

Tab navigation on OverflowMenu causes it to skip the tabbing of upcoming focusable elements and continues tab navigation from top of the DOM.

Environment

Browser
Chrome

Detailed description

What version of the Carbon Design System are you using?
Latest

What did you expect to happen?
After entering (opening) the overflow menu and then tabbing, it should tab to the next focusable element.

What happened instead?
After entering (opening) the overflow menu and then tabbing, it skipped upcoming focusable elements and restarted tab navigation from top of the webpage DOM.

Steps to reproduce the issue

  1. Step one: Tab to an overflow menu
  2. Step two: Open it, then click tab again
  3. Step three: Tab once more and it skips next (upcoming) tab stops

Can be reproduced via the Live demo here: https://www.carbondesignsystem.com/components/overflow-menu/code/

@joshblack
Copy link
Contributor

joshblack commented Aug 16, 2021

Hi there, @DavidC123! 👋

I believe that the OverflowMenu component uses the roving tabindex pattern due to the requirements detailed in Keyboard Navigation: https://w3c.github.io/aria-practices/#keyboard-interaction-12

As a result, you would use arrow keys to navigate instead of the Tab key. Hope this helps! Let me know if you have any questions 😄

@DavidC123
Copy link
Author

Hey @joshblack, thanks for the response! I understand that, but the issue is that it seems to break the flow of tab navigation when tabbing out of the expanded OverflowMenu. It should jump to the next tab element outside of the OverflowMenu component, but it fails to do that. Check out my Steps to reproduce listed in this issue above for a better understanding of what I mean, thanks again Josh. 😃

@joshblack joshblack reopened this Aug 18, 2021
@joshblack
Copy link
Contributor

Thanks, @DavidC123! So sorry I misread that, you're totally right. I really appreciate you clarifying, it helped a ton!

@tw15egan
Copy link
Member

FWIW, adding data-floating-menu-container to the OverflowMenu itself seems to retain the focus order

2021-08-24 12 00 46

@carmacleod
Copy link
Contributor

Please add data-floating-menu-container to the OverflowMenu ul as suggested above. It is really disorienting to a keyboard user to be tabbing along and suddenly have focus thrown back to the body element.

Also, please figure out why the menu ul is in the tab order??? I see that it has tabindex="-1" in the DOM, so either the javascript is setting ul.focus(); or it is temporarily setting tabindex="0" on the ul. The menu itself should never take focus - only the menuitems (li elements) inside the menu should take focus.

Also, I see that the default accessible name for the overflow menu button AND the menu are both aria-label="open and close list of options". This is fine for the button, but weird for the menu. Please change it to simply say aria-label="list of options" for the menu.

Thanks!

@kaushalnavneet
Copy link

Do we have any update on this issue and the above comment added by Carolyn?

@petersandor
Copy link

I'm also affected by this issue in IBM Hyper Protect Crypto Services project, the very same accessibility problem was discovered by our tester.

@tay1orjones tay1orjones added this to the 2023 Q1 milestone Jan 12, 2023
francinelucca added a commit to francinelucca/carbon that referenced this issue Jan 13, 2023
> Fix tab skip content on menu tab-out
> Fix cannot navigate through OverflowMenu with arrows
> Delete unused function
> Small `closeMenuAndFocus` refactor

Closes carbon-design-system#9471
@francinelucca
Copy link
Collaborator

Also, I see that the default accessible name for the overflow menu button AND the menu are both aria-label="open and close list of options". This is fine for the button, but weird for the menu. Please change it to simply say aria-label="list of options" for the menu.

@mbgower looking to get your insight on this? is this a change we should implement for overflowMenu? currently both the button that opens the menu and the menu container have a "Open and close list of options" aria label (or whatever custom ariaLabel is provided but it's the same for both), should we add a separate property for the menu container?

@mbgower
Copy link

mbgower commented Jan 19, 2023

Okay, so I'm assuming the key problems outline in this ticket are addressed, and we're just dealing with the naming comment?

I thought I'd opened a ticket on naming of this already...
I think "open and close list of options" is a real mouthful, even for the button. Why not "options?" It's a button with aria-haspopup set, so the screenreader user is going to hear "Options menu button". Assuming that's also its tooltip, a pointer user will see "options" when they hover over it. Seems like a pretty good generic name for it...

I just had a look at the existing one in storybook right now. I'm seeing an aria-label of "overflow-menu" on the button, with the long string on the SVG.
https://react.carbondesignsystem.com/iframe.html?id=components-overflowmenu--default&viewMode=story

Suggest taking out the aria-label on the button and letting it inherit the name from the svg. Here's what I made it
Screenshot 2023-01-19 at 10 23 54 AM

Sidenote: I note that there is no Carbon tooltip set up on this button. There should be. All icon-only-button should be set up that way so that they're names are revealed on focus and hover. So that should be part of overflow menu.

So, I fired up JAWS just to make sure what I was advising made sense, and there's is something seriously wrong in this implementation on storybook. At the moment, when I open up the kebab, focus goes to Stop app and it is announced, as expected (GOOD). However, arrowing down does nothing.
I tried it with NVDA. Same problem. So there are bigger problems than the naming going on.

@francinelucca
Copy link
Collaborator

francinelucca commented Jan 19, 2023

@mbgower will be fixing tab and arrow navigation on the PR that i'll submit to close out this ticket (here's a demo) just trying to get the aria-labels in order as well .

so in this scenario where we remove the aria-label attribute on the button and leave it in the svg, would the ul element that contains the menu options (when opened) still default to "Open and close list of options" or would it also be changed to "Options"?:
image

("overflow-menu" on this screenshot because we're overriding it by sending in a prop)

@mbgower
Copy link

mbgower commented Jan 19, 2023

I think "options" for the ul should be fine. Because it's a different kind of component, it's fine for them to have the same name. It really shouldn't be in the tab order, etc., but to the degree it is and is announced, the shorter name is better.

@kodiakhq kodiakhq bot closed this as completed in #12977 Feb 2, 2023
kodiakhq bot added a commit that referenced this issue Feb 2, 2023
* fix(overflow-menu): accessibility issues

> Fix tab skip content on menu tab-out
> Fix cannot navigate through OverflowMenu with arrows
> Delete unused function
> Small `closeMenuAndFocus` refactor

Closes #9471

* fix(overflow-menu): a11y updates

* rewrite default aria-labels to "Options"
* use IconButton to display tooltip

* test(overflow-menu): update overflowMenu usage snapshot and update tests

* fix(tooltip): do not override child's onBlur and onFocus eventHandlers

* chore(overflow-menu): remove unnecessary feature flag assertion

* docs(overflow-menu): add tooltip customization docs and test focus el

* fix(overflow-menu): add styles to preserve breadcrumb styling

* fix(icon-button): add closeOnActivation prop on tooltip

icon-button closes on click, enter or space

* fix(overflow-menu): move onKeyDown event to menuBody, fix broken tests

* fix(overflow-menu): do not calculate wrapper height

* fix(overflow-menu): update snapshots to include new wrapper class

* fix(overflow-menu): remove test button from story

---------

Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

9 participants