TouchEvent.changedTouches[] includes unchanged touches in OOP processes

RESOLVED FIXED in mozilla18

Status

()

Core
DOM: Events
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: djf, Assigned: mwu)

Tracking

({regression})

Trunk
mozilla18
x86
Gonk (Firefox OS)
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-basecamp:+, b2g18 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

5 years ago
The pinch-to-zoom gestures in my B2G Gallery app broke when the app was made OOP.

That app uses my gesturedetctor.js library (see gaia/shared/js/gesture_detector.js) which has a finite state machine for dealing with one and two finger gestures.

The FSM works fine when the first finger goes down and then the second, and it works fine when the fingers are moved around the screen...

But when I lift one finger off the screen, I get a touchend event with a changedTouches array that includes both touches, even though only one of the touches actually changed.

This does not happen when the app is in-process.
(Reporter)

Comment 1

5 years ago
Just to be clear, the behavior I'm seeing for OOP is a violation of the spec. http://www.w3.org/TR/touch-events/#touchevent-interface says this:

changedTouches of type TouchList, readonly

    a list of Touches for every point of contact which contributed to the event.

    For the touchstart event this must be a list of the touch points that just became active with the current event. For the touchmove event this must be a list of the touch points that have moved since the last event. For the touchend and touchcancel events this must be a list of the touch points that have just been removed from the surface.
blocking-basecamp: --- → ?
(Reporter)

Comment 2

5 years ago
With a bit of testing, it appears that the first Touch object in changedTouches[] is the real one, and the second is spurious.  The two Touch objects both have valid properties, so there is no way to tell them apart based on that, unfortunately.

I'm going to make the (probably untestable) assumption that the user can't actually lift both fingers at exactly the same time and that I'll always get a sequence of two touchend events when the user stops a pinch gesture. Then, I'm further going to assume that the first element in changedTouches is the real one.  Basically, to work around this bug, I'm only going to use changedTouches[0] when I get a touchend event.

Note that this is a completely fragile workaround that prevents two finger gestures from hanging gesture_detector.js, but opens the door to infrequent bugs if the user is able to lift both fingers at exactly the same time and there really are supposed to be two elements in the changedTouches[] array.

So even though I've got a partial workaround, I think this is still a blocker.
(Reporter)

Comment 3

5 years ago
Very cursory testing suggests that this bug does not occur for touchstart events.  When the second finger goes down, I get an event with only one element in changedTouches.
djf, can you upload your original code here as a test case?  Then we'll verify that bug 774458 fixes the original bug here.
Depends on: 774458
(Reporter)

Comment 5

5 years ago
cjones, I don't see how this bug and #774458 are related.  This one is just about a malformed touchend event.  That one is about the translation of touch events to mouse events, isn't it?

Unfortunately, I don't have a reduced test case for this.  I just added some console.log output to shared/js/gesture_detector.js to figure out what was going on.  

Here's what I'll do: in my gesture detector workaround, I'll emit a console.warn message that references this bug when I see two Touch objects in the changedTouches array on a touchend event.  This won't spam the log too much, because it only happens for two-finger gestures.  Then, when this bug is fixed, you'll be able to confirm by seeing that the warning messages have gone away.

Sound good?
(Reporter)

Comment 6

5 years ago
Okay, so the workaround described above didn't work.  When the second finger goes up, I get two changed touches again, and this time the its the second one I want.  So in general, my "ignore all touches after the first" heuristic doesn't work.

In investigating, however, I learned something interesting.  I put two fingers on the screen, then lift one. I get a touchend that indicates that both fingers have left the screen.  Now I put my finger back down and I get a touchstart that tells me that both fingers are touching the screen again.  

This means that a minor tweak to my FSM fixes the symptom in Gaia, and is probably more robust than the workaround described above.

Still, its pretty bad that events are different in process and out-of-process.  And web content (in the browser app) will be exposed to touch events that violate the spec, I think.  So this should probably be fixed, unless perhapss it is something that was done intentionally for compat with mobile safari.
(Assignee)

Comment 7

5 years ago
Created attachment 655984 [details] [diff] [review]
Improve event handling

I don't know if this fixes your exact problem, but it might be worth a try.
(Reporter)

Comment 8

5 years ago
I tried it, but it didn't fix the problem.
(Assignee)

Comment 9

5 years ago
Created attachment 656125 [details] [diff] [review]
Fix touches sent to content process

This fixes the bug for me. We're sending the wrong set of touches to the content process.
Assignee: nobody → mwu
Attachment #655984 - Attachment is obsolete: true
(Reporter)

Comment 10

5 years ago
Is that the right place to filter the list of touches?

If I have two fingers down and lift one, I'd expect to get a touchend event with one element in touches[] (the finger that is still down) and one element in changedTouches[] (the finger that just lifted).  If you don't send the unchanged touches to the content process, won't we end up with an event with an empty touches[] array?

See also the 2nd paragraph of comment 6.  I was getting double changedTouches for touchstart events too.  This patch that only filters touchend and touchcancel events wouldn't address the touchstart issue.
(Assignee)

Comment 11

5 years ago
(In reply to David Flanagan [:djf] from comment #10)
> Is that the right place to filter the list of touches?
> 
> If I have two fingers down and lift one, I'd expect to get a touchend event
> with one element in touches[] (the finger that is still down) and one
> element in changedTouches[] (the finger that just lifted).  If you don't
> send the unchanged touches to the content process, won't we end up with an
> event with an empty touches[] array?
> 
Nope, the content process keeps track of unchanged touches.

> See also the 2nd paragraph of comment 6.  I was getting double
> changedTouches for touchstart events too.  This patch that only filters
> touchend and touchcancel events wouldn't address the touchstart issue.
Is that a regression from the behavior on the parent process?
(Reporter)

Comment 12

5 years ago
Yes, I think the behavior for touchstart is also different in content processes than it is on the parent process.  It could just be a side effect of the broken touch end stuff, I suppose.

But if the content process is tracking unchanged touches, then we only want to send it changed touches, right?  So wouldn't you want to filter out unchanged touches for all types of touch events, and not just touchend and touchcancel?
Interested to know whether this passes http://people.mozilla.com/~cjones/touch.html .  (mwu, that's the smoketest I mentioned earlier today.)
(Assignee)

Comment 14

5 years ago
(In reply to David Flanagan [:djf] from comment #12)
> But if the content process is tracking unchanged touches, then we only want
> to send it changed touches, right?  So wouldn't you want to filter out
> unchanged touches for all types of touch events, and not just touchend and
> touchcancel?

Nope, because there is only special handling to add unchanged touches on touchend and touchcancel events.

(In reply to Chris Jones [:cjones] [:warhammer] from comment #13)
> Interested to know whether this passes
> http://people.mozilla.com/~cjones/touch.html .  (mwu, that's the smoketest I
> mentioned earlier today.)

I got it to pass, though I wonder if that test is complete enough.
(Assignee)

Updated

5 years ago
Attachment #656125 - Flags: review?(jones.chris.g)
(In reply to Michael Wu [:mwu] from comment #14)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #13)
> > Interested to know whether this passes
> > http://people.mozilla.com/~cjones/touch.html .  (mwu, that's the smoketest I
> > mentioned earlier today.)
> 
> I got it to pass, though I wonder if that test is complete enough.

No, but it was failing previously.
Comment on attachment 656125 [details] [diff] [review]
Fix touches sent to content process

Please add a comment describing what we're munging here and why.

r=me with that
Attachment #656125 - Flags: review?(jones.chris.g) → review+
(Assignee)

Comment 17

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/60438e66056e
Target Milestone: --- → mozilla18
https://hg.mozilla.org/mozilla-central/rev/60438e66056e
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Seems like a blocker and even though it's fixed we don't want it to regress.
blocking-basecamp: ? → +
Depends on: 819119
(Reporter)

Comment 20

5 years ago
This bug is back.

Steps to reproduce:

0) run adb logcat | grep Console
1) Open gallery app
2) Tap on a thumbnail to view an image
3) Put one finger down on the image
4) Put another finger down on the image
5) Lift either one of your fingers

You'll see this message in logcat:

gesture_detector.js: spurious extra changed touch on touchend. See https://bugzilla.mozilla.org/show_bug.cgi?id=785554

This is triggered when the gesture detector module gets a touchend event with two touches in its changedTouches array, and signals that gesture detector is using the workaround I originally put in place when I opened the bug.

This bug is now also affecting the Gaia keyboard, which has been converted to use multitouch.  See https://bugzilla.mozilla.org/show_bug.cgi?id=806540#c31

To be honest, I'd forgotten that this bug had been fixed. I remember noticing the "spurious extra touch" message in logcat a couple of times last week (or maybe earlier than that).  

Chris: could this regression be related to https://bugzilla.mozilla.org/show_bug.cgi?id=774458 or to any of the event work related to that bug?
Status: RESOLVED → REOPENED
Keywords: regression
Resolution: FIXED → ---
Is this bug 822336?  Instead of reopening this, we should address new regressions in a followup.  How about that one?

(Always reopening bugs doesn't scale.  Please only reopen when the original patch fails to perform as advertised when it lands.)
(Reporter)

Comment 22

5 years ago
Chris,

This regression has been around since last week. #822336 is new today (or over the weekend).  So not the same bug.  I'll close this and file a new one.
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
Since 2012-12-13 19:23:48 PST or 2012-12-12 10:59:20 PST or so?
(Reporter)

Comment 24

5 years ago
Chris,

I don't remember when I first started seeing this regression. I know I saw it at least twice last week.  But since my code had a workaround for it, I didn't even clue in that it was a regression... I just thought it had never been fixed.

Meanwhile, I've opened 822558 for this new instance of the bug.
status-b2g18: --- → fixed
Depends on: 823619
You need to log in before you can comment on or make changes to this bug.