Menu initial focus does not follow WAI-ARIA recommendations

[ryancogswell]

This is a follow-up to discussion in #16294 with @eps1lon and @ianschmitz, but I didn't want to add this into a closed issue or merged pull request.

In @ianschmitz's comment here, there are two issues pointed out that I want to comment on:

• The lack of a roving tabindex
• Focus being placed on the ul instead of the first item in the list

Thoughts on roving tabindex

Prior to my overhaul of the Menu focus navigation, it did have a roving tabindex. This was managed using state in the MenuList and had significant performance implications. If the roving tabindex is reintroduced, it should be done purely in the DOM without leveraging state (since using state triggers re-renders of the entire menu on focus changes). I, however, don't see value in adding the roving tabindex back in for Menu. As I understand it, the purpose of the roving tabindex is so that if you tab away from a composite widget (see #15597) and then shift+tab back to it, your focus location will be remembered. But for Menu, tab closes the menu and we always want to reset the initial focus location on open of the Menu, so the complexity of the roving tabindex doesn't seem to add any value.

Reasons for focus being placed on the ul instead of the first item

This decision was in response to #14483 which voiced the following concern:

Highlighting the first option in the menu can trick the user into thinking it is already selected.

At the time of deciding to change the focus behavior, I looked at a number of desktop applications (including Chrome) and the behavior of opening a menu with focus on the list (i.e. first down arrow places focus on first item rather than focus starting on the first item) seemed to be at least as common as starting focus on the first item. Also, the Material Design spec didn't indicate anything one way or the other.

At the time, I was completely ignorant of the WAI-ARIA documentation which clearly states here:

When a menu opens, or when a menubar receives focus, keyboard focus is placed on the first item.

I became considerably less ignorant about the WAI-ARIA documentation while writing up #15597. If I had known then (when I was reworking the menu focus logic) what I know now, I would not have changed the default behavior. I do think the new behavior is reasonable and would be worth retaining as an option (perhaps an initialFocusOnList property?), but the default should be in accord with the WAI-ARIA documentation.

One of the reasons I was excited to change the default behavior is that it made it easier to support disabled menu items and dividers appropriately, since I didn't have to worry about the first item being one of those and therefore needing to put the initial focus elsewhere. However, this can be handled in a fairly straightforward manner if this is done in MenuList leveraging the moveFocus function. So if we continue supporting the current behavior via a property, MenuList would use the property within its autoFocus effect to decide whether to set focus to the list or call moveFocus to set focus to the first focusable child.

[ianschmitz]

Thanks for summarizing @ryancogswell.

I, however, don't see value in adding the roving tabindex back in for Menu. As I understand it, the purpose of the roving tabindex is so that if you tab away from a composite widget (see #15597) and then shift+tab back to it, your focus location will be remembered. But for Menu, tab closes the menu and we always want to reset the initial focus location on open of the Menu, so the complexity of the roving tabindex doesn't seem to add any value.

I agree with you for the case of Menu. Are you suggesting we shouldn't implement roving index for MenuList? Not having a roving index can limit the usefulness of MenuList, as it offers a poor experience for keyboard users when entering/leaving the list, especially with many items.

For example we have large lists in some of our application (think 100s of items - in our case it's layers of a map). We use MenuList as it provides a great experience for keyboard users - if they're tabbing through the application they don't have to tab through 100s of items in the list, they can tab to the list and use arrow keys if that's what they're interested in, or they can tab to the next element in the app. With the current approach, when interacting with the list to select an item, when returning to the list they have to cycle through with their arrow keys through potentially hundreds of items to get back to the selected item.

With all that said we may be using the current a11y benefits of the 3.x MenuList more than the average application and I can respect that there may not be an appetite to have a great keyboard user experience if it requires a lot of development effort.

If the roving tabindex is reintroduced, it should be done purely in the DOM without leveraging state (since using state triggers re-renders of the entire menu on focus changes)

This may be low effort? If so it would be a great way to implement and keeps us as close to spec as possible while giving a great experience for keyboard users.

[ryancogswell]

Are you suggesting we shouldn't implement roving index for MenuList? Not having a roving index can limit the usefulness of MenuList

@ianschmitz I have considered MenuList to primarily be an implementation detail of Menu and that it's main usefulness was for creating alternative Menu implementations. Using it as a list for its a11y features is really a workaround for the fact that List isn't treated as a composite widget (which is what #15597 is about).

This may be low effort?

It is definitely much lower effort to add the roving tabindex back in to MenuList (via the DOM) than to add composite widget support to List. It would likely mean using a ref to point at the DOM element for which we last called focus() and then whenever we call focus() we would also set tabindex=0 and set tabindex=-1 for the DOM element in the ref and then update the ref. The bulk of the work would be creating appropriate tests. Might need to query the DOM for the element with tabindex=0 if the ref isn't yet set when focus is changing.

[ianschmitz]

I was playing with the Tree View component, and the accessibility is implemented beautifully. The roving tabindex works as expected. The focusing works exactly as expected. You can focus outside of the component, and focus back in and return to the previously focused list item. Tree views are arguably a much trickier thing to get right a11y wise, so perhaps we could use that implementation as inspiration to solve Menu/MenuList a11y?

[eps1lon]

I got distracted by the test suite not being able to catch this properly. There are some minor issues to resolve to get a proper a11y test suite for this and then we can work on this. Sorry this takes longer than expected.

I just want to make sure that we have a smooth transition once we get better focus handling primitives from react.

[ianschmitz]

No worries. I wanted to let you know that the Tree View seemed to be fully WCAG AA compliant from my knowledge and was a pleasure to use. Good work to whomever developed that!

[eps1lon]

That would be @joshwooding. Really great work!

[joshwooding]

Thanks for the kind words 😊 That TreeView was going to be WCAG AA compliant even if it killed me 😂