From 81f99c0385ab321e8a6ff4efe372263e9f473f64 Mon Sep 17 00:00:00 2001 From: "ethan.li" Date: Thu, 11 Jul 2013 17:00:51 +1000 Subject: [PATCH 1/4] issue: touch even handlers do not handle jquery wrapped events quite well fix: check if the event is a jquery event (has originalEvent property) and replace event variable with the originalEvent --- src/ngMobile/directive/ngClick.js | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/ngMobile/directive/ngClick.js b/src/ngMobile/directive/ngClick.js index 3fa68cb4b75f..d39ed475d8e9 100644 --- a/src/ngMobile/directive/ngClick.js +++ b/src/ngMobile/directive/ngClick.js @@ -115,6 +115,10 @@ ngMobile.directive('ngClick', ['$parse', '$timeout', '$rootElement', return; // Too old. } + // retrieve original event if it is wrapped by jquery + // the original event will have an array of touches + event = event.originalEvent || event; + var touches = event.touches && event.touches.length ? event.touches : [event]; var x = touches[0].clientX; var y = touches[0].clientY; @@ -141,6 +145,9 @@ ngMobile.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) { + // retrieve original event which has touches + event = event.originalEvent || event; + var touches = event.touches && event.touches.length ? event.touches : [event]; var x = touches[0].clientX; var y = touches[0].clientY; @@ -187,6 +194,8 @@ ngMobile.directive('ngClick', ['$parse', '$timeout', '$rootElement', element.on('touchstart', function(event) { tapping = true; + event = event.originalEvent || event; + tapElement = event.target ? event.target : event.srcElement; // IE uses srcElement. // Hack for Safari, which can target text nodes instead of containers. if(tapElement.nodeType == 3) { @@ -213,6 +222,7 @@ ngMobile.directive('ngClick', ['$parse', '$timeout', '$rootElement', element.on('touchend', function(event) { var diff = Date.now() - startTime; + event = event.originalEvent || event; var touches = (event.changedTouches && event.changedTouches.length) ? event.changedTouches : ((event.touches && event.touches.length) ? event.touches : [event]); From ecbe81b16fa265c4893f0ce677b5c78b1ba6edfb Mon Sep 17 00:00:00 2001 From: "ethan.li" Date: Thu, 11 Jul 2013 17:24:44 +1000 Subject: [PATCH 2/4] issue: browser dispatch 'touchmove' event when user tap on screen (browser or touch screen being sensitive) fix: if the movement is within MOVE_TOLERANCE, trigger clickHandler --- src/ngMobile/directive/ngClick.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ngMobile/directive/ngClick.js b/src/ngMobile/directive/ngClick.js index d39ed475d8e9..c4791482865d 100644 --- a/src/ngMobile/directive/ngClick.js +++ b/src/ngMobile/directive/ngClick.js @@ -231,7 +231,7 @@ ngMobile.directive('ngClick', ['$parse', '$timeout', '$rootElement', var y = e.clientY; var dist = Math.sqrt( Math.pow(x - touchStartX, 2) + Math.pow(y - touchStartY, 2) ); - if (tapping && diff < TAP_DURATION && dist < MOVE_TOLERANCE) { + if (diff < TAP_DURATION && (tapping || dist < MOVE_TOLERANCE)) { // Call preventGhostClick so the clickbuster will catch the corresponding click. preventGhostClick(x, y); From 986adcbbd9bbbaa441aa35e7a8d242a66a51e751 Mon Sep 17 00:00:00 2001 From: "ethan.li" Date: Sat, 13 Jul 2013 22:17:13 +1000 Subject: [PATCH 3/4] fix for broken unit test, and add two more unit tests for tap event --- src/ngMobile/directive/ngClick.js | 2 +- test/ngMobile/directive/ngClickSpec.js | 26 ++++++++++++++++++++++++++ 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/src/ngMobile/directive/ngClick.js b/src/ngMobile/directive/ngClick.js index c4791482865d..7dd722f38b0b 100644 --- a/src/ngMobile/directive/ngClick.js +++ b/src/ngMobile/directive/ngClick.js @@ -231,7 +231,7 @@ ngMobile.directive('ngClick', ['$parse', '$timeout', '$rootElement', var y = e.clientY; var dist = Math.sqrt( Math.pow(x - touchStartX, 2) + Math.pow(y - touchStartY, 2) ); - if (diff < TAP_DURATION && (tapping || dist < MOVE_TOLERANCE)) { + if (diff < TAP_DURATION && dist < MOVE_TOLERANCE) { // Call preventGhostClick so the clickbuster will catch the corresponding click. preventGhostClick(x, y); diff --git a/test/ngMobile/directive/ngClickSpec.js b/test/ngMobile/directive/ngClickSpec.js index dd7ffed00fe8..02c16888b7be 100644 --- a/test/ngMobile/directive/ngClickSpec.js +++ b/test/ngMobile/directive/ngClickSpec.js @@ -80,6 +80,18 @@ describe('ngClick (mobile)', function() { expect($rootScope.tapped).toBeUndefined(); })); + it('should click if the touchend is within mvoe tolerance', inject(function($rootScope, $compile, $rootElement) { + element = $compile('
')($rootScope); + $rootElement.append(element); + $rootScope.$digest(); + + expect($rootScope.tapped).toBeUndefined(); + + browserTrigger(element, 'touchstart', [], 10, 10); + browserTrigger(element, 'touchend', [], 15, 15); + + expect($rootScope.tapped).toEqual(true); + })); it('should not click if a touchmove comes before touchend', inject(function($rootScope, $compile, $rootElement) { element = $compile('
')($rootScope); @@ -95,6 +107,20 @@ describe('ngClick (mobile)', function() { expect($rootScope.tapped).toBeUndefined(); })); + it('should click if the touchend is within mvoe tolerance, even there is a touchmove before touchend', inject(function($rootScope, $compile, $rootElement) { + element = $compile('
')($rootScope); + $rootElement.append(element); + $rootScope.$digest(); + + expect($rootScope.tapped).toBeUndefined(); + + browserTrigger(element, 'touchstart', [], 10, 10); + browserTrigger(element, 'touchmove'); + browserTrigger(element, 'touchend', [], 15, 15); + + expect($rootScope.tapped).toEqual(true); + })); + it('should add the CSS class while the element is held down, and then remove it', inject(function($rootScope, $compile, $rootElement) { element = $compile('
')($rootScope); $rootElement.append(element); From 798e88007e30cdd3be005b8f5f48dbd93c490e28 Mon Sep 17 00:00:00 2001 From: "ethan.li" Date: Fri, 2 Aug 2013 13:34:21 +1000 Subject: [PATCH 4/4] Improve ngClick.js base on discussions in issue #3198 (https://github.com/angular/angular.js/pull/3198) changes: 1. in onTouchStart, only reset tapping to false if the it has moved outside the MOVE_TOLERANCE 2. abstract repeat code into a function 3. add tests for different touch scenarios --- src/ngMobile/directive/ngClick.js | 73 ++++++++++++++------------ test/ngMobile/directive/ngClickSpec.js | 41 ++++++++++++--- 2 files changed, 74 insertions(+), 40 deletions(-) diff --git a/src/ngMobile/directive/ngClick.js b/src/ngMobile/directive/ngClick.js index 7dd722f38b0b..ac71d5097845 100644 --- a/src/ngMobile/directive/ngClick.js +++ b/src/ngMobile/directive/ngClick.js @@ -108,6 +108,19 @@ ngMobile.directive('ngClick', ['$parse', '$timeout', '$rootElement', return false; // No allowable region; bust it. } + // get the touch location for the first touch object in touch event + function getSingleTouchLocation (event) { + // retrieve original event if it is wrapped by jquery + // the original event will have an array of touches + event = event.originalEvent || event; + + // get the first touch object in the event + var touches = event.touches && event.touches.length ? event.touches : [event]; + var e = touches[0].originalEvent || touches[0]; + + return { x: e.clientX, y: e.clientY } + } + // Global click handler that prevents the click if it's in a bustable zone and preventGhostClick // was called recently. function onClick(event) { @@ -115,24 +128,19 @@ ngMobile.directive('ngClick', ['$parse', '$timeout', '$rootElement', return; // Too old. } - // retrieve original event if it is wrapped by jquery - // the original event will have an array of touches - event = event.originalEvent || event; - - var touches = event.touches && event.touches.length ? event.touches : [event]; - var x = touches[0].clientX; - var y = touches[0].clientY; + var p = getSingleTouchLocation(event); + // Work around desktop Webkit quirk where clicking a label will fire two clicks (on the label // and on the input element). Depending on the exact browser, this second click we don't want // to bust has either (0,0) or negative coordinates. - if (x < 1 && y < 1) { + if (p.x < 1 && p.y < 1) { return; // offscreen } // Look for an allowable region containing this click. // If we find one, that means it was created by touchstart and not removed by // preventGhostClick, so we don't bust it. - if (checkAllowableRegions(touchCoordinates, x, y)) { + if (checkAllowableRegions(touchCoordinates, p.x, p.y)) { return; } @@ -145,18 +153,14 @@ ngMobile.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) { - // retrieve original event which has touches - event = event.originalEvent || event; - - var touches = event.touches && event.touches.length ? event.touches : [event]; - var x = touches[0].clientX; - var y = touches[0].clientY; - touchCoordinates.push(x, y); + + var p = getSingleTouchLocation(event); + touchCoordinates.push(p.x, p.y); $timeout(function() { // Remove the allowable region. for (var i = 0; i < touchCoordinates.length; i += 2) { - if (touchCoordinates[i] == x && touchCoordinates[i+1] == y) { + if (touchCoordinates[i] == p.x && touchCoordinates[i+1] == p.y) { touchCoordinates.splice(i, i + 2); return; } @@ -194,8 +198,7 @@ ngMobile.directive('ngClick', ['$parse', '$timeout', '$rootElement', element.on('touchstart', function(event) { tapping = true; - event = event.originalEvent || event; - + tapElement = event.target ? event.target : event.srcElement; // IE uses srcElement. // Hack for Safari, which can target text nodes instead of containers. if(tapElement.nodeType == 3) { @@ -206,14 +209,23 @@ ngMobile.directive('ngClick', ['$parse', '$timeout', '$rootElement', startTime = Date.now(); - var touches = event.touches && event.touches.length ? event.touches : [event]; - var e = touches[0].originalEvent || touches[0]; - touchStartX = e.clientX; - touchStartY = e.clientY; + var p = getSingleTouchLocation(event); + touchStartX = p.x; + touchStartY = p.y; }); element.on('touchmove', function(event) { - resetState(); + event = event.originalEvent || event; + + // calculate the distance to touch start location + var p = getSingleTouchLocation(event); + var dist = Math.sqrt( Math.pow(p.x - touchStartX, 2) + Math.pow(p.y - touchStartY, 2) ); + + // if current position is not far away from touch start + // we still consider it as a tap event + // if it is farther than the MOVE_TOLERANCE, then resetState to cancel tapping + if (dist >= MOVE_TOLERANCE) + resetState(); }); element.on('touchcancel', function(event) { @@ -222,18 +234,13 @@ ngMobile.directive('ngClick', ['$parse', '$timeout', '$rootElement', element.on('touchend', function(event) { var diff = Date.now() - startTime; - 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]; - var x = e.clientX; - var y = e.clientY; - var dist = Math.sqrt( Math.pow(x - touchStartX, 2) + Math.pow(y - touchStartY, 2) ); + var p = getSingleTouchLocation(event); + var dist = Math.sqrt( Math.pow(p.x - touchStartX, 2) + Math.pow(p.y - touchStartY, 2) ); - if (diff < TAP_DURATION && dist < MOVE_TOLERANCE) { + if (tapping && dist < MOVE_TOLERANCE && diff < TAP_DURATION) { // Call preventGhostClick so the clickbuster will catch the corresponding click. - preventGhostClick(x, y); + preventGhostClick(p.x, p.y); // Blur the focused element (the button, probably) before firing the callback. // This doesn't work perfectly on Android Chrome, but seems to work elsewhere. diff --git a/test/ngMobile/directive/ngClickSpec.js b/test/ngMobile/directive/ngClickSpec.js index 02c16888b7be..a4596014e61f 100644 --- a/test/ngMobile/directive/ngClickSpec.js +++ b/test/ngMobile/directive/ngClickSpec.js @@ -93,7 +93,7 @@ describe('ngClick (mobile)', function() { expect($rootScope.tapped).toEqual(true); })); - it('should not click if a touchmove comes before touchend', inject(function($rootScope, $compile, $rootElement) { + it('should click if a touchmove comes before touchend, and the touchmove is inside the radius', inject(function($rootScope, $compile, $rootElement) { element = $compile('
')($rootScope); $rootElement.append(element); $rootScope.$digest(); @@ -101,13 +101,13 @@ describe('ngClick (mobile)', function() { expect($rootScope.tapped).toBeUndefined(); browserTrigger(element, 'touchstart', [], 10, 10); - browserTrigger(element, 'touchmove'); - browserTrigger(element, 'touchend', [], 400, 400); + browserTrigger(element, 'touchmove', [], 13, 13); + browserTrigger(element, 'touchend', [], 15, 15); - expect($rootScope.tapped).toBeUndefined(); + expect($rootScope.tapped).toEqual(true); })); - it('should click if the touchend is within mvoe tolerance, even there is a touchmove before touchend', inject(function($rootScope, $compile, $rootElement) { + it('should not click if there is a touchmove outside the radius, but touchend is inside the radius', inject(function($rootScope, $compile, $rootElement) { element = $compile('
')($rootScope); $rootElement.append(element); $rootScope.$digest(); @@ -115,10 +115,37 @@ describe('ngClick (mobile)', function() { expect($rootScope.tapped).toBeUndefined(); browserTrigger(element, 'touchstart', [], 10, 10); - browserTrigger(element, 'touchmove'); + browserTrigger(element, 'touchmove', [], 25, 25); browserTrigger(element, 'touchend', [], 15, 15); - expect($rootScope.tapped).toEqual(true); + expect($rootScope.tapped).toBeUndefined(); + })); + + it('should not click if touchmoves are inside the radius, but touchend is outside the radius', inject(function($rootScope, $compile, $rootElement) { element = $compile('
')($rootScope); + $rootElement.append(element); + $rootScope.$digest(); + + expect($rootScope.tapped).toBeUndefined(); + + browserTrigger(element, 'touchstart', [], 10, 10); + browserTrigger(element, 'touchmove', [], 20, 20); + browserTrigger(element, 'touchend', [], 25, 25); + + expect($rootScope.tapped).toBeUndefined(); + })); + + it('should not click if touchmoves and touchend are all outside the radius', inject(function($rootScope, $compile, $rootElement) { + element = $compile('
')($rootScope); + $rootElement.append(element); + $rootScope.$digest(); + + expect($rootScope.tapped).toBeUndefined(); + + browserTrigger(element, 'touchstart', [], 10, 10); + browserTrigger(element, 'touchmove', [], 25, 25); + browserTrigger(element, 'touchend', [], 30, 30); + + expect($rootScope.tapped).toBeUndefined(); })); it('should add the CSS class while the element is held down, and then remove it', inject(function($rootScope, $compile, $rootElement) {