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

Not touched, and not clicked component gets a ghost mousedown event #11530

Closed
baharev opened this issue Nov 11, 2017 · 45 comments
Closed

Not touched, and not clicked component gets a ghost mousedown event #11530

baharev opened this issue Nov 11, 2017 · 45 comments

Comments

@baharev
Copy link

baharev commented Nov 11, 2017

The deployed (minimal, trimmed down) app is at:

http://www.baharev.info/sandbox/eventbug/

and the entire source code is at:

https://github.com/baharev/eventbug

Clicking or touching either square should make the clicked / touched square disappear, but only that square. Everything works as intended on my desktop machine both in Chrome and in Firefox. It also shows the correct behavior in Safari on iOS (and I don't care about IE or Edge).

The following triggers the bug in Chrome, either on an Android tablet, or on my desktop machine when emulating a hand-held device. Reload the app, and touch or click the top (blue) square: Both squares disappear, and in the console log I see that the not clicked, and not touched bottom green square received a spurious mousedown event (which then deleted it).

There is touch-action: none; in the app.css applied on the squares. If I didn't use it, I would get the following warning in Chrome when emulating a hand-held device:

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

With touch-action: none; (the way it is in the deployed app), this warning goes away.

  1. Is the ghost mousedown event due to a bug in my code?
  2. Or is it a bug in React?
  3. Or is it a bug in Chrome?
  4. How can I resolve this issue? (I am looking for a workaround if the bug is not in my code.)
@gaearon
Copy link
Collaborator

gaearon commented Nov 12, 2017

Have you tried to write similar code without React to determine if this is reproducible with just DOM APIs?

@baharev
Copy link
Author

baharev commented Nov 12, 2017

No, sorry, I haven't. I am a beginner, just learning these things in my free time.

@bonusrk
Copy link

bonusrk commented Nov 13, 2017

I can try to handle it, if nobody do it before.

@gaearon
Copy link
Collaborator

gaearon commented Nov 13, 2017

@bonusrk

If you'd like to help please try to create a reproducing case without React. Then we can see if it's React causing the issue or not.

@baharev
Copy link
Author

baharev commented Nov 13, 2017

@gaearon I already asked the same question at StackOverflow. Someone has tried to reproduce the bug with jQuery only, although all attempts have failed so far.

https://jsfiddle.net/3efsgd8z/3/

However, I am not sure whether the two approaches are comparable; failing to reproduce this bug with jQuery only does not mean the bug is not in Chrome.

@gaearon
Copy link
Collaborator

gaearon commented Nov 13, 2017

We should try to repro with vanilla DOM API.

@bonusrk
Copy link

bonusrk commented Nov 13, 2017

So here it is:
https://github.com/bonusrk/nonReact-test

Thats what i've found:
Chrome 61.0.3163.100
Win10 (also can try on Mac, if needed)
Chrome Dev Tools for mobile

As our issue starter made 2 functions, i also made them seperate
As we know, ontouchstart invokes before click and mousedown events.

As i investigated mousedown invokes very fast after ontouchstart, and this second event invokes on current mouse position, so if we move mouse pretty fast from the block, the second block will not be deleted.

If we just touch first block, we can se in console, that second event invokes on the same coords, where the newblock appears at that time, so mousedown event get it as his target(and delete)

e.preventDefault on function onMouseDown(e) prevent this situation (and every other actions in onMouseDown function as well, so i cant say, that it is a solution)

Anyway this stuff happens in vanilla js.

As our issue starter said he is new to all this, i've made maximum basic code:

const boxes = document.getElementsByClassName('box')
console.log(boxes)

//click handler
function onMouseDown(e) {
  e.preventDefault()//this wont stop double delete
  console.log('I am CLICK target Id===>', e.target.id)
  //click event info
  console.log('EVENT TYPE ===>', e.type, ', EVENT X ===>', e.clientX, ', EVENT Y ===>', e.clientY)
  const id = e.target.id
  //as we do not have any more link to element, this will delete it
  document.getElementById(id).remove()
}

//ontouchstart handler
function onTouchStart(e) {
  // e.preventDefault()  //This stops double delete

  //Use 'touches' object to get touch event data
  console.log('I am TOUCHSTART target ID ===>', e.touches[0].target.id)
  console.log('EVENT TYPE ===>', e.type, ', EVENT X ===>', e.touches[0].pageX, ', EVENT Y ===>', e.touches[0].pageX)
  const id = e.touches[0].target.id

  //delete for touch events
  document.getElementById(id).remove()
}

//Add eventListener for click
Array.from(boxes).forEach(function (item, i, arr) {
  item.addEventListener('mousedown', onMouseDown)
})

// Add eventListener for touch
Array.from(boxes).forEach(function (item, i, arr) {
  item.addEventListener('touchstart', onTouchStart)
})

@gaearon
Copy link
Collaborator

gaearon commented Nov 13, 2017

Seems like it's not a React bug then?

@bonusrk
Copy link

bonusrk commented Nov 13, 2017

Yes and i can assume even more-it is chrome specific problem, i think.

@baharev
Copy link
Author

baharev commented Nov 13, 2017

This vanilla JS solution behaves differently in Safari (on iPhone) than the React solution: Both squares disappear in Safari with this JS implementation whereas the React solution works the way I wanted it to work. Strange.

@bonusrk
Copy link

bonusrk commented Nov 13, 2017

It is well known safari event flow-you need to use both prevenDefault and stopPropagation to make safari make thing in your way. And it is also seems to be browser-specific problem.

@baharev
Copy link
Author

baharev commented Nov 13, 2017

@bonusrk I am sorry, I am completely lost at this point. Why is this line:

https://github.com/bonusrk/nonReact-test/blob/7675c19a97bd9cc6d8d8b7dcefef0d9f2c99f42b/script.js#L30

in comment?

In my React code I do call e.preventDefault() on touch start. And if I move that line out of comments in your code, then the bug disappears too. Sorry, I am afraid I do not understand something here: As I see it, in the equivalent vanilla JS code, that line should not be in comments, and then the bug would no longer be reproducible in vanilla JS.

@bonusrk
Copy link

bonusrk commented Nov 13, 2017

I've commented it to let this bug happens, to show that bug appears in vanilla js. And yes, if uncomment lines bug will disappear.

@baharev
Copy link
Author

baharev commented Nov 13, 2017

I agree that the vanilla JS code indeed demonstrates a bug. The green square must not receive the events of the blue square, but it does. This is a bug in Chrome.

What still confuses me is that in my React code I do call e.preventDefault() on touch start, so the mouse down event should not fire. In the vanilla JS code it indeed stops the bug, but in React it does not. Why?

I guess the answer has something to do with synthetic events: I am calling preventDefault on a synthetic event, and not on the real event as in the vanilla JS.

@bonusrk
Copy link

bonusrk commented Nov 14, 2017

No problems, man. I'll try to investigate this part in React-based code only, not just emulate it in vanila part.

@bonusrk
Copy link

bonusrk commented Nov 14, 2017

Well, i represented everything i could here:
https://github.com/bonusrk/nonReact-test . (vanilla code placed to /public)

TouchEvent {
defaultPrevented:false
}

MouseEvent {
defaultPrevented:true
}

As we can see, MouseEvent captures event preventions, but ToucheEvent doesnt, even i tried both event types prevention- on synthetic event and native.event

toggle = e => {
    e.nativeEvent.preventDefault()
    e.nativeEvent.stopPropagation()
    e.nativeEvent.stopImmediatePropagation()
    console.log('EVENT===>', e.nativeEvent)
    console.log(e.touches)
    console.log('I AM EVENT=========> ', 'X-', e.pageX, ' Y-', e.pageY, ' Type- ', e.type)
    console.log(e.type)
    console.log('id: ', this.props.id)
  }

@gaearon btw, it seems to me that this issue have some relations with 11547

I am digging now to that part:
react-dom.development.js

//1800
var defaultPrevented = nativeEvent.defaultPrevented != null ? nativeEvent.defaultPrevented : nativeEvent.returnValue === false;

@bonusrk
Copy link

bonusrk commented Nov 27, 2017

Well, i can say, that it seems, that new Chrome touch handling (which they made for performance improve), makes touchstart fires after delay and makes it uncontrollable inside Synthetic event.
Native addEventListener looks like walk-around, even it is not react-way as i think.

@baharev
Copy link
Author

baharev commented Nov 27, 2017

I think there are two things to be done.

(1) The vanilla JavaScript code that you prepared shows a bug in Chrome. This bug won't be fixed unless somebody submits a bug report. Should I do it?

(2) In the meantime, a workaround is needed. Could you expand on your idea (with code) how to "solve" this issue, please?

@bonusrk
Copy link

bonusrk commented Nov 27, 2017

@baharev updated example repo with walk-around:
repo
Pretty ugly way, but here it is (i wrote again the very basic code):
So we add very native event listeners, like we can do with events, like resize. It will be added to native event on componentDidMount and will be killed on componentWillUnmount

    componentDidMount() {
    const tiles = document.getElementsByClassName('tile')
    Array.from(tiles).forEach((item, i, arr) => {
      item.addEventListener('touchstart', this._preventMe)
    })
  }
  componentWillUnmount() {
    const tiles = document.getElementsByClassName('tile')
    Array.from(tiles).forEach((item, i, arr) => {
      item.removeEventListener('touchstart', this._preventMe)
    })
  }

I am not saying, that it is best (or at least clever) solution, but it works on Mac, Chrome 61.0.3163.100, Chrome devtools for mobile

@baharev
Copy link
Author

baharev commented Nov 27, 2017

Great, thanks! An ugly but working solution is still better than a beautiful but not working one.

My other question was: Who should submit a bug report to Chrome?

@bonusrk
Copy link

bonusrk commented Nov 27, 2017

@baharev Does it work for you? I didn't try it on Windows.

@baharev
Copy link
Author

baharev commented Nov 27, 2017

@bonusrk I haven't tried it yet, and I cannot try it on Windows (I am on Linux). I might be able to try it on Linux later this week.

Once again: Who should submit a bug report to Chrome?

@bonusrk
Copy link

bonusrk commented Nov 27, 2017

@baharev Well, i think, that you as a glorious explorer of this bug have all rights to create it and get all honor you deserved =)

@baharev
Copy link
Author

baharev commented Nov 27, 2017

@bonusrk OK, I will do the bug report later this week, and I will let you know whether your suggested workaround works on Linux and on Android. I cannot test Windows, sorry.

Many thanks for your help!

@bonusrk
Copy link

bonusrk commented Nov 27, 2017

@baharev i'll try on Window this evening and let you know. But there is no platform-specific handling in my solution, so i am 99% sure it will work everywhere.

@baharev
Copy link
Author

baharev commented Nov 27, 2017

@bonusrk I can now confirm that your suggested workaround works on Linux (Firefox, Chrome), on iPhone (Safari), and on my Android tablet (Chrome). I think I can get away with this workaround, thanks!

@baharev
Copy link
Author

baharev commented Nov 27, 2017

@bonusrk I submitted the bug report to the chromium project:

https://bugs.chromium.org/p/chromium/issues/detail?id=788933

I greatly appreciate your help!

@bonusrk
Copy link

bonusrk commented Nov 28, 2017

@baharev You are always welcome.

@baharev
Copy link
Author

baharev commented Dec 7, 2017

It turns out it is not a bug in Chrome:

"If the contents of the document have changed during processing of the touch events, then the user agent may dispatch the mouse events to a different target than the touch events."

https://bugs.chromium.org/p/chromium/issues/detail?id=788933#c6

https://w3c.github.io/touch-events/#mouse-events

Although, I personally find this behavior very confusing, but apparently that's how it should be.

@baharev
Copy link
Author

baharev commented Dec 8, 2017

The suggested workaround is feasible though a bit problematic in the real application. The difficulty is that I had to use document.getElementById(uuid) to make sure that I am adding (or removing) the event listener to the right component. It is feasible but painful, and the generated HTML code is ugly due to the UUIDs.

What still confuses me is that in my React code I do call e.preventDefault() on touch start, so the mouse down event should not fire. In the vanilla JS code it indeed stops the bug, but in React it does not. Why?

So where is the bug after all if it is not a bug in Chrome? If e.preventDefault() worked on the synthetic events the way it works in vanilla JS and native events, I believe this issue would go away too.

@gaearon
Copy link
Collaborator

gaearon commented Aug 16, 2018

If e.preventDefault() worked on the synthetic events the way it works in vanilla JS and native events, I believe this issue would go away too.

It does.

I guess the answer has something to do with synthetic events: I am calling preventDefault on a synthetic event, and not on the real event as in the vanilla JS.

Calling preventDefault() on a synthetic event definitely does call preventDefault() on the underlying native event. The only difference is that React is using event delegation, and attaches the listeners at the document level.

@gaearon
Copy link
Collaborator

gaearon commented Aug 16, 2018

@baharev It was quite confusing that you pushed the workaround to master. I cloned your repository and it took a while to figure out that I need to roll a few commits back to reproduce. :-)

@baharev
Copy link
Author

baharev commented Aug 16, 2018

@gaearon I am very sorry for that. :( My apologies.

So, what's the verdict with this issue?

@gaearon
Copy link
Collaborator

gaearon commented Aug 16, 2018

The issue appears to be caused by React using event delegation. (Which is better for performance than attaching every handler.)

Here's an example with addEventListener on the nodes themselves:

<!DOCTYPE html>
<html lang="en">
  <style>
    html, body {
      width: 100%;
      height: 100%;
    }
    .box {
      width: 100px;
      height: 100px;
      margin: 10px;
      touch-action: none;
    }
  </style>
  <body>
    <div>
      <div class="tile">
        <div class="box" style="background-color: blue"></div>
      </div>
      <div class="tile">
        <div class="box" style="background-color: green"></div>
      </div>
    </div>
  </body>
  <script>
    const tiles = document.querySelectorAll('.tile');
    tiles.forEach((tile) => {
      tile.addEventListener('touchstart', e => {
        e.preventDefault();
        tile.remove();
      });
      tile.addEventListener('mousedown', e => {
        tile.remove();
      });
    })
  </script>
</html>

You can see that Chrome respects e.preventDefault() on the touchstart event handler.

Here's the same example with using event delegation:

<!DOCTYPE html>
<html lang="en">
  <style>
    html, body {
      width: 100%;
      height: 100%;
    }

    .box {
      width: 100px;
      height: 100px;
      margin: 10px;
      touch-action: none;
    }
  </style>
  <body>
    <div>
      <div class="tile">
        <div class="box" style="background-color: blue"></div>
      </div>
      <div class="tile">
        <div class="box" style="background-color: green"></div>
      </div>
    </div>
  </body>
  <script>
    document.addEventListener('touchstart', e => {
      e.preventDefault();
      e.target.remove();
    });
    document.addEventListener('mousedown', e => {
      e.target.remove();
    });
  </script>
</html>

You can see that Chrome doesn't respect e.preventDefault() in touchstart when it's attached to the document. This might be a Chrome bug — please feel free to file another report with these reproducing cases. Or maybe this is intentional (is #1254 related?). I don't fully understand what the browser's intended behavior is.

What's interesting to me is that attaching to document.body.firstChild appears to be sufficient. So this will probably get resolved by itself when we do #2043.


Regardless, the workaround is to attach a local event listener manually. #11530 (comment) is right in spirit but I don't understand why it needs to be so complicated. In React, when you want to touch the DOM manually, you can use a ref. Here's an example fix using React 16.3+ ref API:

--- 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>

(Note I reused toggle but it might be confusing that sometimes it receives a native and sometimes a synthetic event. In real code I'd probably duplicate it or extract common logic outside.)

Hope this helps! I'll close because there's nothing directly actionable for us, and the workaround is simple enough.

@gaearon gaearon closed this as completed Aug 16, 2018
@baharev
Copy link
Author

baharev commented Aug 17, 2018

@gaearon Perfect, thank you very much for the detailed explanation!

@gaearon
Copy link
Collaborator

gaearon commented Aug 10, 2020

We're changing React to attach events to roots in 17 which should fix it.

@baharev
Copy link
Author

baharev commented Aug 10, 2020

@gaearon That would be great, thanks for the info! This issue is still causing me a lot of pain in my projects, even with the workaround. It would be great to have a proper solution.

@gaearon
Copy link
Collaborator

gaearon commented Aug 10, 2020

react@next and react-dom@next have the new behavior so please give it a try (and let us know if doesn't work as expected).

@gaearon
Copy link
Collaborator

gaearon commented Aug 11, 2020

@baharev
Copy link
Author

baharev commented Aug 11, 2020

@gaearon I did not report it, but your workaround also solved another issue on iOS with double tap zoom. (So another issue in a different browser.)

I only have preliminary results so far. I have tested React 17.0 Release Candidate in:

  • Chrome 84 on Android 6,
  • Safari 12.1.2 on iOS 12.4.6,
  • in a large, non-trivial React project,
  • thorough manual testing, natively, on the physical device.

Obviously, this testing goes way beyond this issue, but I would like to know whether React 17 will break anything in my projects.

All seems fine so far.

However, please wait for the results on iOS 9.3.6. I have run into two very painful to debug issues in that environment. I won't have access to this particular device before Friday.

Could you give a rough estimate when you plan to release React 17, please?

I greatly appreciate your help.

@gaearon
Copy link
Collaborator

gaearon commented Aug 11, 2020

I was thinking in a week or so.

@baharev
Copy link
Author

baharev commented Aug 13, 2020

@gaearon Manually tested the same large, non-trivial React project on:

  • iOS 9.3.6 (a really old iPad),
  • IE 11 (desktop),
  • Edge 18 (desktop).

All seems fine.

Many thanks for your help, and for letting me know that this issue has been resolved.

@gaearon
Copy link
Collaborator

gaearon commented Aug 13, 2020

Thanks for verifying!

@baharev
Copy link
Author

baharev commented Nov 3, 2020

@gaearon This is embarrassing: One thing I did not test in August was the original, minimal example, and guess what, it still shows the old, buggy behaviour. Please consider re-opening this issue.

I did test some of my projects in August, but not all of them. The old bugs re-appeared after I removed your workaround in all of my projects.

The latest source code of the minimal example with React 17 is here:

https://github.com/baharev/ghost-click

The deployment of this code, showing the buggy behaviour, is here:

https://www.baharev.info/sandbox/ghost-click/index.html

Expected behaviour: Clicking or touching either square should make the clicked / touched square disappear, but only that square.

The following triggers the bug in Chrome, either on an Android tablet, or on my desktop machine when emulating a hand-held device. Reload the app, and touch or click the top (blue) square: Both squares disappear, which should not happen.

New since 2017: I cannot get rid of the following error message in Chrome:

Unable to preventDefault inside passive event listener invocation.

Adding touch-action: none; back then made this error message disappear. As of today, I cannot get rid of this error message.

Tried in index.js:

const root = document.getElementById('root')
root.addEventListener("touchstart", () => {}, {passive: false});

and also in app.css:

#root {
  touch-action: none;
}

but they seem to have no effect; this error message persists. (I double-checked in DevTools that the event listener is there, and the CSS style is also being applied.)

Any help is greatly appreciated.

@baharev
Copy link
Author

baharev commented Nov 3, 2020

@gaearon I grabbed your second code snippet from 2018, reproducing the issue with the vanilla DOM API.

The only notable change is that I pass {passive: false} as the third argument to document.addEventListener('touchstart', ...) call and ta-dah: Problem solved, the code behaves at it should. It was sort of documented in 2019 here.

The touch-action: none; does not seem to work with respect to the blocking/passive event listeners, or I am doing something wrong. In any case, I deleted it from the CSS.

So, your slightly modified code from 2018 is below. Try changing {passive: false} to {passive: true} and you will get the bad behaviour:

<!DOCTYPE html>
<html lang="en">
  <style>
    html, body {
      background-color: lightgrey;
      width: 100%;
      height: 100%;
    }
    .box {
      width: 100px;
      height: 100px;
      margin: 10px;
    }
  </style>
  <body>
    <div>
      <div class="tile">
        <div class="box" style="background-color: blue"></div>
      </div>
      <div class="tile">
        <div class="box" style="background-color: green"></div>
      </div>
    </div>
  </body>
  <script>
    document.addEventListener('touchstart', e => {
      e.preventDefault();
      e.target.remove();
    }, {passive: false});
    document.addEventListener('mousedown', e => {
      e.preventDefault();
      e.target.remove();
    });
  </script>
</html>

How can I achieve something like this in React, without a ref to the underlying DOM node?

In other words, how do I fix the React code, linked in my previous comment?

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

3 participants