Fix issue with Jquery for ngMobile #3198

ethanli83 · 2013-07-11T10:23:33Z

Issue:

I found that the ngClick directive does not work with touch events dispatched by JQuery.
JQuery will wrap touch events, and touches can only be found on originalEvent.
The touch event handlers in ngClick directive are not using the originalEvent but event instead,
so the touches these handlers use, are 'undifined'.

Also, I found browsers are quite sensitive with touch, when I just tap on my touch screen,
the browser will dispatch 'touchstart', 'touchmove', and 'touchend'. Since the touch move handler
also reset status, I never get my click handler triggered.

Fix:

Check if event has an originalEvent property, and use it instead if it is found.
Also I make touch end handler triggers click handler if the distance between touch start and touch end,
is within MOVE_TOLERANCE.

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

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

petebacondarwin · 2013-07-17T20:46:36Z

petebacondarwin · 2013-07-17T20:47:15Z

@ethanli83 - can you please sign the CLA and read our commit message guidelines? Thanks

ethanli83 · 2013-07-22T00:51:34Z

I have done the online form few weeks ago.
My name is Chenguang Li.

bshepherdson · 2013-07-24T17:36:09Z

src/ngMobile/directive/ngClick.js

@@ -221,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 && dist < MOVE_TOLERANCE) {


This condition is important. touchend events can come following moves and other things that invalidate a straight-up-and-down tap, and the tapping flag is set on touchstart and cleared for those illegal motions.

As an example, without this, it's possible to put a finger down, move it two inches away, move it back so it's within the MOVE_TOLERANCE again, and release, triggering a tap.

Was this deliberately removed? If so, why? If not, please put it back when you squash into one commit.

bshepherdson · 2013-07-24T17:39:30Z

I see from the description that you deliberately changed the tapping logic. I follow your argument, but the flag is still necessary, or new logic has to handle the bad case I outlined as well as your case.

What device is it where this unnecessary touchmove is getting included? I've mostly been testing on HTC One, Galaxy Nexus, Nexus 10, Nexus 7 and iPad Mini.

ethanli83 · 2013-07-24T23:49:19Z

Hi Braden,

I never thought about your case, and I think we need to handle that for
sure.

I am creating a kiosk app which is going to use a touch screen (dell
S2340T) rather than mobile device.

So the touch events are dispatched by desktop version of Chrome, and it is
quite sensitive on touch move (try changing Chrome:flags, but still really
sensitive) .

Maybe we do not resetStatus() on 'touchmove' until THRESHOLD exceed?

And sorry for these typos, I am quite famous with bad typing skill in my
team...

Ethan

On Thu, Jul 25, 2013 at 3:40 AM, Braden Shepherdson <
notifications@github.com> wrote:

I see from the description that you deliberately changed the tappinglogic. I follow your argument, but the flag is still necessary, or new
logic has to handle the bad case I outlined as well as your case.

What device is it where this unnecessary touchmove is getting included?
I've mostly been testing on HTC One, Galaxy Nexus, Nexus 10, Nexus 7 and
iPad Mini.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/3198#issuecomment-21502207
.

bshepherdson · 2013-07-25T00:15:23Z

If I understand your suggested fix correctly, it should work. I was thinking of the same thing: rather than a touchmove always canceling the touch, check it against the MOVE_THRESHOLD. Once the difference from the start point gets big enough, cancel it.

Be sure to include some tests, for at least three cases:

normal touchmoves that move only slightly
touchmoves and touchend to outside the radius
touchmoves out and back, with the touchend inside the radius again

Thanks.

ethanli83 · 2013-07-25T04:35:26Z

Yep, exactly what I was thinking.
I will do that and make another pull request shortly.

Cheers

Ethan

On Thu, Jul 25, 2013 at 10:16 AM, Braden Shepherdson <
notifications@github.com> wrote:

If I understand your suggested fix correctly, it should work. I was
thinking of the same thing: rather than a touchmove always canceling the
touch, check it against the MOVE_THRESHOLD. Once the difference from the
start point gets big enough, cancel it.

Be sure to include some tests, for at least three cases:

normal touchmoves that move only slightly

touchmoves and touchend to outside the radius

touchmoves out and back, with the touchend inside the radius again

Thanks.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/3198#issuecomment-21525693
.

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

ethanli83 · 2013-08-02T03:46:01Z

hi @shepheb, I have made code changes base on our discussion.

I made the onTouchStart handler checks the distance between touch start and touch move and only reset tapping to false if it has moved out of the radius

I also add few more tests upon your suggestions

ChristianWeyer · 2013-08-21T11:14:30Z

Any idea when this fix will be merged?

btford · 2013-08-21T21:44:40Z

Ping @shepheb: please provide a status update

bshepherdson · 2013-09-09T14:39:36Z

LGTM.

darrenahunter · 2013-09-09T22:17:45Z

does that mean it will be in next 1.2 build

thanks

kevin-smets · 2013-09-20T13:39:06Z

Sooooo, this will be included in the next (rc) release? Sure hope so, I've rolled my own fastclick for now but I wouldn't mind to throw my code away for a properly integrated solution..

ghost · 2013-10-21T22:37:44Z

This can't work because the touchEnd listener goes through the getSingleTouchLocation(event) function which doesn't look for changedTouches, so it always returns undefined for e.clientX/Y for touchEnd.

ghost · 2013-10-21T22:55:04Z

src/ngMobile/directive/ngClick.js

+    event = event.originalEvent || event;
+
+    // get the first touch object in the event
+    var touches = event.touches && event.touches.length ? event.touches : [event];


This needs to be

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

For this function to work.

ethan.li and others added 3 commits July 11, 2013 17:00

issue:

81f99c0

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

issue:

ecbe81b

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

fix for broken unit test, and add two more unit tests for tap event

986adcb

petebacondarwin mentioned this pull request Jul 17, 2013

fix(ngMobile): touch event handling fix for when jQuery is present. #2773

Closed

bshepherdson reviewed Jul 24, 2013
View reviewed changes

ghost assigned bshepherdson Jul 24, 2013

bshepherdson mentioned this pull request Jul 25, 2013

ngMobile doesn't override ng-click with faster touch-based event when JQuery is loaded #2548

Closed

Improve ngClick.js base on discussions in issue #3198 (#3198)

798e880

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

lefnire mentioned this pull request Aug 19, 2013

Ghost Clicks: can't register, auto-check-boxing, input focus, etc HabitRPG/habitrpg-mobile#75

Closed

ethanli83 closed this Sep 9, 2013

ghost reviewed Oct 21, 2013
View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix issue with Jquery for ngMobile #3198

Fix issue with Jquery for ngMobile #3198

ethanli83 commented Jul 11, 2013

petebacondarwin commented Jul 17, 2013

petebacondarwin commented Jul 17, 2013

ethanli83 commented Jul 22, 2013

bshepherdson Jul 24, 2013

bshepherdson commented Jul 24, 2013

ethanli83 commented Jul 24, 2013

bshepherdson commented Jul 25, 2013

ethanli83 commented Jul 25, 2013

ethanli83 commented Aug 2, 2013

ChristianWeyer commented Aug 21, 2013

btford commented Aug 21, 2013

bshepherdson commented Sep 9, 2013

darrenahunter commented Sep 9, 2013

kevin-smets commented Sep 20, 2013

ghost commented Oct 21, 2013

ghost Oct 21, 2013

Fix issue with Jquery for ngMobile #3198

Fix issue with Jquery for ngMobile #3198

Conversation

ethanli83 commented Jul 11, 2013

petebacondarwin commented Jul 17, 2013

PR Checklist (Minor Bugfix)

petebacondarwin commented Jul 17, 2013

ethanli83 commented Jul 22, 2013

bshepherdson Jul 24, 2013

Choose a reason for hiding this comment

bshepherdson commented Jul 24, 2013

ethanli83 commented Jul 24, 2013

bshepherdson commented Jul 25, 2013

ethanli83 commented Jul 25, 2013

ethanli83 commented Aug 2, 2013

ChristianWeyer commented Aug 21, 2013

btford commented Aug 21, 2013

bshepherdson commented Sep 9, 2013

darrenahunter commented Sep 9, 2013

kevin-smets commented Sep 20, 2013

ghost commented Oct 21, 2013

ghost Oct 21, 2013

Choose a reason for hiding this comment