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

Input click triggered by label click immediately following a touch event is busted #6302

Closed
wants to merge 1 commit into from

Conversation

cconstantin
Copy link
Contributor

To reproduce

  • an element with ng-click
  • a radio button (with ng-model) and associated label
    In a quick sequence tap on the element and then on the label.
    The radio button will not be checked, unless PREVENT_DURATION has passed

@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 (#6302)

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!

@caitp
Copy link
Contributor

caitp commented Feb 18, 2014

I love the tests! I need you to get this passing jshint, though, so that we can make sure the tests are passing on all of the supported browsers.

Thanks~

@caitp caitp added this to the 1.2.14 milestone Feb 18, 2014
@caitp caitp self-assigned this Feb 18, 2014
@cconstantin
Copy link
Contributor Author

Updated

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

@cconstantin
Copy link
Contributor Author

I've just submitted the CLA as a corporation, Aligned Software Solutions Inc.

Cheers

@@ -370,40 +370,128 @@ describe('ngClick (touch)', function() {
}));


it('should not cancel clicks that come long after', inject(function($rootScope, $compile) {
element1 = $compile('<div ng-click="count = count + 1"></div>')($rootScope);
describe('when clicking on label immediately following a touch event on other element', function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

this description is really long and hard to understand, I think

@caitp
Copy link
Contributor

caitp commented Mar 18, 2014

Okay, so the number of tests is pretty good, but ideally we could do this better. The beforeEach is doing a scary amount of work, and this means that extending this suite will be more difficult later on, and it's not clear what each test is doing.

I don't have a lot to say about the implementation, though. Unfortunately I don't have a device to verify this at the moment

lastLabelClickCoordinates = null;
}
// remember label click coordinates to prevent click busting of trigger click event on input
if (event.target.tagName.toLowerCase() === 'label') {
Copy link
Contributor

Choose a reason for hiding this comment

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

this seems oddly specific, is this the only situation that we'd care about?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

AFAICT yes

@IgorMinar
Copy link
Contributor

I like the fix. The test needs more love. Can you just fix up the tests (remove console.log and wrap the long line) and it should be good to be merged.

@IgorMinar
Copy link
Contributor

I verified the CLA

Fix click busting of input click triggered by a label click quickly
following a touch event on a different element, in desktop
and mobile WebKit

To reproduce the issue fixed by this commit set up a page with
 - an element with ng-click
 - a radio button (with hg-model) and associated label
In a quick sequence tap on the element and then on the label.
The radio button will not be checked, unless PREVENT_DURATION has passed

Fixes #6302
@cconstantin
Copy link
Contributor Author

Hope the tests read better now.

@caitp
Copy link
Contributor

caitp commented Mar 19, 2014

the description is still really long winded, but I can't really think of anything better. I'm going to merge it.

caitp pushed a commit to caitp/angular.js that referenced this pull request Mar 19, 2014
Fix click busting of input click triggered by a label click quickly
following a touch event on a different element, in desktop
and mobile WebKit

To reproduce the issue fixed by this commit set up a page with
 - an element with ng-click
 - a radio button (with hg-model) and associated label
In a quick sequence tap on the element and then on the label.
The radio button will not be checked, unless PREVENT_DURATION has passed

Closes angular#6302
@caitp caitp closed this in bc42950 Mar 19, 2014
vojtajina pushed a commit that referenced this pull request Mar 21, 2014
Fix click busting of input click triggered by a label click quickly
following a touch event on a different element, in desktop
and mobile WebKit

To reproduce the issue fixed by this commit set up a page with
 - an element with ng-click
 - a radio button (with hg-model) and associated label
In a quick sequence tap on the element and then on the label.
The radio button will not be checked, unless PREVENT_DURATION has passed

Closes #6302
tbosch pushed a commit to tbosch/angular.js that referenced this pull request Mar 21, 2014
Fix click busting of input click triggered by a label click quickly
following a touch event on a different element, in desktop
and mobile WebKit

To reproduce the issue fixed by this commit set up a page with
 - an element with ng-click
 - a radio button (with hg-model) and associated label
In a quick sequence tap on the element and then on the label.
The radio button will not be checked, unless PREVENT_DURATION has passed

Closes angular#6302
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.

6 participants