Fix/jquery touch #10797

pomerantsev · 2015-01-18T20:41:39Z

This is an addition to PR #8584. The discussion is also there.

When jQuery is included touches are found on event.originalEvent

googlebot · 2015-01-18T20:41:42Z

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

lgalfaso · 2015-01-18T21:31:15Z

the patch looks fine, but please

it looks like the new tests only check 2 of the 3 changes
rebase this into a single commit
follow the commit message conventions

@mzgol can you double check that everything is fine?

mgol · 2015-01-22T15:58:10Z

test/ngTouch/directive/ngClickSpec.js

+
+    browserTrigger(element, 'touchstart');
+    browserTrigger(element, 'touchend');
+    expect($rootScope.event.originalEvent).toBeUndefined();


I'd rather you check here for $rootScope.event.{clientX,clientY} as this is what's actually important here. This should still fail without the patch.

BTW, this test will pass even if only touchend is patched. It'd be good to test them both.

I'd rather you check here for $rootScope.event.{clientX,clientY} as this is what's actually important here.

This would be a false positive on browserTrigger(element, 'click'), though (which you should test as well as it's patched, too).

@lgalfaso We should most likely just create a Protractor test for such stuff, too much hacking must happen here to test anything.

pomerantsev · 2015-01-22T17:55:12Z

@mzgol - sorry, I'm not following.

This would be a false positive on browserTrigger(element, 'click'), though (which you should test as well as it's patched, too).

What exactly should I test in browserTrigger? You're absolutely right about the false positive - clientX and clientY are always there on the event object in tests.

BTW, this test will pass even if only touchend is patched. It'd be good to test them both.

Right, I couldn't find a way to unit-test the rest.

I'll be happy to write a protractor test for this change. Should I put it in test/e2e/tests?

mgol · 2015-01-22T18:33:54Z

I'll be happy to write a protractor test for this change. Should I put it in test/e2e/tests?

I wrote it too fast. We run Protractor tests on Chrome, Firefox & Safari so touch events won't work there, we probably need to stay with just unit tests for now.

This would be a false positive on browserTrigger(element, 'click'), though (which you should test as well as it's patched, too).

What exactly should I test in browserTrigger? You're absolutely right about the false positive - clientX and clientY are always there on the event object in tests.

I've applied your test on master and just changed it to check for clientX/clientY and the test failed as expected in jQuery tests (it didn't fail on jqLite as expected).

How about you add those two clientX/clientY checks without removing your originalEvent one and create a separate test for the click event where you only check for the missing originalEvent property?

pomerantsev · 2015-01-22T22:05:02Z

I've updated tests and I actually had to include the same transformation to the click handler.
The failing test is most probably a flake - some WebDriver stuff.
I will squash / rebase commits if needed, but I feel slightly bad about squashing someone else's commits with mine.

petebacondarwin · 2015-01-24T22:33:39Z

@lgalfaso and @mzgol can you decide who should own this and then see if we can get it into 1.4.x?

mgol · 2015-01-24T23:08:54Z

@petebacondarwin I can take it.

petebacondarwin · 2015-01-25T01:11:58Z

Magic! Thanks @mzgol

pomerantsev · 2015-01-25T13:14:19Z

@mzgol - like I said, I'll be happy to help to get it merged.

LeonardoBraga · 2015-02-10T20:26:15Z

src/ngTouch/directive/ngClick.js

@@ -238,6 +244,8 @@ ngTouch.directive('ngClick', ['$parse', '$timeout', '$rootElement',
    element.on('touchend', function(event) {
      var diff = Date.now() - startTime;

+      // Use JQuery originalEvent
+      event = event.originalEvent || event;
      var touches = (event.changedTouches && event.changedTouches.length) ? event.changedTouches :
          ((event.touches && event.touches.length) ? event.touches : [event]);
      var e = touches[0].originalEvent || touches[0];


Just a suggestion: as far as I know, there's no "originalEvent" in touches[0] - only in the event itself. This would allow you to reduce these two variables to:

var e = (event.changedTouches && event.changedTouches.length) ? event.changedTouches[0] : ((event.touches && event.touches.length) ? event.touches[0] : event;

You're right - thanks for noticing it. I guess I'd just like to get it merged sooner rather than later, and then we can clean it up. Let's see what @mzgol says.

@mcmar

If jQuery was used with Angular the touch logic was looking for touches under the original event objects. However, jQuery wraps all events, keeping the original one under the originalEvent property and copies/normalizes some of event properties. Not all properties are copied, e.g. touches which caused them to not be recognized properly. Thanks to @mcmar & @pomerantsev for original patch ideas. Fixes #4001 Closes #8584 Closes #10797

mgol · 2015-03-31T16:12:35Z

I went in another direction as just assigning $event.originalEvent to $event breaks the general assumption that $event is a jQuery/jqLite event object, not a native one. Please see #11471.

@mcmar

If jQuery was used with Angular the touch logic was looking for touches under the original event objects. However, jQuery wraps all events, keeping the original one under the originalEvent property and copies/normalizes some of event properties. Not all properties are copied, e.g. touches which caused them to not be recognized properly. Thanks to @mcmar & @pomerantsev for original patch ideas. Fixes angular#4001 Closes angular#8584 Closes angular#10797

@mcmar

If jQuery was used with Angular the touch logic was looking for touches under the original event object. However, jQuery wraps all events, keeping the original one under the originalEvent property and copies/normalizes some of event properties. Not all properties are copied, e.g. touches which caused them to not be recognized properly. Thanks to @mcmar & @pomerantsev for original patch ideas. Fixes angular#4001 Closes angular#8584 Closes angular#10797

@mcmar

If jQuery was used with Angular the touch logic was looking for touches under the original event object. However, jQuery wraps all events, keeping the original one under the originalEvent property and copies/normalizes some of event properties. Not all properties are copied, e.g. touches which caused them to not be recognized properly. Thanks to @mcmar & @pomerantsev for original patch ideas. Fixes angular#4001 Closes angular#8584 Closes angular#10797

@mcmar

If jQuery was used with Angular the touch logic was looking for touches under the original event object. However, jQuery wraps all events, keeping the original one under the originalEvent property and copies/normalizes some of event properties. Not all properties are copied, e.g. touches which caused them to not be recognized properly. Thanks to @mcmar & @pomerantsev for original patch ideas. Fixes angular#4001 Closes angular#8584 Closes angular#10797

@mcmar

If jQuery was used with Angular the touch logic was looking for touches under the original event object. However, jQuery wraps all events, keeping the original one under the originalEvent property and copies/normalizes some of event properties. Not all properties are copied, e.g. touches which caused them to not be recognized properly. Thanks to @mcmar & @pomerantsev for original patch ideas. Fixes angular#4001 Closes angular#8584 Closes angular#10797

@mcmar

If jQuery was used with Angular the touch logic was looking for touches under the original event object. However, jQuery wraps all events, keeping the original one under the originalEvent property and copies/normalizes some of event properties. Not all properties are copied, e.g. touches which caused them to not be recognized properly. Thanks to @mcmar & @pomerantsev for original patch ideas. Fixes #4001 Closes #8584 Closes #10797 Closes #11488

@mcmar

If jQuery was used with Angular the touch logic was looking for touches under the original event object. However, jQuery wraps all events, keeping the original one under the originalEvent property and copies/normalizes some of event properties. Not all properties are copied, e.g. touches which caused them to not be recognized properly. Thanks to @mcmar & @pomerantsev for original patch ideas. Fixes angular#4001 Closes angular#8584 Closes angular#10797 Closes angular#11488

jasonals and others added 2 commits January 17, 2015 23:33

fix(ngTouch): replace event with originalEvent

9df0531

When jQuery is included touches are found on event.originalEvent

Test that jQuery-wrapped touch event gets unwrapped

02d9274

googlebot added the cla: no label Jan 18, 2015

Remove trailing whitespace

286593a

pomerantsev mentioned this pull request Jan 18, 2015

Allowing angular-touch to work with jQuery. #8584

Closed

lgalfaso added 1.4-candidate component: ngTouch frequency: moderate severity: confusing type: bug and removed 1.4-candidate labels Jan 18, 2015

mgol reviewed Jan 22, 2015
View reviewed changes

Additional tests for touchstart/end and click

9c59d7e

petebacondarwin added this to the 1.4.x milestone Jan 24, 2015

petebacondarwin assigned lgalfaso and unassigned lgalfaso Jan 24, 2015

mgol self-assigned this Jan 24, 2015

LeonardoBraga reviewed Feb 10, 2015
View reviewed changes

mgol mentioned this pull request Mar 31, 2015

fix(ngTouch): register touches properly when jQuery is used #11471

Closed

mgol closed this in 06a9f0a Apr 2, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix/jquery touch #10797

Fix/jquery touch #10797

pomerantsev commented Jan 18, 2015

googlebot commented Jan 18, 2015

lgalfaso commented Jan 18, 2015

mgol Jan 22, 2015

mgol Jan 22, 2015

mgol Jan 22, 2015

mgol Jan 22, 2015

pomerantsev commented Jan 22, 2015

mgol commented Jan 22, 2015

pomerantsev commented Jan 22, 2015

petebacondarwin commented Jan 24, 2015

mgol commented Jan 24, 2015

petebacondarwin commented Jan 25, 2015

pomerantsev commented Jan 25, 2015

LeonardoBraga Feb 10, 2015

pomerantsev Feb 10, 2015

mgol commented Mar 31, 2015

Fix/jquery touch #10797

Fix/jquery touch #10797

Conversation

pomerantsev commented Jan 18, 2015

googlebot commented Jan 18, 2015

lgalfaso commented Jan 18, 2015

mgol Jan 22, 2015

Choose a reason for hiding this comment

mgol Jan 22, 2015

Choose a reason for hiding this comment

mgol Jan 22, 2015

Choose a reason for hiding this comment

mgol Jan 22, 2015

Choose a reason for hiding this comment

pomerantsev commented Jan 22, 2015

mgol commented Jan 22, 2015

pomerantsev commented Jan 22, 2015

petebacondarwin commented Jan 24, 2015

mgol commented Jan 24, 2015

petebacondarwin commented Jan 25, 2015

pomerantsev commented Jan 25, 2015

LeonardoBraga Feb 10, 2015

Choose a reason for hiding this comment

pomerantsev Feb 10, 2015

Choose a reason for hiding this comment

mgol commented Mar 31, 2015