adobe / react-spectrum

A collection of libraries and tools that help you build adaptive, accessible, and robust user experiences.
https://react-spectrum.adobe.com
Apache License 2.0
12.23k stars 1.07k forks source link

Press seems to be double triggered for overlaying buttons on chrome simulated touch #1279

Open djfarly opened 3 years ago

djfarly commented 3 years ago

πŸ› Bug Report

Given a dialog with a close button exactly overlaying the open button (not unusual for burger-menu-style navigation), clicking the open button with chrome simulated touch, triggers the close callback of the close button directly after the open callback (in ~90% of the cases?! πŸ˜“) resulting in the dialog closing immediately. (See Code Sample)

(open and close refer to stately's useOverlayTriggerState setters)

πŸ€” Expected Behavior

Clicking the open button with simulated touch in chrome only triggers open.

😯 Current Behavior

Clicking the open button with simulated touch in chrome triggers open, then close immediately after.

πŸ’ Possible Solution

🧐

πŸ”¦ Context

We're building a "Drawer" component / mobile navigation where the open and close button are in the same screen location.

It's probably not the worst thing ever. I could not reproduce this with any form of real click or real touch input. Also Firefox simulated touch seems to work fine.

It still is kinda frustrating since chrome is our main development environment.

πŸ’» Code Sample

Here is a code sandbox showcasing the behavior.

https://codesandbox.io/s/ckmr5?file=/src/App.js

https://ckmr5.csb.app/

🌍 Your Environment

Software Version(s)
react-aria 3.1.1
react-stately 3.0.1
Browser Chrome Version 87.0.4280.63
Operating System macOS 10.15.7 (19H15)

🧒 Your Company/Team

πŸ•· Tracking Issue (optional)

djfarly commented 3 years ago

Also note, that this only happens the first few times using the button. After a clicking for a while it magically ✨ starts working as intended - adding to my confusion.

snowystinger commented 3 years ago

Could you disable one of the buttons based on which one should be interacted with? or just not render it? <Button onPress={open} isDisabled={isOpen}>MENU</Button> or {!isOpen && <Button onPress={open}>MENU</Button>}

I've also updated the sandbox to display all the events: https://codesandbox.io/s/nervous-volhard-rut48?file=/src/Button.js

which gives us this in chrome

MENU onPointerDown
MENU onTouchStart
MENU onPointerUp
MENU onTouchEnd
CLOSE onClick

which now that i'm looking at it, looks like my above suggestion wouldn't work, though i didn't try it

djfarly commented 3 years ago

So… simulated chrome touches add an additional click event a tick(? - since the overlay had time to render) later… that seems super strange. πŸ˜– To circumvent the click event hitting the close button we could try to render the close button after a short timeout. That doesn't quite sound like a reasonable solution for something strange in one browsers dev mode. πŸ˜„

To be honest I don't know how big of a deal this is, still it is very intriguing to me. Im not familiar how react-aria handles all these events. Maybe it's a chrome bug even?

thanks for looking at it! πŸ₯³

snowystinger commented 3 years ago

Yes, I believe this to be a browser bug, I've recreated it here without react or react-spectrum https://codesandbox.io/s/inspiring-lehmann-i1hjf?file=/index.html In Chrome click is fired on the new button, in Safari and FF it is not, I'll log a bug with them and post a link here

snowystinger commented 3 years ago

Here's the bug link https://bugs.chromium.org/p/chromium/issues/detail?id=1150073

djfarly commented 3 years ago

tyvm @snowystinger - let's see how that goes. :)

josharens commented 3 years ago

Just FYI I've run into this issue on a real device, my Pixel 2. My use case has an overlay that just happens to be on top of some links. The overlay is removed when a button is clicked. The links underneath the overlay fire an unexpected click event. This is a problem in projects that use a client side router.

For now I've worked around this issue by using the deprecated onClick prop instead of onPress. I'm not really sure how that's fixed the problem but it seems to be working πŸ€·β€β™‚οΈ .

https://codepen.io/j-arens/full/ExgjrdP

LosYear commented 1 year ago

Hey, guys!

I really appreciate things you’re doing with react-aria!

Are there any plans on providing any workaround or fix inside react-aria? Seems like it add extra mess with setTimeout or any other hacks inside E2E tests :( Also spend around couple days to figure out why it does not work properly

Fixing with onClick also adds extra problems for developers to write code with react-aria.

I understand that it's a Chromium issue but it seems like react-aria tries to encapsulate different browser-specific behaviour inside

Thank you ☺️

snowystinger commented 1 year ago

The only way we've seen to prevent the compat click event fired is to preventDefault on touch start. Unfortunately, I think this also comes with scroll prevention, so we don't have a good way to encapsulate it in usePress at the moment. We're open to suggestions or PRs to address it though.

However, what you can do, is attach your own onTouchStart (make sure it's non-passive) and preventDefault on the event See https://codesandbox.io/s/jovial-hooks-bg82y5?file=/src/Button.js for how to do that and noticed the compat 'click' is not fired.

snowystinger commented 1 year ago

Extra history on us trying to do exactly this https://github.com/adobe/react-spectrum/pull/2942

rodogir commented 2 months ago

Is there any progress on this topic? We are running into the same problems using react-aria-components .

I set up a minimal example only using RACs: https://stackblitz.com/edit/vitejs-vite-guzxrh?file=src%2FApp.jsx

Opening the CombobBox and then selecting either Aardvark or Dog also triggers the link behind the options. The actual touch position seems to matter. If unable to reproduce, try again with a different touch position.

This problem affects both actual touch devices (Android tablets using Chrome) and emulated touch.

How would the workaround described in https://github.com/adobe/react-spectrum/issues/1279#issuecomment-1472634908 apply here as the RACs handle the events internally?

IonelLupu commented 1 month ago

as a workaround, I added a 200ms await on the first line of the onPress handler. So hacky, so bad. But nothing else I can do:

<Button
    onPress={async () => {
        await sleep(200)
        setIsVisible(false)
    }}
>
    <X size={16} weight="bold" />
</Button>

this is my hacky sleep function:

export function sleep(duration: number) {
    return new Promise(resolve => setTimeout(resolve, duration))
}
cevr commented 1 month ago

Can using onClick be a valid work around for this issue? I know it says its deprecated, but I have found that it doesn't seem to have the same issues as onPress.

My worry is that this also gives up all the niceities that react-aria provides

pnicolli commented 1 month ago

Facing the same issue with a modal that has a Button placed exactly over a Link, where the Link was also triggered by the button onPress, thus navigating when I just wanted to confirm my cookie choices.

Adding await new Promise((resolve) => setTimeout(resolve, 0)); to the very beginning of the onPress handler solved the issue. I don't get why it works, but it does. Maybe this helps somebody that knows the library better when debugging this.