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

fix(Select): Touch scroll triggers the select content #807

Merged
merged 5 commits into from
Apr 5, 2024

Conversation

ChrisGV04
Copy link
Contributor

This PR aims to improve the behavior of the Select component on touch devices.

Fixes: #804

The Problem

On the latest version of Radix UI (v1.6.1) if you try to scroll on a touch screen device and the initial tap to begin the scroll gesture fell on the SelectTrigger, it prevents scrolling and immediately opens the SelectContent.

This behavior feels wrong to the end user because they expect to be able to scroll by tapping anywhere on the screen. But if they tap on any SelectTrigger, not only does it lock scrolling, it also opens the SelectContent and feels like a bug.

See #804 for more details.

The Cause

The issue was caused by the SelectTrigger opening the SelectContent via the @pointerdown event, which feels normal with a mouse, but on touch devices it gets triggered immediately after touching the screen, even if it's for a gesture.

This was done because if we triggered the open event on @click, restoring the focus on the SelectTrigger after the content closes caused it to get triggered again, so the SelectContent did not get closed on Enter.

The Solution

This proposal replaces the @pointerdown with @pointerup and follows the same behavior as the DropdownMenuTrigger to prevent focus competition.

It also removes the watchEffect from SelectContentImpl.vue that prevented the @pointerdown event from selecting an option accidentally. Since we now use @pointerup, the pointer is no longer interacting and cannot trigger an item. (This was also the behavior that prevented an option from being selected until a second pointerup event was triggered)

I updated the tests to use @pointerup as well. I also tested on Safari, Chrome and Firefox on both iOS, MacOS, Windows and Android and the behavior seems to work as expected.

@ChrisGV04
Copy link
Contributor Author

Hi @zernonia!

I believe this PR is ready to be reviewed if you're available.

Please let me know if there are any improvements to be made. I'll be glad to help.

@zernonia
Copy link
Collaborator

zernonia commented Apr 3, 2024

Cool! Will have a look shortly 😁

@zernonia
Copy link
Collaborator

zernonia commented Apr 4, 2024

Unfortunately this changes causes the change in behaviour. This idea for this Select component is to mimic the native <select> as close as possible. I have a quick solution.

in pointerdown event, we add

      if (event.pointerType === 'touch')
            return 

in pointerup event, we add

     if (event.pointerType === 'touch')
          handleOpen()

this way, it fixes the touch scrolling issue, and persist the original behavior.

@ChrisGV04
Copy link
Contributor Author

Oh, I didn't know those behaviors of the native select. Always learning something new 😅

I reverted the changes and implemented what you suggested.

I also had to add the touch prevention to the handlePointerUp inside the SelectContentImpl.vue because it was causing it to sometimes need to tap on an option twice to be selected.

Copy link
Collaborator

@zernonia zernonia left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @ChrisGV04 ! 💚🚀

@zernonia zernonia merged commit ad914c5 into radix-vue:main Apr 5, 2024
2 checks passed
@ChrisGV04 ChrisGV04 deleted the fix/select-touch-scroll branch April 5, 2024 04:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Select triggers on touch scroll
2 participants