facebook / react

The library for web and native user interfaces.
https://react.dev
MIT License
228.41k stars 46.74k forks source link

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

Closed baharev closed 6 years ago

baharev commented 6 years ago

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 commented 6 years ago

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

baharev commented 6 years ago

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

bonusrk commented 6 years ago

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

gaearon commented 6 years ago

@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 commented 6 years ago

@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 commented 6 years ago

We should try to repro with vanilla DOM API.

bonusrk commented 6 years ago

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 commented 6 years ago

Seems like it's not a React bug then?

bonusrk commented 6 years ago

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

baharev commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

@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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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

bonusrk commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

@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 commented 6 years ago

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 commented 6 years ago

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

baharev commented 6 years ago

@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 commented 6 years ago

@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 commented 6 years ago

@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 commented 6 years ago

@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 commented 6 years ago

@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 commented 6 years ago

@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 commented 6 years ago

@baharev You are always welcome.

baharev commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

@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 commented 6 years ago

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

So, what's the verdict with this issue?

gaearon commented 6 years ago

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 https://github.com/facebook/react/issues/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 https://github.com/facebook/react/issues/2043.


Regardless, the workaround is to attach a local event listener manually. https://github.com/facebook/react/issues/11530#issuecomment-347163076 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.

baharev commented 6 years ago

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

gaearon commented 4 years ago

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

baharev commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

https://reactjs.org/blog/2020/08/10/react-v17-rc.html

baharev commented 4 years ago

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

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 commented 4 years ago

I was thinking in a week or so.

baharev commented 4 years ago

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

All seems fine.

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

gaearon commented 4 years ago

Thanks for verifying!

baharev commented 3 years ago

@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 commented 3 years ago

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