Overflow menu needs to trap tab keys within menu

[carmacleod]

See https://github.com/IBM/carbon-components-react/issues/1742

[asudoh]

@carmacleod Thank you for writing this up! Just double-checking - Tab from the last and shift-tab from the first in latest code moves focus to the trigger button. It's happening at https://www.carbondesignsystem.com/components/overflow-menu/code, though "floating menu" mode need to be turned on in our React variant to get the similar behavior (you can see it by going to knobs tab at the bottom and check off floatingMenu). Does this issue request for looping within menu instead?

[carmacleod]

Hi, @asudoh. Interesting. Ok, here's what I think, in order of importance:

1. support ESC key closing the menu in react, without accidentally closing the side nav like in vanilla
2. support arrow key navigation within the menu (and wrapping from bottom to top and vice versa) in both vanilla and react
3. then we can talk about whether the current vanilla behavior of wrapping tab/shift+tab to the trigger and closing the menu is ok, or if it should loop within the menu (one problem that might happen is that screen reader users might not know they have reached the end of the menu - we would need to test with screen reader to help decide design). We should also have the discussion whether to only support arrow navigation for these menus (and not support tab navigation within the menu at all... unless the menu contains links). This is one area where user testing would be useful. (i.e. do users really expect to tab through these menus? or do most users expect the arrow keys to work?)

Please note that the current react behavior of dropping down a menu and then allowing tab/shift+tab to escape the menu without closing is not ok. The react floatingMenu behavior for tab/shift+tab is better (we should do the same as whatever we decide in point 3. above).

[asudoh]

💯 thanks a lot for sharing your thoughts @carmacleod!

[stale]

We've marked this issue as stale because there hasn't been any activity for a couple of weeks. If there's no further activity on this issue in the next three days then we'll close it. Thanks for your contributions.

[elizabethsjudd]

@carmacleod @asudoh I've submitted updates to the HTML semantics of the overflow menu (Vanilla only) which should help clarify to screen reader users that they are no longer on the menu. Can we re-test this and see if this issue is still relevant? Personally, I feel like the way that it's currently working is a bit more friendly than trapping the tab key since the overflow menu isn't a dialog and sighted users can interact with the page outside of the overflow menu component.

[carmacleod]

Thanks @elizabethsjudd. So, just to confirm, do the overflow menus at this link contain your new code?
https://www.carbondesignsystem.com/components/overflow-menu/code
(At a glance, it looks like good markup, but I don't want to accidentally test the wrong thing. :)

[elizabethsjudd]

@carmacleod yes the website has the updated code (FYI, if you click on the code pen link it does NOT have the latest JS version and has some missing HTML that is outlined in this issue: #3603)

[carmacleod]

Much better! Thanks @elizabethsjudd !
That works really well with keyboard, and with JAWS and NVDA. I tested in Chrome and FF (except for "JAWS/FF" - I think it wants a reboot, and I have too many windows open at the moment. ;)

The only other thing I would suggest is to add role="presentation" (or role="none") to the <li>'s in the menu. JAWS and NVDA seem to handle it perfectly well without that (they must be using heuristics), but it's the recommended way to mark up a menu, and some validation tools might complain that "list items aren't supposed to be children of a menu". ;)

Regarding the 3rd menu - the one with link items - maybe if the <li> contains a link, then mark up the <li>'s as role="menuitem" and delete the role from the <a>'s so that the screen reader has an opportunity to say "menuitem link"? I tested quickly with that, and NVDA does say "menuitem link" (JAWS just treats it like a menu item, but that seems ok). Anyhow, there are so many arguments in the area of links in menus that I don't really know what to recommend, but if testing shows that marking up link menuitems as <li role="menuitem"><a></a></li> works well in some screen readers and doesn't break anything in others, then I don't see the harm in doing it that way. (Happy to open a new issue for this link menuitem issue, to keep the conversation separate).

[elizabethsjudd]

@carmacleod

The only other thing I would suggest is to add role="presentation" (or role="none")

This is interesting because I saw that and asked @snidersd about it and she said it wasn't needed which is why I removed it from Carbon's code. I also viewed using the native list it informs the user (at least with VO) how many items are in the menu because with lists it tells you your on X item of Y so that it actually helped inform the user know when they are at the end of the list.

the one with link items - maybe if the <li> contains a link, then mark up the <li>'s as role="menuitem" and delete the role from the <a>'s

The reason I had the role on the links and buttons instead of the <li> is because the buttons and links are what are actually getting focused so would they be informed to the role on the <li>? I thought I had done this before (because of the automated testing results) and at least with VO you never heard the menuitem role announced. Given I haven't tested that setup in a while (~ 1 year ago).

I'm currently working on updating the dropdown component which is very closely related to this component so I'd be curious about these points.

[carmacleod]

This is interesting because I saw that and asked @snidersd about it and she said it wasn't needed which is why I removed it from Carbon's code. I also viewed using the native list it informs the user (at least with VO) how many items are in the menu because with lists it tells you your on X item of Y so that it actually helped inform the user know when they are at the end of the list.

Oh, cool! I always thought that was needed because they use it in the Authoring Practices Guide menu examples. But I see now that the mapping for li (parent is a menu) says:

listitem role with aria-setsize value reflecting number of li elements within the parent menu and aria-posinset value reflecting the li elements position within the set.

Which is exactly what you said, and what VO is doing (I assume you tested on Safari and not Chrome?).
I'll raise an issue on the APG to have the role="none" removed from the examples.

[Edit: Confusingly, NVDA/Chrome reads the "1 of 6, 2 of 6" correctly for this APG example, which uses role="none" on the li: http://w3c.github.io/aria-practices/examples/menu-button/menu-button-links.html]

Also, Chrome and FF on Windows aren't following the mapping guidance in the HTML-AAM (Chrome doesn't set aria-setsize or aria-posinset at all, and FF sets aria-setsize=1 aria-posinset=0 so that NVDA says "1 of 1" for every item) - I'll raise issues there, too.

The reason I had the role on the links and buttons instead of the <li> is because the buttons and links are what are actually getting focused so would they be informed to the role on the <li>?

Good point. The screen readers must be special-casing to understand the menu roles when menuitem role is on the li and focus is on the li's child.

[elizabethsjudd]

@carmacleod yes I typically test in VO in Safari as that's the most used combination for mac users. I do have a windows laptop as well that I use for testing JAWS when specific issues arise for it but it's a bit of a pain so I typically focus on VO for my initial testing.

[elizabethsjudd]

I always found it odd how screen readers with various browsers can be so drastically different experiences for the user (VO with Safari vs VO with Chrome). Trying to cover all scenarios is impossible for our very small team so we've really tried to focus on the main combinations (I usually review AIM's yearly report for numbers) but I'd love to know the numbers for actual IBM users but I don't think we (at least in Watson Health) are gathering that information. Do you know how other teams are scoping their testing based on data while still trying to cover while covering any many users as we can?

[asudoh]

Arrow/ESC keys supports in overflow menu have landed in React codebase, so closing. Anybody don't hesitate to speak up if any other discussions like tab between trigger/menu should remain open. Thanks!