zilverline / react-tap-event-plugin

Instant TapEvents for React
http://facebook.github.io/react/
MIT License
1.07k stars 110 forks source link

Double event call in Chrome on iPhone6 #15

Closed watch-the-stars closed 9 years ago

watch-the-stars commented 9 years ago

Hello! Has anyone experienced triggering the tap event twice in Chrome on iPhone6? Right now I don't have demo link, but maybe this issue has already come up?

iPhone6 iOS8.1 Chrome 41.0.2272.58

kellyrmilligan commented 9 years ago

What are you binding it to? If it's a link it will Fire twice and you have to bind to click and stop the event yourself

watch-the-stars commented 9 years ago

No, it's just a div element and this behavior is noticed only in iOS Chrome.

watch-the-stars commented 9 years ago

Guys, this looks like ghost click action.

tomhicks-bsf commented 9 years ago

@lemmy- Can you try something? In TapEventPlugin.js around line 124, you'll see something like:

    if (isTouch(topLevelType)) {
      lastTouchEvent = nativeEvent.timeStamp
    } else {
      if (lastTouchEvent && (nativeEvent.timeStamp - lastTouchEvent) < ignoreMouseThreshold) {
        return null;
      }
    }

Try changing the lastTouchEvent = line to:

lastTouchEvent = Date.now()

and see if the problem goes away.

We've just been banging our heads against a wall with this and finally found some weird bug in UIWebView, which Chrome might be using.

watch-the-stars commented 9 years ago

Thanks for the feedback on this issue. I also had to hack the plugin and include it in my project codebase. So the hack looks like this. It seems to work fine for now, but I'm not sure :) If this might cause a trouble, please, let me know.

geekyme commented 9 years ago

I'm getting the same issue too.

abergs commented 9 years ago

I'm seeing this too, in Safari on Ipad with ios 8.3.

Scenario: User clicks an <li> to open a Modal. The <li> has an onTouchTap handler (no onClick handler). The repeated event then clicks a whatever happens to be under your finger (where we originally tapped) in the Modal. in our case another button or a input field.

I have not tried the workaround by @tomhicks-bsf

tomhicks-bsf commented 9 years ago

Try this unobtrusive way of overriding the plugin (just converted from CoffeeScript so it might be a bit weird):

/*
A fix for a WebKit bug that causes all touch events to have incorrect timestamps when an app wakes
up from the background

http://stackoverflow.com/questions/26177087/ios-8-mobile-safari-wrong-timestamp-on-touch-events
 */
var EventConstants, React, TapEventPlugin, injectTapEventPlugin, isTouch, merge, oldEventExtractor, topLevelTypes;

React = require("react");

React.initializeTouchEvents(true);

merge = require("ramda").merge;

TapEventPlugin = require("react-tap-event-plugin/src/TapEventPlugin");

EventConstants = require("react/lib/EventConstants");

topLevelTypes = EventConstants.topLevelTypes;

isTouch = function(topLevelType) {
  var touchTypes;
  touchTypes = [topLevelTypes.topTouchCancel, topLevelTypes.topTouchEnd, topLevelTypes.topTouchStart, topLevelTypes.topTouchMove];
  return touchTypes.indexOf(topLevelType) >= 0;
};

oldEventExtractor = TapEventPlugin.extractEvents;

TapEventPlugin.extractEvents = function(topLevelType, topLevelTarget, topLevelTargetID, nativeEvent) {
  if (isTouch(topLevelType)) {
    nativeEvent = merge(nativeEvent, {
      timeStamp: Date.now()
    });
  }
  return oldEventExtractor(topLevelType, topLevelTarget, topLevelTargetID, nativeEvent);
};

injectTapEventPlugin = require("react-tap-event-plugin");

injectTapEventPlugin();
s0meone commented 9 years ago

I cannot reproduce this issue on iOS 8.3, iPhone 6, Chrome 42.xx. Maybe you can increase the ignore threshold?

The possible fix by @tomhicks-bsf, is now merged into master (0a29717f161a5b04f2e201418ca6050279cf1f11).

watch-the-stars commented 9 years ago

Indeed, this bug has come up unexpectedly in Chrome 41 and it seems that it works fine in 42. As far as I remember, the second event was triggered by the late native click event (ghost click), which wasn't suppressed. So increasing threshold might've helped.

s0meone commented 9 years ago

Alright, closing this now. Please reopen if anyone still has problems with this and cannot resolve it by increasing the threshold.

abergs commented 9 years ago

Tried 1.7 and still had double click issues on safari ios 8.3 - @lemmy- what threshold do you mean? Could you demonstrate?

tomhicks-bsf commented 9 years ago

@abergs if your rendering takes more than 750ms then you will still get the double click. You can try increasing the ignoreMouseThreshold variable in TapEventPlugin.js to see if that helps?

Also, make sure that in your project you only use onTouchTap and never onClick. The reason is that onClick handlers don't go through this plugin that 'de-dupes' pointer interactions, so you get weird behaviour. Usually this is when you render a new page, and there is a clickable element on page 2 in the same place where you did a tap on page 1.

I'm trying to think of some way we can make this less reliant on the arbitrary 750ms timeout. Perhaps by observing the time taken in the React DOM update cycle. Or at the very least, make it possible to inject the plugin with options:

var injectTapPlugin = require("react-tap-event-plugin");
injectTapPlugin({ignoreMouseThreshold: 1000}); // for slower devices
s0meone commented 9 years ago

@abergs Another option is to execute heavy JS in a different frame, so the browser can finish creating events before executing your event handler. I choose the value of 750ms because most handlers should be finished way before that timeout is reached, but if you're doing heavy (synchronous) stuff, the event handlers get blocked.

Like so:

handleTouchTap: function(e) {
  setTimeout(function() {
    // do heavy stuff, like a heavy React render
  }, 0);
}

By setting a timeout of zero, you'll queue the execution to the end of the execution queue.

EDIT: I've explained the above more detailed in React's issue report: https://github.com/facebook/react/issues/2061#issuecomment-53548251

abergs commented 9 years ago

That's interesting! I wouldn't say that the rendering is slow, I believe it is under 750ms, but I can't swear on it. I'll do a test with the setTimeout just to validate that this is not the issue.

While writing this answer I re-read your answer. We do mix onClicks and onTouchTap, so that (ofcourse) might lead the problems, but it gets even deeper...

Example: We have customs button which wraps the React.DOM.button. These have onClick props but internally wire them to onTouchTap on the DOm button. This should be OK, as of my understanding.

<OurButton onClick={handler}> => <React.dom.button onTouchTap={handler} />

But in the current scenario where we have double taps is where the button opens a dialog, and inside the dialog, at the same position as the button, we have an <Input />. And the input gets focused by the original button tap, opening the onscreen keyboard. Wouldn't there be more handlers triggering than onClick, such as onFocus causing trouble?

If that is the case, i guess you would really have to hook into reacts event system and do the de-duping - but i'm just guessing here...

s0meone commented 9 years ago

You're correct. The onClick on your custom button is just a prop. This can be any name you like as long as you bind this handler to onTouchTap.

We've had the exact same scenario, a button which opens a dialog with an overlay. In our case, this was quite a heavy operation, causing a parent component very high in the hierarchy to re-render. Clicking/tapping the overlay closes the dialog, so every time we opened a dialog it would close immediately. That's what I explained on React's issue and why I even patched this plugin. The example fiddle's also show this scenario. We fixed this with a setTimeout(..., 0);.

tomhicks-bsf commented 9 years ago

The setImmediate solution is very interesting - I'll have to take a look into that.

We have all settled on the exact same button wrapping prop name - safety in numbers. Although we recently made a decision to ensure we don't use DOM event names for our own handlers - it creates confusion, and doesn't let us do things like linting to make sure there are no onClicks in the application layer.

We had the same issue with buttons opening dialogs and focussing inputs. The way we solved this was to add a mask to the dialog while it was animating to prevent any user interaction. The way you suggested, @s0meone, is better, although we still want to prevent any interaction during animation so the mask may well stay.

Touch events are such a minefield!