1443231 - OnScaleEnd() leaves APZC in pinching state if we don't trigger a momentum scroll

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect, P3)

Description

[Botond Ballo [:botond]]

In bug 1180799, we made some modifications to AsyncPanZoomController::OnScaleEnd() to support momentum scrolling after a two-finger pan.

As part of that change, we removed the SetState(NOTHING) call in that function, and didn't replace it with any other SetState() calls, except in the case where the trigger the momentum scrolling. As a result, if we don't trigger momentum scrolling, the APZC remains in the PINCHING state.

This is undesirable; after completion of a gesture and associated movement / animation, the APZC should be in the NOTHING state.

Comment 1

[Botond Ballo [:botond]]

Attachment: Bug 1443231 - Ensure we are in the NOTHING state after a pinch gesture, if both fingers are lifted and no animation is triggered.

Review commit: https://reviewboard.mozilla.org/r/225078/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/225078/

Comment 2

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

Comment on attachment 8956160 [details]
Bug 1443231 - Ensure we are in the NOTHING state after a pinch gesture, if both fingers are lifted and no animation is triggered.

https://reviewboard.mozilla.org/r/225078/#review231036

::: gfx/layers/apz/src/AsyncPanZoomController.cpp:1598
(Diff revision 1)
>          if (HasReadyTouchBlock()) {
>            return HandleEndOfPan();
>          }
>        }
>      }
> +    SetState(NOTHING);

We probably don't want to do this if we enter the ScrollSnap() call on line 1586 and trigger a smooth scroll animation to the snap point.

Comment 3

[Botond Ballo [:botond]]

Comment on attachment 8956160 [details]
Bug 1443231 - Ensure we are in the NOTHING state after a pinch gesture, if both fingers are lifted and no animation is triggered.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/225078/diff/1-2/

Comment 4

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #2)
> We probably don't want to do this if we enter the ScrollSnap() call on line
> 1586 and trigger a smooth scroll animation to the snap point.

Good point! Fixed and test added.

Comment 5

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

Comment on attachment 8956160 [details]
Bug 1443231 - Ensure we are in the NOTHING state after a pinch gesture, if both fingers are lifted and no animation is triggered.

https://reviewboard.mozilla.org/r/225078/#review231050

Nice, thanks!

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f0306a56960c
Ensure we are in the NOTHING state after a pinch gesture, if both fingers are lifted and no animation is triggered. r=kats

Comment 7

[Botond Ballo [:botond]]

Attachment: Bug 1443231 - Follow-up to fix warning-as-error on Windows (CLOSED TREE).

Review commit: https://reviewboard.mozilla.org/r/225136/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/225136/

Comment 8

[Botond Ballo [:botond]]

Comment on attachment 8956224 [details]
Bug 1443231 - Follow-up to fix warning-as-error on Windows (CLOSED TREE).

https://reviewboard.mozilla.org/r/225136/#review231104

Comment 9

[Botond Ballo [:botond]]

Comment on attachment 8956224 [details]
Bug 1443231 - Follow-up to fix warning-as-error on Windows (CLOSED TREE).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/225136/diff/1-2/

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6d6dc27d428c
Follow-up to fix warning-as-error on Windows (CLOSED TREE). r=botond

Comment 11

[Botond Ballo [:botond]]

Attachment: Bug 1443231 - Another follow-up to fix a debug assertion in the gtest.

Review commit: https://reviewboard.mozilla.org/r/225142/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/225142/

Comment 12

[Botond Ballo [:botond]]

Comment on attachment 8956235 [details]
Bug 1443231 - Another follow-up to fix a debug assertion in the gtest.

https://reviewboard.mozilla.org/r/225142/#review231108

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cfe4f208baac
Another follow-up to fix a debug assertion in the gtest. r=botond

Comment 14

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/f0306a56960c
https://hg.mozilla.org/mozilla-central/rev/6d6dc27d428c
https://hg.mozilla.org/mozilla-central/rev/cfe4f208baac