[MenuList] Add text keyboard focus navigation #15495
Conversation
* [ListItem] Remove obsolete comment on focusVisible
|
@material-ui/core: parsed: +0.22% , gzip: +0.33% Details of bundle changes.Comparing: d1df9a8...bb2b2ba
|
* Improved the innerText test to do a more meaningful check
|
I'm merging, I would suspect Sebastian wants to review it too. Now, I believe it's already a great step forward. I'm confident any reported problem will be quickly solved in a continuing pull request. I think that we can focus our energy on #15484. @ryancogswell It took us months, but we are almost at the end of the tunnel! The team is proud of your contributions on Material-UI. |
@oliviertassinari Thanks! It’s been a very good experience. You’re very welcoming and encouraging toward contributors. I’ve learned a lot in the process — about React (especially the full details of refs), Material-UI, and JavaScript testing tools (I’m primarily a Java developer). Our company’s application has a lot of great functionality, but we were losing sales due to “an outdated UI”. We chose Material-UI to help turn that around, and we have received really positive feedback from clients on the parts of our app that have been updated so far. |
…8704) - Initial values of `repeating` and `previousKeyMatched` should be `false` instead of `true` because there are no comparisons initially. - Also, when `handleKeyDown` is called for the first time, there is a comparison between `currTime` (which is always a number) and `criteria.lastTime` (which is initially `null`) causing the code inside `if` statement to exexute which might have been anticipated by the author in this [PR](mui#15495).
…8704) - Initial values of `repeating` and `previousKeyMatched` should be `false` instead of `true` because there are no comparisons initially. - Also, when `handleKeyDown` is called for the first time, there is a comparison between `currTime` (which is always a number) and `criteria.lastTime` (which is initially `null`) causing the code inside `if` statement to execute which might have not been anticipated by the author in this [PR](mui#15495).
Closes #8191
Dependent on #15484 before it will work properly.
There are two aspects in my implementation that I consider optional that could be removed if they aren't considered worth the additional code:
previousKeyMatchedportion of the text criteria. The user-observable behavior is unchanged if this is removed. The purpose is to avoid cycling through all the menu items checking for a match when we already know a match isn't possible because the previously accumulated keys didn't match anything and now the user is adding another key to that unmatched sequence.innerTextwhen available (otherwise falls back totextContent). IfinnerTextandtextContentare different,innerTextwould be the better one to use; howeverinnerTextisn't supported by jsdom (thus the fallback totextContent). In practice, I wouldn't expect these to hardly ever be different in real-world menu item scenarios, so it would likely be fine to just always usetextContent.Neither of these two aspects adds much code, but they only add a very small amount of (mostly hypothetical) value.
I based the behavior on what I observed in Chrome on Windows for native
selectelements (a state list is a convenient case to test out the browser behavior). The 500ms threshold for reset of the key sequence is based on the 0.x implementation mentioned in @kgregory's comment and seems to at least be close to what browsers use, but I didn't go looking for a more official source for what the timing of the reset should be.The gist of the behavior is: