MenuList is not accessible

[ianschmitz]

Creating this to track the follow-up fixes from #16644 for MenuList. #17506 solves the Menu focus issue which seems to work well, but we still need proper keyboard support in MenuList.

See #16644 for in-depth discussion on this issues.

Tech	Version
Material-UI	v4.4.2

[eps1lon]

Yep thanks for opening this. I want to move the keyboard navigation to menulist and menu only handles initial focus. Basically MenuList implements listbox and Menu is just a styled wrapper.

[eps1lon]

It won't be a listbox since this would require full focus managment (either via roving tabIndex or activedescendant).

However, MenuList will have proper keyboard navigation documented. Since it's only a component that should be used in the menu (hence the MenuList) it won't work without configuration. We can't just focus it when it mounts since we don't know if it's acccessible and autoFocus is generally a bad idea.

With #17571 we will end up with <MenuList autoFocusItem /> where library consumers need to decide if it's ok to set this prop.

[ianschmitz]

Since it's only a component that should be used in the menu (hence the MenuList) it won't work without configuration.

I'm not sure I understand this part. Are you saying MenuList shouldn't be used by itself?

[eps1lon]

Since it's only a component that should be used in the menu (hence the MenuList) it won't work without configuration.

I'm not sure I understand this part. Are you saying MenuList shouldn't be used by itself?

At least it's not intended to. It should already work perfectly fine with regard to keyboard navigation if used with MenuItem. But it doesn't implement (nor is the intent of this component) to implement [role="listbox"] since that requires some persistent focus managment which we don't need for Menu.

It probably makes sense to implement a listbox at some point or provide persistent focus managment out of the box. Otherwise I'm not sure what issue we're talking about? I thought it was mainly about our MenuList usage in the docs.

[ianschmitz]

We've been using MenuList as the solution for a listbox type control as in the docs it says:

The primary responsibility of the MenuList component is to handle the focus.

This worked fantastic in v3 and is a great way to give a good user experience for keyboard users when browsing long lists instead of having to tab through each item in the list.

The main thing missing right now is that the MenuItems don't have a tabindex set so we're unable to focus on the first item in the MenuList.

[ianschmitz]

I seem to remember in v3 that the MenuList was responsible for the roving tabindex focusing management, and the Menu added the popover and autofocus behavior. This was refactored in v4.

[mbrookes]

See also #15597...

[eps1lon]

Yeah I this is a bit unfortunate that this was used that way. Managing the active descendant was never necessary for MenuList since it was only ever used in a menu context. The MenuItem itself is a bit to powerful right now. It doesn't make sense to use it for a Select for example.

So MenuList does implement keyboard navigation but is not concerned with persisting the active descendant.

Otherwise it would help if you could add a test that is currently failing. MenuItem should always have a tabIndex. Anything else would be a bug.

[ianschmitz]

I understand where you guys are coming from now. I think the root of the issue is we were using MenuList for uses it wasn't intended for. My apologies.

Here's an example to illustrate a couple of common cases we've run into where MenuList's previous keyboard a11y features were extremely beneficial for keyboard users: https://codesandbox.io/s/material-demo-b1z97

Indeed #15597 seems promising @mbrookes.

I've said it before but once again - thanks for taking accessibility seriously in MUI!