Copy touch object for firstTouch #1809

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
4 participants
@steverandy

Using object reference will result in touchDistance value is always 0.
By copying touch object, firstTouch.pageX and firstTouch.pageY values
will not change during touchmove event.

Steve Randy Tantra Steve Randy Tantra
Copy touch object for firstTouch
Using object reference will result in touchDistance value is always 0.
By copying touch object, firstTouch.pageX and firstTouch.pageY values
will not change during touchmove event.
@gkatsev

This comment has been minimized.

Show comment
Hide comment
@gkatsev

gkatsev Jan 18, 2015

Member

Can you give a bit more background on when you're seeing the touches being zero when you don't copy the object? I've not had that issue before.

Member

gkatsev commented Jan 18, 2015

Can you give a bit more background on when you're seeing the touches being zero when you don't copy the object? I've not had that issue before.

@steverandy

This comment has been minimized.

Show comment
Hide comment
@steverandy

steverandy Jan 19, 2015

I find that tap is too sensitive on mobile. Scrolling on top of video.js player often triggers tap and play the video.

xdiff = event.touches[0].pageX - firstTouch.pageX;
ydiff = event.touches[0].pageY - firstTouch.pageY;
touchDistance = Math.sqrt(xdiff * xdiff + ydiff * ydiff);

On line 1063, touchDistance is always 0, because firstTouch.pageX and firstTouch.pageY values are equal to event.touches[0].pageX and event.touches[0].pageY respectively.

This makes scrolling always emits tap event, which is bad on small screen, because users may often perform touch scroll on top of video.js player.

Further, I think we should also reduce tapMovementThreshold to 10 and touchTime limit to 200.

I find that tap is too sensitive on mobile. Scrolling on top of video.js player often triggers tap and play the video.

xdiff = event.touches[0].pageX - firstTouch.pageX;
ydiff = event.touches[0].pageY - firstTouch.pageY;
touchDistance = Math.sqrt(xdiff * xdiff + ydiff * ydiff);

On line 1063, touchDistance is always 0, because firstTouch.pageX and firstTouch.pageY values are equal to event.touches[0].pageX and event.touches[0].pageY respectively.

This makes scrolling always emits tap event, which is bad on small screen, because users may often perform touch scroll on top of video.js player.

Further, I think we should also reduce tapMovementThreshold to 10 and touchTime limit to 200.

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff Jan 19, 2015

Member

I'm not fully understanding the issue. @steverandy, could you describe the steps to reproduce the issue. Bonus points if you can record a video of it happening.

Member

heff commented Jan 19, 2015

I'm not fully understanding the issue. @steverandy, could you describe the steps to reproduce the issue. Bonus points if you can record a video of it happening.

@heff heff added the bug label Jan 19, 2015

@steverandy

This comment has been minimized.

Show comment
Hide comment
@steverandy

steverandy Jan 20, 2015

This is what I did to validate the issue:

  1. Add the following lines to video.dev.js after line 2750, to log the values.

    console.log("xdiff: " + xdiff);
    console.log("ydiff: " + ydiff);
    console.log("touchDistance: " + touchDistance);
    
  2. Launch iOS simulator and Safari inspector

  3. Do a quick scroll on top of video player

Actual result: xdiff, ydiff, and touchDistance values are always 0. Video starts playing.

Expected result: ydiff should be > 0, touchDistance should be > 0. Video should not start playing.

Video: https://dl.dropboxusercontent.com/u/2271268/simulator-result.mov

This is what I did to validate the issue:

  1. Add the following lines to video.dev.js after line 2750, to log the values.

    console.log("xdiff: " + xdiff);
    console.log("ydiff: " + ydiff);
    console.log("touchDistance: " + touchDistance);
    
  2. Launch iOS simulator and Safari inspector

  3. Do a quick scroll on top of video player

Actual result: xdiff, ydiff, and touchDistance values are always 0. Video starts playing.

Expected result: ydiff should be > 0, touchDistance should be > 0. Video should not start playing.

Video: https://dl.dropboxusercontent.com/u/2271268/simulator-result.mov

@gkatsev

This comment has been minimized.

Show comment
Hide comment
@gkatsev

gkatsev Jan 20, 2015

Member

Is this happening only on the simulator or on the ios device as well? Unfortunately, the simulator often does things differently.

Member

gkatsev commented Jan 20, 2015

Is this happening only on the simulator or on the ios device as well? Unfortunately, the simulator often does things differently.

@steverandy

This comment has been minimized.

Show comment
Hide comment
@steverandy

steverandy Jan 20, 2015

Got the same result on device.

Got the same result on device.

@gkatsev

This comment has been minimized.

Show comment
Hide comment
@gkatsev

gkatsev Jan 20, 2015

Member

Thanks for verifying!

Member

gkatsev commented Jan 20, 2015

Thanks for verifying!

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff Jan 22, 2015

Member

Thanks for sticking with this @steverandy. The original issue now makes sense to me. Assuming the browser doesn't create a new touch object for touches[0] when the page X/Y changes, this would be a problem. Thanks for the fix.

@steverandy, we have some exiting tests around touch events. Do you think you could add some for this issue?

As a separate issue, I think it makes sense to reduce the touch more range to 10px. The current 22px referred to half the size of a standard iOS button. At the time HammerJS was using 10px but now they're using 2px, so without knowing more myself it seem like we could at least go down to 10px.

Member

heff commented Jan 22, 2015

Thanks for sticking with this @steverandy. The original issue now makes sense to me. Assuming the browser doesn't create a new touch object for touches[0] when the page X/Y changes, this would be a problem. Thanks for the fix.

@steverandy, we have some exiting tests around touch events. Do you think you could add some for this issue?

As a separate issue, I think it makes sense to reduce the touch more range to 10px. The current 22px referred to half the size of a standard iOS button. At the time HammerJS was using 10px but now they're using 2px, so without knowing more myself it seem like we could at least go down to 10px.

Steve Randy Tantra Steve Randy Tantra
Add two new tests for tap event
To cover the case when touch object in touchmove event isn’t a new
object.
@steverandy

This comment has been minimized.

Show comment
Hide comment
@steverandy

steverandy Jan 22, 2015

Hi @heff, I have added two new tests to cover this issue.

Hi @heff, I have added two new tests to cover this issue.

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff Jan 23, 2015

Member

This looks good to me. @videojs/core-committers, can we get a second review on this?

Member

heff commented Jan 23, 2015

This looks good to me. @videojs/core-committers, can we get a second review on this?

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff Feb 11, 2015

Member

@gkatsev can you do a quick review of this one so we can get it pulled in for the next release?

Member

heff commented Feb 11, 2015

@gkatsev can you do a quick review of this one so we can get it pulled in for the next release?

@heff heff added the confirmed label Feb 11, 2015

@dmlap

This comment has been minimized.

Show comment
Hide comment
@dmlap

dmlap Feb 12, 2015

Member

Code-wise, this lgtm. Thanks for adding the additional test cases, @steverandy!

Member

dmlap commented Feb 12, 2015

Code-wise, this lgtm. Thanks for adding the additional test cases, @steverandy!

@heff heff closed this in fbb2197 Feb 12, 2015

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff Feb 12, 2015

Member

Thanks @steverandy!

Member

heff commented Feb 12, 2015

Thanks @steverandy!

@steverandy

This comment has been minimized.

Show comment
Hide comment
@steverandy

steverandy Feb 13, 2015

Thanks! Hope to see a release soon.

Thanks! Hope to see a release soon.

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