937343 - APZC doesn't handle PINCHGESTURE_END correctly in all cases

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Botond Ballo [:botond]]

While investigating bug 937274, I discovered an issue with PINCHGESTURE_END events.

When GestureEventListener sees a MULTITOUCH_END event and it was in the PINCH_GESTURE state, it sends a PINCHGESTURE_END event to the APZC with the MULTITOUCH_END event's first touch point. 

This point is stored in the mFocusPoint field of the PinchGestureInput - this is already fishy since the focus point is not a touch point.

Upon receiving a PINCHGESTURE_END, APZC calls OnScaleEnd(), which starts a pan at the PinchGestureInput's mFocusPoint.

This is reasonable behaviour if only one of the pinch's two touch points was released (in that case the only strange thing about how we implement this is that the remaining touch point is stored in a field called mFocusPoint, which is misleading).

However, this also happens if both touch points were released, in which case we don't want to start a pan.

To fix this, I propose that:
  - APZC not start a pan when receiving a PinchGestureInput.
  - When seeing a MULTITOUCH_END in the PINCH_GESTURE state, GestureEventListener 
    sends a PINCHGESTURE_END with the pinch's focus point as the mFocusPoint field
    of the PinchGestureEvent. Since APZC no longer starts a pan at this point,
    this field will currently be ignored, but at least it will be what its name
    says it is.
      - If only one of the two touch points was lifted, GestureEventListener will
        also send a MULTITOUCH_START event to the APZC, thereby causing it to
        start a pan as desired.

Comment 1

[Doug Sherk (:drs) (inactive)]

Attachment: Remove unchanged touches from touch event sent to RenderFrameParent

It turns out that we have 2 events in the TabParent code that passes events along to content and the panning and zooming logic. [1] One of them is processed correctly and the other isn't.

One event, "event", gets sent to RenderFrameParent, and will eventually make its way to APZC. On the way, it will encounter APZTreeManager, which forwards the out event to APZC instead of the second one, "e". [2]

Suppose a user has two fingers on the screen and is in a pinch gesture. They then lift one finger off to exit the gesture and have the other one still on the screen, with the intention of continuing to pan. Since nsPresShell adds touch events to events of type NS_TOUCH_END, we end up receiving a touch end event at APZC and GEL which has two single touches, when we really should only have one. [3]

I have a proposed fix for this, which is just to mutate both events and strip out these extra touch points, which is attached. I don't think we can propagate the mChanged flag and then strip out the touches once they get to APZC, since the MultiTouchInput struct has no mChanged field or anything like that.

Another possibility is that we could pass along aOutEvent at [2], but that seems incorrect to me since aOutEvent is intended f

[1] http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabParent.cpp#804
[2] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/APZCTreeManager.cpp#447
[3] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/GestureEventListener.cpp#136

Comment 2

[Doug Sherk (:drs) (inactive)]

Attachment: Spoof a touch start event on pinch end in GestureEventListener

This is what was originally talked about in comment 1.

Comment 3

[Doug Sherk (:drs) (inactive)]

Attachment: Remove pan start logic on pinch end from APZC

This is just cleanup after attachment 8365350 [details] [diff] [review].

Comment 4

[Doug Sherk (:drs) (inactive)]

(In reply to Doug Sherk (:drs) from comment #1)
> Another possibility is that we could pass along aOutEvent at [2], but that
> seems incorrect to me since aOutEvent is intended f

for content.

Comment 5

[Michael Wu [:mwu]]

Comment on attachment 8365349 [details] [diff] [review]
Remove unchanged touches from touch event sent to RenderFrameParent

I did touch this code, but I don't feel qualified enough to put a r+ on this change. The change makes sense, but I don't know enough about what other code does with this event.

Comment 6

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8365349 [details] [diff] [review]
Remove unchanged touches from touch event sent to RenderFrameParent

Review of attachment 8365349 [details] [diff] [review]:
-----------------------------------------------------------------

Ideally I would like to get rid of the three-parameter ReceiveInputEvent function on APZCTreeManager and just use the two-parameter one instead. The WidgetInputEvent can be modified in-place. But I think the three-parameter version deals with more types (e.g. mouse event) right now so we'd have to move that code over the to the two-parameter version before we can do that.

::: dom/ipc/TabParent.cpp
@@ +809,5 @@
>      }
>    }
>  
> +  // Create an out event for remote content that is identical to the event that
> +  // we send to the render frame.

Since you're touching this code, update this comment to say that the "out event" e will be transformed so that it unapplies any async transform in the compositor, or something to that effect. (i.e. provide some explanation as to why we're making a copy here).

Comment 7

[Doug Sherk (:drs) (inactive)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #6)
> Since you're touching this code, update this comment to say that the "out
> event" e will be transformed so that it unapplies any async transform in the
> compositor, or something to that effect. (i.e. provide some explanation as
> to why we're making a copy here).

While I was going through this code, I noticed an inconsistency here:
http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabParent.cpp#771
vs here:
http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabParent.cpp#758

I'll look more into this.

Comment 8

[Kartikaya Gupta (email:[e-mail])]

That wheel one is probably wrong but doesn't matter because we don't use it on B2G. And even if we did, untransforming the coordinate at which the mouse was when it generated the wheel event isn't that useful.

Comment 9

[Doug Sherk (:drs) (inactive)]

Attachment: Remove unchanged touches from touch event sent to RenderFrameParent

Addressed review comment, also renamed all instances of "e" when used as the out event to "outEvent". Carrying r+.

Comment 10

[Doug Sherk (:drs) (inactive)]

Attachment: Use out event instead of immutable event for wheel events in TabParent

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #8)
> That wheel one is probably wrong but doesn't matter because we don't use it
> on B2G. And even if we did, untransforming the coordinate at which the mouse
> was when it generated the wheel event isn't that useful.

Okay, this patch fixes that.

Comment 11

[Botond Ballo [:botond]]

Comment on attachment 8365351 [details] [diff] [review]
Remove pan start logic on pinch end from APZC

Review of attachment 8365351 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thanks!

I think these two patches (this and "Spoof a touch start event...") would make sense as a single patch since they go hand in hand. I don't feel strongly about this though.

Comment 12

[Doug Sherk (:drs) (inactive)]

(In reply to Botond Ballo [:botond] from comment #11)
> Comment on attachment 8365351 [details] [diff] [review]
> Remove pan start logic on pinch end from APZC
> 
> Review of attachment 8365351 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good, thanks!
> 
> I think these two patches (this and "Spoof a touch start event...") would
> make sense as a single patch since they go hand in hand. I don't feel
> strongly about this though.

Well, patches in the same bug generally require each other. I separated them so it would be easier to review because the patches do different tasks.

Comment 13

[Botond Ballo [:botond]]

(In reply to Doug Sherk (:drs) from comment #12)
> (In reply to Botond Ballo [:botond] from comment #11)
> > I think these two patches (this and "Spoof a touch start event...") would
> > make sense as a single patch since they go hand in hand. I don't feel
> > strongly about this though.
> 
> Well, patches in the same bug generally require each other. I separated them
> so it would be easier to review because the patches do different tasks.

This is getting a bit academic, but I find it easiest to review patches when they're sufficiently self-contained that each can stand on its own (possibly building on previous patches). 

For example, if you look at my 12 patches in bug 958596, they have the property that for any 1 <= N <= 12, Part1...PartN could be committed without breaking anything. 

In contrast, if we were to commit in this bug e.g. just "Spoof a touch start event..." but not "Remove pan logic...", we would technically break things as APZC would now be getting a duplicate touch-start event. (OK, maybe APZC actually handles that gracefully, but that's besides the point.)

Anyways, as I've said I don't feel strongly about this, so please go ahead and land the patches at whatever granularity you like :)

Comment 14

[Doug Sherk (:drs) (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/87b8624fd4ee
https://hg.mozilla.org/integration/mozilla-inbound/rev/66295044612f
https://hg.mozilla.org/integration/mozilla-inbound/rev/998753d54995
https://hg.mozilla.org/integration/mozilla-inbound/rev/3e2262b19eb8

Comment 15

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/3e2262b19eb8
https://hg.mozilla.org/mozilla-central/rev/998753d54995
https://hg.mozilla.org/mozilla-central/rev/66295044612f
https://hg.mozilla.org/mozilla-central/rev/87b8624fd4ee