[autocomplete] Enable clearing highlight when mouse leaves popup#48354
Conversation
mj12albert
added
scope: autocomplete
Deploy previewBundle size
Check out the code infra dashboard for more information about this PR. |
fb60010 to
68399e4
Compare
| setHighlightedIndex({ | ||
| event, | ||
| index: -1, | ||
| reason: 'mouse', |
There was a problem hiding this comment.
| reason: 'mouse', | |
| reason: null, |
Should it be null instead to indicate reset?
There was a problem hiding this comment.
index: -1 indicates that this is a reset, reason: 'mouse' indicates that it was a mouse interaction that caused the reset
Though this is internal, reason is not nullable in the implementation, since all highlight changes must be caused by one of mouse/keyboard/touch
siriwatknp
left a comment
siriwatknp
left a comment
There was a problem hiding this comment.
Looks good overall, need final decision from @silviuaavram for thenull reason
| function getActiveDescendant(textbox) { | ||
| const activeDescendantId = textbox.getAttribute('aria-activedescendant'); | ||
|
|
||
| return activeDescendantId ? document.getElementById(activeDescendantId) : null; | ||
| } | ||
|
|
There was a problem hiding this comment.
I guess this PR fixes the accessibility issue as well right?
There was a problem hiding this comment.
@siriwatknp Yep, it goes through the normal setHighlightedIndex which removes aria-activedescendant accordingly when the highlight is cleared (if that's what you're referring to)
The helper fn here is just to help make the actual assertions more readable
6871ee1 to
aca7926
Compare
ZeeshanTamboli
left a comment
ZeeshanTamboli
left a comment
There was a problem hiding this comment.
Should we add this prop example in the Autocomplete docs playground: https://mui.com/material-ui/react-autocomplete/#playground or perhaps a new demo altogether?
9e50efa to
bb703f3
Compare
Added it to the free solo demos where stale mouse highlights can affect the behavior of Enter which should be enough here (I'm refreshing this doc in a different PR) |
7b85bee to
68256d1
Compare
| readOnly?: boolean | undefined; | ||
| /** | ||
| * If `true`, clears an option highlighted by mouse movement when the mouse leaves the listbox. | ||
| * @deprecated This behavior will be enabled by default in the next major version. |
There was a problem hiding this comment.
What's the logic here?
As a developer, this is how the demo looks:
So I would try to remove that resetHighlightOnMouseLeave prop. So I would read the prop description to figure out how, but then the description tells me that it's already correct 🤔? So I guess we need to remove the deprecation.
There was a problem hiding this comment.
This is a good point which got unnoticed!
There was a problem hiding this comment.
Alright, it looks like we are good now. Thanks
While I was at it, I have created #48547 so we can keep track of all those ideas for future breaking changes.
5 participants
Add a
resetHighlightOnMouseLeaveprop that clears mouse highlight when the pointer moves out of the listbox when true. (Matches Base UI combobox behavior) Prop marked@deprecatedso we can make this the default behavior in the future, replaceable withdisableResetHighlightOnMouseLeaveif necessary.Demo: https://stackblitz.com/edit/njytbidp?file=src%2FDemo.tsx
Manual testing steps:
Movieinput to open the popup.The Godfather. It should become highlighted.ArrowDown. Expected: keyboard navigation starts from the first option,The Shawshank Redemption, confirming the internal highlight was reset.Closes #23916
Preview: https://deploy-preview-48354--material-ui.netlify.app/material-ui/react-autocomplete/