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

onTouchTap triggering handler twice #2061

Closed
piranha opened this issue Aug 18, 2014 · 27 comments
Closed

onTouchTap triggering handler twice #2061

piranha opened this issue Aug 18, 2014 · 27 comments

Comments

@piranha
Copy link
Contributor

piranha commented Aug 18, 2014

We're using TapEventPlugin in our application and it calls every handler twice. Those calls are triggered by different events - first time by touchend, and second time by mouseup, which is generated by a browser as a click. This doesn't make any sense, so I think TapEventPlugin should prevent triggering by a mouseup when event was triggered by touches.

Not sure if there is a good way. I guess calling e.preventDefault() inside of TapEventPlugin is not the wisest solution, is it?

@syranide
Copy link
Contributor

It's arguable for e.preventDefault() to be called by TapEventPlugin if a handler is defined, but it's not always what you want and there would be no way of "undoing" it, I don't see a way around that.

Since what's really going on here is the browser trying to play nice by emulating mouse events for touch, a way to selectively disable emulation is an idea. But node-wise sounds kind of useless (and becomes yet another reserved non-standard attribute) and I'm not sure how you would do it "owner component"-wise, it would probably introduce additional issues for when components are supplied as children to another component.

PS. You can currently return false instead I think, but that's about to be deprecated it seems, so don't do it. :)

@piranha
Copy link
Contributor Author

piranha commented Aug 19, 2014

Yeah, I can return false, but that's more or less the same as calling .preventDefault(). What if I don't want to prevent an event? Should I get two calls of the same handler?

@syranide
Copy link
Contributor

It seems this is a wide-spread issue, some quick googling reveals that a lot of people seem quite confused about what to do, didn't find any useful answer during my quick search. Additionally, it doesn't seem to be preventDefaultable in IE10 if "some guy is right".

@piranha
Copy link
Contributor Author

piranha commented Aug 20, 2014

Ok, so I fixed it for myself the hard way - if taps are enabled then don't react to clicks.

And I check for taps like that:

  React.initializeTouchEvents(('ontouchstart' in window) ||
         window.DocumentTouch && document instanceof DocumentTouch);

That of course rules out laptops with touch screens, and is not working very good on IE10, but I can live with it for now.

@syranide
Copy link
Contributor

@piranha It also means you have to support touchevents for all components, you cannot fallback in places you don't care.

@piranha
Copy link
Contributor Author

piranha commented Aug 20, 2014

Yeah, we're doing onTap={...} everywhere.

@s0meone
Copy link

s0meone commented Aug 26, 2014

@piranha, I believe your solution isn't ideal.

First off, the touch detection you're using gives false positives (including my current version of Chrome, 36.0.1985.143. 'ontouchstart' in window does return true, but ontouchstart never gets triggered.) And since you're either listening for mouse or touch events based on this feature detection, your tap handler never gets fired.
See modernizr's source for more info about this.

Second, I'm not sure about the environment you're working with, but you're either listening for mouse or touch events. But how about devices that support both?

When trying to fix this problem, we also tried to return false and preventDefault but this failed. Because we change the state after a touch and render an extra element to the DOM that overlays the tap area, all following mouse events are triggered on the overlayed element, and not on the same element the touch handlers had been fired on.

The solution we came up with is to ignore all mouse events within 750ms of the last touch event. Since all emulated mouse events are triggered directly after the touch events, we can easily ignore these. We chose 750ms because it's a reasonable amount of time to be considered as the same user interaction.

With this solution the tap handler is always invoked once per user interaction. It also never excludes events, so the user is allowed to use both their finger and mouse.

Our solution: zilverline/react-tap-event-plugin@zilverline:v0.11.0...patch-tap-event

@syranide
Copy link
Contributor

@s0meone Hmm, regarding 750ms, are there actually emulated mouse events that fire delayed from the touch action/does not have a touch action? I would assume that a delay of "0" would be enough (if it happens immediately it's from the touch event, else it's from a mouse).

@piranha
Copy link
Contributor Author

piranha commented Aug 26, 2014

Wow, that's certainly much better, I just needed a really quick fix and couldn't come up with better idea. Your patch seems a good one, though I think it makes sense decreasing threshold to, say, 350ms. IIRC 300ms is a delay between a tap and an emulated click event.

@s0meone
Copy link

s0meone commented Aug 26, 2014

Well, you'd hope the emulated mouse events would be fired immediately after the touch events but they aren't. We've had delays up to ~450ms.

Logs from the iOS Simulator:


[Log] touch event topTouchStart 1409061289113 (react-with-addons.js, line 16870)
[Log] touch event topTouchEnd 1409061289183 (react-with-addons.js, line 16870)
[Log] non touch event topMouseMove time diff 363 (react-with-addons.js, line 16872)
[Log] non touch event topMouseDown time diff 365 (react-with-addons.js, line 16872)
[Log] non touch event topMouseUp time diff 366 (react-with-addons.js, line 16872)
[Log] non touch event topClick time diff 366 (react-with-addons.js, line 16872)


[Log] touch event topTouchStart 1409061349552 (react-with-addons.js, line 16870)
[Log] touch event topTouchEnd 1409061349767 (react-with-addons.js, line 16870)
[Log] non touch event topMouseMove time diff 47 (react-with-addons.js, line 16872)
[Log] non touch event topMouseDown time diff 49 (react-with-addons.js, line 16872)
[Log] non touch event topMouseUp time diff 50 (react-with-addons.js, line 16872)
[Log] non touch event topClick time diff 51 (react-with-addons.js, line 16872)


Also, lowering the threshold doesn't really make sense, who's going to switch between a finger and a mouse in 750ms? Probably some nerdy developer who's trying to double click using a finger and mouse :)

@iamdustan
Copy link
Contributor

I’m fairly certain that the “click” isn’t an emulated event, but the somewhat input device agnostic event (similar to “input” instead of key* events on form elements).

The 300ms delay was to allow for double-tap to zoom gestures (and potentially other accessibility features).

Is there any idea or testing how this would interact with other browser features that disable the 300ms delay such as:

// IE10+
-ms-touch-action: none;

// Chrome 32+ for Android
<meta name="viewport" content="width=device-width, initial-scale=1">
// or
<meta name="viewport" content="user-scalable=no">

@iamdustan
Copy link
Contributor

@s0meone

Also, lowering the threshold doesn't really make sense, who's going to switch between a finger and a mouse in 750ms? Probably some nerdy developer who's trying to double click using a finger and mouse :)

You mean like this? http://smus.com/touch-laptop-experiments/

@syranide
Copy link
Contributor

@s0meone Oh right, the click/tap event has "intentional" delays, but all other emulated mouse events I assume would be called immediately.

@s0meone
Copy link

s0meone commented Aug 26, 2014

@iamdustan

You mean like this? http://smus.com/touch-laptop-experiments/

That looks pretty awesome! Never seen it before. But I think when you're about to support this interaction you probably won't use a tap handler but handle all the events separately.

@syranide
I know about the 300ms click delay. So by "intentional" you mean by React or are you talking about that 300ms delay? Because I've also seen delays on mousedown and mouseup and didn't see any intentional delays in React's source.

@s0meone
Copy link

s0meone commented Aug 26, 2014

@iamdustan I've tested this patch in the Chrome device emulator, which only has a delay of a few milliseconds. Ignoring the events works perfect.

Log:


touch event topTouchStart 1409062331329 react-with-addons.js?body=1:16870
touch event topTouchEnd 1409062331531 react-with-addons.js?body=1:16870
non touch event topMouseOver time diff 13 react-with-addons.js?body=1:16872
non touch event topMouseMove time diff 17 react-with-addons.js?body=1:16872
non touch event topMouseDown time diff 18 react-with-addons.js?body=1:16872
non touch event topMouseUp time diff 18 react-with-addons.js?body=1:16872
non touch event topClick time diff 19 react-with-addons.js?body=1:16

@syranide
Copy link
Contributor

@s0meone I was talking about the "browser-imposed" click delay. Hmm, it's interesting that there's a delay on mousedown, could it be that the delay is actually because of significant processing associated (i.e. re-rendering) with a touch event, which would delay the emulated mouse event?

Intuitively I would expect emulated mouse events to fire in the same frame as the touch events (but not necessarily with the same timestamp), if so, simply setting a global touched = true and clearing it with setTimeout(, 0) would be more exact. (Obviously can't be used for click though)

And I'm assuming you've checked that there aren't properties added/set for the emulated mouse event that would indicate that it's emulated?

@s0meone
Copy link

s0meone commented Aug 27, 2014

I did some testing today, here are my results. Please tell me if I'm testing wrong or am making the wrong assumptions.

Looks like the events are triggered in separate frames. So resetting a global variable has no effect.

if (isTouch(topLevelType)) {
  touched = true;
  setTimeout(function() {
    touched = false;
  }, 0);
}

console.log("handle", topLevelType, touched);

The example above resets the touched variable before the next event is handled (mousemove). See the log:

handle topTouchStart true react-with-addons.js?body=1:16886
handle topTouchEnd true react-with-addons.js?body=1:16886
handle topMouseMove false react-with-addons.js?body=1:16886
handle topMouseDown false react-with-addons.js?body=1:16886
handle topMouseUp false react-with-addons.js?body=1:16886
handle topClick false react-with-addons.js?body=1:16886

I also tested with expensive touch handlers to explain why mouse events are delayed (besides the click event).

Example fiddle with expensive touch handlers: http://jsfiddle.net/es9qzL1f/

Log from iOS simulator:

# logging: event, touched?, timestamp, timediff
[Log] touchstart undefined 1409127930524 NaN  (show, line 72)
[Log] touchend true 1409127930579 55  (show, line 72)
[Log] mousedown false 1409127931583 1004  (show, line 72)
[Log] mouseup false 1409127931584 1  (show, line 72)
[Log] click false 1409127931585 1  (show, line 72)

Example fiddle without expensive touch handlers: http://jsfiddle.net/es9qzL1f/2/

Log from iOS simulator:

# logging: event, touched?, timestamp, timediff
[Log] touchstart false 1409127759718 3314  (show, line 70)
[Log] touchend true 1409127759765 47  (show, line 70)
[Log] mousedown false 1409127760123 358  (show, line 70)
[Log] mouseup false 1409127760124 1  (show, line 70)
[Log] click false 1409127760125 1  (show, line 70)

So it looks like the 300ms applies to the mousedown event, the mouseup and click event follow directly after.


Then there is a seperate problem, the mouse events are not triggered on the same component as the touch events.

When you look at this fiddle http://jsfiddle.net/2k4pzjth/ you can see (in the console) that the mouse events are triggered on the container, not on the clicked element (the box).
Make sure you enabled the touch sensor emulation in the Chrome developer tools. Or use the iOS simulator.

# Logging: event, target, which component handled the event
touchstart <div class="box" data-reactid=".0.0"></div> box
touchstart <div class="box" data-reactid=".0.0"></div> container
touchend <div class="box move-box" data-reactid=".0.0"></div> box
touchend <div class="box move-box" data-reactid=".0.0"></div>container
mousedown <div class="container" data-reactid=".0">…</div> container
mouseup <div class="container" data-reactid=".0">…</div> container
click <div class="container" data-reactid=".0">…</div> container

When we're not moving the box, all handlers are triggered on both components: http://jsfiddle.net/2k4pzjth/1/

# Logging: event, target, which component handled the event
touchstart <div class="box" data-reactid=".0.0"></div> box
touchstart <div class="box" data-reactid=".0.0"></div> container
touchend <div class="box" data-reactid=".0.0"></div> box
touchend <div class="box" data-reactid=".0.0"></div> container
mousedown <div class="box" data-reactid=".0.0"></div> box
mousedown <div class="box" data-reactid=".0.0"></div> container
mouseup <div class="box" data-reactid=".0.0"></div> box
mouseup <div class="box" data-reactid=".0.0"></div> container
click <div class="box" data-reactid=".0.0"></div> box
click <div class="box" data-reactid=".0.0"></div> container

This isn't a problem in React, but in webkit probably. Tested in Chrome and on iOS. Same example without React: http://jsfiddle.net/37qmsrkf/1/

When running this example in Firefox (with and without touch sensor emulation), all handlers are called on the correct targets.


The event model, weird stuff :)

So my conclusion after all these tests is that ignoring the mouse events is probably the best workaround for these problems. The 750ms threshold could be lowered but it totally depends on how expensive your touch handlers are and how fast the device is you're running on.

@appsforartists
Copy link

Since TapEventPlugin isn't being made publicly available until 1.0 (#1170, #436), how would you feel about making a repo exposing your fork of TapEventPlugin (with the double-tap-event fix) and publishing it to npm?

@appsforartists
Copy link

@s0meone: I took the liberty of separating out your changes from the rest of React:

zilverline#1

@s0meone
Copy link

s0meone commented Sep 15, 2014

@appsforartists: Thanks for your pull request. I'll take a look and merge it as soon as possible.

@appsforartists
Copy link

I'm in the middle of debugging an issue in Chrome for Android where I'm getting a second touchtap on a different element after the first has dispatched. In my particular case, the second event is closing the modal the first event opened (so it looks to the user like nothing happens when you tap).

Here are the logs on Chrome for MacOS, Safari for iOS, and Chrome for Android. Interestingly, I am unable to replicate this issue in ChromeOS with a touchscreen - it is very Android-specific. Unfortunately, I don't have the Chromebook handy to log with.

extractEvents: function(
    topLevelType,
    topLevelTarget,
    topLevelTargetID,
    nativeEvent) {
  // …
  console.log(arguments);
  console.log(event);
  // …
}


// Chrome 37.0.2062.120 on MacBook

["topMouseDown", img, ".1jmtvo16r5s.0.0.0", MouseEvent, callee: (...), caller: (...)]
null
["topMouseUp", img, ".1jmtvo16r5s.0.0.0", MouseEvent, callee: (...), caller: (...)]
SyntheticUIEvent {dispatchConfig: Object, dispatchMarker: ".1jmtvo16r5s.0.0.0", nativeEvent: MouseEvent, type: "mouseup", target: img…}


// Safari Mobile/12A365 on iOS 8

["topTouchStart", <img src=​"/​static/​generic/​images/​nav.svg" data-reactid=​".4bwvosidc0.0.0.0">​, ".4bwvosidc0.0.0.0", TouchEvent]
null
["topTouchEnd", <img src=​"/​static/​generic/​images/​nav.svg" data-reactid=​".4bwvosidc0.0.0.0">​, ".4bwvosidc0.0.0.0", TouchEvent]
SyntheticUIEvent


// Chrome 37.0.2062.117 on Android 4.4.99 LPV79

["topTouchStart", img, ".245z5a53cow.0.0.0", TouchEvent, callee: (...), caller: (...)]
null
["topTouchEnd", img, ".245z5a53cow.0.0.0", TouchEvent, callee: (...), caller: (...)]
SyntheticUIEvent {dispatchConfig: Object, dispatchMarker: ".245z5a53cow.0.0.0", nativeEvent: TouchEvent, type: "touchend", target: img…}
["topMouseDown", div.Scrim, ".245z5a53cow.1.0", MouseEvent, callee: (...), caller: (...)]
null
["topMouseUp", div.Scrim, ".245z5a53cow.1.0", MouseEvent, callee: (...), caller: (...)]
SyntheticUIEvent {dispatchConfig: Object, dispatchMarker: ".245z5a53cow.1.0", nativeEvent: MouseEvent, type: "mouseup", target: div.Scrim…}

img is the button the user is trying to tap. Scrim is an element that appears after the first tap to give the user a way to go back. It covers the entire screen behind the modal as soon as the button is tapped.

@appsforartists
Copy link

I just wrote a reduction for that issue and noticed it's the same one @s0meone is talking about above. :suspect:

@s0meone
Copy link

s0meone commented Sep 22, 2014

@appsforartists did a great job in extracting my fix in a separate package. For anyone interested, it's on npm now: https://www.npmjs.org/package/react-tap-event-plugin

@iamdustan
Copy link
Contributor

@s0meone @appsforartists how are you unit testing this? Using React.addons.TestUtils.simulator.Click(target) isn’t working for me.

@appsforartists
Copy link

What's unit testing? 😝

Less snarkfully, I'm using this in a prototype, so I haven't tried automated testing.

@iamdustan
Copy link
Contributor

I came across this post which includes a solution for eating all subsequent click events.

preventGhostClick is the important function that sounds like a strong candidate solution for the delayed event issue. https://developers.google.com/mobile/articles/fast_buttons#ghost

@gaearon
Copy link
Collaborator

gaearon commented Oct 27, 2016

TapEventPlugin is not officially supported so I'm closing this.

@gaearon gaearon closed this as completed Oct 27, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants