Do not rely on targetTouches for determining "relevant" touch

[elemoine]

goog.events.BrowserEvent#init and goog.style.getClientPosition currently make the same mistake: they use event.targetTouches[0] as the "relevant" touch.

As mdn documents it, targetTouches includes « the Touch objects for touch points that are still in contact with the touch surface and whose touchstart event occurred inside the same target element as the current target element ». So if there are multiple touches on the surface targetTouches[0] may not be the touch that caused the current event to happen!

This pull request fixes both goog.events.BrowserEvent#init and goog.style.getClientPosition by relying on changedTouches[0] instead. changedTouches includes « the Touch objects for touch points that contributed to this touch event », so using changedTouches[0] rather than targetTouches[0] is more correct.

Please review.

Fixes #337.

[elemoine]

I think this is a good patch :-) Anything I can do to get this reviewed?

[elemoine]

Any change to get this reviewed? Anything I can do on my side? Thanks.

[joeltine]

Unfortunately, it looks like the original change this was based on got clobbered in 889d16a. This could make merging messy.

I'll take a look at a PR based off the current master, if you're still interested.

[elemoine]

I'll take a look at a PR based off the current master, if you're still interested.

Yes, I am very much interested in this PR. I do think this is a good change. I'll look at updating the PR.

[joeltine]

Does this PR make #337 obsolete?

[elemoine]

Yes. This is why the issue description includes "Fixes #337". #337 will be automatically closed when this one is closed.

[elemoine]

@joeltine, I updated my targettouches branch. The branch includes two commits:

• 6d440bf This commit is the same as the one that was merged with Fix dragger on touch devices #405, and that was then clobbered by 889d16a.
• 853329f This commit is the one effectively fixing the issue described in the description of this PR.

Both commits include more details about the changes they hold.

[joeltine]

Overall, the PR LGTM, but I just want to be 100% sure I understand the issue with targetTouches.

Is it this phrase that can cause unexpected results: ...whose touchstart event occurred inside the same target element as the current target element.? In other words, the event target is the constraining factor, whereas changedTouches is agnostic to where the event actually occurred.

[elemoine]

The comment attached to d9c1478 explains the problem.

goog.events.BrowserEvent.prototype.init uses the "relevant touch" to set the values of the BrowserEvent's clientX, clientY, screenX and screenY values.

But the question is: what is the "relevant touch"?

As indicated in the comment attached to d9c1478 targetTouches[0] is unlikely to be the "relevant touch" when there are multiple touches on the touch surface, because, as documented on MDN, targetTouches is the list of "all the Touch objects for touch points that are still in contact with the touch surface". targetTouches[0] may be unrelated to the current touch event, so using its clientX, clientY, screenX and screenY values does not make sense.

changedTouches is the list of "all the Touch objects for touch points that contributed to this touch event", so it makes more sense to use changedTouches[0] as the "relevant touch" when setting the BrowserEvent's clientX, clientY, screenX and screenY values.

Hope this clarifies it.

[joeltine]

This method is exactly the same as createChangedTouchEvent below. Can you consolidate into one?

[elemoine]

Thanks for the additional comments. I'll update the PR.

[joeltine]

Fixes #41

[elemoine]

PR updated. Thanks again for the comments.

[joeltine]

Cool, LGTM. Ignore the Travis failure. It's from an unrelated change.

[joeltine]

Looks like a lint error in this PR.

---- FILE : /home/travis/build/google/closure-library/closure/goog/fx/dragger.js -----
Line 454, E:0132: Unused private member: this.mouseDownTime_.
Found 1 error, including 0 new errors, in 1 file (2 files OK).

[elemoine]

Looks like a lint error in this PR.

Really? That doesn't seem to be related to my changes to me.

[joeltine]

Haha! You're right. I'm surprised this was never caught internally.

[elemoine]

I'm happy to add a commit that removes it if you think it makes sense to do it as part of this PR.

[joeltine]

No worries, I can do it internally.

[joeltine]

FYI lint fixed in af3bd97

[joeltine]

Thanks for your work and persistence on this.

[joeltine]

This PR ended up breaking tests internally. This change was the cause of some of them. It's mainly due to tests that mock the touch event and don't include "target".

The question I have is, how confident are we that using the "target" of the touch instead of the event, "e", is the right thing? Per MDN the touch.target is:

The Element on which the touch point started when it was first placed on the surface, even if the touch point has since moved outside the interactive area of that element or even been removed from the document.

Do you have any clue how this could differ from e.target?

[joeltine]

BTW, to do today's external push, I'll likely be pushing the rollback of this change out. Once we decide on the correct fix, this will be pushed out again.

[elemoine]

Do you have any clue how this could differ from e.target?

FYI, that particular change (this.target) came from an old patch: https://code.google.com/p/closure-library/issues/detail?id=382.

So here we're talking about initializing a goog.events.BrowserEvent instance from a touch event, i.e. a browser event resulting from a touch action on the touch surface.

And we want to provide the most relevant values for this.client*, this.screen* and this.target, this being the goog.events.BrowserEvent instance.

This PR suggests relying on e.changedTouches[0] for this.client* and this.screen*, as discussed already.

Now we're discussing this.target, trying to figure out whether it should be set to e.target or e.changedTouches[0].target.

So e.target refers to the Element the browser event was dispatched to (see this mdn page for a good reference). e.changedTouches[0].target is the Element associated with the touch itself, which may be totally unrelated to the e.target. this.target should refer to the event's target, so I think it makes more sense to set always set it to e.target, even when e is a touch event.

If you agree, I can just remove that particular change from my branch.

[elemoine]

This would be the new commits: master...elemoine:changedtouches

I can create a new PR if you agree.

[joeltine]

e.target does seem correct. Specifically the docs for this.target says Target that fired the event., which is exactly what e.target represents.

I'll just make this fix internally when I try to roll forward the change.

[elemoine]

I'll have a closer look Monday or Tuesday.

[myphysicslab]

Chiming in because I use touch events and have had perhaps related difficulties. My comments:

1. touch events seem to not be well defined in either goog.events or the externs. A case-insensitive search for "touch" or "touches" doesn't reveal where targetTouches or changedTouches is defined, except for in the file externs/iPhone.js. Is iPhone the accepted standard for this? The other relevant file seems to be goog/labs/events/touch.js but it doesn't define targetTouches or changedTouches either, and being in the labs directory implies to me it is experimental.
2. I therefore am using the externs/iPhone.js definitions. After a bunch of experiments, I don't trust the changedTouches or targetTouches at all. Instead I just use TouchEvent.touches with the following idea:

a. A single touch in the target element (a Canvas) is treated like a mouseDown, and I start a mouse drag operation. This is in response to a TOUCHSTART event.

b. Multiple touches during TOUCHSTART or TOUCHMOVE events (or a TOUCHEND event) ends an ongoing mouse drag.

This works for me on Android, iPhone, iPad, Mac, Windows.

[joeltine]

Can you expand on why you don't trust changedTouches or targetTouches?

[myphysicslab]

I think because both of changedTouches and targetTouches are leaving out some touches, and I'm not confident about understanding how they decide what to leave out (and whether all browsers/platforms make those decisions the same way). For my situation it seems pretty solid to regard a single touch to be equivalent to a mouse-down and any multiple touch event cancels an existing mouse drag operation.

At the time I was working on this (October 2014) I read this old post from 2010 on the closure list; this seemed to be the most current info I could find about using touch events with closure-library

https://groups.google.com/forum/#!searchin/closure-library-discuss/touch/closure-library-discuss/8XE2c8HZicE/xEptT6CovqYJ

The recommendation given there was:

For rarely-accessed event properties, we often just use BrowserEvent's
getBrowserEvent method to get the underlying native event object. See
goog.events.MouseWheelHandler for a good example of this.

So that's what I'm still doing:

SimController.prototype.touchStart = function(evt) {
  if (evt.target == this.labCanvas_.getCanvas()) {
    var bevt = /** @type {!TouchEvent} */(evt.getBrowserEvent());
    if (bevt != null) {
      var touches = bevt.touches;
      if (touches.length == 1) {
        // single touch in our canvas is treated as mouseDown.
        this.doMouseDown(evt, touches[0].clientX, touches[0].clientY);
      } else {
        // Multiple touch cancels an ongoing mouse drag.
        this.finishDrag(evt);
      }
    }
  }
};

From my point of view I would like a "patched cross platform" way of getting all the current touches during any touch event.

[joeltine]

@myphysicslab I can understand your skepticism. In general, dealing with touch events uniformly is non-trivial.

Unfortunately, not being confident in how browsers handle it isn't enough to affect this PR. If you can provide reproducible evidence, I'd be happy to take a look.

[joeltine]

FYI: finally got this to stick internally. Rolled forward in b2cc62a

[elemoine]

Cool. Thanks for following up on this @joeltine.