962243 - Ending pinching and then panning with one finger doesn't work

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Doug Sherk (:drs) (inactive)]

If you pinch to zoom on B2G, then let go of one of your fingers and try to continue panning with the other, you will be locked in place until you lift that finger and then set it down again.

My intuition and current working theory without doing any debugging is that it's something here causing it:
http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/GestureEventListener.cpp#275

I remember having issues where touches associated with a pinch weren't combined into one event with multiple touches in it, but rather two events with one touch in each. Will look more.

Comment 1

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

Bug 985541 rewrites all of the GEL.cpp, so marking it as a dependency. This bug should be easier to fix with the rewrite.

Comment 2

[Doug Sherk (:drs) (inactive)]

Ironically, this is now a problem again as a result of bug 985541.

Comment 3

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

Was this ever fixed?

Comment 4

[Doug Sherk (:drs) (inactive)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #3)
> Was this ever fixed?

You're right. I just checked on my Flame with 1.4 on it and it wasn't.

Comment 5

[dmitry.rojkov]

Attachment: Allow transitioning from pinch to pan, panning the same APZC

Please have a look at my attempt to fix the gesture.

Try build: https://tbpl.mozilla.org/?tree=Try&rev=730bbd52b51b

Comment 6

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

Comment on attachment 8445267 [details] [diff] [review]
Allow transitioning from pinch to pan, panning the same APZC

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

Wrong email

Comment 7

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

Comment on attachment 8445267 [details] [diff] [review]
Allow transitioning from pinch to pan, panning the same APZC

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

Didn't look at the test, but I have some comments below.

::: gfx/layers/apz/src/APZCTreeManager.cpp
@@ +483,5 @@
>            mApzcForInputBlock = nullptr;
>            mInOverscrolledApzc = false;
>            mRetainedTouchIdentifier = -1;
>            ClearOverscrollHandoffChain();
> +        } else if (mTouchCount == 1 && mApzcForInputBlock && mOverscrollHandoffChain.length() == 0) {

When we drop from two fingers down to one, we might need to recompute mApzcForInputBlock. For example, say you're on a webpage with an iframe. You start with one finger on the iframe, panning it. Then you add a second finger and pinch. At this point mApzcForInputBlock changes from the iframe to the root page, because that's the APZC that is zoomable. Later, if you remove the second finger, do we want to pan the iframe, or the top-level page? If it's the iframe (which is what I would expect) then we need to move mApzcForInputBlock back to the iframe.

::: gfx/layers/apz/src/GestureEventListener.cpp
@@ +364,5 @@
>        mAsyncPanZoomController->HandleGestureEvent(pinchEvent);
> +      if (mTouches.Length() == 1) {
> +        // User still keeps one finger down, move APZC back to TOUCHING state.
> +        MultiTouchInput fakeInput(MultiTouchInput::MULTITOUCH_START,
> +                                  mLastTouchInput.mTime,

I don't really like this fake input generation. Can we not just pass along something in the PINCHGESTURE_END event that indicates where the touch point is, and have the OnScaleEnd handler in APZC use that to do the appropriate state changes?

Comment 8

[dmitry.rojkov]

Attachment: Allow transitioning from pinch to pan, re-targeting the pan

It seems APZCTreeManager::mTouchCount is not enough to track the last touch point. I've replaced it with ::mTouches, but now GEL::mTouches look redundant.

Comment 9

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

Sorry for the back and forth here. I was looking at the history of this code. Before it was converted to a state machine, we had this change:

https://hg.mozilla.org/mozilla-central/rev/548b80dc1998

which did something similar to what you're doing here. Then Doug did this:

https://hg.mozilla.org/mozilla-central/rev/3e2262b19eb8

which changed it to sending a MULTITOUCH_START with the focus point. There's some discussion on the bug for that as well. And then when it was converted to a state machine I think this code was dropped, probably by accident.

Finally, I want to make sure you're aware of bug 1027309 and bug 1030181 which I'm actively working on which also touch some of this code in APZCTreeManager. Bug 1027309 needs to be uplifted to 2.0 so I would like to make sure that lands first, and then rebase this patch on top of that if needed.

Comment 10

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

Comment on attachment 8445872 [details] [diff] [review]
Allow transitioning from pinch to pan, re-targeting the pan

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

See comment below. I'm not really sure if we understand all the scenarios well enough to come up with a solution yet.

::: gfx/layers/apz/src/APZCTreeManager.cpp
@@ +514,5 @@
>            ClearOverscrollHandoffChain();
> +        } else if (mTouches.Length() == 1 && mOverscrollHandoffChain.length() == 0) {
> +          // Now it may well be the start of panning after pinching.
> +          mApzcForInputBlock = GetTargetAPZC(ScreenPoint(mTouches[0].mScreenPoint),
> +                                             &mInOverscrolledApzc);

I spent some more time thinking about this, and it seems to me that this won't always work. For example, consider the case where you have a page with two iframes, iframeA and iframeB.

You start with one finger on iframeA, and can pan it up and down. Then you add the second finger on iframeB. Now you're in a pinch state on the root page, and can pinch the entire thing. Then you lift up the finger that was on iframeA. For one thing I'm not entirely sure what the expected behaviour is at this point. I assume that you should now be able to scroll iframeB with the remaining finger that is down.

This code will correctly identify the target APZC as that of iframeB, but it will never send it a MULTITOUCH_START event - it will just start delivering it MOVE events, and so the internal state machine of the APZC will not know what to do with them.

Does that make sense?

With the patch that I landed on bug 1027309 (part 3) yesterday, the same thing will happen if you lift the other finger and go back to panning iframeA.

Comment 11

[dmitry.rojkov]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #10)
> This code will correctly identify the target APZC as that of iframeB, but it
> will never send it a MULTITOUCH_START event - it will just start delivering
> it MOVE events, and so the internal state machine of the APZC will not know
> what to do with them.

Hm... that's very true.
What if all AZPCs would share their gesture state through AZPCTreeManager?

Comment 12

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

Yeah I was wondering that too. It might make sense to move GestureEventListener to be between the APZCTreeManager and the APZC instances, rather than having one GEL per APZC instance. I haven't thought this through properly though. There's definitely some architectural changes needed here, I think.

Comment 13

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

I looked at what we do in Fennec for some of these scenarios and what we do there is totally NOT what I described above. Whenever going from pinching -> panning, we always end up panning the same frame that we were pinching (i.e. the root frame). That simplifies things a lot for this bug, because we don't have to worry about fiddling with finding the right APZC on a touchend.

I think it will also help to maintain the invariant that a single touch block (defined as a series of events starting with a touchstart, and going up to but not including the next touchstart) always goes to the same APZC. In particular, that means not switching APZCs on a touchend. I think maintaining this invariant will help with dealing both with touch-action and touch event listeners prevent-default behaviour. (Again, I'm basing this on what we do for Fennec and seems to work well).

So, I think we should go back to the first version of your patch (attachment 8445267 [details] [diff] [review]) which is probably really close to what we want. I don't know if I'll be able to review it today (and I'm away until Wednesday next week) but I'll try to look over it again at some point.

And finally the other change I think we should make is allow panning with two fingers in scenarios even where zooming is disabled. Basically, we should enter the pinch state, and allow movement based on the focus point, but not do anything with the finger span change. That's a separate bug though; just putting it here for completeness.

Comment 14

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

Filed bug 1031443 for the last paragraph of comment 13.

Comment 15

[Botond Ballo [:botond]]

This patch probably needs some unbitrotting at this point.

Comment 16

[Botond Ballo [:botond]]

Assigning the bug to Lynn who expressed interest in it in bug 1031443 comment 20.

Lynn, the first step is to read through the comments in the bug and try to understand the approach being pursued in the patch. Note that as per comment 13, it is the original patch (attachment 8445267 [details] [diff] [review] [diff] [review]) that we will try to clean up and land.

Comment 17

[Botond Ballo [:botond]]

Comment on attachment 8445267 [details] [diff] [review]
Allow transitioning from pinch to pan, panning the same APZC

Un-obsoleting patch and removing r- as per comment 13.

Comment 18

[Botond Ballo [:botond]]

Comment on attachment 8445872 [details] [diff] [review]
Allow transitioning from pinch to pan, re-targeting the pan

Obsoleting patch as per comment 13.

Comment 19

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #16)
> Note that as per comment
> 13, it is the original patch (attachment 8445267 [details] [diff] [review]
> [diff] [review]) that we will try to clean up and land.

I un-obsoleted this patch (and obsoleted the other one) to avoid confusion.

Comment 20

[Botond Ballo [:botond]]

(In reply to Doug Sherk (:drs) (use needinfo?) from comment #4)
> (In reply to Kartikaya Gupta (email:[e-mail]) from comment #3)
> > Was this ever fixed?
> 
> You're right. I just checked on my Flame with 1.4 on it and it wasn't.

So I did some digging, and it turns out this actually *had* been fixed. The issue actually has a somewhat colourful history:

  - I fixed it in bug 937274, using the "let's smuggle the remaining touch
    point to APZC via the PINCHGESTURE_END event's mFocusPoint field"
    approach.

  - Doug changed the fix in bug 937343 to use the "let's fake a
    MULTITOUCH_START event" approach instead.

  - Finally, the refactoring in bug 985541 broke the fix by removing the 
    fake MULTITOUCH_START event (most likely accidentally).

Comment 21

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #7)
> ::: gfx/layers/apz/src/GestureEventListener.cpp
> @@ +364,5 @@
> >        mAsyncPanZoomController->HandleGestureEvent(pinchEvent);
> > +      if (mTouches.Length() == 1) {
> > +        // User still keeps one finger down, move APZC back to TOUCHING state.
> > +        MultiTouchInput fakeInput(MultiTouchInput::MULTITOUCH_START,
> > +                                  mLastTouchInput.mTime,
> 
> I don't really like this fake input generation. Can we not just pass along
> something in the PINCHGESTURE_END event that indicates where the touch point
> is, and have the OnScaleEnd handler in APZC use that to do the appropriate
> state changes?

Kats, judging by bug 937274 comment 3 (and the fact that you r+'d Doug's patch in bug 937343), you used to prefer the MULTITOUCH_START approach, but in this comment you say otherwise. What is your most recent preference?

(My preference is for the MULTITOUCH_START approach, because smuggling in a touch point in a variable called mFocusPoint feels wrong.)

Comment 22

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

(In reply to Botond Ballo [:botond] from comment #21)
> Kats, judging by bug 937274 comment 3 (and the fact that you r+'d Doug's
> patch in bug 937343), you used to prefer the MULTITOUCH_START approach, but
> in this comment you say otherwise. What is your most recent preference?

I think my preference has always been in favour of putting something on the PINCHGESTURE_END, although not necessarily in the mFocusPoint field (It was you, not I, that r+'d Doug's patch in bug 937343, and in bug 937274 comment 3 I think I was just referring to what you said in comment 2). However I don't feel strongly about it, so if you prefer MULTITOUCH_START I'm perfectly ok with that.

Comment 23

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #22)
> It was you, not I, that r+'d Doug's patch in bug 937343

My bad, I mixed up the different patches in that bug.

> I think my preference has always been in favour of putting 
> something on the PINCHGESTURE_END, although not necessarily 
> in the mFocusPoint field. [...] However I don't feel strongly 
> about it, so if you prefer MULTITOUCH_START I'm perfectly ok 
> with that.

We need to pass the remaining touch point to APZC somehow, so
it knows what coordinates to call StartTouch() with. If we want
to use PINCHGESTURE_END but not pass the touch point in
mFocusPoint, we either have to store the touch points in pinch
gesture events in general, or add a special-purpose field for
this purpose. I'd rather avoid that.

---------

Lynn, I hope this side discussion hasn't confused you :)
The conclusion is to go with the MULTITOUCH_START approach 
taken in the patch in this bug (except that, as I mentioned on
IRC, it would be nicer to send the touch-start event using
HandleInputEvent(), the way it was done in [1], rather than 
HandleGestureEvent(), because then we don't need to add code
to HandleGestureEvent()).

[1] https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=937343&attachment=8365350

Comment 24

[Botond Ballo [:botond]]

Hi Lynn,

Just wanted to check in to make sure you aren't stuck on anything. If you are, I'm always happy to help on IRC!

Comment 25

[Botond Ballo [:botond]]

Hi Lynn,

I know we've talked on IRC shortly after my last comment, but I haven't heard from you since - are you still working on this?

Comment 26

[Lynn Tran]

Hello, 

Sorry for the really long delay. I'm almost done with my coop this week, and would love to finish this before I head back to school. Sorry for keeping having you checking up on this.

Comment 27

[dmitry.rojkov]

Attachment: Allow transitioning from pinch to pan, panning the same APZC

It seems nowadays it's enough to update only GestureEventListener and AsyncPanZoomController to make the transition work.

I tested it with EmbedLite-based Gecko38 browser on SailfishOS.

Comment 28

[Botond Ballo [:botond]]

Comment on attachment 8686568 [details] [diff] [review]
Allow transitioning from pinch to pan, panning the same APZC

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

> It seems nowadays it's enough to update only GestureEventListener and AsyncPanZoomController to make the transition work.

I agree, no change to APZCTreeManager is necessary any more.

As per comment 23, the approach we'd like to take to fix this is synthesizing a touch-start event when the gesture ends. Dmitry, would you be interested in updating your patch to do that? (Keeping the test from the original attachment would also be great.) I'd be happy to review it.

::: gfx/layers/apz/src/GestureEventListener.cpp
@@ +100,5 @@
>        rv = HandleInputTouchMultiStart();
>      }
>      break;
>    case MultiTouchInput::MULTITOUCH_MOVE:
> +    for (size_t i = 0; i < aEvent.mTouches.Length(); i++) {

Why is this necessary?

Comment 29

[Botond Ballo [:botond]]

Clearing assignee, as we haven't heard from Lynn in quite a while. (Lynn, if you'd like to continue working on this, feel free to re-assign to yourself.)

Comment 30

[dmitry.rojkov]

(In reply to Botond Ballo [:botond] from comment #28)
> I agree, no change to APZCTreeManager is necessary any more.
> 
> As per comment 23, the approach we'd like to take to fix this is
> synthesizing a touch-start event when the gesture ends. Dmitry, would you be
> interested in updating your patch to do that? (Keeping the test from the
> original attachment would also be great.) I'd be happy to review it.

Unfortunately I've just got notified that I'm about to be laid off and need to return all the equipment to my (now ex-) employer. I guess I'll be busy with real life for some time before I get back to Gecko :)
 
> ::: gfx/layers/apz/src/GestureEventListener.cpp
> @@ +100,5 @@
> >        rv = HandleInputTouchMultiStart();
> >      }
> >      break;
> >    case MultiTouchInput::MULTITOUCH_MOVE:
> > +    for (size_t i = 0; i < aEvent.mTouches.Length(); i++) {
> 
> Why is this necessary?

Without the move updates only touch_start point coordinates are taken into consideration when calculating initial pan velocity: usually this manifests in too fast panning.

Comment 31

[Botond Ballo [:botond]]

(In reply to dmitry.rojkov from comment #30)
> (In reply to Botond Ballo [:botond] from comment #28)
> > I agree, no change to APZCTreeManager is necessary any more.
> > 
> > As per comment 23, the approach we'd like to take to fix this is
> > synthesizing a touch-start event when the gesture ends. Dmitry, would you be
> > interested in updating your patch to do that? (Keeping the test from the
> > original attachment would also be great.) I'd be happy to review it.
> 
> Unfortunately I've just got notified that I'm about to be laid off and need
> to return all the equipment to my (now ex-) employer. I guess I'll be busy
> with real life for some time before I get back to Gecko :)

Sorry to hear that!

I'm going to leave this bug unassigned then, so a contributor who might be interested can finish the work.
  
> > ::: gfx/layers/apz/src/GestureEventListener.cpp
> > @@ +100,5 @@
> > >        rv = HandleInputTouchMultiStart();
> > >      }
> > >      break;
> > >    case MultiTouchInput::MULTITOUCH_MOVE:
> > > +    for (size_t i = 0; i < aEvent.mTouches.Length(); i++) {
> > 
> > Why is this necessary?
> 
> Without the move updates only touch_start point coordinates are taken into
> consideration when calculating initial pan velocity: usually this manifests
> in too fast panning.

Hmm, on second look this does indeed seems broken. Thanks for catching this!

Comment 32

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

(In reply to dmitry.rojkov from comment #30)
> Unfortunately I've just got notified that I'm about to be laid off and need
> to return all the equipment to my (now ex-) employer. I guess I'll be busy
> with real life for some time before I get back to Gecko :)

Ugh, sorry to hear that :(

I'll take this patch and finish it off.

Comment 33

[Botond Ballo [:botond]]

Cool. Pretty sure you won't need mentoring :)

Comment 34

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

So for the record I started to reimplement this by generating a touch-start event from GEL right after the pinchgesture-end. From a technical point of view it looked complicated because if we send this touch-start to APZC via HandleInputEvent, it will re-enter GEL which we don't really want. Alternatively we could go back to sending it via HandleGestureEvent and add machinery to handle touch events there.

Also from a conceptual point of view I'm feeling more strongly now that we don't want to create a fake touch-start event, because we might end up running touch-start-related code that it doesn't make sense to run while transitioning from a pinch-to-pan. Conceptually they are different operations and I'd rather have separate code for them. I took Dmitry's patch and cleaned it up a bit and it seems to work fine so I'd like to go with that.

Comment 35

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

Attachment: Allow transitioning from pinch to pan (v2)

This is Dmitry's patch, minus an unnecessary state change event, plus a comment in PanGestureEvent to document the overloading of mFocusPoint.

Comment 36

[Botond Ballo [:botond]]

Comment on attachment 8689670 [details] [diff] [review]
Allow transitioning from pinch to pan (v2)

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

Thanks! r+ with the gtest from the original patch resurrected.

Comment 37

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #31)
> > > ::: gfx/layers/apz/src/GestureEventListener.cpp
> > > @@ +100,5 @@
> > > >        rv = HandleInputTouchMultiStart();
> > > >      }
> > > >      break;
> > > >    case MultiTouchInput::MULTITOUCH_MOVE:
> > > > +    for (size_t i = 0; i < aEvent.mTouches.Length(); i++) {
> > > 
> > > Why is this necessary?
> > 
> > Without the move updates only touch_start point coordinates are taken into
> > consideration when calculating initial pan velocity: usually this manifests
> > in too fast panning.
> 
> Hmm, on second look this does indeed seems broken. Thanks for catching this!

I didn't fully understand the problem here when I wrote this. I thought the problem was that, in general, things in GestureEventListener that check touch coordinates use possible out-of-date values from the touch-start, and not up-to-date values from subsequent touch-moves. This turns out not to be the case, as whenever we're interested in up-to-date values, we read mLastTouchInput.mTouches rather than mTouches, and mLastTouchInput is updated for each touch event.

The problem is, rather, that in the specific scenario being addressed in this bug (lifting one of two fingers that are down), we are introducing code that _does_ look at mTouches (to get the position of the remaining finger, which is not present in the the touch-end event which only talks about lifted fingers), and so we need that to be up-to-date.

Comment 38

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

Attachment: Part 2 - Gtest

Comment 39

[Botond Ballo [:botond]]

Comment on attachment 8689838 [details] [diff] [review]
Part 2 - Gtest

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

Thanks!

Comment 40

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b12a19262a28
https://hg.mozilla.org/integration/mozilla-inbound/rev/dddbad2aa1ae

Comment 41

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b12a19262a28
https://hg.mozilla.org/mozilla-central/rev/dddbad2aa1ae