1180799 - Support momentum scrolling after a two-fingered pan

Status: VERIFIED FIXED

(Core :: Panning and Zooming, defect)

Description

[Botond Ballo [:botond]]

APZ supports "momentum scrolling" where, after panning (with one finger), when the finger leaves the screen, the page continues to move for a while in the direction of the pan (eventually slowing down and stopping).

APZ also supports two-finger ("pinch") gestures, which can zoom the page (if zooming is allowed and the distance between the two fingers changes), but they can also pan it (if the "focus point" of the pinch, that is, the midpoint of the two fingers, changes).

In bug 1031443, we allowed pages that don't allow zooming to still be panned with two fingers if the focus point changes.

Since we have momentum scrolling after a regular (one-fingered) pan, for a consistent user experience we should also have it after a two-fingered pan. 

To prevent surprises, we should restrict the momentum scrolling to the case where zooming is not allowed.

Comment 1

[Botond Ballo [:botond]]

Assigning to Tuhina who has expressed interest in working on this.

Comment 2

[tuhina chatterjee]

I have a few questions

1-  What is OverScrollHandOff [1] for ?
2- What is the difference between mState [2] and aEvent.mInputType[3]?
3- How to get focuspoint of two fingered gesture event ? aEvent.mLocalFocusPoint or aEvent.mFocusPoint ? how does each differ?
4- How to determine whether the focuspoint before two fingered pan has changed? 
4- OnTouchMove() [4] calls TrackTouch() [5] to handle PAN_MOMENTUM. Can it be used in the current case?
5- What does GetInputQueue()->GetCurrentTouchBlock() do?


Some possible use cases in OnTouchEnd :
1- when the relative distance between two fingers is constant => MomentumPan
2- relative distance is not fixed => Zoom
What are other possible use cases not considered above?

[1] http://searchfox.org/mozilla-central/rev/3582398bc9db5a83e25c2a8299cc780a52ad7ca8/gfx/layers/apz/src/AsyncPanZoomController.cpp#1454
[2]http://searchfox.org/mozilla-central/rev/3582398bc9db5a83e25c2a8299cc780a52ad7ca8/gfx/layers/apz/src/AsyncPanZoomController.cpp#1083
[3]http://searchfox.org/mozilla-central/rev/3582398bc9db5a83e25c2a8299cc780a52ad7ca8/gfx/layers/apz/src/AsyncPanZoomController.cpp#1043
[4]http://searchfox.org/mozilla-central/rev/3582398bc9db5a83e25c2a8299cc780a52ad7ca8/gfx/layers/apz/src/AsyncPanZoomController.cpp#1153
[5]http://searchfox.org/mozilla-central/rev/3582398bc9db5a83e25c2a8299cc780a52ad7ca8/gfx/layers/apz/src/AsyncPanZoomController.cpp#2533

Comment 3

[Botond Ballo [:botond]]

(In reply to tuhina chatterjee from comment #2)
> 1-  What is OverScrollHandOff [1] for ?

Scrollable elements can be nested. For example, you can have a page that's scrollable, and inside the page a <div> which is itself scrollable.

When the user performs a scroll gesture, the first scrollable element to be scrolled is the innermost one under the cursor / finger. In the example above with a scrollable <div> on a scrollable page, if the user scrolls on the <div>, the <div> will scroll first. However, if the user continues scrolling after the <div> has been scrolled as far as it can go, the page will start scrolling instead.

We refer to this process as scroll handoff: the scroll is being handed off from the scrollable element that initially scrolled (the <div>) to a parent scrollable element (the page).

> 2- What is the difference between mState [2] and aEvent.mInputType[3]?

AsyncPanZoomController::mState tracks what state a given scrollable element is in, related to panning and zooming. The set of possible states and their descriptions can be found here [1].

InputData::mInputType specifies what type of user input (such as touch, or mouse, or mouse wheel) as input event represents. The set of possible input types can be found here [2].

> 3- How to get focuspoint of two fingered gesture event ?
> aEvent.mLocalFocusPoint or aEvent.mFocusPoint ? how does each differ?

mFocusPoint and mLocalFocusPoint both represent the same point, but in different coordinate systems: mFocusPoint is in the global (screen) coordinate system, while mLocalFocusPoint is in a scrollable element's local coordinate system.

In the code that you'll be working with, such as the OnScale*() methods, you should generally work with mLocalFocusPoint.

Note that this code has change a bit recently, in bug 1304729. Please pull the latest mozilla-central code which includes that fix, and write your patch on top of that.

(If you've already started writing code on top of an older mozilla-central, and need help with updating to the latest code, let me know; I can help you with that.)

> 4- How to determine whether the focuspoint before two fingered pan has
> changed?

I think that, rather than deciding whether to allow momentum scrolling on a per-gesture basis (whether the gesture involving zooming, or was just a two-fingered pan), we should only allow it on pages where zooming is not allowed at all.

You can test for whether zooming is allowed the same way we do in OnScale(): by checking mZoomConstraints.mAllowZoom [3].

> 4- OnTouchMove() [4] calls TrackTouch() [5] to handle PAN_MOMENTUM. Can it
> be used in the current case?

PAN_MOMENTUM refers to something different than the momentum scrolling we want to do in this bug.

PAN_MOMENTUM refers to a state where Mac trackpads continue to generate pan events for a short time even after the user's finger has left the trackpad.

The momentum scrolling we do after touch events, on the other hand, is an animation driven by the AsyncPanZoomController. It is referred to as a "fling" in the code, and started by calling the APZCTreeManager::DispatchFling() function, as OnTouchEnd() does [4]. The associated PanZoomState is "FLING".

The two effects (PAN_MOMENTUM and FLING) look similar from a user's point of view, but they are different internally in that for PAN_MOMENTUM, the OS continues to generate events and Firefox just listen to and handles them, while for FLING, Firefox drives the effect itself. PAN_MOMENTUM is also specific to Mac, while a FLING can happen on any platform that supports touch events.

> 5- What does GetInputQueue()->GetCurrentTouchBlock() do?

Input events are grouped together into blocks that represent complete gestures (roughly speaking). GetCurrentTouchBlock() returns the block of touch events that is currently being processed.

> Some possible use cases in OnTouchEnd :
> 1- when the relative distance between two fingers is constant => MomentumPan
> 2- relative distance is not fixed => Zoom
> What are other possible use cases not considered above?

Note that two-fingered touch gestures are not handled by the OnTouchStart/OnTouchMove/OnTouchEnd functions. These functions are for one-fingered touch gestures only.

Two-fingered touch gestures are handled by the OnScaleBegin/OnScale/OnScaleEnd functions.

The behaviour where changing the distance between the two fingers during a two-fingered touch gesture causes zooming, is already implemented in OnScale(). The behaviour where you can pan with a two-fingered gesture by moving the fingers together without changing the distance between them, is also implemented.

What we'd like to implement, in this bug, is some new behaviour when a two-fingered touch gesture *ends*. If the page does not allow zooming, we would like the panning that happened during the two-fingered gesture to be followed by a momentum scroll (fling). The relevant code will probably need to be in OnScaleEnd().


Hope that answers your questions! Please feel free to ask more if you have more questions.

[1] http://searchfox.org/mozilla-central/rev/05ed82e50b45df5aa5a8fad219dece1b56757261/gfx/layers/apz/src/AsyncPanZoomController.h#781
[2] http://searchfox.org/mozilla-central/rev/05ed82e50b45df5aa5a8fad219dece1b56757261/widget/InputData.h#33
[3] http://searchfox.org/mozilla-central/rev/05ed82e50b45df5aa5a8fad219dece1b56757261/gfx/layers/apz/src/AsyncPanZoomController.cpp#1377
[4] http://searchfox.org/mozilla-central/rev/05ed82e50b45df5aa5a8fad219dece1b56757261/gfx/layers/apz/src/AsyncPanZoomController.cpp#1271

Comment 4

[tuhina chatterjee]

Attachment: prelim.patch

Comment 5

[Botond Ballo [:botond]]

Comment on attachment 8796979 [details] [diff] [review]
prelim.patch

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

Thanks, Tuhina, this is a good start.

A few comments:

::: gfx/layers/apz/src/AsyncPanZoomController.cpp
@@ +1375,5 @@
>                     (spanRatio < 1.0 && userZoom > realMinZoom);
>  
>      if (!mZoomConstraints.mAllowZoom) {
>        doScale = false;
> +      SetState(PANNING);

We don't want to change the state to PANNING during a two-fingered pan. The rest of the code is written to assume the state will remain PINCHING during this time.

For example, if we change the state to PANNING, subsequent calls to OnScale() generated by subsequent touch-move events with two fingers down, will exit early due to the check |mState != PINCHING| near the top of the function.

@@ +1468,2 @@
>      } else {
> +      switch (mState) {

The only state we need to handle here is PINCHING, as that is the state we'll be in during a two-finger pan. The code to handle it will be similar to the code here for the PANNING cases: the call to DispatchFling() and the preceding code.

Comment 6

[tuhina chatterjee]

Attachment: patch1.diff

Comment 7

[Botond Ballo [:botond]]

Comment on attachment 8797760 [details] [diff] [review]
patch1.diff

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

Thanks, this looks better!

Do you have access to a touch-enabled device on which you can test the patch? It could be a touchscreen laptop, or an Android phone or tablet.

::: gfx/layers/apz/src/AsyncPanZoomController.cpp
@@ +1467,3 @@
>      } else {
> +      if (mState == PINCHING) {
> +        MOZ_ASSERT(GetCurrentTouchBlock());

Instead of repeating this code from OnTouchEnd(), please factor it out into a function which is called from both OnTouchEnd() and here. It can be called something like HandleEndOfPan().

Comment 8

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #7)
> Do you have access to a touch-enabled device on which you can test the
> patch? It could be a touchscreen laptop, or an Android phone or tablet.

I tried the patch out on a touchscreen laptop, and discovered that a few more things need to be done to get it to work:

We start the fling with a velocity given by GetVelocityVector(), but we don't track the velocity during a two-fingered pan. To track the velocity, I believe we need to do the following things:

  - Call Axis::StartTouch() on both axes in OnScaleBegin().

  - Call Axis::UpdateWithTouchAtDevicePoint() on both axes in 
    OnScale(), using the focus point as the touch point.

  - Call Axis::EndTouch() in OnScaleEnd(), before querying the
    velocity with GetVelocityVector().

    In the case where a finger is still down and we are
    transitioning into a one-finger pan (the block where we
    set the state to TOUCHING), we shouldn't call EndTouch(),
    but we don't need to call StartTouch() either, because we
    already did at the beginning of the pinch gesture. This way,
    the velocity will be calculated based on the entire pan,
    including both the two-finger and one-finger potions.

Let me know if this makes sense, or if there is any further clarification I can provide.

Comment 9

[Botond Ballo [:botond]]

Tuhina, have you had a chance to look at comment 7 and comment 8? Please let me know if anything is unclear, or anything I should go into more detail about.

Comment 10

[tuhina chatterjee]

Attachment: patch2.diff

Comment 11

[Botond Ballo [:botond]]

Comment on attachment 8804857 [details] [diff] [review]
patch2.diff

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

Thanks - I tried this out, and it's working much better!

I noticed one remaining issue: if you don't release the two fingers at exactly the same time, we don't get momentum scrolling.

Here's what happens in this scenario:

  - While the two fingers are down, we get touch-move events with 2 touch points,
    which generate PINCHGESTURE_SCALE events.

  - When the first finger is lifted, we get a touch-end which generates a
    PINCHGESTURE_END with a non-negative focus point, so in OnScaleEnd()
    we transition into the TOUCHING state.

  - (We may get a few touch-move events with 1 touch point. If we get enough,
    we may transition into the PANNING state but assume we don't.)

  - When the second finger is lifted, we get a touch-end which goes into
    OnTouchEnd(), while we're still in the TOUCHING state. In this case,
    we don't momentum-scroll (because usually the TOUCHING state is for
    when you've touched the screen but haven't moved the finger enough to
    even start panning yet).

Let me think a bit about what would be the best way to address this case.

Comment 12

[tuhina chatterjee]

Sure. I needed some help debugging C++ code.How to log messages to the android debugger?

Comment 13

[Botond Ballo [:botond]]

(In reply to tuhina chatterjee from comment #12)
> How to log messages to the android debugger?

You can use the printf_stderr function to print debugging information. It works similar to printf, e.g.:

  printf_stderr("Variable = %d\n", var);

The messages will go to the android log, which is accessible via the "adb logcat" with the android device connected via USB.

If you haven't used adb with the device before, you may have to enable USB debugging as described here [1].

[1] https://developer.android.com/studio/run/device.html#device-developer-options

Comment 14

[tuhina chatterjee]

As a suggestion to resolve the above problem:
Can we call HandleEndOfPan() in OnTouchEnd when the state is TOUCHING ?
The block : 
   if (flingVelocity.Length() < gfxPrefs::APZFlingMinVelocityThreshold()) {
    return nsEventStatus_eConsumeNoDefault;
  } 

would dispatch the fling based on the fling velocity length.

Comment 15

[Botond Ballo [:botond]]

(In reply to tuhina chatterjee from comment #14)
> As a suggestion to resolve the above problem:
> Can we call HandleEndOfPan() in OnTouchEnd when the state is TOUCHING ?
> The block : 
>    if (flingVelocity.Length() < gfxPrefs::APZFlingMinVelocityThreshold()) {
>     return nsEventStatus_eConsumeNoDefault;
>   } 
> 
> would dispatch the fling based on the fling velocity length.

We have to be careful that in the case where the user puts one finger down, doesn't move it enough to get into the PANNING state, and then releases it, we don't get a fling, even though we already have a velocity (because OnTouchMove() calls UpdateWithTouchAtDevicePoint() in the TOUCHING state).

Comment 16

[Botond Ballo [:botond]]

I have an idea: if we were doing a two-fingered pan (that is, if we were doing a pinch gesture with mAllowZoom being false) and we release one finger, go directly into the PANNING state.

See, going into the TOUCHING state is useful after a true pinch, so that we don't get unintended panning right away during the small period of time while one finger is still down, but after a two-finger pan there's not much point - might as well continue panning with the one finger right away.

To do this, we could:

  - Modify AsyncPanZoomController::StartPanning() so that it takes a point
    (ParentLayerPoint) instead of a MultiTouchInput.

    The existing call sites can obtain the point from the MultiTouchInput
    the way StartPanning() currently does, at the call site.

  - In OnScaleEnd(), in the "one finger is still down" block, do different
    things based on whether mAllowZoom is true:
   
      - If mAllowZoom is true, so that the gesture was a true pinch,
        call SetState(TOUCHING) as we do now.

      - If mAllowZoom is false, call StartPanning() with the focus point
        as argument.

Would you like to give this a try?

Comment 17

[tuhina chatterjee]

I will give this a try.Will post the patch soon.

Comment 18

[Botond Ballo [:botond]]

Hey Tuhina, how are things going here? Anything I can help with or clarify?

Comment 19

[Botond Ballo [:botond]]

Hey Tuhina, are you still working on this?

Comment 20

[Botond Ballo [:botond]]

Since we haven't heard from Tuhina, I'm going to un-assign the bug. It's now free for someone else to take up and finish.

Note that a lot of the work is already done. The remaining work is to take the work-in-progress patch attached to the bug, and implement the suggestion from comment 16.

Comment 21

[Gregory Moore [:gmoore]]

Hi Botond, yes I am interested in working on this bug, thanks. I don't really have access to a touch-enabled device though, but I could maybe borrow one. Would that be a problem for this bug? It looks like the remaining code changes might be pretty simple. Let me know, thanks.

Comment 22

[Botond Ballo [:botond]]

(In reply to Gregory Moore [:gmoore] from comment #21)
> I don't
> really have access to a touch-enabled device though, but I could maybe
> borrow one. Would that be a problem for this bug?

That's fine. I can test your patch on a device.

If you're interested, since you now have some experience writing APZ GTests, we could probably also exercise the code being added in a GTest.

Comment 23

[Gregory Moore [:gmoore]]

Attachment: Implemented changes described in comment 16.

(In reply to Botond Ballo [:botond] from comment #22)
> That's fine. I can test your patch on a device.

Okay cool, great. Sorry for the late reply by the way. 

> If you're interested, since you now have some experience writing APZ GTests,
> we could probably also exercise the code being added in a GTest.

Sure, of course. :) I implemented the changes you talked about in comment 16 and it compiles, but I don't really know if it's working yet. I will start looking into writing a new GTest(s). Let me know if there are any problems with the new changes as well. Thanks.

Comment 24

[Botond Ballo [:botond]]

Comment on attachment 8851715 [details] [diff] [review]
Implemented changes described in comment 16.

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

Thanks, this generally looks good, and I tried it out on a touchscreen laptop and it's working well!

It does cause one existing GTest (APZCPinchTester.Panning_TwoFinger_ZoomDisabled) to trip some assertions; that will need to be fixed.

Couple of tips:
  1) Run |mach gtest 'APZ*'| to run all APZ GTests
  2) Add |ac_add_options --enable-debug| to your .mozconfig to get a 
     debug build where assertions are checked.

Comment 25

[Gregory Moore [:gmoore]]

Attachment: Stack trace from GTest Failure.

(In reply to Botond Ballo [:botond] from comment #24)
> It does cause one existing GTest
> (APZCPinchTester.Panning_TwoFinger_ZoomDisabled) to trip some assertions;
> that will need to be fixed.

Alright, thanks. I looked into why this was failing, and found it was when we call StartPanning() from OnScaleEnd(). The only caller of StartPanning() previously was OnTouchMove(), where we first do:
> MOZ_ASSERT(GetCurrentTouchBlock());
before calling StartPanning(). StartPanning() calls HandlePanningWithTouchAction(), which does the same above assertion at the very beginning of the function.

It seems that AsyncPanZoomController::GetCurrentTouchBlock(), as well as AsyncPanZoomController::GetCurrentInputBlock()() return NULL at this point in the code. So I'm not really sure how we can fix that. The variable those functions are querying (InputQueue::mQueuedInputs) seems to only be added to (making it non-empty) in the InputQueue::Receive*Input() functions. We don't call those in this case though, and instead just call AsyncPanZoomController::HandleInputEvent(). So I'm not sure how to make it non-empty (or if it should be), or if we should just get rid of that assertion. 

I'm not really sure why APZCPinchTester.Panning_TwoFinger_ZoomDisabled is even getting to this case anyway though. Shouldn't the two fingers leave at exactly the same time, since this is an automated test?

Also, I realized in patch2.diff and my patch, we removed the lines:
> mX.StartTouch(aEvent.mLocalFocusPoint.x, aEvent.mTime);
> mY.StartTouch(aEvent.mLocalFocusPoint.y, aEvent.mTime);
even when zooming is allowed. Should we do that? It seems like we should maybe be leaving them in.

Also, I'm not sure if we should start working on this yet, but what cases should the new GTest's cover? The only two that come to mind right now are:
1) Momentum scrolling after two-fingered pan on pages that don't allow zooming.
2) Same as (1) but don't release the two fingers at the same time.
Please let me know, thanks.

Comment 26

[Botond Ballo [:botond]]

(In reply to Gregory Moore [:gmoore] from comment #25)
> (In reply to Botond Ballo [:botond] from comment #24)
> It seems that AsyncPanZoomController::GetCurrentTouchBlock(), as well as
> AsyncPanZoomController::GetCurrentInputBlock()() return NULL at this point
> in the code. So I'm not really sure how we can fix that. The variable those
> functions are querying (InputQueue::mQueuedInputs) seems to only be added to
> (making it non-empty) in the InputQueue::Receive*Input() functions. We don't
> call those in this case though, and instead just call
> AsyncPanZoomController::HandleInputEvent(). So I'm not sure how to make it
> non-empty (or if it should be), or if we should just get rid of that
> assertion.

To give a bit of background here: what happens in production code is the OS sends us touch events, we run them through a gesture detector to see if the touch events represent a pinch gesture, and if so we synthesize "pinch gesture events" which we then handle (e.g. in OnScaleBegin()/OnScaleEnd()).

The test suite, however, can also just create and send pinch gesture events directly, and that's what it's doing in this test.

GetCurrentTouchBlock() will be non-null whenever we're currently processing a series ("block") of touch events. When processing a pinch gesture event, this will be the case if it's coming from the gesture detector (because then it was created while processing the touch events), but not if it's sent directly from the test.

So, for a given codepath in OnScaleEnd(), we need to either:

  (1) make sure the codepath isn't hit for pinch gesture events
      generated directly by a test; or

  (2) make sure OnScaleEnd() gracefully handles the case where
      the pinch gesture event is generated directly by a test

Which brings us to your next point...

> I'm not really sure why APZCPinchTester.Panning_TwoFinger_ZoomDisabled is
> even getting to this case anyway though. Shouldn't the two fingers leave at
> exactly the same time, since this is an automated test?

I agree, we should treat the "one finger remaining" codepath as case #1 above: just make sure the test doesn't get into it.

In fact, the test is already intending to avoid that. See the comment here [1], above the line where the test passes |-1.0, -1.0|, which is intended to make sure we don't get into here [2].

Unfortunately, the test code has a bug :) At [2], it's the coordinates of the focus point that we're testing against (-1, -1), but at [1], the -1 values are initializing the |aCurrentSpan| and |aPreviousSpan| parameters of CreatePinchGestureInput(), rather than the coordinates of the focus point. Fixing that should fix the assertion.

> Also, I realized in patch2.diff and my patch, we removed the lines:
> > mX.StartTouch(aEvent.mLocalFocusPoint.x, aEvent.mTime);
> > mY.StartTouch(aEvent.mLocalFocusPoint.y, aEvent.mTime);
> even when zooming is allowed. Should we do that? It seems like we should
> maybe be leaving them in.

We moved the calls to StartTouch() to OnScaleBegin(), which calls them unconditionally. Therefore, OnScaleEnd() does not need to call them in any situation.

Alternatively, we could have OnScaleBegin() only call StartTouch() if zooming isn't allowed (and then correspondingly, OnScale() would have to only call UpdateWithTouchAtDevicePoint() if zooming isn't allowed, and OnScaleEnd() would need to call StartTouch() if zooming _is_ allowed), but I think that's complicating things unnecessarily. All these functions do is track the velocity at which we are panning. Granted, there's no need to do that during a true (zooming) pinch, but it doesn't harm either.

> Also, I'm not sure if we should start working on this yet, but what cases
> should the new GTest's cover? The only two that come to mind right now are:
> 1) Momentum scrolling after two-fingered pan on pages that don't allow
> zooming.
> 2) Same as (1) but don't release the two fingers at the same time.
> Please let me know, thanks.

Yep, those two test cases sound good. We could add one more, that checks that we _don't_ do momentum scrolling after a pinch if zooming is allowed (we may just be able to add that as an assertion to an existing test).

Note that, given my proposed solution for the assertion failure above, the test will not be able to exercise the "fingers released at different times" case if it's sending pinch gesture events directly. It will need to exercise that case by sending touch events instead (in practice, this means using PinchWithTouchInput() instead of PinchWithPinchInput() in the test code).

In fact, it's probably better to use PinchWithTouchInput() in all our new tests, since it reflects what happens in the production code more accurately.

[1] http://searchfox.org/mozilla-central/rev/72fe012899a1b27d34838ab463ad1ae5b116d76b/gfx/layers/apz/test/gtest/InputUtils.h#139
[2] http://searchfox.org/mozilla-central/rev/72fe012899a1b27d34838ab463ad1ae5b116d76b/gfx/layers/apz/src/AsyncPanZoomController.cpp#1483

Comment 27

[Gregory Moore [:gmoore]]

Cool, thanks for the detailed response. I'm actually going to start work tomorrow so I'm not sure if I will be able to finish this bug. I might be able to find some time to finish it eventually, but I don't really want to make any promises. Sorry about that, and thanks for the help with this bug. I do have some questions though, in case I get time to finish it, or that might be helpful for whoever works on it next.

(In reply to Botond Ballo [:botond] from comment #26)
> Unfortunately, the test code has a bug :) At [2], it's the coordinates of
> the focus point that we're testing against (-1, -1), but at [1], the -1
> values are initializing the |aCurrentSpan| and |aPreviousSpan| parameters of
> CreatePinchGestureInput(), rather than the coordinates of the focus point.
> Fixing that should fix the assertion.

Oh okay, I thought that looked a little odd when I was looking through the code. I fixed it and tried running the test again, and we now do go down the correct path. Unfortunately we still run into the same assertion at the beginning of HandleEndOfPan() though. Not sure exactly how we should handle that situation. Perhaps we should treat this as the case #2 you mentioned, and perform different actions depending on if we have a touch block, similar to this?:
https://dxr.mozilla.org/mozilla-central/rev/38894655c89e68bcd8f45d31a0d3005f2c2b53db/gfx/layers/apz/src/AsyncPanZoomController.cpp#1500-1504

> Note that, given my proposed solution for the assertion failure above, the
> test will not be able to exercise the "fingers released at different times"
> case if it's sending pinch gesture events directly. It will need to exercise
> that case by sending touch events instead (in practice, this means using
> PinchWithTouchInput() instead of PinchWithPinchInput() in the test code).
> 
> In fact, it's probably better to use PinchWithTouchInput() in all our new
> tests, since it reflects what happens in the production code more accurately.

Is it possible to use PinchWithTouchInput() directly for the behavior we want? It seems like it just does a horizontal pinch, and both fingers move at the same rate so the focus point will remain constant. Would we need to modify the function or create a similar function? Also, PinchWithTouchInput() seems to release the two fingers at the same time, so it seems like we would need to have the ability to change that as well. Thanks.

Comment 28

[Botond Ballo [:botond]]

(In reply to Gregory Moore [:gmoore] from comment #27)
> I'm actually going to start work
> tomorrow so I'm not sure if I will be able to finish this bug. I might be
> able to find some time to finish it eventually, but I don't really want to
> make any promises. Sorry about that, and thanks for the help with this bug.

If it helps, we could commit just the code changes in this bug, and leave the new tests for a follow-up bug (much like the first bug you fixed, bug 1204502, concerned tests for a previous change).

> Unfortunately we still run into the same assertion at the
> beginning of HandleEndOfPan() though. Not sure exactly how we should handle
> that situation. Perhaps we should treat this as the case #2 you mentioned,
> and perform different actions depending on if we have a touch block, similar
> to this?:

Yeah. The reason OnScaleEnd() calls HandleEndOfPan() is to kick off the momentum scrolling. We could say that, in the case of pinch gesture events sent directory from the test, we don't need momentum scrolling, and guard the block containing the OnScaleEnd() call with HasReadyTouchBlock() as an extra condition.

> Is it possible to use PinchWithTouchInput() directly for the behavior we
> want? It seems like it just does a horizontal pinch, and both fingers move
> at the same rate so the focus point will remain constant. Would we need to
> modify the function or create a similar function? Also,
> PinchWithTouchInput() seems to release the two fingers at the same time, so
> it seems like we would need to have the ability to change that as well.

Good point, we'd need to either make some changes to PinchWithTouchInput(), or just send the touch events (similar how to how PinchWithTouchInput() does it, but with our desired modifications) in our test body directly.

Comment 29

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #28)
> and guard
> the block containing the OnScaleEnd() call with HasReadyTouchBlock() as an
> extra condition.

I meant to say "guard the block containing the HandleEndOfPan() call".

Comment 30

[Gregory Moore [:gmoore]]

Attachment: Made changes so APZCPinchTester.Panning_TwoFinger_ZoomDisabled would pass.

(In reply to Botond Ballo [:botond] from comment #28)
> If it helps, we could commit just the code changes in this bug, and leave
> the new tests for a follow-up bug (much like the first bug you fixed, bug
> 1204502, concerned tests for a previous change).

Okay, sure, yeah that sounds like a good idea. Doing the tests might uncover some bugs as well. 

> Yeah. The reason OnScaleEnd() calls HandleEndOfPan() is to kick off the
> momentum scrolling. We could say that, in the case of pinch gesture events
> sent directory from the test, we don't need momentum scrolling, and guard
> the block containing the OnScaleEnd() call with HasReadyTouchBlock() as an
> extra condition.

Alright, I updated the patch with that change. I also fixed the test function PinchWithPinchInput() so it would pass in the right values at the end of the gesture. Thanks.

Comment 31

[Botond Ballo [:botond]]

Comment on attachment 8856312 [details] [diff] [review]
Made changes so APZCPinchTester.Panning_TwoFinger_ZoomDisabled would pass.

Looks good, thanks!

Comment 32

[Botond Ballo [:botond]]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=80e246aa5eddf31bfb91c2afb9983c95c490f379

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/456dbb5c2b93
Support momentum scrolling after two-fingered pans on pages that don't allow zooming. r=botond

Comment 34

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/456dbb5c2b93

Comment 35

[Botond Ballo [:botond]]

Thanks for getting this enhancement across the finish line, Gregory!

I filed bug 1355656 for the tests.

Comment 36

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

I think this patch is the cause of some erratic jumping I'm seeing after pinch actions. Fennec on a Z3C, latest nightly. I'll file something tomorrow once I have a chance to nail down STR.

Comment 37

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

I ended up backing this out for now, until we can figure out why the patch introduced behaviour regressions on Fennec. Backed out in bug 1355944: https://hg.mozilla.org/integration/mozilla-inbound/rev/5e1026baaf23

Comment 38

[Botond Ballo [:botond]]

I have a theory as to what caused the regression.

When zooming is allowed, if at the end of a pinch one finger is lifted before the other, we enter the TOUCHING state, as before. However, since we've added calls to StartTouch() in OnScaleBegin(), any motion of the focus point over the course of the pinch gesture has already contributed to overcoming the "pan threshold" that gets us from the TOUCHING state into the PANNING state. As a result, we get into the PANNING state earlier (possibly at the very next touch event after the first finger is lifted), and when the second finger is lifted we get a fling.

Doing the alternative I described in comment 26:

(In reply to Botond Ballo [:botond] from comment #26)
> Alternatively, we could have OnScaleBegin() only call StartTouch() if
> zooming isn't allowed (and then correspondingly, OnScale() would have to
> only call UpdateWithTouchAtDevicePoint() if zooming isn't allowed, and
> OnScaleEnd() would need to call StartTouch() if zooming _is_ allowed)

should fix this.

Comment 39

[Botond Ballo [:botond]]

I can reproduce bug 1355944 on Android. I should have tested this patch on Android in addition to a touchscreen laptop before landing, sorry.

Comment 40

[Botond Ballo [:botond]]

I can also confirm that the approach described in comment 38 fixes the issue.

Comment 41

[Botond Ballo [:botond]]

Attachment: Updated patch that fixes regression

This is Gregory's patch plus the changes described in comment 38.

Tested on both a touchscreen laptop and an Android device this time.

Comment 42

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #41)
> This is Gregory's patch plus the changes described in comment 38.

Just my changes can be seen in this interdiff:

https://bugzilla.mozilla.org/attachment.cgi?oldid=8856312&action=interdiff&newid=8858903&headers=1

Comment 43

[Botond Ballo [:botond]]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2a141c5f348ce0908a546a84b5f86f2e68810adc

Comment 44

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e5054dd06d75
Support momentum scrolling after two-fingered pans on pages that don't allow zooming. r=botond,kats

Comment 45

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e5054dd06d75

Comment 46

[Gregory Moore [:gmoore]]

Thanks for figuring out the problem and fixing it Botond. Glad this got finished.

Comment 47

[Petruta Horea [:phorea], Desktop QA]

Verified as fixed using latest Nightly 55.0a1 on a Microsoft Surface Pro 2 tablet with Win 10 64-bit insider Preview build, a Nexus tablet and a Samsung Galaxy J5 phone, both running with Android 6.0.1.

Thanks Botond for your help!