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 fires twice on Lists and other components #5070

Closed
deepfriedbrain opened this issue Aug 25, 2016 · 11 comments
Closed

onTouchTap fires twice on Lists and other components #5070

deepfriedbrain opened this issue Aug 25, 2016 · 11 comments
Labels
bug 🐛 Something doesn't work

Comments

@deepfriedbrain
Copy link

I have a List component. Click on an item in the list replaces the original list with another list. On mobile devices, when I tap on the first list, it fires a second tap on the new list when the new list comes up. This ends up clicking the item in the new list too and leads to unpredictable UI behavior. I found a similar issue reported on react-tap-plugin-event page. I'm wondering whether it's that plugin's issue or an MUI issue, and is there a way to resolve it?

Versions

  • Material-UI: 0.15.4
  • React: 15.3
  • Browser: Chrome 52x
@terry-fei
Copy link

How long we can not use react-tap-plugin-event?

@JamesAlias
Copy link

In case you didn't see the post in the other issue you linked:

I found a solution so it may help you too.

After the onTouchTap event another onClick event is fired after a delay (~300ms), whether the onTouchTap is handled or not.
The onClick event is triggered on an element at the same position as the onTouchTap event but after the delay.

FIX:
Adding e.preventDefault to the onTouchTap event handler prevented the onClick event for me.

Example:
<Button onTouchTap={(e) => {e.preventDefault(); handleTouch();}}

Hope this helps.

@jamespizzurro
Copy link

I'm using React 15.3.1, and while I'm not sure when it was introduced, it looks like TapEventPlugin is now included in React's libs by default, so I've given up using react-tap-plugin-event and have switched to using the following:

require('react/lib/EventPluginHub').injection.injectEventPluginsByName({
  'TapEventPlugin': require('react/lib/TapEventPlugin')
});

I haven't tested it extensively yet, but seems to work well in desktop Chrome and in Chrome for Android. Might be worth a shot to see if it fixes your issue, as the raw TapEventPlugin code is indeed different than react-tap-plugin-event's.

@deepfriedbrain
Copy link
Author

Unfortunately, none of the solutions mentioned above work.

@JamesAlias - when I implement your solution, touch tap on ListItem stops working.

@jamespizzurro - I tried your solution, but the behavior is exactly the same with React's TapEventPlugin too.

I have implemented the workaround for Ghost Clicks posted on react-tap-event-plugin's page and it does help prevent the ghost clicks, but I still get a ripple effect on the list items on the next page.

injectTapEventPlugin({
    shouldRejectClick: function (lastTouchEventTimestamp, clickEventTimestamp) {
        if (lastTouchEventTimestamp && (clickEventTimestamp - lastTouchEventTimestamp) < 2000) {
            console.log("Reject click : " + (clickEventTimestamp - lastTouchEventTimestamp));
            return true;
        }
    }
});

Is there a reason for MUI to have onTouchTap listener instead of onClick now that the 300ms delay has been removed from the browsers?

@jadus
Copy link

jadus commented Oct 14, 2016

any news about this ?
How are you dealing with this ?
onClick doesn't work on safari
onTouchTap gets called twice (or you have to reject clicks in injectTapEventPlugin but you still have a ghost ripple effect)
I see that de material-ui docs works fine without shouldRejectClick... How are you doing this ???
@deepfriedbrain did you find a workaround ?

@jadus
Copy link

jadus commented Oct 17, 2016

I've finally followed @JamesAlias advice :
calling e.preventDefault() inside the onTouchTap event handler
Is there any drawback to this approach ?

@BlockChange
Copy link

Same here, this seems to happen sporadically. For example when expanding a Card element, it often fires twice so that it will immediately close again.

Wat does seem to help sometimes, but this is on the user side, is to press and hold the CardHeader or Button or whatever element is receiving the onTouchTap, then it will only fire once it seems.

But there are times where I load my page and this does not occur at all, so whether this is an issue with injectTapEventPlugin or using e.preventDefault(), I think it has mostly to do with the implementation of that 300ms delay?

@ethan-deng
Copy link

Okay this is called ghost click and documented here with solution.

https://github.com/zilverline/react-tap-event-plugin

Ignoring ghost clicks

When a tap happens, the browser sends a touchstart and touchend, and then 300ms later, a click event. This plugin ignores the click event if it has been immediately preceeded by a touch event (within 750ms of the last touch event).

Occasionally, there may be times when the 750ms threshold is exceeded due to slow rendering or garbage collection, and this causes the dreaded ghost click.

The 750ms threshold is pretty good, but sometimes you might want to override that behaviour. You can do this by supplying your own shouldRejectClick function when you inject the plugin.

The following example will simply reject all click events, which you might want to do if you are always using onTouchTap and only building for touch devices:

var React = require('react'),
injectTapEventPlugin = require("react-tap-event-plugin");
injectTapEventPlugin({
shouldRejectClick: function (lastTouchEventTimestamp, clickEventTimestamp) {
return true;
}
});

@geohuz
Copy link

geohuz commented Jan 4, 2017

what's the progress on this issue? I have a list on one page and touch on one of the items will transit to item's detail page, I can use e.preventDefault to prevent the ghost click when going back to the list page, but still seeing a ripple there. Is there a workaround or universal fixing to deal this issue?

Joonpark13 pushed a commit to Joonpark13/serif.nu-old that referenced this issue Feb 17, 2017
TouchTapping the "All" Chip in the browse menu previously resulted two touchtaps. Solved by this method: mui/material-ui#5070 (by adding an event.preventDefault())
@jampy
Copy link

jampy commented Apr 25, 2017

After several attempts at fixing this problem I came to the conclusion that the only solution is to call preventDefault() within the onTouchTap handler.

It would be relatively easy to extend the react-tap-event-plugin in a way that it always prevents the default when firing the onTouchTap event. Unfortunately, that doesn't help much because it breaks functionality for components that do not use onTouchTap but rather "normal" events like onChange. The MUI Checkbox component is one of these.

Having nearly 300 onTouchTap handlers in my project it was not really an option to manually add preventDefault to all of these - especially because it's rather error-prone.

In search of an automated solution and after a day of different attempts the only solution seems to be monkey-patching React.createElement (!) so that it wraps all onTouchTap props on native elements, adding the preventDefault() call.

Ugly, I know, but it works...

import React from 'react';


let orig = React.createElement;

React.createElement = function(type, props, ...children) {

  // only wrap native elements (string-ish `type`) which have an onTouchTap prop
  if (typeof type === "string" && props && props.onTouchTap) {

    let orig = props.onTouchTap;

    props = {
      ...props,
      onTouchTap: function(e) {
        e.preventDefault();
        return orig.apply(this, arguments);
      }
    }

  }

  return orig.call(this, type, props, ...children);

};

Tested on...

  • Chrome Desktop 58
  • Chrome Mobile 57
  • Crosswalk 23
  • Firefox 52
  • Firefox 53
  • Edge Desktop 38
  • Edge Mobile
  • Internet Explorer 11

@oliviertassinari
Copy link
Member

We have removed the react-tap-event-plugin dependency on the v1-beta branch. People can rely on the onClick event. We should be good with that issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work
Projects
None yet
Development

No branches or pull requests

10 participants