[Select] Fix display when no value is selected

[ianschmitz]

• I have followed (at least) the PR section of the contributing guide.

Fixes #15973.

Outstanding issues to be fixed:

- [ ] Selected menu item background color? See https://material-components.github.io/material-components-web-catalog/#/component/select and notice the use of the primary color on the selected item. Keyboard user can't differentiate the selected item (item equal to the value of the select) vs the item they have highlighted with their keyboard

[mui-pr-bot]

Details of bundle changes.

Comparing: 1052126...95a9fed

bundle	parsed diff	gzip diff	prev parsed	current parsed	prev gzip	current gzip
@material-ui/core	+0.01% 🔺	+0.01% 🔺	320,481	320,509	88,441	88,449
@material-ui/core/Paper	0.00%	0.00%	68,278	68,278	20,363	20,363
@material-ui/core/Paper.esm	0.00%	0.00%	61,573	61,573	19,161	19,161
@material-ui/core/Popper	0.00%	0.00%	28,945	28,945	10,391	10,391
@material-ui/core/Textarea	0.00%	0.00%	5,513	5,513	2,376	2,376
@material-ui/core/TrapFocus	0.00%	0.00%	3,753	3,753	1,578	1,578
@material-ui/core/styles/createMuiTheme	0.00%	0.00%	16,009	16,009	5,791	5,791
@material-ui/core/useMediaQuery	0.00%	0.00%	2,597	2,597	1,102	1,102
@material-ui/lab	0.00%	0.00%	140,293	140,293	43,470	43,470
@material-ui/styles	0.00%	0.00%	51,698	51,698	15,348	15,348
@material-ui/system	0.00%	0.00%	15,420	15,420	4,391	4,391
Button	0.00%	0.00%	84,430	84,430	25,758	25,758
Modal	0.00%	0.00%	14,427	14,427	5,086	5,086
Portal	0.00%	0.00%	3,473	3,473	1,571	1,571
Slider	0.00%	0.00%	74,701	74,701	23,243	23,243
colorManipulator	0.00%	0.00%	3,904	3,904	1,544	1,544
docs.landing	0.00%	0.00%	55,119	55,119	13,940	13,940
docs.main	0.00%	+0.01% 🔺	649,010	649,042	204,784	204,800
packages/material-ui/build/umd/material-ui.production.min.js	+0.01% 🔺	+0.03% 🔺	293,643	293,672	84,340	84,362

Generated by 🚫 dangerJS against 95a9fed

[ianschmitz]

I spent a fair bit of time trying to address the existing Menu/MenuList accessibility regression. After digging into it and comparing the behavior to v3 and the WAI-ARIA guidelines, I believe there are a number of issues that may require a significant rewrite of the current focusing logic. Here are the issues I've identified based on my understanding of the WAI-ARIA guidelines (https://www.w3.org/TR/wai-aria-practices-1.1/#menu) and through practical use:

• The items of a MenuList should be set to tabindex="-1" on all elements except the first item which by default should be tabindex="0". See https://www.w3.org/TR/wai-aria-practices-1.1/#kbd_roving_tabindex for details
  • Select would be an exception in that we should set tabindex="0" on the selected item by default
  • tabindex value should be retained on the currently focused item in the MenuList, except for when using it via Menu. When focusing outside of the MenuList and subsequently focusing back in the list, the focus should return to the previously selected item in the list
• MenuList root element (the ul) shouldn't have a tabindex, nor should it programatically focus on the list itself as it is today in the useEffect
  • This useEffect in MenuList runs on initial mount even in the case of using a Menu that has open={false} and keepMounted. We would need to focus on the first item in the menu when open={true} as the menu is already mounted in the document

Other possible issues unrelated to a11y:

• Menu is being constantly re-rendered when mousing over unrelated parts of the document. Set a console.log in Menu to see it in action - maybe just in the docs? i haven't tested extensively
• The autoFocus prop on Menu doesn't jive IMO. I'm not sure in what scenario a user would want to auto focus on a rendered menu. The menu itself shouldn't be focusable, only the items within it. They may want to focus on a button that opens a menu, or the appropriate item in the menu (which should be done by default), but not the menu itself.
• I see that Menu recently received an additional variant prop ([Menu] Improve performance and add support for variants #15360) that is used in the logic to control focus. I may be wrong but initial first reaction when reading the code is it seems like a work around for the bigger issues we are having with focus?

Hope this helps move things forward. I tried to fix the code as is, but when digging deeper it seems like i would have to significantly refactor these components and wanted to chat with you guys first. I may require your assistance in these changes as well as they appear to be fairly complex with the nature of our component composition and different requirements for the focus depending on the component.

[eps1lon]

Just some quick thoughts without going through all the resources. But I will prioritize this for the rest of the week (research, impl and test). We should get this right at some point.

@ryancogswell did most of the work regarding focus logic in the menu. He should have a better overview over outstanding issues.

In regards to test it would be sufficient for now to write a fixture and place it in ./pages. This can be used for manual testing until we figure out how to proceed with the current test setup. There's currently no robust solution to test this behavior automatically.

Menu is being constantly re-rendered when mousing over unrelated parts of the document. Set a console.log in Menu to see it in action - maybe just in the docs? i haven't tested extensively

Don't bother about it for now. If there is an actual performance issue we should profile it and address it in another PR. As of right now a render call isn't a bottleneck for us.

The autoFocus prop on Menu doesn't jive IMO.

I guess this may be used to forward to focus to the first item in the list. I don't think you focus the whole menu. But I might be missing something.

[ianschmitz]

Just some quick thoughts without going through all the resources. But I will prioritize this for the rest of the week (research, impl and test). We should get this right at some point.

Thank you very much. Let me know if I can help in any way. I'd be happy to help validate any solutions from a user point of view as well! This has been an unfortunate blocker to upgrade to v4 for us due to our accessibility requirements in our products. I appreciate your team's focus on accessibility as it's one of the few component libraries that take it seriously.

Don't bother about it for now. If there is an actual performance issue we should profile it and address it in another PR. As of right now a render call isn't a bottleneck for us.

Agreed it's not an issue for this PR. Seemed fishy so i thought i'd point it out. May be a performance issue for some applications that have many Menu components visible at one time - think a large list of items that each have a context menu.

I guess this may be used to forward to focus to the first item in the list. I don't think you focus the whole menu. But I might be missing something.

I believe you're correct yes, sorry for the confusion. I guess i was thinking more from a API point of view, we have autoFocus on things like Button which will focus when they are mounted, but in this case autoFocus is actually forwarded to MenuList. My thinking is i don't believe there is a good use case for opting out of Menu's behavior to focus on the appropriate menu item when it becomes open (but i may have missed something 😃).

[ryancogswell]

I'll try to take some time within the next few days to respond to @ianschmitz's points above as far as the reasoning behind the changes and links to the discussions related to the changes. I definitely think it is worth more discussion before embarking on a significant rewrite of the focus logic.

[ryancogswell]

@ianschmitz Rather than try to respond to all your points more comprehensively (which would significantly delay receiving my feedback), I'm going to respond to them one-by-one. Then I can at least quickly provide info on some of the more straightforward questions.

The autoFocus prop on Menu doesn't jive IMO.

This functionality existed in v3, but was handled via disableAutoFocusItem. I deprecated this old name and replaced it with autoFocus since focus is no longer always placed on an item and sometimes is placed on the list instead (I'll discuss that decision in a separate comment). The main use case for autoFocus=false is if you want to manage the focus yourself in some different manner than what Material-UI does by default (for instance, by setting the autoFocus property on a particular MenuItem other than the selected or first one). I agree that it would be a bad practice to use autoFocus=false without also specifying some sort of alternative focus management.

[oliviertassinari]

Selected menu item background color? See https://material-components.github.io/material-components-web-catalog/#/component/select and notice the use of the primary color on the selected item. Keyboard user can't differentiate the selected item (item equal to the value of the select) vs the item they have highlighted with their keyboard

@ianschmitz Correct, we had a regression somewhere, the baseline is #5186 (comment):

1. disabled
2. focus
3. selected
4. hover

[oliviertassinari]

The color fix seems simple:

diff --git a/packages/material-ui/src/ListItem/ListItem.js b/packages/material-ui/src/ListItem/ListItem.js
index 054ecfc05..2e41fd85a 100644
--- a/packages/material-ui/src/ListItem/ListItem.js
+++ b/packages/material-ui/src/ListItem/ListItem.js
@@ -23,7 +23,7 @@ export const styles = theme => ({
     paddingTop: 8,
     paddingBottom: 8,
     '&$focusVisible': {
-      backgroundColor: theme.palette.action.selected,
+      backgroundColor: theme.palette.action.hover,
     },
     '&$selected, &$selected:hover': {
       backgroundColor: theme.palette.action.selected,

[eps1lon]

From what I can tell looking at the implementation #15973 is caused by a combination of:

• SelectInput setting focus on the div instead of the menu
• Menu not disabling autofocus of TrapFocus

[oliviertassinari]

Should we already merge these changes? I believe they are already a good first step.

[eps1lon]

Should we already merge these changes? I believe they are already a good first step.

I'd like to wait till the next release. I don't understand why/how this fix works and like to explore the other two possibilities raised above.

[eps1lon]

Alright I got a bit side-tracked by the Menu issue but I think I understand this particular issue now.

I suspect that the current implementation is based on the tests. For example we check if there is an element focused in the keydown handler. But a keydown event will never hit the list unless the list or some child was focused:

The event target of a key event is the currently focused element which is processing the keyboard activity. [...] If no suitable element is in focus, the event target will be the HTML body element if available, otherwise the root element.

-- https://www.w3.org/TR/uievents/#events-keyboard-event-order

So the target of the keydown will be the body, root element or the focused element. If it's the body or root element the keydown will not be triggered in the list. Ergo a keydown on the list will always have an activeElement.

Which brings us to the !list.contains(currentFocus) check: That will never evaluate to true because list.contains(currentFocus) is always true in the keydown handler. It either bubbled up from one of its children or its the list itself which contains itself as well.

So as far as I can tell the fix is "just"

- !list.contains(currentFocus)
+ list === currentFocus

This causes a bunch of tests in MenuList integration tests to fail. I'll try the same approach as with the forwardRef implementation: Rewrite the tests (this time with react-testing-library). Then check if they either already fail, or pass with the new implementation and just require a new test.

Hope that makes sense.

[ryancogswell]

For reference, here are the two main pull requests related to this:

#15495
#15360

The issues closed by these are:
#8191
#10847
#14483
#13708
#13464

[ryancogswell]

So as far as I can tell the fix is "just" list === currentFocus

@eps1lon I agree. Whenever I looked at that code in the past, I wrongly assumed that list.contains was exclusive of list rather than inclusive. This change makes the change in moveFocus unnecessary (and I think the other changes are non-impactful tweaks) since that was dealing with the case where the list has the current focus but disableListWrap=true. With your proposed change, currentFocus will be passed as null in that case.

Along with this change a test should be added for arrow keys where focus is on the list and disableListWrap=true.

[oliviertassinari]

I'm reopening to solve the no value selected height issue.

[eps1lon]

👍

updated

[oliviertassinari]

The new regression test: