Buttons can be clicked even if touchmoves are thrown #1111
Conversation
Previous behavior would suppress Button clicks if any touchmove events are thrown. Some devices are super sensitive and throw touchmove events for even slight taps. So instead, let's track movement during touchmoves and allow clicks to happen if the touch does not move very far. Fixes #1108
|
Very cool, thanks! I think what we might actually want to do is update emitTapEvents to do the same, and then update button to listen for its own tap events. emitTapEvents is relatively new and I think we just didn't get around to updating button to use it. |
|
Makes sense to me.. In the meantime, I can't help but tidy this up just a little |
|
Gah, broke the build. Will fix |
| } else if (firstTouch) { | ||
| // Some devices will throw touchmoves for all but the slightest of taps. | ||
| // So, if we moved only a small distance, this could still be a tap | ||
| // 44 pixels is somewhat abitrary, based on Apple HIG 44px min button size |
There was a problem hiding this comment.
44 seems reasonable. HammerJS uses 10px for another data point. I'm not sure what would be considered too much.
There was a problem hiding this comment.
Maybe use 22px? Since we probably don't want to click a button if the finger was moved away from it? But I would not be opposed to 44px.
There was a problem hiding this comment.
I like the idea of erring on the smaller side and expanding if we need to. 22px sounds good. With 44, someone could touch the center of one button and move their finger completely over another button and have it still be a tap. That seems a little too forgiving.
|
Looking good to me, and great tests, thanks! I want to do a little manual testing myself to see how it feels before pulling this in. @gkatsev, look good to you? |
| xdiff = event.touches[0].pageX - firstTouch.pageX; | ||
| ydiff = event.touches[0].pageY - firstTouch.pageY; | ||
| touchDistance = Math.sqrt(xdiff * xdiff + ydiff * ydiff); | ||
| if (touchDistance > 44) { |
There was a problem hiding this comment.
this 44 should be a variable/constant.
@heff does vjs have a convention for constants like these?
There was a problem hiding this comment.
I don't like having numbers like these just sit around in the code, it starts creeping into "magic number" territory. Also, we might need/want this number elsewhere at some point.
There was a problem hiding this comment.
Seems reasonable to me. I dunno where's the appropriate place to define this constant though.
There was a problem hiding this comment.
Since we don't have plans to use it anywhere else yet or have need to configure it externally, I think you could just create a var at the top of this function to store the value. We can decide if there's a better global place later when it's needed. That's good enough for me at least. @gkatsev?
|
code looks good. Made a comment about making the distance be a constant. |
|
I somehow just found out that this causes issues in iOS. It's now firing both clicks and taps, so the click handler gets run twice. :/ Looking into it |
|
Should be fixed now |
|
I'll give it a spin tomorrow. Maybe @paruls or @bcjwhisenant could help as well. |
|
I haven't been able to look at this (first I saw it, to be honest), but I'd suggest looking at it in in a few different iOS (7.1 and 7.0.x, and 6.1.x if possible) and Android versions. On the desktop, I'd suggest that the pass include at least one flavor of IE, preferably the Surface IE in UI mode. |
|
Just finished testing this on a Surface, Android, and a few different iOS. It looks good! |
|
Thanks again! |
Previous behavior would suppress Button clicks if any touchmove events are thrown. Some devices are super sensitive and throw touchmove events for even slight taps. So instead, let's track movement during touchmoves and allow clicks to happen if the touch does not move very far.
Fixes #1108
Tested on Android Jellybean, Windows Surface, iPad, and desktop browsers