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

Buttons can be clicked even if touchmoves are thrown #1111

Closed
wants to merge 6 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@jonzeper
Contributor

jonzeper commented Mar 25, 2014

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

Buttons can be clicked even if touchmoves are thrown
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
@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff Mar 26, 2014

Member

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.
https://github.com/videojs/video.js/blob/master/src/js/component.js#L835

Member

heff commented Mar 26, 2014

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.
https://github.com/videojs/video.js/blob/master/src/js/component.js#L835

@jonzeper

This comment has been minimized.

Show comment
Hide comment
@jonzeper

jonzeper Mar 26, 2014

Contributor

Makes sense to me.. In the meantime, I can't help but tidy this up just a little

Contributor

jonzeper commented Mar 26, 2014

Makes sense to me.. In the meantime, I can't help but tidy this up just a little

@jonzeper

This comment has been minimized.

Show comment
Hide comment
@jonzeper

jonzeper Mar 27, 2014

Contributor

Gah, broke the build. Will fix

Contributor

jonzeper commented Mar 27, 2014

Gah, broke the build. Will fix

Show outdated Hide outdated src/js/component.js
} 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

This comment has been minimized.

@heff

heff Mar 31, 2014

Member

44 seems reasonable. HammerJS uses 10px for another data point. I'm not sure what would be considered too much.

@heff

heff Mar 31, 2014

Member

44 seems reasonable. HammerJS uses 10px for another data point. I'm not sure what would be considered too much.

This comment has been minimized.

@gkatsev

gkatsev Mar 31, 2014

Member

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.

@gkatsev

gkatsev Mar 31, 2014

Member

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.

This comment has been minimized.

@heff

heff Apr 1, 2014

Member

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.

@heff

heff Apr 1, 2014

Member

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.

// If more than one finger, don't consider treating this as a click
if (event.touches.length > 1) {
couldBeTap = false;
} else if (firstTouch) {

This comment has been minimized.

@heff

heff Mar 31, 2014

Member

In what case would firstTouch be false here?

@heff

heff Mar 31, 2014

Member

In what case would firstTouch be false here?

This comment has been minimized.

@jonzeper

jonzeper Apr 1, 2014

Contributor

It shouldn't ever be false. The thought here was that the code will fail if firstTouch is not set, so we should check first. I dunno, maybe somebody triggers a touchmove manually? I'm ok to remove this check if you think it's overkill

@jonzeper

jonzeper Apr 1, 2014

Contributor

It shouldn't ever be false. The thought here was that the code will fail if firstTouch is not set, so we should check first. I dunno, maybe somebody triggers a touchmove manually? I'm ok to remove this check if you think it's overkill

This comment has been minimized.

@heff

heff Apr 1, 2014

Member

Ok yeah, the independent touchmove is good enough reason for me, since firstTouch is used after that.

@heff

heff Apr 1, 2014

Member

Ok yeah, the independent touchmove is good enough reason for me, since firstTouch is used after that.

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff Mar 31, 2014

Member

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?

Member

heff commented Mar 31, 2014

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?

Show outdated Hide outdated src/js/component.js
xdiff = event.touches[0].pageX - firstTouch.pageX;
ydiff = event.touches[0].pageY - firstTouch.pageY;
touchDistance = Math.sqrt(xdiff * xdiff + ydiff * ydiff);
if (touchDistance > 44) {

This comment has been minimized.

@gkatsev

gkatsev Mar 31, 2014

Member

this 44 should be a variable/constant.
@heff does vjs have a convention for constants like these?

@gkatsev

gkatsev Mar 31, 2014

Member

this 44 should be a variable/constant.
@heff does vjs have a convention for constants like these?

This comment has been minimized.

@heff

heff Mar 31, 2014

Member

What's the reasoning behind that?

@heff

heff Mar 31, 2014

Member

What's the reasoning behind that?

This comment has been minimized.

@gkatsev

gkatsev Mar 31, 2014

Member

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.

@gkatsev

gkatsev Mar 31, 2014

Member

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.

This comment has been minimized.

@jonzeper

jonzeper Apr 1, 2014

Contributor

Seems reasonable to me. I dunno where's the appropriate place to define this constant though.

@jonzeper

jonzeper Apr 1, 2014

Contributor

Seems reasonable to me. I dunno where's the appropriate place to define this constant though.

This comment has been minimized.

@heff

heff Apr 1, 2014

Member

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?

@heff

heff Apr 1, 2014

Member

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?

@gkatsev

This comment has been minimized.

Show comment
Hide comment
@gkatsev

gkatsev Mar 31, 2014

Member

code looks good. Made a comment about making the distance be a constant.

Member

gkatsev commented Mar 31, 2014

code looks good. Made a comment about making the distance be a constant.

@jonzeper

This comment has been minimized.

Show comment
Hide comment
@jonzeper

jonzeper Apr 2, 2014

Contributor

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

Contributor

jonzeper commented Apr 2, 2014

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

@jonzeper

This comment has been minimized.

Show comment
Hide comment
@jonzeper

jonzeper Apr 2, 2014

Contributor

Should be fixed now

Contributor

jonzeper commented Apr 2, 2014

Should be fixed now

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff Apr 4, 2014

Member

This is looking good to me. @gkatsev would you want to test this on any specific devices before pulling it in? @jonzeper what devices/versions were you able to test this on?

Member

heff commented Apr 4, 2014

This is looking good to me. @gkatsev would you want to test this on any specific devices before pulling it in? @jonzeper what devices/versions were you able to test this on?

@gkatsev

This comment has been minimized.

Show comment
Hide comment
@gkatsev

gkatsev Apr 4, 2014

Member

I'll give it a spin tomorrow. Maybe @paruls or @BCjwhisenant could help as well.

Member

gkatsev commented Apr 4, 2014

I'll give it a spin tomorrow. Maybe @paruls or @BCjwhisenant could help as well.

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff Apr 14, 2014

Member

@gkatsev @paruls @BCjwhisenant, let me know if you've been able to look at this, or if you can soon.

Member

heff commented Apr 14, 2014

@gkatsev @paruls @BCjwhisenant, let me know if you've been able to look at this, or if you can soon.

@BCjwhisenant

This comment has been minimized.

Show comment
Hide comment
@BCjwhisenant

BCjwhisenant Apr 14, 2014

Contributor

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.

Contributor

BCjwhisenant commented Apr 14, 2014

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.

@paruls

This comment has been minimized.

Show comment
Hide comment
@paruls

paruls Apr 15, 2014

Just finished testing this on a Surface, Android, and a few different iOS. It looks good!

paruls commented Apr 15, 2014

Just finished testing this on a Surface, Android, and a few different iOS. It looks good!

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff May 7, 2014

Member

Thanks again!

Member

heff commented May 7, 2014

Thanks again!

@heff heff referenced this pull request May 7, 2014

Closed

Touch events on Buttons #1108

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment