1040226 - Stuck after panning a subframe causes a parent frame to overscroll

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Botond Ballo [:botond]]

STR:
  1. Apply the patch for bug 1039623 if it hasn't landed yet.
  2. Run the "UI Tests" app and go to the "Scrollbar" test.
  3. Overscroll the first div on the page at the top. Observe 
     that this causes the page, not the div, to overscroll.
  4. Keep doing this. Eventually, you will get stuck in an
     overscrolled state. (My repro rate for this was fairly
     low, 10-20%)

I strongly suspect that what's happening is that the div gets a scroll offset update, which calls CancelAnimation(), which calls ClearOverscroll(), but the thing that's overscrolled is the not the div but the next thing in the handoff chain.

If so, the solution is to replace the call to CancelAnimation() here with a call to the CancelAnimationForHandoffChain() that Kats conveniently just added in bug 1039979.

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

Comment 1

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

But on the other hand we might not always want to cancel animations on the whole handoff chain if the scroll offset of a subframe changes. If you do a normal fling on the outer frame and the content changes the scroll position on the inner frame that sounds like something we should support.

But still, better to make the change since it fixes a more severe issue, and then worry about supporting it better afterwards.

Comment 2

[Chris Lord [:cwiiis]]

Marking this as blocking bug 1040087, though this may well end up fixing it.

Comment 3

[Botond Ballo [:botond]]

After some testing, it seems my suspicion in comment #0 was wrong, although that may still be an independent issue. Rather, this is what's happening:

  - Touch events continue to be delivered to the child APZC (the div's) as it hands off
    overscroll of the parent APZC (the page's).
  - When the user lifts their finger, the touch-end goes to the child APZC, and a fling
    is started on that APZC.
  - Normally, this fling would be handed off to the parent APZC here [1] on the first
    call to FlingAnimation::Sample().
  - However, if the fling is sufficiently slow that on the first call to 
    FlingAnimation::Sample() it stops [2], then nothing is handed off, and the parent
    APZC is left in an overscrolled state.

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/AsyncPanZoomController.cpp?rev=1709c5fdd182#567
[2] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/AsyncPanZoomController.cpp?rev=1709c5fdd182#497

Comment 4

[Botond Ballo [:botond]]

Attachment: bug1040226.patch

My fix is to add a method APZCTreeManager::SnapBackOverscrolledApzc() which walks the handoff chain to find the overscrolled APZC and starts a snap-back animation for it, and calling it when a fling winds down.

I ran into another problem where, when starting a snap-back in this way on the parent APZC, which has only panned and not flung, the initial velocity of the snap-back if the velocity from the pan, which is in the direction of overscroll (as opposed to the direction of relieving overscroll). To resolve this, I explicitly set the velocity to zero when starting a snap-back.

Comment 5

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

Comment on attachment 8459727 [details] [diff] [review]
bug1040226.patch

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

::: gfx/layers/apz/src/AsyncPanZoomController.cpp
@@ +500,5 @@
> +      // Start a snap-back animation on the overscrolled APZC.
> +      APZCTreeManager* treeManagerLocal = mApzc.mTreeManager;
> +      if (treeManagerLocal) {
> +        mDeferredTasks.append(NewRunnableMethod(treeManagerLocal,
> +            &APZCTreeManager::SnapBackOverscrolledApzc, &mApzc));

NewRunnableMethod will call AddRef/Release on treeManagerLocal which I think we should avoid. It might not technically introduce a reference cycle but I'd feel better if you made a NewRunnableMethod using some APZC function that then invoked APZCTM::SnapBackOverscrolledApzc. Thoughts?

Comment 6

[Botond Ballo [:botond]]

Attachment: bug1040226.patch

> NewRunnableMethod will call AddRef/Release on treeManagerLocal which I think
> we should avoid. It might not technically introduce a reference cycle but
> I'd feel better if you made a NewRunnableMethod using some APZC function
> that then invoked APZCTM::SnapBackOverscrolledApzc. Thoughts?

Sounds reasonable. This way we'll also follow the convention set by CallDispatchScroll and HandleFlingOverscroll.

Comment 7

[Botond Ballo [:botond]]

Try push that includes this patch: https://tbpl.mozilla.org/?tree=Try&rev=bde65e260011

Comment 9

[Chris Lord [:cwiiis]]

Attachment: bug1040226.patch rebased

Here's the patch rebased on master, in case it saves anyone some time.

Comment 10

[Botond Ballo [:botond]]

[Blocking Requested - why for this release]: Being stuck in overscroll is very bad UX, and even though the STR here is in a test app, the pattern that triggers the bug (a scrollable subframe inside a scrollable parent frame) can easily occur in websites.

Comment 11

[Milan Sreckovic [:milan] (needinfo for best results)]

In this case, it may be enough to ask for an uplift, given that it is test app only.  However, we're in the middle of train change, so I'm not sure if we should be asking for beta uplift or specific b2g-2.0 one, so whoever triages this into 2.0+, consider the alternative if you know what flags to set.

Comment 12

[Botond Ballo [:botond]]

Group-landed with bug 1041471 to conserve TBPL resources.

https://hg.mozilla.org/integration/mozilla-inbound/rev/abe6b92d330b
https://hg.mozilla.org/integration/mozilla-inbound/rev/9bc64b0290c2

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9bc64b0290c2

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/611a9dae1168