Closed Bug 1190469 Opened 4 years ago Closed 4 years ago

Interrupting a pan will break scroll snapping with apz

Categories

(Core :: Panning and Zooming, defect)

All
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla44
blocking-b2g 2.5?
Tracking Status
firefox42 --- affected
firefox44 --- fixed

People

(Reporter: cwiiis, Assigned: kats)

References

()

Details

Attachments

(3 files)

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.
CC'ing Markus because we were recently discussing how there might be more places we need to do a RequestFlingSnap call.
Duplicate of this bug: 1211730
[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.
blocking-b2g: --- → 2.5?
See Also: → 1179453
Duplicate of this bug: 1217514
Duplicate of this bug: 1218203
Duplicate of this bug: 1218318
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.
Flags: needinfo?(mstange)
I can take a look at this. I'm looking at other APZ+snapping issues anyway.
Assignee: nobody → bugmail.mozilla
Flags: needinfo?(mstange)
Bug 1190469 - Refactor some code to have a general-purpose snap function available in APZC. r=
Attachment #8679431 - Flags: review?(botond)
Bug 1190469 - Request scroll snapping under roughly the same conditions that we clear overscroll. r=
Attachment #8679432 - Flags: review?(botond)
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.
See Also: 1179453
Duplicate of this bug: 1179453
Attachment #8679431 - Flags: review?(botond) → review+
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
Attachment #8679432 - Flags: review?(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?
(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.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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!
(In reply to Botond Ballo [:botond] from comment #16)
> (In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
(Also, I inserted a new patch to remove an unused function, MozReview probably won't match up the patches properly. Sorry...)
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 on attachment 8679432 [details]
MozReview Request: Bug 1190469 - Remove unused function. r=

Bug 1190469 - Remove unused function. r=
Attachment #8679432 - Attachment description: MozReview Request: Bug 1190469 - Request scroll snapping under roughly the same conditions that we clear overscroll. r= → MozReview Request: Bug 1190469 - Remove unused function. r=
Attachment #8679432 - Flags: review?(botond)
Bug 1190469 - Request scroll snapping in a few places that animations are cancelled so that we don't leave things unsnapped. r=
Attachment #8680123 - Flags: review?(botond)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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 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
Attachment #8679432 - Flags: review?(botond)
Attachment #8680123 - Flags: review?(botond) → review+
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
(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.
https://hg.mozilla.org/mozilla-central/rev/9f858616f8a2
https://hg.mozilla.org/mozilla-central/rev/6ec5fb135e42
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.