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

[a11y]: OverflowMenuItem, None of the descendent elements with "menuitem" role is tabbable #12728

Open
2 tasks done
mateBe95 opened this issue Nov 25, 2022 · 21 comments · Fixed by #13823
Open
2 tasks done

Comments

@mateBe95
Copy link

Package

carbon-components

Browser

Chrome

Operating System

No response

Package version

Newest

React version

16.12.0

Automated testing tool and ruleset

IBM Equal Accessibility Checker

Assistive technology

No response

Description

image

None of the descendent elements with "menuitem" role is tabbable

WCAG 2.1 Violation

No response

Reproduction/example

https://react.carbondesignsystem.com/iframe.html?id=components-overflowmenu--default&viewMode=story

Steps to reproduce

https://react.carbondesignsystem.com/iframe.html?id=components-overflowmenu--default&viewMode=story

1.Open menu.
2. Scan.

Code of Conduct

@tay1orjones
Copy link
Member

@mbgower @tombrunet Is this a valid failure? @francinelucca just addressed the keyboard navigation in #12977. OverflowMenuItem navigation happens via arrow keys, not by tabbing.

@mateBe95
Copy link
Author

@francinelucca
Copy link
Collaborator

francinelucca commented Mar 13, 2023

Any updates here @tay1orjones the issue still seems to be occurs https://react.carbondesignsystem.com/iframe.html?id=components-overflowmenu--default&viewMode=story

@mateBe95 In the overflowmenu item navigation is done using the up/down arrow keys. @mbgower bumping, is there something else we should be considering here regarding this violation?

@mbgower
Copy link

mbgower commented Mar 14, 2023

I don't understand the issue. I'm pulling in @tombrunet to have a look.
I conjecture it might be because you're using buttons for the menuitems and putting the negative tabindex on them, and it's setting off a rule. But since they've been given a role of menuitem, I don't see the problem; they're reachable by arrow key.

I am seeing an oddity where when I press tab after opening the menu, the focus is going around the whole list of options. That suggests there's something a little dodgy in the code?

@tombrunet
Copy link
Contributor

I need to think about this one a little bit. It's a little weird because as soon as you tab away, it goes away, so the need to tab back isn't really there. The button that launches the menu may need an aria-controls on it to indicate that it's what is launching the menu. But, I'm not sure we handle that situation for this rule yet.
One bit that gives me a little pause is that if you use the mouse, and then click to the browser, the menu is still open. It seems like you can shift tab back to the menu without it disappearing. If the menu went away any time that focus left, it might be a better argument for the tab to not be required.
All of that said, typical menus would have the menu be tabbable with active-descendant set to the appropriate menuitem, or would allow the menuitems to be tabbable. It certainly wouldn't hurt anything to do that here, but again, I need to think a little more about if there are any issues in this particular situation if you don't do that.

@francinelucca
Copy link
Collaborator

One bit that gives me a little pause is that if you use the mouse, and then click to the browser, the menu is still open. It seems like you can shift tab back to the menu without it disappearing. If the menu went away any time that focus left, it might be a better argument for the tab to not be required.

Not quite sure how to reproduce this one but we should be able to make the menu close when it loses focus if desired, it might be a bug that it doesn't currently. We can also explore the tabbable-menu option is that is the guidance.

@tay1orjones tay1orjones removed the status in Design System Apr 11, 2023
@tay1orjones tay1orjones moved this to 🕵️‍♀️ Triage in Design System Apr 11, 2023
@tay1orjones tay1orjones changed the title [a11y]: Overflowmenu item [a11y]: OverflowMenuItem, None of the descendent elements with "menuitem" role is tabbable Apr 17, 2023
@tay1orjones tay1orjones moved this from 🕵️‍♀️ Triage to 🪆 Needs Refined in Design System Apr 17, 2023
@tay1orjones
Copy link
Member

Would the new OverflowMenuV2 implementation solve this to some extent? The items are still not tabbable, but it uses the new Menu primitives and might be more properly applying active-descendant?

@francinelucca
Copy link
Collaborator

Synced up with a11y folks, tried adding aria-controls to button that controls ul visibility and confirmed still experience Accessibility Checker violation. This is potentially a false positive. Further research upcoming on IBMa/equal-access#1402.

*We still want to add the aria-controls attribute to the button on our end.

@tay1orjones tay1orjones moved this from 🪆 Needs Refined to ⏱ Backlog in Design System Apr 18, 2023
@zhanttbj
Copy link

Hi Team, can we have this issue fixed by end of April? thanks

@francinelucca
Copy link
Collaborator

@zhanttbj It was determined this violation is a false positive from the a11y checker in this case; the fix for that is being tracked IBMa/equal-access#1402. The menu does not use tabs to navigate, it uses up/down arrow.
We're going to add the aria-controls attribute as part of this issue but that won't change the behavior of the component and won't fix the violation until IBMa closes that issue out.

@francinelucca francinelucca moved this from ⏱ Backlog to 🏗 In Progress in Design System May 17, 2023
@francinelucca francinelucca moved this from 🏗 In Progress to 🚦 In Review in Design System May 17, 2023
@janhassel
Copy link
Member

@francinelucca @tombrunet Is aria-controls still needed when the relationship between the button and the menu is already established with aria-owns (as the OverflowMenuV2 is doing)?

@tombrunet
Copy link
Contributor

A button shouldn't own a menu. aria-owns is puts the referenced subtree as a descendant of that node. Since a button can't have any semantics within it, that would strip the semantics of the menu.

@francinelucca
Copy link
Collaborator

does that mean we should remove the aria-owns property from OverflowMenuV2 and MenuButton? @janhassel @tombrunet 🤔.
I see OverflowMenuV2 is setting it on the parent div , but MenuButton is setting it on the button itself

@tombrunet
Copy link
Contributor

tombrunet commented May 19, 2023

If the intent is to have the menu item appear immediately after the button, but you can't do that because of CSS / portal issues, you would have some sort of empty node after the button that owns the menu (e.g., <div aria-owns="menuId" />) or perhaps the parent.

@janhassel
Copy link
Member

@tombrunet @francinelucca

Good point. The OverflowMenuV2 currently places the aria-owns on a container node:

<div aria-owns="menu-id">
  <button></button>
</div>

<ul id="menu-id">...</ul>

From my manual testing with VoiceOver aria-owns worked great for announcement and focus navigation.

I can update MenuButton and ComboButton as part of #13678 if you agree!

@francinelucca
Copy link
Collaborator

@tombrunet any input/concern here about Jan's proposal of using aria-owns in the button's parent div/span over using aria-controls in the button itself?

@tombrunet
Copy link
Contributor

The menu would be owned on the div, but the button still needs aria-controls to indicate what the button is doing, so you need both.

@francinelucca
Copy link
Collaborator

francinelucca commented May 23, 2023

The menu would be owned on the div, but the button still needs aria-controls to indicate what the button is doing, so you need both.

got it, thanks!! @tombrunet

@kodiakhq kodiakhq bot closed this as completed in #13823 May 24, 2023
@github-project-automation github-project-automation bot moved this from 🚦 In Review to ✅ Done in Design System May 24, 2023
@tay1orjones
Copy link
Member

The menu would be owned on the div, but the button still needs aria-controls to indicate what the button is doing, so you need both.

@tombrunet This was done in #13678 but it still fails aria_child_tabbable #13678 (comment)

So my understanding (correct me if I'm wrong here) is that this is still blocked by IBMa/equal-access#1402. I'll reopen this for now until that issue is resolved.

@tay1orjones tay1orjones reopened this Jun 6, 2023
@github-project-automation github-project-automation bot moved this from ✅ Done to ⏱ Backlog in Design System Jun 6, 2023
@tw15egan
Copy link
Collaborator

I believe I'm also running into this same issue with Dropdown and adding more AVT tests. You can see the issue by running the a11y checker with the Dropdown menu open: https://react.carbondesignsystem.com/iframe.html?args=&id=components-dropdown--default&viewMode=story

Refs #14211

@tombrunet
Copy link
Contributor

@tay1orjones It looks like our issue was closed shortly after you re-opened this one. Are you still getting this issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment