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 list scrolls to the top when new data is added on touch devices #36231

Merged
merged 7 commits into from
Mar 6, 2023

Conversation

SaidMarar
Copy link
Contributor

@SaidMarar SaidMarar commented Feb 16, 2023

closes: #36212

The reason it is working on desktop is because when we mouseover an option we update highlightedIndex in handleOptionMouseOver , However, on mobile, the index is not updated when we touch the option, which results in the index remaining at -1 and causing the list to scroll to the top

before: https://codesandbox.io/s/mui-autocomplete-before-tmnvsm
after: https://codesandbox.io/s/mui-autocomplete-after-q32885

@mui-bot
Copy link

mui-bot commented Feb 16, 2023

Netlify deploy preview

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

Bundle size report

Details of bundle changes

Generated by 🚫 dangerJS against 7983071

@zannager zannager added the component: autocomplete This is the name of the generic UI component, not the React module! label Feb 17, 2023
@zannager zannager requested a review from michaldudak February 17, 2023 09:14
@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 Feb 22, 2023
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.

In the preview package CodeSandbox of this PR, the scrollbar is still resetting in mobile view - https://codesandbox.io/s/mui-autocomplete-after-silyqj

@ZeeshanTamboli ZeeshanTamboli added the bug 🐛 Something doesn't work label Feb 26, 2023
@ZeeshanTamboli ZeeshanTamboli changed the title [Autocomplete] fix scroll on mobile when options list is changed [Autocomplete] Fix scroll on mobile when options list is changed Feb 26, 2023
@SaidMarar
Copy link
Contributor Author

In the preview package CodeSandbox of this PR, the scrollbar is still resetting in mobile view - https://codesandbox.io/s/mui-autocomplete-after-silyqj

@ZeeshanTamboli Thanks, I just updated the dependencies, you can check the same link.

@ZeeshanTamboli
Copy link
Member

In the preview package CodeSandbox of this PR, the scrollbar is still resetting in mobile view - https://codesandbox.io/s/mui-autocomplete-after-silyqj

@ZeeshanTamboli Thanks, I just updated the dependencies, you can check the same link.

@SaidMarar I am still not able to see it fixed in https://codesandbox.io/s/mui-autocomplete-after-silyqj.

@SaidMarar
Copy link
Contributor Author

@ZeeshanTamboli Did you tested it in mobile simulator using the touch pointer to scroll ? If yes, are you observing the same behavior of resting scroll to the top ?

@ZeeshanTamboli
Copy link
Member

ZeeshanTamboli commented Feb 28, 2023

@ZeeshanTamboli Did you tested it in mobile simulator using the touch pointer to scroll ? If yes, are you observing the same behavior of resting scroll to the top ?

I didn't test in mobile simulator but in desktop browser in mobile view and saw it's still resetting. It has the touch pointer.

@SaidMarar
Copy link
Contributor Author

SaidMarar commented Feb 28, 2023

@ZeeshanTamboli I tested it in both, could you please check the video. I can make another for browser, if i get some time.

Upload.from.GitHub.for.iOS.MOV

@SaidMarar
Copy link
Contributor Author

An example on mobile view:
scroll mobile

@ZeeshanTamboli
Copy link
Member

Okay, I checked on mobile instead and it works. I don't know why it didn't work in mobile device mode in desktop browser:

Screen.Recording.-.1.March.2023.mp4

@SaidMarar
Copy link
Contributor Author

Okay, I checked on mobile instead and it works. I don't know why it didn't work in mobile device mode in desktop browser:

Ah I see ! It is because you are using the wheel to scroll and it is not the case in mobile, you need to scroll using the touch pointer. (this is the fix we did we save the option on touchStart in mobile. For desktop we already have mouseOver)

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Mar 2, 2023
@SaidMarar
Copy link
Contributor Author

@ZeeshanTamboli just updated, thank you for reviewing this 👍

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Mar 2, 2023
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.

Looks good!

@ZeeshanTamboli ZeeshanTamboli changed the title [Autocomplete] Fix scroll on mobile when options list is changed [Autocomplete] Fix list scrolls to the top when new data is added on touch devices Mar 6, 2023
@ZeeshanTamboli ZeeshanTamboli merged commit 009b223 into mui:master Mar 6, 2023
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] List scrolls to the top when new data is added (only on mobile)
4 participants