Menu keyboard navigation broken in demos

[ryancogswell]

In #15901, the Menu demos were changed to use the keepMounted property. This breaks the auto-focus functionality in MenuList since it is then mounted before it is open and the focus call happens on mount.

• This is not a v0.x issue.
• I have searched the issues of this repository and believe that this is not a duplicate.

Expected Behavior 🤔

After opening the menu in the Simple Menu demo (and other menu demos), the arrow keys should change menu item focus.

Current Behavior 😯

After opening the menu, focus is on the Paper element that wraps the MenuList ul and the arrow keys have no effect. Hitting tab after opening the menu moves the focus from the Paper to the MenuList and then the arrow keys work as intended.

Steps to Reproduce 🕹

Link: https://codesandbox.io/s/material-demo-3obkw

1. Click on the button (or tab to it and hit space or enter
2. Try to use down arrow key to change focus (doesn't work)
3. Tab once
4. Use down arrow key to change focus (does work)

Your Environment 🌎

Tech	Version
Material-UI	v4.1.3

Possible Resolution

The easiest fix is to remove the keepMounted prop from the demo. I'm not sure what the reason was for adding it. Of course, ideally we would fix Menu such that the autoFocus works even with the keepMounted prop. I think this could be achieved by only specifying autoFocus (on MenuList or MenuItem) when open is true. Since focus is called when autoFocus changes to true (in addition to on mount when autoFocus=true), this should take care of the problem.

[eps1lon]

The PR that added keepMounted includes a rationale for it. But even if not it's still a documented prop and the component should support it. I want to revisit the keyboard navigation soon but we have other priorities that come first (see roadmap)

[ryancogswell]

Ah, I found it:

• aria-controls (was aria-owns) pointing to elements that aren't part of the DOM. They should be otherwise aria-controls doesn't add value. aria-controls is used in https://www.w3.org/TR/wai-aria-practices/examples/menu-button/menu-button-links.html

I was looking for something explicitly mentioning keepMounted on my first search through the pull request and missed this bullet point.

But even if not it's still a documented prop and the component should support it.

I agree and it is now more important to support keepMounted well since the examples use it and developers will pattern their usage on the examples (which means they are much more likely to use keepMounted in the future and thus notice this accessibility issue).