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

If the page re-renders quickly, can click on the wrong thing #13

Closed
glenjamin opened this issue Sep 22, 2015 · 9 comments · Fixed by #17
Labels

Comments

@glenjamin
Copy link

@glenjamin glenjamin commented Sep 22, 2015

If the element under the tap changes between touchstart and touchend, the new element is clicked.

Ideally this lib should detect the target and check that it's the same element at the end of the tap.

@JakeSidSmith

This comment has been minimized.

Copy link
Owner

@JakeSidSmith JakeSidSmith commented Sep 22, 2015

Ahh, that's something I hadn't considered. At the moment it just adds a touchend listener to the window. This shouldn't be a problem to implement.

@JakeSidSmith

This comment has been minimized.

Copy link
Owner

@JakeSidSmith JakeSidSmith commented Oct 1, 2015

Hey, @glenjamin, I've done a little experimenting. Can I ask how you're removing the elements? As it should currently remove the listeners if the element is removed from the DOM. Though it does still trigger a click if the element becomes hidden e.g. with display: none;.

Could you tell me if you're hiding the elements in either of the following ways. Or if you're using a library e.g. react-router, it'd be good to know, thanks. :)

Currently still triggers a click event if hidden. Easy fix.

<button style={{display: this.state.hide ? 'none' : 'block'}}
  onTouchStart={this.hide}
  onClick={this.doSomething} />

Should not trigger a click if removed.

{
  !this.state.hide ? (
    <button
      onTouchStart={this.hide}
      onClick={this.doSomething} />
  ) : undefined
}
@glenjamin

This comment has been minimized.

Copy link
Author

@glenjamin glenjamin commented Oct 1, 2015

Something like this:

tapping on A fires the event on B.

{
  !this.state.hide ? (
    <button
      onTouchStart={this.hide}
      onClick={this.doSomething}>A</button>
  ) : <button
      onTouchStart={this.hide}
      onClick={this.doSomething}>B</button>
}
@JakeSidSmith

This comment has been minimized.

Copy link
Owner

@JakeSidSmith JakeSidSmith commented Oct 1, 2015

Ahhh, I see what you mean. A gets triggered on touch start & B gets triggered on touch end... very weird.

I'll keep looking into this.

@JakeSidSmith

This comment has been minimized.

Copy link
Owner

@JakeSidSmith JakeSidSmith commented Oct 1, 2015

Ok, @glenjamin, this seems to be how it would function on mobile even without react-fastclick. I would suggest using the same element for both buttons (if they are similar enough) and just changing the button's content, and swapping the onClick listener like so:

<button
  onClick={!this.state.hide ? this.doSomething : this.doSomethingElse}>
  {!this.state.hide ? 'A' : 'B'}
</button>
@glenjamin

This comment has been minimized.

Copy link
Author

@glenjamin glenjamin commented Oct 1, 2015

The actual scenario is hitting a "[clear]" button and having the footer links jump to under my thumb.

This doesn't happen without fastclick, as the browser (I guess) won't fire onClick if the target changes.

@JakeSidSmith

This comment has been minimized.

Copy link
Owner

@JakeSidSmith JakeSidSmith commented Oct 1, 2015

What device / OS are you testing on? I'll see if I can get my hands on one today. :)

@glenjamin

This comment has been minimized.

Copy link
Author

@glenjamin glenjamin commented Oct 1, 2015

Happens on iOS 9 safari

A more accurate example than my last one would be if the key or tag name were different so there's a new component instance - I now realise that example doesn't actually do that, so would be equivalent to a single button.

@JakeSidSmith

This comment has been minimized.

Copy link
Owner

@JakeSidSmith JakeSidSmith commented Oct 7, 2015

So, been looking at this for hours now... Here's some findings (mostly just notes to myself).

React seems to automatically add these events:

#document "change" "function bound () { [native code] }"
#document "click" "function bound () { [native code] }"
#document "input" "function bound () { [native code] }"
#document "keydown" "function bound () { [native code] }"
#document "keyup" "function bound () { [native code] }"
#document "selectionchange" "function bound () { [native code] }"
#document "mousedown" "function bound () { [native code] }"
#document "touchstart" "function bound () { [native code] }"

And something prevents any of my events being called when the target element changes. Kind of hard to explain. But I'm not sure it has anything to do with these events.

Anyway, I tried removing react-fastclick from my test project, and it still has the same issue. I'm beginning to wonder if this is a bug (or at least unintended side effect) with React.

Luckily, you can prevent the accidental click triggers by adding the following to the elements that are accidentally being clicked. So, I have something to go by... Though this isn't an ideal solution (in fact it kind of defeats the point of react-fastclick).

preventDefault: function (event) {
  event.preventDefault();
}

// ...

<button onClick={doSomething} onTouchEnd={preventDefault}>

Could you try removing react-fastclick from your project and check if it still has the same issues? Would be good to find out that's it's not just my environment affecting it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.