1031443 - Allow panning in the pinching state even if zooming is disabled

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

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

As mentioned at https://bugzilla.mozilla.org/show_bug.cgi?id=962243#c13, I think we should allow panning with multiple fingers even if zooming is disabled. Right now we call OnScaleBegin in the APZC/GestureEventListener, but then because the zoom constraints (or touch action) say zooming is not allowed, we never actually enter the pinching state. I think it would better if we did enter the pinching state, but then make sure the zoom is never changed. This would allow two-finger or multi-finger panning because we'd still scroll based on the movement of the focal point.

Comment 1

[Lynn Tran]

Hello, I'm interested in working on this bug. Please guide me to the right direction if I'm going in the wrong way, but here is what I think of doing:

- In HandleInputTouchMultiStart function and the case GESTURE_MULTI_TOUCH_DOWN state, we will set its state to GESTURE_PINCH to it will go to the pinching state. 
- We then can create a PinchGesture event to trigger the event. 
- However, I'm not sure if the pinch event will change the zooming or not.

Comment 2

[Botond Ballo [:botond]]

Thanks for your interest, Lynn! I've assigned the bug to you.

(In reply to Lynn Tran from comment #1)
> - In HandleInputTouchMultiStart function and the case
> GESTURE_MULTI_TOUCH_DOWN state, we will set its state to GESTURE_PINCH to it
> will go to the pinching state. 
> - We then can create a PinchGesture event to trigger the event. 

Right, the gesture event listener generates a pinch gesture event regardless of whether zooming is allowed. That part can stay as is.

> - However, I'm not sure if the pinch event will change the zooming or not.

The zooming happens in the ScaleWithFocus() call in AsyncPanZoomController::OnScale() [1].

Right now, we don't get there if zooming is not allowed, because AsyncPanZoomController::OnScaleBegin() exits early and doesn't enter the PINCHING state [2], and so OnScale() will also exit early [3].

The panning which we'd like to happen even if zooming is not allowed, is done in the ScrollBy() call in OnScale() [4].

So, I think what we want to do is:

  - Do not exit early in OnScaleBegin() if zooming is not allowed, so that
    we still enter the PINCHING state.

  - In OnScale(), if zooming is not allowed, perform the ScrollBy() call [4]
    but not the ScaleWithFocus() call [1]. (An easy way to do this might be
    to set doScale [5] to false if zooming is not allowed).

Let me know if that's enough info for you to go on and get started! Don't hesitate to ask any questions, either in this bug or on IRC (my IRC nick is 'botond' and you can find me in the #apz or #gfx channels; if I'm not around, try 'kats').

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/AsyncPanZoomController.cpp?rev=c65b97cfc0c7#1342
[2] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/AsyncPanZoomController.cpp?rev=c65b97cfc0c7#1261
[3] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/AsyncPanZoomController.cpp?rev=c65b97cfc0c7#1280
[4] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/AsyncPanZoomController.cpp?rev=c65b97cfc0c7#1312
[5] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/AsyncPanZoomController.cpp?rev=c65b97cfc0c7#1329

Comment 3

[Lynn Tran]

Attachment: bug1031443.patch

Do I need to write test for this?

Comment 4

[Botond Ballo [:botond]]

Comment on attachment 8609943 [details] [diff] [review]
bug1031443.patch

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

::: gfx/layers/apz/src/AsyncPanZoomController.cpp
@@ -1259,5 @@
>    }
>  
> -  if (!mZoomConstraints.mAllowZoom) {
> -    return nsEventStatus_eConsumeNoDefault;
> -  }

This part is good.

@@ +1273,5 @@
>      return nsEventStatus_eIgnore;
>    }
>  
>    if (mState != PINCHING) {
> +    bool doScale = false;

There are two problems here:

  - This is declaring a new variable 'doScale' and setting it to
    false. No one reads the value of this variable, so this has
    no effect.

    What we want to do instead, is modify the value of the
    existing doScale variable [1] to be false.

  - The condition under which we need to disable zooming is
    not 'mState != PINCHING'. Since we modify OnScaleBegin()
    to let us enter the PINCHING state even if zooming is
    not allowed, 'mState != PINCHING' here should never happen.
    Rather, it's the thing OnScaleBegin() was checking for,
    '!mZoomConstraints.mAllowZoom', that we need to check here.

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/AsyncPanZoomController.cpp?rev=c65b97cfc0c7#1329

Comment 5

[Botond Ballo [:botond]]

> Do I need to write test for this?

That would definitely help! Especially if you don't have a device you're able to test your patch on, writing a unit test that exercises it is a good way to test that it works.

We have gtests that exercise the pinching code here [1]. It would make sense to add one for this bug. You can use APZCPinchTester and call UpdateZoomConstraints() on the 'apzc' member with a zoom constraints object that has mAllowZoom set to false, to test zooming being disabled.

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/tests/gtest/TestAsyncPanZoomController.cpp?rev=bee4bdb3624c#840

Comment 6

[Lynn Tran]

Attachment: bug1031443-1.patch

Comment 7

[Botond Ballo [:botond]]

Comment on attachment 8613186 [details] [diff] [review]
bug1031443-1.patch

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

Thanks, the changes to the code itself look good now (at least by inspection; I'll test it in a device build to confirm).

The test needs some more work. It currently doesn't compile.

Have you tried building the Firefox code, as described here [1]? If not, please give that a try (let me know if you run into any issues).

When you try building with your patch applied, you will see some compiler errors in the new test you added. Once you fix those, here's what needs to be done to complete the test:

  - To set up the APZC for the test, you need to give it some appropriate frame metrics.
    You can do the same thing DoPinchTest() does here [2].

  - After doing the pinch, you need to add some assertions that test that scrolling
    actually occurred. (Without assertions, the test just always passes so there's
    not much point to it.)

[1] https://developer.mozilla.org/en-US/docs/Simple_Firefox_build
[2] http://mxr.mozilla.org/mozilla-central/source/gfx/tests/gtest/TestAsyncPanZoomController.cpp?rev=098390cdba51#771

::: gfx/tests/gtest/TestAsyncPanZoomController.cpp
@@ +655,5 @@
>                                    int aFocusX, int aFocusY, float aScale,
>                                    bool aShouldTriggerPinch)
>  {
>    nsEventStatus statuses[3];  // scalebegin, scale, scaleend
> +  PinchWithPinchInput(aTarget, aFocusX, aFocusY, 0, 0, aScale, &statuses);

To preserve behaviour, this call site should continue passing the same value for both focus points, instead of passing (0, 0) for the second one.

Comment 8

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #7)
> Thanks, the changes to the code itself look good now (at least by
> inspection; I'll test it in a device build to confirm).

I tested the code changes in a device build, and they seem to be working well!

There are a couple of improvements we can make:

  - support two-fingered panning in subframes (currently it's only
    allowed in the page's root scrollable frame)

  - support momentum scrolling after a two-fingered pan

but those are out of scope for this bug; I'll file follow-up bugs for them.

Comment 9

[Botond Ballo [:botond]]

Hi Lynn,

Just wanted to check how things are going as I haven't heard from you for a while. Last we talked on IRC, you almost had the test working, and were debugging an issue where you were getting a different scroll offset than what you expected. Were you able to solve that? I'm happy to provide more guidance if you're stuck :)

Comment 10

[Lynn Tran]

Attachment: bug1031443-2.patch

Comment 11

[Lynn Tran]

Attachment: bug1031443-2.patch

Comment 12

[Lynn Tran]

Attachment: bug1031443-3.patch

I added the test for zooming as suggested

Comment 13

[Botond Ballo [:botond]]

Comment on attachment 8628030 [details] [diff] [review]
bug1031443-3.patch

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

Thanks, the test looks good! I just have a few small comments about style:

::: gfx/tests/gtest/TestAsyncPanZoomController.cpp
@@ +1033,5 @@
>  
>    childApzc->Destroy();
>  }
>  
> +TEST_F(APZCPinchTester, Panning_MultiFinger_ZoomDisabled) {

Please call it 'Panning_TwoFinger_ZoomDisabled'.

@@ +1034,5 @@
>    childApzc->Destroy();
>  }
>  
> +TEST_F(APZCPinchTester, Panning_MultiFinger_ZoomDisabled) {
> +  nsEventStatus statuses[3];  // scalebegin, scale, scaleend

Please move this variable declaration to just before the PinchWithPinchInput() call which uses it.

@@ +1037,5 @@
> +TEST_F(APZCPinchTester, Panning_MultiFinger_ZoomDisabled) {
> +  nsEventStatus statuses[3];  // scalebegin, scale, scaleend
> +  //set up APZC
> +  apzc->SetFrameMetrics(GetPinchableFrameMetrics());
> +  apzc->UpdateZoomConstraints(ZoomConstraints(false, false, CSSToParentLayerScale(1.0f), CSSToParentLayerScale(1.0f)));

APZCBasicTester (which is a base class of APZCPinchTester) has a helper function MakeApzcUnzoomable() which does exactly this apzc->UpdateZoomConstraints() call - please use that instead.

@@ +1040,5 @@
> +  apzc->SetFrameMetrics(GetPinchableFrameMetrics());
> +  apzc->UpdateZoomConstraints(ZoomConstraints(false, false, CSSToParentLayerScale(1.0f), CSSToParentLayerScale(1.0f)));
> +
> +  EXPECT_CALL(*mcc, SendAsyncScrollDOMEvent(_,_,_)).Times(AtLeast(1));
> +  EXPECT_CALL(*mcc, RequestContentRepaint(_)).Times(1);

These two EXPECT_CALL statements are unrelated to what's being tested. Please remove them.

@@ +1045,5 @@
> +
> +  PinchWithPinchInput(apzc, 250, 350, 200, 300, 10, &statuses);
> +
> +  FrameMetrics fm = apzc->GetFrameMetrics();
> +  EXPECT_EQ(325, fm.GetScrollOffset().x);

Please add a comment saying that moving the focus point from (250, 350) to (200, 300) pans by (50, 50) screen pixels, which at 2x zoom causes the scroll offset to change by (25, 25) pixels.

Comment 14

[Lynn Tran]

Attachment: bug1031443-4.patch

I made changes to the styling and added more comments.

Comment 15

[Botond Ballo [:botond]]

Comment on attachment 8628389 [details] [diff] [review]
bug1031443-4.patch

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

Thanks, Lynn, it looks good!

I pushed the patch to the Try server to make sure it builds on all platforms and passes tests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=01fdeec05282

Comment 16

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #15)
> I pushed the patch to the Try server to make sure it builds on all platforms
> and passes tests:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=01fdeec05282

All green! I will go ahead and commit the patch.

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/aaebaa5cf6d2

Comment 18

[Phil Ringnalda (:philor)]

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

Comment 19

[Botond Ballo [:botond]]

Thanks again, Lynn. Good work!

You mentioned on IRC that you'd be interested in continuing to contribute. Here are a couple of ideas:

  - Bug 962243 concerns a somewhat related scenario, where a pinch transitions
    into a pan by lifting one of the fingers. It has a patch posted, but it's
    quite old (a little over a year), and needs to be updated to apply to the
    current code.

  - Bug 1180799 is about momentum scrolling after a two-fingered pan (the second
    improvement mentioned in comment 8).

As for the first improvement mentioned in comment 8, two-fingered panning of subframes, I discussed it with Kats and we believe that should wait until we have subframe zooming (bug 990972).

I'd be happy to continue mentoring you for either of the above bugs. If you're interested, please feel free to take either one.

Comment 20

[Lynn Tran]

Since bug 962243 are halfway done, I'd be happy to finish it up and then continue with the other one if no one claims it by then.

Comment 21

[Botond Ballo [:botond]]

(In reply to Lynn Tran from comment #20)
> Since bug 962243 are halfway done, I'd be happy to finish it up and then
> continue with the other one if no one claims it by then.

Sounds good! I assigned for bug 962243 to you.