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

touchstart preventDefault() does not prevent click event. #9809

Closed
benwiley4000 opened this issue May 29, 2017 · 27 comments
Closed

touchstart preventDefault() does not prevent click event. #9809

benwiley4000 opened this issue May 29, 2017 · 27 comments

Comments

@benwiley4000
Copy link

benwiley4000 commented May 29, 2017

Do you want to request a feature or report a bug?
Bug

What is the current behavior?
Calling e.preventDefault() on a synthetic onTouchStart event fails to prevent the click event. I also tried e.nativeEvent.preventDefault(), but this didn't make any difference.

If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem via https://jsfiddle.net or similar (template: https://jsfiddle.net/84v837e9/).

Here's a div which is supposed to handle a hover case but not process a click the first time is is tapped via touch (click on desktop is fine). However tapping with touch (on a mobile device or using dev tools touch emulation) will trigger both touchstart and click immediately.

jsfiddle

const style = {
  background: 'red',
  width: 100,
  height: 100,
  // to ensure `touchstart` `preventDefault()` is allowed on mobile
  touchAction: 'none'
};

class SomeButton extends React.Component {
  constructor (props) {
    super(props);

    this.state = {
      hover: false,
      click: false
    };
  }

  render () {
    return (
      <div
        style={style}
        onMouseEnter={() => this.setState({ hover: true })}
        onClick={() => this.setState({ click: true })}
        onTouchStart={e => {
          if (!this.state.hover) {
            e.preventDefault(); // doesn't work!
            this.setState({ hover: true });
          }
        }}
      >
        {this.state.hover && 'hover!'}
        {this.state.click && 'click!'}
      </div>
    );
  }
}

However if I move the touchstart listener to componentDidMount and use the normal DOM API, everything works:

jsfiddle

// ...
class SomeButton extends React.Component {
  constructor (props) {
    // ...
  }
  
  componentDidMount () {
    this.elem.addEventListener('touchstart', e => {
      if (!this.state.hover) {
        e.preventDefault(); // WORKS!
        this.setState({ hover: true });
      }
    });
  }

  render () {
    return (
      <div
        ref={elem => this.elem = elem}
        { /* ... (removed onTouchStart) ... */}
      >
        {/* ... */}
      </div>
    );
  }
}

What is the expected behavior?
The first time a touchstart is processed, we only treat it as a hover, and wait to process the click event until after the next touchstart. If the pointer is a mouse, both events can be processed at once.

Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?
React 15.5.4. Not sure about previous React versions. Chrome for Android, Chrome for Mac emulating touch, Firefox for Mac emulating touch.

@EnoahNetzach
Copy link
Contributor

EnoahNetzach commented May 29, 2017

Just for completeness, it works as expected on Firefox 53.0.2 on Android 6.0.1, while on chromium based browsers I still get the issue.

@benwiley4000
Copy link
Author

benwiley4000 commented May 29, 2017

@EnoahNetzach I became curious after your comment so I tested again...

What I'm finding that is peculiar now is that in Chrome for Mac (58.0.3029.110) emulating touch, the second example solves the issue... but in Firefox for Mac (53.0.3) emulating touch, neither example works as desired!

For the record, the Touch Events spec is clear about what should happen.

@EnoahNetzach
Copy link
Contributor

EnoahNetzach commented May 30, 2017

Adding to the list, in safari on iOS 10.3 (simulated) works as expected..

@nhunzaker
Copy link
Contributor

nhunzaker commented May 30, 2017

Hmm. Bummer. Thanks everyone for digging into this. We should do the following:

  1. Produce a test case that does not use React.
  2. Produce a test case for our DOM fixture manual test suite

Additionally, I have a PR out that attaches touch listeners local to an element. I am curious if this might have an unexpected side-effect of fixing this issue (assuming it's a React issue):

#9333

@benwiley4000
Copy link
Author

benwiley4000 commented May 30, 2017

@nhunzaker: thanks! Based on the findings so far I suspect that PR might solve the issue (except for Firefox for Mac [not mobile] where it appears to persist regardless).. Would there be an easy way to test it with my code?

@nhunzaker
Copy link
Contributor

nhunzaker commented May 30, 2017

I have a custom build up for that branch, you could pull in React and ReactDOM from:

React:
https://nh-dom-fixtures-scroll.surge.sh/react.development.js

ReactDOM:
https://nh-dom-fixtures-scroll.surge.sh/react-dom.development.js

I've forked and updated your gist with those scripts here:
http://jsfiddle.net/69z2wepo/79803/

Though I won't be able to dig into testing it myself until later today or tomorrow.

@benwiley4000
Copy link
Author

benwiley4000 commented May 30, 2017

@nhunzaker thank you.

I tested just now (using Chrome for Android and Chrome with emulated touch for Mac). Unfortunately the behavior seems to be the same.

@benwiley4000
Copy link
Author

benwiley4000 commented Jun 2, 2017

@nhunzaker here is the non-React comparison case you asked for. https://jsfiddle.net/a2k4whf0/2/

@gaearon
Copy link
Member

gaearon commented Jul 11, 2017

@nhunzaker What's the next step here?

@OZhurbenko
Copy link

OZhurbenko commented Nov 8, 2017

It would be great if this was fixed at some point.
I was very confused about this behavior, especially after I saw Chromium response saying that everything works as expected, but it clearly didn't.

if I move the touchstart listener to componentDidMount and use the normal DOM API, everything works

Thanks, that worked for me.

@OZhurbenko
Copy link

OZhurbenko commented Nov 16, 2017

Hmm.. actually "normal DOM API" solution didn't work.
It didn't fix it for Firefox for Android, there's a bug there. Firefox triggers mousedown-mousemove-mouseup events following touchstart no matter what😕

Had to work around that and add a flag to the touchStart handler:

handleTouchStart(event) {
  this.hasBeenTouchedRecently = true;
  setTimeout(() => { this.hasBeenTouchedRecently = false; }, 500);
  ...
}

And a simple return from mouseDown handler:

  handleMouseDown(event) {
    if(this.hasBeenTouchedRecently) {
      return;
    }
  ...

In this case it doesn't matter how you add "touchstart" event listener, I did it via standard React way.

@RandScullard
Copy link

RandScullard commented Jan 16, 2018

Any update on this? It's been nearly eight months and this is still an issue. I was able to work around it with a solution similar to the one suggested by @OZhurbenko, but it would be really nice to see this fixed.

@seancolsen
Copy link

seancolsen commented Jun 8, 2018

I had this same problem and was able to eliminate the superfluous mousedown event by calling preventDefault() from within onTouchEnd like this:

render() {
  return (
    <div
      onMouseDown={e => this.handleMouseDownOrTouchStart(e)}
      onTouchStart={e => this.handleMouseDownOrTouchStart(e)}
      onTouchEnd={e => e.preventDefault()}
    >
      Foo
    </div>
  );
}

Reference: https://developers.google.com/web/updates/2017/01/scrolling-intervention

To suppress the default behavior of a tap (such as the generation of a click event), call preventDefault() inside of a touchend listener.

@benwiley4000
Copy link
Author

benwiley4000 commented Jul 18, 2018

@seanmadsen that works, and is a less obtrusive workaround! But it shouldn't be necessary according to the spec. Preventing default on touchstart should cancel a click event, since it requires both a pointer start and pointer end.

Any updates, React folks? 🙂 @nhunzaker?

@nhunzaker
Copy link
Contributor

nhunzaker commented Jul 23, 2018

Sorry... I let this slide. :( I wonder if there is an event plugin that is getting in the way. I'll investigate what event plugins fire on touch.

@benwiley4000
Copy link
Author

benwiley4000 commented Jul 23, 2018

Thanks!

@nhunzaker
Copy link
Contributor

nhunzaker commented Jul 25, 2018

What fun!

It looks like event.defaultPrevented is only set on the SyntheticEvent, not the native event:

image

So for some reason, this synthetic event.preventDefault event implementation isn't working:

https://github.com/facebook/react/blob/master/packages/events/SyntheticEvent.js#L115-L128

preventDefault: function() {
    this.defaultPrevented = true;
    const event = this.nativeEvent;
    if (!event) {
      return;
    }

    if (event.preventDefault) {
      event.preventDefault();
    } else if (typeof event.returnValue !== 'unknown') {
      event.returnValue = false;
    }
    this.isDefaultPrevented = functionThatReturnsTrue;
  },

Now to figure out why...

@nhunzaker
Copy link
Contributor

nhunzaker commented Jul 25, 2018

I think I see the issue.

React attaches (most) events to the document:
https://github.com/facebook/react/blob/master/packages/react-dom/src/events/ReactBrowserEventEmitter.js#L128

It looks like, in Chrome, anyway, that setting event.preventDefault() does not set event.defaultPrevented on the event when attached to the document:
https://jsfiddle.net/a2k4whf0/18/

This is also true of document.body, and window.

Maybe this is another reason to revisit "Attach event per react container root, rather than on the document" #2043.

A change like that is serious. I wonder if there's another way we could achieve this.

/cc @philipp-spiess

@philipp-spiess
Copy link
Contributor

philipp-spiess commented Jul 25, 2018

@nhunzaker I think the problem here is that React adds event listeners as passive (the new default) and that passive event listeners do not allow calling preventDefault() (also check out #6436).

Checking out your example makes this pretty clear by the following line in the console (you have to tap a few times for the message to pop up):

(index):44 [Intervention] Unable to preventDefault inside passive event listener due to target being treated as passive. See https://www.chromestatus.com/features/5093566007214080

If I update your example and and mark the event listeners as { passive: false } it works like expected: https://jsfiddle.net/dL4obakf/5/ (am I overlooking something?)

A simple workaround in React) is to add a native event listener using the { passive: false } option:

document.addEventListener(
  "touchstart",
  function(e) {
    e.preventDefault();
  },
  { passive: false }
);

It's also possible to feature-detect passive-event listener support:

let supportsPassiveOption = false;
const opts = Object.defineProperty({}, "passive", {
  get: function() {
    supportsPassiveOption = true;
  }
});
try {
  window.addEventListener("test", null, opts);
} catch (e) {}

@gaearon
Copy link
Member

gaearon commented Aug 17, 2018

Seems like the same issue as #11530.
Chrome made a breaking change and this broke us.

As a workaround you can attach the event listener directly to the node via a ref.
Here's an example from the linked issue:

--- a/src/components/App.js
+++ b/src/components/App.js
@@ -9,12 +9,17 @@ const swallow = (e) => {
 }
 
 class Tile extends PureComponent {
+  node = React.createRef();
 
   constructor(props) {
     super(props)
     this.toggle = this.toggle.bind(this)
   }
 
+  componentDidMount() {
+    this.node.current.ontouchstart = this.toggle;
+  }
+
   toggle(e) {
     swallow(e)
     console.log('id: ', this.props.id)
@@ -27,8 +32,8 @@ class Tile extends PureComponent {
   render() {
     return (
       <div className={`tile`}
+        ref={this.node}
         onMouseDown={this.toggle}
-        onTouchStart={this.toggle}
         onTouchEnd={swallow}>
         <div className="box" style={{backgroundColor: this.props.id}}> </div>
       </div>

If you attach the listener manually then e.preventDefault() in it would work.

Longer term, #2043 will fix it. Since a workaround exists, and this particular issue itself is not actionable for us, I'll close this in favor of #2043 (which would solve this in the longer term).

@gaearon gaearon closed this as completed Aug 17, 2018
@gaearon
Copy link
Member

gaearon commented Aug 18, 2018

Got some feedback this wasn't clear enough.

The problem is that Chrome made a breaking change and e.preventDefault() in document-level touchstart listeners doesn't work anymore. React binds events at document level for better performance. This is why e.preventDefault() in React onTouchStart doesn't currently work.

You can work around this by attaching a listener at the individual node level with refs. I showed how to do this in the previous comment (#9809 (comment)).

Longer term, we plan to change React to attach events at the root container level instead of document level anyway. This is what #2043 is about. So when we implement #2043 the problem will go away by itself. Until then, the workaround with a ref and manual ontouchstart listener should work fine.

@mseddon
Copy link

mseddon commented Jan 11, 2019

I am still confused why ReactJS has been broken for over a year, when @philipp-spiess has provided a trivial fix. Chrome does allow { passive: false } handlers on the document, just by default it is true. Is Chrome no longer supported by ReactJS?

@philipp-spiess
Copy link
Contributor

philipp-spiess commented Jan 12, 2019

@mseddon My fix is not practical to be applied at the framework level. There are a number of arguments why Chrome made some events passive by default and why we don't want to revert that decision. You can read up on that here: #6436

The recommended approach is to only add non-passive events if you're certain that you will need to call event.preventDefault() and you can do this within React as well by manually adding event listeners (as Dan and I pointed out in the comments above).

That said, we would really like a declarative way to add passive/non-passive event listeners within the framework but changing that would break numerous React applications in subtle ways which is why we need to be especially careful here. We're working on a large scale revamp of React DOM called React Fire which should give us what's needed to address this issue though.

@kadamska
Copy link

kadamska commented Jul 2, 2019

I had this same problem and was able to eliminate the superfluous mousedown event by calling preventDefault() from within onTouchEnd like this:

render() {
  return (
    <div
      onMouseDown={e => this.handleMouseDownOrTouchStart(e)}
      onTouchStart={e => this.handleMouseDownOrTouchStart(e)}
      onTouchEnd={e => e.preventDefault()}
    >
      Foo
    </div>
  );
}

Reference: https://developers.google.com/web/updates/2017/01/scrolling-intervention

To suppress the default behavior of a tap (such as the generation of a click event), call preventDefault() inside of a touchend listener.

This works, most of the time. But when I have a nested element and implement double handlers on both a parent and a child, the child's e.preventDefault() only prevents the touch event, but the mouse event is still called on the parent.

@craigkovatch
Copy link

craigkovatch commented Nov 11, 2020

@gaearon is it safe to assume this old issue is fixed in React 17, via the changes to where event listeners are bound?

@sneakyfildy
Copy link

sneakyfildy commented Nov 26, 2020

Still doing preventDefault in touchstart handler does not suppress any following mouse event. Latest React. Handler attached using onTouchStart. (so passive, I presume)

allantokuda added a commit to allantokuda/radraw that referenced this issue Dec 6, 2020
Which caused relations to annoyingly go right into edit mode on click

facebook/react#9809 (comment)
@nz-chris
Copy link

nz-chris commented Feb 1, 2021

I get the following error when trying to invoke preventDefault on a touch event (from onTouchStart).
react-dom.development.js:6202 Unable to preventDefault inside passive event listener invocation.

Therefore, with the latest version of React, this issue's title (touchstart preventDefault() does not prevent click event.) is still an issue.
@gaearon Can this issue be re-opened?

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