1190469 - Interrupting a pan will break scroll snapping with apz

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Chris Lord [:cwiiis]]

Assume a page with mandatory scroll snapping setup.

- If you put a second finger on the screen during a pan gesture, when you release your fingers scroll snapping will not activate.

- If you lock your phone while scrolling, then remove your finger, upon unlocking, the view will not have snapped.

There are some situations where interrupting the scroll not only doesn't snap, but then disables scrolling until you do something more drastic to recover it, but I've not managed to make a reduced test-case for that.

On the given URL, it should be impossible to end up in a stable scroll position with red showing, but following either of the two steps listed above allows this.

Comment 1

[Kartikaya Gupta (email:[e-mail]) (away until Feb-2020)]

CC'ing Markus because we were recently discussing how there might be more places we need to do a RequestFlingSnap call.

Comment 3

[Chris Lord [:cwiiis]]

[Blocking Requested - why for this release]: This causes some pretty bad looking/feeling behaviour in top-level components in Gaia. Namely, the home screen can get 'stuck' between the apps and pinned pages panel, and the task manager can get stuck between windows.

It would be nice to have this fixed so that authors can rely on this feature without timeout work-arounds.

Comment 7

[Chris Lord [:cwiiis]]

The amount of duplicates we're getting of this is starting to get a little silly :) Markus, do you have time to look at this, or can you delegate to someone that does?

Also cc'ing Botond as it's an apz issue.

Comment 8

[Kartikaya Gupta (email:[e-mail]) (away until Feb-2020)]

I can take a look at this. I'm looking at other APZ+snapping issues anyway.

Comment 9

[Kartikaya Gupta (email:[e-mail]) (away until Feb-2020)]

Attachment: MozReview Request: Bug 1190469 - Refactor some code to have a general-purpose snap function available in APZC. r=

Bug 1190469 - Refactor some code to have a general-purpose snap function available in APZC. r=

Comment 10

[Kartikaya Gupta (email:[e-mail]) (away until Feb-2020)]

Attachment: MozReview Request: Bug 1190469 - Remove unused function. r=

Bug 1190469 - Request scroll snapping under roughly the same conditions that we clear overscroll. r=

Comment 11

[Kartikaya Gupta (email:[e-mail]) (away until Feb-2020)]

Try push with above patches: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9d69a35e360e

Chris, if you get a chance, please apply these patches and let me know if you still see issues with them. We don't have a lot of automated test coverage of this code so extra eyes on it would be great.

Comment 13

[Botond Ballo [:botond]]

Comment on attachment 8679431 [details]
MozReview Request: Bug 1190469 - Refactor some code to have a general-purpose snap function available in APZC. r=

https://reviewboard.mozilla.org/r/23413/#review21039

Comment 14

[Botond Ballo [:botond]]

Comment on attachment 8679432 [details]
MozReview Request: Bug 1190469 - Remove unused function. r=

https://reviewboard.mozilla.org/r/23415/#review21045

::: gfx/layers/apz/src/AsyncPanZoomController.cpp:2508
(Diff revision 1)
> +    RequestSnap();

I looked through the call sites of CancelAnimation() where we do not pass DoNotSnap:

  - CancelAnimationAndGestureState(), called
    on touch-cancel and similar scenarios.
    I agree we want to snap there.
    
  - Destroy()
    It doesn't seem like we want to snap there.
    
  - OnScrollWheel()
    Here the CancelAnimation() call happens before
    the scroll. It doesn't seem like we want to
    snap there. (We probably want to snap *after*
    the scroll, but I guess that's handled by a
    separate mechanism in bug 1141884).
    
  - OnPan*()
    Not sure about these. I don't have a good
    mental model of pan gesture events.
    
On a different note, as these snap requests are sent for every APZC, we're generating some extra IPC traffic. Might it make sense to have a flag in FrameMetrics that indicates whether the scroll frame has snap points, and only send the requests if it does?

Comment 15

[Kartikaya Gupta (email:[e-mail]) (away until Feb-2020)]

(In reply to Botond Ballo [:botond] from comment #14)
> Comment on attachment 8679432 [details]
> MozReview Request: Bug 1190469 - Request scroll snapping under roughly the
> same conditions that we clear overscroll. r=
> 
> https://reviewboard.mozilla.org/r/23415/#review21045
> 
> ::: gfx/layers/apz/src/AsyncPanZoomController.cpp:2508
> (Diff revision 1)
> > +    RequestSnap();
> 
> I looked through the call sites of CancelAnimation() where we do not pass
> DoNotSnap:
> 
>   - CancelAnimationAndGestureState(), called
>     on touch-cancel and similar scenarios.
>     I agree we want to snap there.
>     
>   - Destroy()
>     It doesn't seem like we want to snap there.
>     
>   - OnScrollWheel()
>     Here the CancelAnimation() call happens before
>     the scroll. It doesn't seem like we want to
>     snap there. (We probably want to snap *after*
>     the scroll, but I guess that's handled by a
>     separate mechanism in bug 1141884).
>     
>   - OnPan*()
>     Not sure about these. I don't have a good
>     mental model of pan gesture events.

Yeah it probably makes most sense to not snap for the CancelAnimations calls in both OnScrollWheel and OnPan* because in all of those cases a user action is about to start scrolling, and we don't want to trigger a snap to the pre-scroll position.

Doing that means that there's only a couple of calls which don't pass DoNotSnap, so I'm thinking of inverting the condition and calling it RequestSnap.

> On a different note, as these snap requests are sent for every APZC, we're
> generating some extra IPC traffic. Might it make sense to have a flag in
> FrameMetrics that indicates whether the scroll frame has snap points, and
> only send the requests if it does?

Yeah that's a good idea, I can file a follow-up for that. We should also clean up the flags in FrameMetrics as there are a billion of them floating around and probably could be packed better.

Comment 16

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #15)
> Doing that means that there's only a couple of calls which don't pass
> DoNotSnap, so I'm thinking of inverting the condition and calling it
> RequestSnap.

I was just going to suggest the same thing :)

> > On a different note, as these snap requests are sent for every APZC, we're
> > generating some extra IPC traffic. Might it make sense to have a flag in
> > FrameMetrics that indicates whether the scroll frame has snap points, and
> > only send the requests if it does?
> 
> Yeah that's a good idea, I can file a follow-up for that.

We could alternatively just punt on this until we fix bug 1219296; I'll leave that up to you.

> We should also
> clean up the flags in FrameMetrics as there are a billion of them floating
> around and probably could be packed better.

Good idea!

Comment 17

[Kartikaya Gupta (email:[e-mail]) (away until Feb-2020)]

(In reply to Botond Ballo [:botond] from comment #16)
> (In reply to Kartikaya Gupta (email:[e-mail]) from comment #15)
> > Doing that means that there's only a couple of calls which don't pass
> > DoNotSnap, so I'm thinking of inverting the condition and calling it
> > RequestSnap.
> 
> I was just going to suggest the same thing :)

I did this, but ended up having to use CancelAnimationFlags::RequestSnap at the call sites because of the naming conflict with the function RequestSnap().

Also, I discovered that in the Destroy() function we do actually want to do a snap; that's what fixes the "lock the phone with finger down" scenario described in comment 0.

> > > On a different note, as these snap requests are sent for every APZC, we're
> > > generating some extra IPC traffic. Might it make sense to have a flag in
> > > FrameMetrics that indicates whether the scroll frame has snap points, and
> > > only send the requests if it does?
> > 
> > Yeah that's a good idea, I can file a follow-up for that.
> 
> We could alternatively just punt on this until we fix bug 1219296; I'll
> leave that up to you.

With the updated patch it's more obvious where the extra snap calls are, and I think the extra overhead won't be too high. So yeah I'll punt on this for now.

> > We should also
> > clean up the flags in FrameMetrics as there are a billion of them floating
> > around and probably could be packed better.
> 
> Good idea!

I'll file a bug for this anyway.

Comment 18

[Kartikaya Gupta (email:[e-mail]) (away until Feb-2020)]

(Also, I inserted a new patch to remove an unused function, MozReview probably won't match up the patches properly. Sorry...)

Comment 19

[Kartikaya Gupta (email:[e-mail]) (away until Feb-2020)]

Comment on attachment 8679431 [details]
MozReview Request: Bug 1190469 - Refactor some code to have a general-purpose snap function available in APZC. r=

Bug 1190469 - Refactor some code to have a general-purpose snap function available in APZC. r=

Comment 20

[Kartikaya Gupta (email:[e-mail]) (away until Feb-2020)]

Comment on attachment 8679432 [details]
MozReview Request: Bug 1190469 - Remove unused function. r=

Bug 1190469 - Remove unused function. r=

Comment 21

[Kartikaya Gupta (email:[e-mail]) (away until Feb-2020)]

Attachment: MozReview Request: Bug 1190469 - Request scroll snapping in a few places that animations are cancelled so that we don't leave things unsnapped. r=

Bug 1190469 - Request scroll snapping in a few places that animations are cancelled so that we don't leave things unsnapped. r=

Comment 22

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #17)
> Also, I discovered that in the Destroy() function we do actually want to do
> a snap; that's what fixes the "lock the phone with finger down" scenario
> described in comment 0.

Ah, good catch! I was thinking we wouldn't need to because the APZC is being destroyed anyways, but RequestFlingSnap also sets content-side state and we want that.

Comment 23

[Botond Ballo [:botond]]

Comment on attachment 8679432 [details]
MozReview Request: Bug 1190469 - Remove unused function. r=

https://reviewboard.mozilla.org/r/23415/#review21055

::: gfx/layers/apz/src/APZCTreeManager.h
(Diff revision 2)
> -  void CancelAnimation(const ScrollableLayerGuid &aGuid);

This function call has a call site: https://dxr.mozilla.org/mozilla-central/rev/2b333a1d94e805a59c619ee41a6dec7fdcce505d/widget/android/AndroidJNI.cpp#336

Comment 24

[Botond Ballo [:botond]]

Comment on attachment 8680123 [details]
MozReview Request: Bug 1190469 - Request scroll snapping in a few places that animations are cancelled so that we don't leave things unsnapped. r=

https://reviewboard.mozilla.org/r/23561/#review21057

Comment 25

[Kartikaya Gupta (email:[e-mail]) (away until Feb-2020)]

(In reply to Botond Ballo [:botond] from comment #23)
> This function call has a call site:
> https://dxr.mozilla.org/mozilla-central/rev/
> 2b333a1d94e805a59c619ee41a6dec7fdcce505d/widget/android/AndroidJNI.cpp#336

Oh, good catch! dxr failed to find it, presumably because android. I'll drop that patch.

Comment 26

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9f858616f8a2
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ec5fb135e42

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9f858616f8a2
https://hg.mozilla.org/mozilla-central/rev/6ec5fb135e42