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

Fix/jquery touch #10797

Closed
wants to merge 4 commits into from
Closed

Fix/jquery touch #10797

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9df0531
fix(ngTouch): replace event with originalEvent
jasonals Aug 13, 2013
02d9274
Test that jQuery-wrapped touch event gets unwrapped
pomerantsev Jan 18, 2015
286593a
Remove trailing whitespace
pomerantsev Jan 18, 2015
9c59d7e
Additional tests for touchstart/end and click
pomerantsev Jan 22, 2015
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
11 changes: 11 additions & 0 deletions src/ngTouch/directive/ngClick.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,8 @@ ngTouch.directive('ngClick', ['$parse', '$timeout', '$rootElement',
return; // Too old.
}

// Use JQuery originalEvent
event = event.originalEvent || event;
var touches = event.touches && event.touches.length ? event.touches : [event];
var x = touches[0].clientX;
var y = touches[0].clientY;
Expand Down Expand Up @@ -165,6 +167,8 @@ ngTouch.directive('ngClick', ['$parse', '$timeout', '$rootElement',
// Global touchstart handler that creates an allowable region for a click event.
// This allowable region can be removed by preventGhostClick if we want to bust it.
function onTouchStart(event) {
// Use JQuery originalEvent
event = event.originalEvent || event;
var touches = event.touches && event.touches.length ? event.touches : [event];
var x = touches[0].clientX;
var y = touches[0].clientY;
Expand Down Expand Up @@ -210,6 +214,8 @@ ngTouch.directive('ngClick', ['$parse', '$timeout', '$rootElement',
}

element.on('touchstart', function(event) {
// Use JQuery originalEvent
event = event.originalEvent || event;
tapping = true;
tapElement = event.target ? event.target : event.srcElement; // IE uses srcElement.
// Hack for Safari, which can target text nodes instead of containers.
Expand Down Expand Up @@ -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];
Copy link
Contributor

Choose a reason for hiding this comment

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

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;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Expand Down Expand Up @@ -275,6 +283,9 @@ ngTouch.directive('ngClick', ['$parse', '$timeout', '$rootElement',
// - But the browser's follow-up slow click will be "busted" before it reaches this handler.
// Therefore it's safe to use this directive on both mobile and desktop.
element.on('click', function(event, touchend) {
// Use JQuery originalEvent
event = event.originalEvent || event;

scope.$apply(function() {
clickHandler(scope, {$event: (touchend || event)});
});
Expand Down
7 changes: 3 additions & 4 deletions src/ngTouch/swipe.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,11 +40,10 @@ ngTouch.factory('$swipe', [function() {
};

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

return {
x: e.clientX,
Expand Down
21 changes: 21 additions & 0 deletions test/ngTouch/directive/ngClickSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,27 @@ describe('ngClick (touch)', function() {
}));


it('should unwrap a jQuery-wrapped event object on touchstart/touchend', inject(function($rootScope, $compile) {
element = $compile('<div ng-click="event = $event"></div>')($rootScope);
$rootScope.$digest();

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

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

expect($rootScope.event.clientX).toBeDefined();
expect($rootScope.event.clientY).toBeDefined();
}));


it('should unwrap a jQuery-wrapped event object on click', inject(function($rootScope, $compile) {
element = $compile('<div ng-click="event = $event"></div>')($rootScope);
$rootScope.$digest();

browserTrigger(element, 'click');
expect($rootScope.event.originalEvent).toBeUndefined();
}));


it('should not click if the touch is held too long', inject(function($rootScope, $compile, $rootElement) {
element = $compile('<div ng-click="count = count + 1"></div>')($rootScope);
$rootElement.append(element);
Expand Down