Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

fix(ngTouch): fix ghost click on small touch delta #6995

Closed
wants to merge 1 commit into from

Conversation

jayvan
Copy link

@jayvan jayvan commented Apr 4, 2014

Request Type: bug

How to reproduce: http://plnkr.co/w1q2AJ

Small movements of your finger or touch device when tapping causes both a touchEnd and Click event to trigger the ngClick callback causing it to fire twice. Rapidly tap the numbers from bottom to top to observe this happening occasionally. By tapping rapidly in an upward directions it's easier to get your finger to skid a little. Try out this plunker on a mobile device or with touch emulation enabled.

Component(s): ngTouch

Impact: medium

Complexity: small

This issue is related to:

Detailed Description:

ngClick was leaking ghost click events when a touch moved a small
distance. This is because if there was a touchMove event between the
touchStart and touchEnd preventGhostClick would not be called. This also
caused the MOVE_TOLERANCE threshold to be ignored because any change in
position would stop preventGhostClick from being run.

Cancelling a tap is now correctly based on the MOVE_TOLERANCE, and ghost
clicks are not leaked.

Other Comments:

Closes #6251

Review on Reviewable

@mary-poppins
Copy link

Thanks for the PR! Please check the items below to help us merge this faster. See the contributing docs for more information.

  • Uses the issue template (#6995)

If you need to make changes to your pull request, you can update the commit with git commit --amend.
Then, update the pull request with git push -f.

Thanks again for your help!

ngClick was leaking ghost click events when the touch Startmoved a small
distance. This is because if there was a touchMove event between the
touchStart and touchEnd preventGhostClick would not be called. This also
caused the MOVE_TOLERANCE threshold to be ignored because any change in
position would stop preventGhostClick from being run.

Cancelling a tap is now correctly based on the MOVE_TOLERANCE, and ghost
clicks are not leaked.

Closes angular#6251
@mary-poppins
Copy link

I'm sorry, but I wasn't able to verify your Contributor License Agreement (CLA) signature. CLA signature is required for any code contributions to AngularJS.

Please sign our CLA and ensure that the CLA signature email address and the email address in this PR's commits match.

If you signed the CLA as a corporation, please let us know the company's name.

Thanks a bunch!

PS: If you signed the CLA in the past then most likely the email addresses don't match. Please sign the CLA again or update the email address in the commit of this PR.
PS2: If you are a Googler, please sign the CLA as well to simplify the CLA verification process.

1 similar comment
@mary-poppins
Copy link

I'm sorry, but I wasn't able to verify your Contributor License Agreement (CLA) signature. CLA signature is required for any code contributions to AngularJS.

Please sign our CLA and ensure that the CLA signature email address and the email address in this PR's commits match.

If you signed the CLA as a corporation, please let us know the company's name.

Thanks a bunch!

PS: If you signed the CLA in the past then most likely the email addresses don't match. Please sign the CLA again or update the email address in the commit of this PR.
PS2: If you are a Googler, please sign the CLA as well to simplify the CLA verification process.

@jayvan jayvan added cla: no and removed cla: yes labels Apr 6, 2014
@mary-poppins
Copy link

CLA signature verified! Thank you!

Someone from the team will now triage your PR and it will be processed based on the determined priority (doc updates and fixes with tests are prioritized over other changes).

@jayvan jayvan added the cla: yes label Apr 7, 2014
@mary-poppins
Copy link

CLA signature verified! Thank you!

Someone from the team will now triage your PR and it will be processed based on the determined priority (doc updates and fixes with tests are prioritized over other changes).

@jayvan jayvan added cla: yes and removed cla: no labels Apr 7, 2014
@jeffbcross jeffbcross self-assigned this Apr 7, 2014
@jeffbcross jeffbcross added this to the 1.3.0 milestone Apr 7, 2014
@jeffbcross jeffbcross removed their assignment Apr 7, 2014
@jayvan
Copy link
Author

jayvan commented May 5, 2014

This is an important bug fix. Without it apps that require behavior like the given plnkr do not work. We've been using a forked version of angular with this change to make our game function correctly.

@onefifth
Copy link

This fix was/is super useful for internal projects. Would love to see it merged.

@jayvan
Copy link
Author

jayvan commented Jun 29, 2015

Someone wrote to me asking why I closed this PR, here is my response in case anyone else is curious.

The PR was open for over 1 year and didn't receive any attention from any maintainers of Angular. I think that whoever was originally in charge of the ngTouch module is no longer actively involved in Angular, and none of the other maintainers feel comfortable enough with the module to merge in the PR (though the specs pass, and the change makes sense). I saw that the PR had a merge conflict, and I don't feel it's worth the time to resolve it just to have the PR sit open for another year. It's unfortunate that the fix wasn't merged, I've seen how many people are affected by this problem in the issues(#6251). At Uken Games (where I work) we've been running with this patch in prod for over a year because without it our game has a number of bugs caused by click events firing twice.

If you are feeling adventurous, feel free to make the PR merge-able and try to get it in :)
🍻

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ngClick using ngTouch fires twice
5 participants