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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Dialog][AlertDialog][Popover][DropdownMenu] Prevent pointer-events reenabling prematurely (mobile) #767

Merged
merged 4 commits into from Jul 6, 2021

Conversation

jjenzz
Copy link
Contributor

@jjenzz jjenzz commented Jul 5, 2021

Fixes #631
Fixes #761
Fixes #762
Fixes #763

I managed to fix these all in one go eventually.

@benoitgrelard I believe I've also managed to provide a solution that merges both of our suggestions. My main concern was to keep the fix within useBodyPointerEvents since the issue is a side-effect of pointer-events enabling too soon, but I've only enabled the delay on pointerdown (because like you say, that's when it's an issue) and it's a touch or pen pointer.

Happy to continue to discuss alternate solutions as well if we have any because obviously, setTimeouts are never ideal.


UPDATE: Delay has been removed in favour of waiting for click event. I gave myself the idea here #767 (comment) 馃槄

@jjenzz jjenzz force-pushed the fix-pointer-events-mobile branch 3 times, most recently from 23978da to 93babbd Compare July 5, 2021 14:09
@jjenzz jjenzz force-pushed the fix-pointer-events-mobile branch from 93babbd to 876939e Compare July 5, 2021 14:16
@jjenzz
Copy link
Contributor Author

jjenzz commented Jul 5, 2021

Alternatively, we can change onPointerDownOutside to onClickOutside. I know that will make closing feel a bit slower but I'm not sure that making it feel more instantaneous is worth these side effects/timers.

@jjenzz jjenzz force-pushed the fix-pointer-events-mobile branch 2 times, most recently from c2484eb to 83fff03 Compare July 5, 2021 17:19
@jjenzz jjenzz marked this pull request as draft July 5, 2021 17:20
@jjenzz jjenzz force-pushed the fix-pointer-events-mobile branch 2 times, most recently from bf88e97 to f302975 Compare July 5, 2021 17:32
@jjenzz jjenzz marked this pull request as ready for review July 5, 2021 17:40
@jjenzz jjenzz force-pushed the fix-pointer-events-mobile branch from f302975 to c22c2c9 Compare July 5, 2021 17:45
Copy link
Collaborator

@benoitgrelard benoitgrelard left a comment

Choose a reason for hiding this comment

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

Love the direction of this new solution!

One thing I've been wondering: does this also negate the need for the change I have made in DismissableLayer's usePointerDownOutside?

@andy-hook
Copy link
Collaborator

andy-hook commented Jul 6, 2021

Love this!

@jjenzz
Copy link
Contributor Author

jjenzz commented Jul 6, 2021

One thing I've been wondering: does this also negate the need for the change I have made in DismissableLayer's usePointerDownOutside?

@benoitgrelard Potentially, I'll try replicate the issue you were experiencing and confirm.

@jjenzz
Copy link
Contributor Author

jjenzz commented Jul 6, 2021

Spoke to Benoit about his issue and we think the fix there is still required since the fix in this PR will only apply to modal things.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants