[MenuList] Ignore disableListWrap for text focus navigation

[ryancogswell]

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

Addresses issues noted here.

• Native select behavior wraps the navigation when doing text matching but not for arrow keys, so the text matching now ignores the disableListWrap property.

• This makes an attempt to regression test focus-visible logic for MenuList, however it is difficult to do this robustly since the simulate function does not bubble events up the same way as dispatchEvent, so events never reach the document event listeners set up by focusVisible.js. I added a couple usages of dispatchEvent to cause changes to hadKeyboardEvent in focusVisible.js and verified that the focus visible class gets added or not appropriately after focus change.

[mui-pr-bot]

Details of bundle changes.

Comparing: 1ee592a...cd6c90b

bundle	parsed diff	gzip diff	prev parsed	current parsed	prev gzip	current gzip
@material-ui/core	0.00%	0.00%	310,611	310,612	84,595	84,598
@material-ui/core/Paper	0.00%	0.00%	67,705	67,705	20,130	20,130
@material-ui/core/Paper.esm	0.00%	0.00%	61,003	61,003	19,038	19,038
@material-ui/core/Popper	0.00%	0.00%	28,590	28,590	10,290	10,290
@material-ui/core/Textarea	0.00%	0.00%	5,513	5,513	2,381	2,381
@material-ui/core/TrapFocus	0.00%	0.00%	3,780	3,780	1,577	1,577
@material-ui/core/styles/createMuiTheme	0.00%	0.00%	15,943	15,943	5,777	5,777
@material-ui/core/useMediaQuery	0.00%	0.00%	2,106	2,106	977	977
@material-ui/lab	0.00%	0.00%	139,644	139,644	42,535	42,535
@material-ui/styles	0.00%	0.00%	51,234	51,234	15,156	15,156
@material-ui/system	0.00%	0.00%	11,765	11,765	3,924	3,924
Button	0.00%	0.00%	85,318	85,318	25,686	25,686
Modal	0.00%	0.00%	20,366	20,366	6,692	6,692
colorManipulator	0.00%	0.00%	3,904	3,904	1,543	1,543
docs.landing	0.00%	0.00%	51,531	51,531	11,368	11,368
docs.main	0.00%	0.00%	648,808	648,809	202,575	202,577
packages/material-ui/build/umd/material-ui.production.min.js	0.00%	0.00%	292,314	292,315	82,476	82,480

Generated by 🚫 dangerJS against cd6c90b

[eps1lon]

To quickly summarize this for me: Do we wrap or not?

Because not wrapping is only a mac feature. Windows and linux wrap when navigating through items in a list. The WAI-ARIA authoring practices also recommend wrapping. Mac is the clear outlier here and we shouldn't match their behavior.

[oliviertassinari]

@eps1lon 👍, there is a precedent for ignoring MacOS behavior on this select component:
https://github.com/mui-org/material-ui/blob/e36d33a0f3148a21065885a1d91b6fe800153a4c/packages/material-ui/src/Select/SelectInput.js#L156-L157

[ryancogswell]

Do we wrap or not?

@eps1lon Before this change, if disableListWrap: true, text navigation would not wrap. For a select with "ten", "twenty", "thirty", typing "tttt" would stop at "thirty". After this change, text navigation always wraps regardless of the disableListWrap value, so "tttt" would wrap back around to "ten". The arrow key navigation is unchanged -- it always honors disableListWrap. Also unchanged, SelectInput sets disableListWrap to true.

Far as I can tell, this is the behavior for <select> elements in the browser on both Mac and Windows -- arrow keys do not wrap, but text matching does wrap.

[eps1lon]

That's not matching the WAI-ARIA recommendations: Open the listbox and press "f" repeatedly. It will stop at the last item and not wrap.

[eps1lon]

Maybe the question is if our Select is actually implementing the listbox pattern.

[ryancogswell]

Open the listbox and press "f" repeatedly. It will stop at the last item and not wrap.

@eps1lon Where is this quote from. This doesn't seem to be in sync with the behavior here: https://www.w3.org/TR/wai-aria-practices/examples/listbox/listbox-collapsible.html. In their example arrow keys don't wrap, but text matching does.

[eps1lon]

That was github not properly formatting my comment. This was the behavior I observed. The issue was that wrapping is debounced. Press is semi-quickly and it won't work. You need to explicitly pause. Not sure if it is just laggy or intended.

[eps1lon]

Oh now I understand it. It buffers the input:

Type multiple characters in rapid succession: focus moves to the next item with a name that starts with the string of characters typed.

[ryancogswell]

My implementation is modeled off of the browser select behavior which is slightly more sophisticated. They buffer in a similar way, but if the buffer only contains a single repeated character then it just matches against the first character. This, for example, allows for quickly getting to Minnesota in a select of states by repeatedly typing “m” without having to wait for the buffer reset. Once a non-repeating character is entered, the full buffer is honored for matching. My tests cover these conditions pretty thoroughly leveraging “aardvark” to test the transition from repeating to non-repeating mode.

[eps1lon]

I think the WAI-ARIA example works similarly but I find it pretty unintuitive since the buffer is invisible. Thanks for clearing this up.

[ryancogswell]

The WAI-ARIA example seems to always honor the full buffer, so you can’t do fast repeated characters. Instead you have to wait for a buffer reset after each key in order for repeated characters to advance to the next one.