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

Touch tap results in mouse events #231

Open
aweibell opened this issue Feb 18, 2021 · 9 comments
Open

Touch tap results in mouse events #231

aweibell opened this issue Feb 18, 2021 · 9 comments
Assignees
Labels
bug 🐛 has example ✅ has reproducible example has solution ✅ not fixing in swipeable, but solution provided

Comments

@aweibell
Copy link

Describe the bug
Tapping in the component tracked by the handlers causes multiple handlers to be called

  1. The onTap handler is called with event of type touchend
  2. The onSwiping handler is called with some delta values
  3. The onTap handler is called twice, both with event of type mouseup

Steps or Sandbox to reproduce
Observe the following codesandbox application console log when touch-tapping in the blue area.

https://codesandbox.io/s/react-swipeable-tap-bug-yrbyu

Expected behavior
WHEN both trackMouse and trackTouch options are set to true
GIVEN a single touch tap in the component
THEN only onTap handler is called, with one event of type touchend

Device/Browser
Tested on

  • Dell XPS with touch screen, running Fedora 34, in Firefox, Chrome and Brave
  • Samsung Galaxy S10, running Android 11, in Chrome and Brave

Sliding/swiping works very well in all the mentioned environments, but a single touch tap also gives the same result in all of them.

Additional context
The react-swipeable hook as such is very time saving and smooth! Thanks for the great work!

@hartzis hartzis self-assigned this Feb 19, 2021
@hartzis
Copy link
Collaborator

hartzis commented Mar 2, 2021

@aweibell Thank you very much for the awesome codesandbox! I am able to replicate this luckily on a mac with chrome in dev tools "as an Ipad".

It looks like "a tap" is additionally being fired by tracked mouse events... 🤔

I was able to find some really really amazing information from @patrickhlauke and his examples:

Anyways, looking at the data from https://patrickhlauke.github.io/touch/tests/results/, I think we may be safe in ignoring the mouseup for the onTap callback if both trackTouch and trackMouse are true.

I'm going to take a rough attempt at a solution and get a PR open soon.

💭 @aweibell, curious your thoughts on a solution/fix here being backwards compatible, meaning only bumping as a bug fix for it. I'm currently leaning that route as this is a new feature for v6 and i think your assumptions are the correct assumptions that only a single onTap should be triggered in this case.

@aweibell
Copy link
Author

aweibell commented Mar 2, 2021

Thank you for looking into this, @hartzis!

I was not aware of that nice resource from patrickhlauke either. Great compilation!

This is my first time using the react-swipeable library, so I'm not sure what it means for backwards compatibility, but logically it sounds justifiable to ignore mouseup on onTap as you describe.

A (temporary, at least) workaround for me could be to track either mouse or touch, by letting the users choose according to their requirement.

@patrickhlauke
Copy link

Thank you thank patrick! I dont know how i havent stumbled upon your great work over that past 6yrs.

heh, no worries. some of the info might be a touch (hah) out of date (certainly my magnum opus presentation on the topic is in dire need of a refresh and update https://patrickhlauke.github.io/getting-touchy-presentation/) but i'm glad it's still proving helpful

@patrickhlauke
Copy link

incidentally, you might want to consider switching to/adding pointer events as an alternative, where supported (as you don't end up with having to deal with compatibility events, i.e. fake mouse events fired at the end of a tap), and you can detect the type of point and react accordingly (e.g. if it's an actual touch device, or pen, or mouse).

@hartzis
Copy link
Collaborator

hartzis commented Mar 3, 2021

incidentally, you might want to consider switching to/adding pointer events as an alternative, where supported (as you don't end up with having to deal with compatibility events, i.e. fake mouse events fired at the end of a tap), and you can detect the type of point and react accordingly (e.g. if it's an actual touch device, or pen, or mouse).

@patrickhlauke 🙇 thank you for replying and the amazing information!

I've been thinking about pointer events for a few months now too, #208. Additionally I've noticed another library i track moved to pointer events too recently, https://github.com/pmndrs/react-use-gesture. I'd love to explore this avenue soon.

@aweibell While working on a solution in #234 i realized that one of the root culprits here is the "hard coding" of the passive event option via preventDefaultTouchMoveEvent prop and i'm going to explore removing that prop and replacing it with a prop that lets the user set the event options. related issue and chat #224 (comment)

Then users will be in complete control over when to call event.preventDefault either to stop scrolling or in your case to call event.preventDefault for the onTap, which should stop the mouseup event... should have another PR up hopefully this week.

This would be a change would definitely be a major version bump, but i think a very approachable and sensible major bump that would give our users more control.

@hartzis
Copy link
Collaborator

hartzis commented Mar 4, 2021

@aweibell This has turned into a slightly more complicated issue and i dont want to delay a solution for you so i tried the solution from #234 by attaching an onTouchEnd callback to the swipeable div and it appears to work.

Check this out and tell let me know if this can work for now as we iterate on a longer term solution with #235

  const tapHandler = (evt) => {
    console.log("tap event", evt.event.type);
  };
  const handlers = useSwipeable({
    onSwiped: (evnt) => swipedHandler(evnt),
    onSwiping: (dt) => swipingHandler(dt),
    onTap: (evnt) => tapHandler(evnt),
    preventDefaultTouchmoveEvent: true,
    trackMouse: true,
    trackTouch: true
  });
  return (
    <div>
      <Container ref={sliderRef}>
        <div {...handlers} onTouchEnd={(e) => e.preventDefault()}>

@aweibell
Copy link
Author

aweibell commented Mar 5, 2021

Sorry for my late response, @hartzis. I am really appreciating your efforts here! At least for now I only use this in a side project, and am unfortunately not able to work on it every day. This week I'm also only on 4G, and partly vacationing. codesandbox is super slow, but it seems to work as intended! Will try this out in my app as soon as I can!

The resources you found from @patrickhlauke was very valuable and I learned a lot from them – still have a lot to learn on this subject too, though. Pointer events looked very interesting. I may actually try to implement a simple home made solution using that to see the difference.

@hartzis hartzis added the has example ✅ has reproducible example label Mar 5, 2021
@aweibell
Copy link
Author

aweibell commented Mar 28, 2021

I apologize for my very poor contribution here, @hartzis. I finally sat down to give this a try, and your onTouchEnd suggestion seems to be a great workaround! Now I am able to control both tap and slide with both mouse and touch. Excellent! Thank you very much for your efforts and kind help!

@hartzis
Copy link
Collaborator

hartzis commented Mar 29, 2021

@aweibell Glad to hear, and no worries at all mate. I appreciate all your effort and time with this issue! Cheers.

@hartzis hartzis added the has solution ✅ not fixing in swipeable, but solution provided label Aug 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 has example ✅ has reproducible example has solution ✅ not fixing in swipeable, but solution provided
Projects
None yet
3 participants