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

Buttons can be clicked even if touchmoves are thrown #1111

Closed
wants to merge 6 commits into from
Closed
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 2 additions & 16 deletions src/js/button.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,23 +15,9 @@ vjs.Button = vjs.Component.extend({
init: function(player, options){
vjs.Component.call(this, player, options);

var touchstart = false;
this.on('touchstart', function(event) {
// Stop click and other mouse events from triggering also
event.preventDefault();
touchstart = true;
});
this.on('touchmove', function() {
touchstart = false;
});
var self = this;
this.on('touchend', function(event) {
if (touchstart) {
self.onClick(event);
}
event.preventDefault();
});
this.emitTapEvents();

this.on('tap', this.onClick);
this.on('click', this.onClick);
this.on('focus', this.onFocus);
this.on('blur', this.onBlur);
Expand Down
35 changes: 29 additions & 6 deletions src/js/component.js
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Member

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?

Copy link
Contributor Author

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

Copy link
Member

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.

// 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
Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The 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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the reasoning behind that?

Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The 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
Expand Down
26 changes: 20 additions & 6 deletions test/unit/component.js
Original file line number Diff line number Diff line change
Expand Up @@ -229,7 +229,7 @@ test('should use a defined content el for appending children', function(){
});

test('should emit a tap event', function(){
expect(1);
expect(2);

// Fake touch support. Real touch support isn't needed for this test.
var origTouch = vjs.TOUCH_ENABLED;
Expand All @@ -241,13 +241,27 @@ test('should emit a tap event', function(){
comp.on('tap', function(){
ok(true, 'Tap event emitted');
});
comp.trigger('touchstart');

// A touchstart followed by touchend should trigger a tap
vjs.trigger(comp.el(), {type: 'touchstart', touches: [{}]});
comp.trigger('touchend');

// A touchmove with a lot of movement should not trigger a tap
vjs.trigger(comp.el(), {type: 'touchstart', touches: [
{ pageX: 0, pageY: 0 }
]});
vjs.trigger(comp.el(), {type: 'touchmove', touches: [
{ pageX: 100, pageY: 100 }
]});
comp.trigger('touchend');

// This second test should not trigger another tap event because
// a touchmove is happening
comp.trigger('touchstart');
comp.trigger('touchmove');
// A touchmove with not much movement should still allow a tap
vjs.trigger(comp.el(), {type: 'touchstart', touches: [
{ pageX: 0, pageY: 0 }
]});
vjs.trigger(comp.el(), {type: 'touchmove', touches: [
{ pageX: 10, pageY: 10 }
]});
comp.trigger('touchend');

// Reset to orignial value
Expand Down