Closed Bug 1040226 Opened 5 years ago Closed 5 years ago

Stuck after panning a subframe causes a parent frame to overscroll

Categories

(Core :: Panning and Zooming, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34
blocking-b2g 2.0+
Tracking Status
firefox32 --- wontfix
firefox33 --- wontfix
firefox34 --- fixed
b2g-v2.0 --- fixed
b2g-v2.1 --- fixed

People

(Reporter: botond, Assigned: botond)

References

Details

Attachments

(2 files, 1 obsolete file)

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
See Also: → 1040094
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.
Marking this as blocking bug 1040087, though this may well end up fixing it.
Blocks: 1040087
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
Attached patch bug1040226.patch (obsolete) — Splinter Review
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.
Attachment #8459727 - Flags: review?(bugmail.mozilla)
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?
Attached patch bug1040226.patchSplinter Review
> 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.
Attachment #8459727 - Attachment is obsolete: true
Attachment #8459727 - Flags: review?(bugmail.mozilla)
Attachment #8459776 - Flags: review?(bugmail.mozilla)
Attachment #8459776 - Flags: review?(bugmail.mozilla) → review+
Try push that includes this patch: https://tbpl.mozilla.org/?tree=Try&rev=bde65e260011
Duplicate of this bug: 1040094
Blocks: 1039519
Here's the patch rebased on master, in case it saves anyone some time.
[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.
blocking-b2g: --- → 2.0?
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.
https://hg.mozilla.org/mozilla-central/rev/9bc64b0290c2
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in before you can comment on or make changes to this bug.