-
Notifications
You must be signed in to change notification settings - Fork 7.5k
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
Changes from 4 commits
640ce99
4ac0795
9899b92
f0e6b70
254eb1b
771bbe7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -833,29 +833,52 @@ vjs.Component.prototype.onResize; | |
* @private | ||
*/ | ||
vjs.Component.prototype.emitTapEvents = function(){ | ||
var touchStart, touchTime, couldBeTap, noTap; | ||
var touchStart, firstTouch, touchTime, couldBeTap, noTap, | ||
xdiff, ydiff, touchDistance; | ||
|
||
// Track the start time so we can determine how long the touch lasted | ||
touchStart = 0; | ||
firstTouch = null; | ||
|
||
this.on('touchstart', function(event) { | ||
// Record start time so we can detect a tap vs. "touch and hold" | ||
touchStart = new Date().getTime(); | ||
// Reset couldBeTap tracking | ||
couldBeTap = true; | ||
// If more than one finger, don't consider treating this as a click | ||
if (event.touches.length === 1) { | ||
firstTouch = event.touches[0]; | ||
// Record start time so we can detect a tap vs. "touch and hold" | ||
touchStart = new Date().getTime(); | ||
// Reset couldBeTap tracking | ||
couldBeTap = true; | ||
} | ||
}); | ||
|
||
this.on('touchmove', function(event) { | ||
// If more than one finger, don't consider treating this as a click | ||
if (event.touches.length > 1) { | ||
couldBeTap = false; | ||
} 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's the reasoning behind that? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seems reasonable to me. I dunno where's the appropriate place to define this constant though. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? |
||
couldBeTap = false; | ||
} | ||
} | ||
}); | ||
|
||
noTap = function(){ | ||
couldBeTap = false; | ||
}; | ||
// TODO: Listen to the original target. http://youtu.be/DujfpXOKUp8?t=13m8s | ||
this.on('touchmove', noTap); | ||
this.on('touchleave', noTap); | ||
this.on('touchcancel', noTap); | ||
|
||
// When the touch ends, measure how long it took and trigger the appropriate | ||
// event | ||
this.on('touchend', function(event) { | ||
firstTouch = null; | ||
// Proceed only if the touchmove/leave/cancel event didn't happen | ||
if (couldBeTap === true) { | ||
// Measure how long the touch lasted | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In what case would firstTouch be false here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok yeah, the independent touchmove is good enough reason for me, since firstTouch is used after that.