Open aweibell opened 3 years ago
@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.
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.
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
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
).
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, orpen
, ormouse
).
@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 https://github.com/FormidableLabs/react-swipeable/issues/224#issuecomment-760365636
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.
@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()}>
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.
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!
@aweibell Glad to hear, and no worries at all mate. I appreciate all your effort and time with this issue! Cheers.
Describe the bug Tapping in the component tracked by the handlers causes multiple handlers to be called
onTap
handler is called with event of typetouchend
onSwiping
handler is called with some delta valuesonTap
handler is called twice, both with event of typemouseup
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
andtrackTouch
options are set totrue
GIVEN a single touch tap in the component THEN onlyonTap
handler is called, with one event of typetouchend
Device/Browser Tested on
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!