Closed Bug 1201098 Opened 4 years ago Closed 4 years ago

Nesting a vertical scrollable element in a horizontally scrollable element with no vertical overflow breaks vertical overscroll in the inner element


(Core :: Panning and Zooming, defect)

Gonk (Firefox OS)
Not set



Tracking Status
firefox43 --- fixed


(Reporter: cwiiis, Assigned: cwiiis, Mentored)




(Keywords: polish, Whiteboard: [lang=c++])


(1 file, 3 obsolete files)

As the summary says, the configuration of

<frame 1>
  <frame 2 />
</frame 1>

Where frame 1 is horizontally scrollable only and frame 2 is vertically scrollable only results in a situation where frame 2 cannot be vertically overscrolled, despite it being vertically scrollable.

See the given URL on a FirefoxOS device for a demonstration.
frame 1 is "eating" the overscroll, because it's scrollable, even though it's not scrollable in the direction of the gesture.

There's a TODO about this in the code, from back when I first wrote the overscrolling implementation:
The idea here is that currently, when an APZC hands off a requested displacement to the next APZC in the handoff chain, that APZC (or an APZC further down the chain) either takes it wholesale, or not at all.

To fix this, we need to relax this to a model where the APZC further down the handoff chain can consume a *portion* of the handed-off displacement, and "hand back" the remainder to the original APZC so it can overscroll if the one later in the chain didn't want to.

Rough outline of how to implement this:

  - Modify the following functions to take their (start point, end point) 
    arguments by modifiable reference:
        - AsyncPanZoomController::AttemptScroll
        - AsyncPanZoomController::CallDispatchScroll
        - APZCTreeManager::DispatchScroll

    and modify them to represent the displacement that *wasn't* consumed
    (in effect, the displacement being "handed back" to the caller).

    In the case of APZCTreeManager::DispatchScroll(), the "handed back"
    displacement needs to be transformed *back* into the coordinate space
    of the previous APZC.

  - In AttemptScroll(), call OverscrollForPanning() if CallDispatchScroll()
    left any nonzero amount of displacement unconsumed.

This sounds like it might make a nice mentored bug, so I'm marking it as such. Chris, if you need this fixed more urgently, feel free to take it (or ask me to).
Mentor: botond
Whiteboard: [lang=c++]
Weren't we discussing recently a way to split the animations into per-axis versions? My memory is already hazy on that even though we talked about it recently... but maybe if we do handoff/overscroll on a per-axis basis then that might solve this problem too? Just a thought.
The fix I described concerns overscrolling during panning; code-wise, that's orthogonal to anything we do with animations.

However, you're right that this problem can also occur when a fling goes into overscroll, and having animations separated by axis would make it easier to fix that part.
This is my attempt at fixing this...

So, with this, I'm able to overscroll the container that previously couldn't be overscrolled, but it doesn't snap back after I let go. I assume this is because it's not in any state that would trigger an overscroll-snap-back animation (I guess the outer container is somehow stealing this?)

Any ideas?
Attachment #8658316 - Flags: feedback?(botond)
Assignee: nobody →
Interestingly, this same problem happens regardless of css scroll-snapping - the inner container won't snap back... Trying to track down why.
So I've at least managed to ascertain (probably) that the snap back from overscroll is a result of the fling animation that starts in AcceptFling.

I thought a reasonable solution would be for AttemptFling and AcceptFling to modify the aVelocity parameter, like AttemptScroll/CallDispatchScroll, and for DispatchFling to carry on down the overscroll handoff chain until aVelocity is zero... But that doesn't seem to work as I expect (it appears to make no difference).

Still digging.
This was tricky to track down, but got a fix - the patch makes it more evident than my random comments will, so will tidy it up and submit for review.
My issue was that as well as DispatchScroll, DispatchFling also needs to consume the velocity per-axis and return how much is consumed, otherwise HandleFlingOverscroll can erroneously do nothing when only one axis of the fling-overscroll was consumed.
Attachment #8658316 - Attachment is obsolete: true
Attachment #8658316 - Flags: feedback?(botond)
Attachment #8659411 - Flags: review?(botond)
Comment on attachment 8659411 [details] [diff] [review]

Review of attachment 8659411 [details] [diff] [review]:

Thanks a lot for taking this on, Chris! Sorry I didn't respond to your questions sooner but it looks like you figured them all out.

This generally looks great, r+ with comments addressed.

Would I be pushing my luck if I asked you to write gtests for this? ("Yes" is an acceptable answer, in which case please file a follow-up bug, and I will write gtests when I get a chance.)

::: gfx/layers/apz/src/APZCTreeManager.cpp
@@ +1259,3 @@
>    // (if |aPrev| is the APZC that is initiating the scroll and there is no
>    // scroll grabbing to grab the scroll from it), don't bother doing the
>    // transformations in that case.

Please remove the second sentence of the comment above ("Since |aPrev| may be the same as |next| ...").

@@ +1265,5 @@
>    // Scroll |next|. If this causes overscroll, it will call DispatchScroll()
>    // again with an incremented index.
> +  if (!next->AttemptScroll(aStartPoint, aEndPoint, aOverscrollHandoffState)) {
> +    if (!TransformDisplacement(this, next, aPrev, aStartPoint, aEndPoint)) {

// Transform |aStartPoint| and |aEndPoint| (which now represent the
// portion of the displacement that wasn't consumed by APZCs later
// in the handoff chain) back into |aPrev|'s coordinate space. This
// allows the caller (which is |aPrev|) to interpret the unconsumed
// displacement in its own coordinate space, and make use of it
// (e.g. by going into overscroll).

@@ +1268,5 @@
> +  if (!next->AttemptScroll(aStartPoint, aEndPoint, aOverscrollHandoffState)) {
> +    if (!TransformDisplacement(this, next, aPrev, aStartPoint, aEndPoint)) {
> +      NS_WARNING("Failed to untransform scroll points during dispatch");
> +    }
> +    return;

This |return| doesn't do anything useful, we're at the end of the function.

@@ +1292,4 @@
>    // rather than (0, 0).
>    ParentLayerPoint startPoint;  // (0, 0)
>    ParentLayerPoint endPoint;
> +  ParentLayerPoint transformedVelocity;

Move this into the loop as it's not used across iterations.

@@ +1342,5 @@
> +      // Subtract the proportion of used velocity from aVelocity
> +      if (!FuzzyEqualsAdditive(transformedVelocity.x,
> +                               usedTransformedVelocity.x, COORDINATE_EPSILON)) {
> +        aVelocity.x = aVelocity.x *
> +          (usedTransformedVelocity.x / transformedVelocity.x);

I like how you avoid having to transform the velocity back into |aPrev|'s coordinate space by using proportions. Very clever :)

::: gfx/layers/apz/src/AsyncPanZoomController.cpp
@@ +2156,5 @@
>    // scroll to begin with.
>    bool xCanScroll = mX.CanScroll();
>    bool yCanScroll = mY.CanScroll();
> +
> +  bool xScrolled = FuzzyEqualsAdditive(aOverscroll.x, 0.0f, COORDINATE_EPSILON);

I find this naming a bit counterintuitive - maybe |xConsumed|?

::: gfx/layers/apz/src/AsyncPanZoomController.h
@@ +854,2 @@
>     * is handed off to the next APZC in the handoff chain via
> +   * mTreeManager->DspatchFling(). Returns true iff. the entire velocity of

DispatchFling() [not your typo, I know]

@@ +854,3 @@
>     * is handed off to the next APZC in the handoff chain via
> +   * mTreeManager->DspatchFling(). Returns true iff. the entire velocity of
> +   * the fling was consumed by the APZCs in the handoff chain. aVelocity is

This comment is out-of-date (it was already prior to your changes): unlike AttemptScroll/DispatchScoll, AttemptFling/DispatchFling are not mutually recursive. AttemptFling just attempts to fling the current APZC (and DispatchFling loops over the handoff chain, calling AttemptFling for each APZC in it).

So the comment should read something like "Returns true iff. the entire velocity of the fling was consumed by this APZC."

@@ +967,5 @@
>     * handoff chain, accepted the scroll (possibly entering an overscrolled
>     * state). If this returns false, the caller APZC knows that it should enter
>     * an overscrolled state itself if it can.
> +   * aStartPoint and aEndPoint are modified depending on how much of the
> +   * scroll gesture was concumsed by APZCs in the handoff chain.


@@ +1024,5 @@
>    /**
>     * Try to overscroll by 'aOverscroll'.
> +   * If we are pannable on a particular axis, that component of 'aOverscroll'
> +   * is transferred to any existing overscroll. The function returns true if
> +   * all of aOverscroll was consumed, otherwise it returns false.

Mention that |aOverscroll| has the consumed portion subtracted out of it.

Also, I wonder if we can get rid of the return value, and just have the caller check if the overscroll IsZero() after the call?

::: gfx/layers/apz/src/OverscrollHandoffState.cpp
@@ +108,5 @@
>    for (; i < Length(); ++i) {
>      AsyncPanZoomController* apzc = mChain[i];
>      if (!apzc->IsDestroyed() && apzc->SnapBackIfOverscrolled()) {
> +      // Multiple APZCs can be overscrolled, but individual APZCs will handle
> +      // snapping back from an overscrolled state.

What makes you say that individual APZCs will handle snapping back from an overscrolled state? I believe that in the scenario this function is meant to address (described bug 1040226 comment 3), a snap-back needs to be invoked on all overscrolled APZCs, and so if there can be more than one (as this patch makes possible), we need to call SnapBackIfOverscrolled() on each. That is, we don't want the |break| in this loop any more.
Attachment #8659411 - Flags: review?(botond) → review+
Blocks: 1204502
Made all suggested changes, carrying r=botond - waiting on green try before pushing to inbound:

Botond, I've filed bug 1204502 - I wouldn't mind tackling it, but I'll need some hand-holding to figure out gtests. I have them running locally and I had a quick look at it, but I have too many outstanding questions to take a shot at it right now (and I have a lot of other blockers on my plate).
Attachment #8659411 - Attachment is obsolete: true
Attachment #8660734 - Flags: review+
This is causing some assertion failures and for the life of me, I can't get a build of Firefox working on my laptop... I'll debug this when I'm next in the office (Thursday), unless someone beats me to it.

It's looking like I'm missing some locking somewhere.
I applied the patch locally and ran the gtests. Here's the relevant part of a stack trace:

#9  0x00007fde6529f0b9 in mozilla::ReentrantMonitor::AssertCurrentThreadIn (this=0x7fde5e565228) at ../../../dist/include/mozilla/ReentrantMonitor.h:129
#10 0x00007fde66d47315 in mozilla::layers::AsyncPanZoomController::GetFrameMetrics (this=0x7fde5e565000)
    at /home/botond/dev/mozilla/refactoring/gfx/layers/apz/src/AsyncPanZoomController.cpp:3061
#11 0x00007fde66d4c229 in mozilla::layers::Axis::GetFrameMetrics (this=0x7fde5e565610) at /home/botond/dev/mozilla/refactoring/gfx/layers/apz/src/Axis.cpp:558
#12 0x00007fde66d4c014 in mozilla::layers::Axis::GetPageLength (this=0x7fde5e565610) at /home/botond/dev/mozilla/refactoring/gfx/layers/apz/src/Axis.cpp:546
#13 0x00007fde66d45e99 in mozilla::layers::Axis::CanScroll (this=0x7fde5e565610) at /home/botond/dev/mozilla/refactoring/gfx/layers/apz/src/Axis.cpp:415
#14 0x00007fde66d4840b in mozilla::layers::AsyncPanZoomController::AcceptFling (this=0x7fde5e565000, aVelocity=..., aOverscrollHandoffChain=..., aHandoff=false)
    at /home/botond/dev/mozilla/refactoring/gfx/layers/apz/src/AsyncPanZoomController.cpp:2195
#15 0x00007fde66d3f461 in mozilla::layers::AsyncPanZoomController::AttemptFling (this=0x7fde5e565000, aVelocity=..., aOverscrollHandoffChain=..., aHandoff=false)
    at /home/botond/dev/mozilla/refactoring/gfx/layers/apz/src/AsyncPanZoomController.cpp:2249
#16 0x00007fde66d3f267 in mozilla::layers::APZCTreeManager::DispatchFling (this=0x7fde77e13600, aPrev=0x7fde5e565000, aVelocity=..., aOverscrollHandoffChain=..., aHandoff=false)
    at /home/botond/dev/mozilla/refactoring/gfx/layers/apz/src/APZCTreeManager.cpp:1335
#17 0x00007fde66d4224a in mozilla::layers::AsyncPanZoomController::OnTouchEnd (this=0x7fde5e565000, aEvent=...)
    at /home/botond/dev/mozilla/refactoring/gfx/layers/apz/src/AsyncPanZoomController.cpp:1239
#18 0x00007fde66d41563 in mozilla::layers::AsyncPanZoomController::HandleInputEvent (this=0x7fde5e565000, aEvent=..., aTransformToApzc=...)
    at /home/botond/dev/mozilla/refactoring/gfx/layers/apz/src/AsyncPanZoomController.cpp:976
#19 0x00007fde66d4e937 in mozilla::layers::CancelableBlockState::DispatchEvent (this=0x7fde5e547ec0, aEvent=...)
    at /home/botond/dev/mozilla/refactoring/gfx/layers/apz/src/InputBlockState.cpp:160
#20 0x00007fde66d4fd4b in mozilla::layers::TouchBlockState::DispatchEvent (this=0x7fde5e547ec0, aEvent=...)
    at /home/botond/dev/mozilla/refactoring/gfx/layers/apz/src/InputBlockState.cpp:680
#21 0x00007fde66d4e8ea in mozilla::layers::CancelableBlockState::DispatchImmediate (this=0x7fde5e547ec0, aEvent=...)
    at /home/botond/dev/mozilla/refactoring/gfx/layers/apz/src/InputBlockState.cpp:154
#22 0x00007fde66d50738 in mozilla::layers::InputQueue::MaybeHandleCurrentBlock (this=0x7fde5e55ac60, block=0x7fde5e547ec0, aEvent=...)
    at /home/botond/dev/mozilla/refactoring/gfx/layers/apz/src/InputQueue.cpp:73
#23 0x00007fde66d5018b in mozilla::layers::InputQueue::ReceiveTouchInput (this=0x7fde5e55ac60, aTarget=..., aTargetConfirmed=true, aEvent=..., aOutInputBlockId=0x0)
    at /home/botond/dev/mozilla/refactoring/gfx/layers/apz/src/InputQueue.cpp:159
#24 0x00007fde66d3b4e5 in mozilla::layers::InputQueue::ReceiveInputEvent (this=0x7fde5e55ac60, aTarget=..., aTargetConfirmed=true, aEvent=..., aOutInputBlockId=0x0)
    at /home/botond/dev/mozilla/refactoring/gfx/layers/apz/src/InputQueue.cpp:40
#25 0x00007fde65358d98 in TestAsyncPanZoomController::ReceiveInputEvent (this=0x7fde5e565000, aEvent=..., aOutInputBlockId=0x0)
    at /home/botond/dev/mozilla/refactoring/gfx/tests/gtest/TestAsyncPanZoomController.cpp:179
#26 0x00007fde65378d69 in TestAsyncPanZoomController::ReceiveInputEvent (this=0x7fde5e565000, aEvent=..., aDummy=0x0, aOutInputBlockId=0x0)
    at /home/botond/dev/mozilla/refactoring/gfx/tests/gtest/TestAsyncPanZoomController.cpp:175
#27 0x00007fde653188d2 in TouchUp<TestAsyncPanZoomController> (aTarget=..., aX=10, aY=10, aTime=...)
    at /home/botond/dev/mozilla/refactoring/gfx/tests/gtest/TestAsyncPanZoomController.cpp:472
#28 0x00007fde6534e9b1 in Pan<TestAsyncPanZoomController> (aTarget=..., aMcc=0x7fde61bb6600, aTouchStart=..., aTouchEnd=..., aKeepFingerDown=false, aAllowedTouchBehaviors=0x0,
    aOutEventStatuses=0x0, aOutInputBlockId=0x7fff20856a68) at /home/botond/dev/mozilla/refactoring/gfx/tests/gtest/TestAsyncPanZoomController.cpp:575
#29 0x00007fde653173ce in Pan<TestAsyncPanZoomController> (aTarget=..., aMcc=0x7fde61bb6600, aTouchStartY=50, aTouchEndY=10, aKeepFingerDown=false, aAllowedTouchBehaviors=0x0,
    aOutEventStatuses=0x0, aOutInputBlockId=0x0) at /home/botond/dev/mozilla/refactoring/gfx/tests/gtest/TestAsyncPanZoomController.cpp:601
#30 0x00007fde6531716f in APZCBasicTester_Fling_Test::TestBody (this=0x7fde5e51bc80) at /home/botond/dev/mozilla/refactoring/gfx/tests/gtest/TestAsyncPanZoomController.cpp:1197
Grabbing mMonitor at the top of AcceptFling() makes the tests pass for me.
(In reply to Botond Ballo [:botond] from comment #14)
> Grabbing mMonitor at the top of AcceptFling() makes the tests pass for me.

oh, cheers! Unfortunately, running this on my dogfood device, I've noticed some odd behaviour with the scrollgrab container of the browser - it overscrolls and pans at the same time (and when doing this, the overscroll doesn't snap back). I'm guessing I'm missing some zeroing somewhere when scrolling is accepted, then consuming it in overscroll.

So once these are both fixed, I'll upload the patch here for you to give another look.
Actually, that was much easier to track down than expected - the error was obvious, so I think it's ok to carry the r+ (there was a return before aStartPoint was updated to reflect consumed scroll).

Attachment #8660734 - Attachment is obsolete: true
Attachment #8661692 - Flags: review+
I don't believe the failures that are left on that try push are related to this patch and it's mostly green, adding checkin-needed.
Keywords: checkin-needed
Keywords: polish
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.