Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Autocomplete] Fix navigation issue on mouse hover #35196

Merged
merged 25 commits into from
Apr 4, 2023

Conversation

sai6855
Copy link
Contributor

@sai6855 sai6855 commented Nov 18, 2022

Fixes #30178

Problem: handleOptionMouseOver from below file is expected to run only when mouse is over an option but the function is called even when dom updates happening in backgroud when listbox is scrolled.

https://github.com/mui/material-ui/blob/master/packages/mui-base/src/AutocompleteUnstyled/useAutocomplete.js.

I learnt this from downshift code base.

https://github.com/downshift-js/downshift/blob/4c3d3eab8a5d1a826f792d0b8d1302715400d7ed/src/downshift.js#L923

Solution: instead of using onMouseOver event, i used onMouseMove event as onMouseMove runs only when mouse moves.

Just FYI: even v4 autocomplete also has same issue

Before: https://www.loom.com/share/81bc73d696a84d87acc2919018350244

After: https://www.loom.com/share/cdb34134e21643c3a1ab04c730cb4361

@sai6855 sai6855 marked this pull request as draft November 18, 2022 14:05
@sai6855 sai6855 changed the title use onMouseMove instead of onmouseOver [Autocomplete] use onMouseMove instead of onmouseOver in useAutoComplete.js Nov 18, 2022
@mui-bot
Copy link

mui-bot commented Nov 18, 2022

Netlify deploy preview

https://deploy-preview-35196--material-ui.netlify.app/

Bundle size report

Details of bundle changes

Generated by 🚫 dangerJS against 953fa54

@sai6855 sai6855 marked this pull request as ready for review November 18, 2022 16:44
@sai6855 sai6855 changed the title [Autocomplete] use onMouseMove instead of onmouseOver in useAutoComplete.js [Autocomplete] fix navigation issue on mouse hover Nov 18, 2022
@zannager zannager added the component: autocomplete This is the name of the generic UI component, not the React module! label Nov 21, 2022
@sai6855
Copy link
Contributor Author

sai6855 commented Nov 22, 2022

@ZeeshanTamboli can you review this pr

Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test passes on master too, it's probably not testing exactly what is needed.

@sai6855 sai6855 marked this pull request as draft November 24, 2022 06:28
@sai6855
Copy link
Contributor Author

sai6855 commented Nov 29, 2022

@ZeeshanTamboli i'm having a trouble to write the test case for the #35141 , can you please help me on how to write the test case, the test i've written in this PR is also passing in master branch.

To make things easy for you, i'm attaching video of the bug.

In the video you can see, after pulpfiction option Godfather option is getting highlighted ideally Lord of the rings option should be highlighted.

Screen.Recording.2022-11-29.at.10.07.43.PM.mov

@ZeeshanTamboli
Copy link
Member

@sai6855 Even I am not able to write a failing test case. Maybe @michaldudak or @mnajdova can help.

However, your fix looks good to me. Also, your issue is a duplicate of #30178. So I updated the PR description to fix #30178 and marked #35141 as a duplicate.

@sai6855 sai6855 marked this pull request as ready for review December 1, 2022 07:26
@sai6855
Copy link
Contributor Author

sai6855 commented Dec 1, 2022

Thanks @ZeeshanTamboli for taking a look, let's wait for team to help us out.

@sai6855
Copy link
Contributor Author

sai6855 commented Dec 19, 2022

@michaldudak can you help me to write failing test case in master

@michaldudak
Copy link
Member

michaldudak commented Feb 10, 2023

I can't think of a way to unit test this either, as it depends on having a mouse cursor.
It may be possible to use the e2e test for this (/test/e2e/fixtures), but I'm not 100% sure.

As for the new behavior, it seems to be consistent with the native select tags. We will likely change this in the future, so hovering over an item won't highlight it, but this will do for now.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 21, 2023
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Mar 5, 2023
test/e2e/index.test.ts Outdated Show resolved Hide resolved
test/e2e/index.test.ts Outdated Show resolved Hide resolved
@sai6855
Copy link
Contributor Author

sai6855 commented Mar 5, 2023

@ZeeshanTamboli @michaldudak @mnajdova added test in e2e fixtures. Tested newly added test in master branch it was failing

test/e2e/fixtures/Autocomplete/HoverJoyAutocomplete.tsx Outdated Show resolved Hide resolved
test/e2e/index.test.ts Outdated Show resolved Hide resolved
test/e2e/index.test.ts Outdated Show resolved Hide resolved
test/e2e/index.test.ts Outdated Show resolved Hide resolved
test/e2e/index.test.ts Outdated Show resolved Hide resolved
test/e2e/index.test.ts Outdated Show resolved Hide resolved
@sai6855 sai6855 requested review from mnajdova and removed request for michaldudak March 16, 2023 09:39
@ZeeshanTamboli ZeeshanTamboli added the bug 🐛 Something doesn't work label Mar 20, 2023
@ZeeshanTamboli ZeeshanTamboli changed the title [Autocomplete] fix navigation issue on mouse hover [Autocomplete] Fix navigation issue on mouse hover Mar 20, 2023
test/e2e/index.test.ts Outdated Show resolved Hide resolved
test/e2e/index.test.ts Outdated Show resolved Hide resolved
Copy link
Member

@ZeeshanTamboli ZeeshanTamboli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work @sai6855. It looks good to me. But I'll let @michaldudak review it before merging.

Copy link
Member

@siriwatknp siriwatknp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 I love this fix! I encountered this bug multiple times before.

@siriwatknp siriwatknp merged commit 6776cb2 into mui:master Apr 4, 2023
@sai6855 sai6855 deleted the autocomplete-hover-navigation branch April 20, 2023 08:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: autocomplete This is the name of the generic UI component, not the React module!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Autocomplete] cursor resets highlighted item when using arrow keys
7 participants