1042974 - Scrollgrab elements can't be flinged

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Chris Lord [:cwiiis]]

It would appear that you can't initiate a fling on a scrollgrab element. I have a vague recollection that this may be intended, but is the opposite behaviour of what I want in bug 1039519.

STR:

- Build and install gaia from https://github.com/Cwiiis/gaia/tree/rocketbar-showhide-2 (with HAIDA=0)
- Type 'google' into the homescreen search box and pick the 'Google' icon result
- Initiate a fling with a short motion that doesn't entirely scroll the rocket-bar off of the screen

Expected:

The initiated fling scrolls the rocketbar the rest of the way to the top of the screen

Actual:

The fling applies only to the inner iframe and the rocketbar remains only partially visible (before being reset by the code, causing a jarring visual effect)

Comment 1

[Botond Ballo [:botond]]

I haven't tested this, but I think I know why it doesn't work: touch events are still dispatched to the leaf-most APZC that was hit - the iframe in this case - rather than the first APZC in the handoff chain. (This is important because otherwise gestures like taps would go to the scroll-grabbing div as well.)

For scrolling, the way we ensure it's the scroll-grabbing div that scrolls first even though the iframe is getting the touch events, is that when the iframe's APZC calls TrackTouch() (from OnTouchMove()), rather than calling AttemptScroll() directly on that APZC, we call APZCTreeManager::DispatchScroll(), which calls AttemptScroll() on the first APZC in the handoff chain.

To fix this bug, we need to do something similar for flings: instead of OnTouchEnd() calling StartAnimation(new FlingAnimation()) directly, it should call something like APZCTreeManager::HandOffFling(), which then starts a fling on the first APZC in the handoff chain.

Comment 2

[Botond Ballo [:botond]]

Attachment: WIP fix attempt

Attached is an attempt to fix the problem as described in comment 1.

Unfortunately, this attempt runs into a problem: 

While scroll handoff uses the handoff chain built on touch-start, fling handoff relies on rebuilding the handoff chain at each handoff (since the handoff chain built on touch-start is cleared on touch-end, and fling handoffs happen after touch-end). 

This breaks down when a scroll-grabbing APZC is being flung: when it tries to hand off its fling, we construct a handoff chain starting from the scroll-grabbing APZC, but that handoff chain does not contain the child APZC that we want to hand off the fling to (since handoff chain construction only goes from child to parent). As a result, the scroll-grabbing APZC goes into overscroll instead, and all sorts of Bad Things happen.

To resolve this, we'll probably need to revise the lifetime of the handoff chain built on touch-start, so that it lives on until any fling generated by that touch is completely consumed, and thus we can use it during fling handoff.

Comment 3

[Botond Ballo [:botond]]

Bug 1039992 also concerns the lifetime of the overscroll handoff chain; the same approach might fix both issues.

Comment 4

[Kevin Grandon :kgrandon]

Chris - can you clarify if this is a problem with the current rocketbar implementation in master? Thanks!

Comment 5

[Botond Ballo [:botond]]

(In reply to Kevin Grandon :kgrandon from comment #4)
> Chris - can you clarify if this is a problem with the current rocketbar
> implementation in master? Thanks!

It is.

Say the rocketbar is expanded, and you do a scroll that's small enough to scroll the scroll-grabbing <div> but not collapse the rocketbar. The <iframe> element will now be partially obscured by the rocketbar. Usually this is fine, as it just looks as if the page scrolled, but if the page has a fixed-position header, then _that_ will be obscured by the rocketbar, which obviously looks bad.

Usually, the momentum of the scroll (i.e. the fling) is enough to cause the rocketbar to collapse, making the above scenario happen very rarely. However, due to this bug, the fling applies to the <iframe> instead of to the <div>, and so the rocketbar does not collapse, and so the above scenario happens much more often.

The fix on the APZ side is relatively straightforward; I basically just need to rebase the patch that I posted after bug 1039992 (now on m-i) lands.

Comment 6

[Botond Ballo [:botond]]

Attachment: Fix

Rebased and tested fix, seems to be working.

Comment 7

[Botond Ballo [:botond]]

Attachment: Fix

Er, I had accidentally reposted the old patch. Here's the new one.

Comment 8

[Botond Ballo [:botond]]

Attachment: Fix

Updated patch to fix an issue I caught while writing tests (yay tests!)

Comment 9

[Botond Ballo [:botond]]

Attachment: Gtests

And here are the tests.

Comment 10

[Botond Ballo [:botond]]

Try push: https://tbpl.mozilla.org/?tree=Try&rev=2dfe2b9e4044

Comment 11

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

Comment on attachment 8476053 [details] [diff] [review]
Fix

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

r+ with comments addressed

::: gfx/layers/apz/src/APZCTreeManager.cpp
@@ +902,5 @@
> +  if (aHandoff) {
> +    // Find |aPrev| in the handoff chain.
> +    uint32_t i;
> +    for (i = 0; i < aOverscrollHandoffChain->Length(); ++i) {
> +      if (aOverscrollHandoffChain->GetApzcAtIndex(i) == aPrev) {

This bit to find "next" given "aPrev" could be encapsulated as a function on OverscrollHandoffChain. If you extract an IndexOf-type function from OverscrollHandoffChain::CanBePanned you can reuse it for this.

::: gfx/layers/apz/src/AsyncPanZoomController.cpp
@@ +429,5 @@
>      // boost the velocity to be the sum of the two. Check separate axes separately
>      // because we could have two vertical flings with small horizontal components
>      // on the opposite side of zero, and we still want the y-fling to get accelerated.
>      // Note that the acceleration code is only applied on the APZC that receives the
>      // actual touch event; the accelerated velocities are then handed off using the

s/receives the actual touch event/initiates the fling/

Comment 12

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

Comment on attachment 8476058 [details] [diff] [review]
Gtests

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

::: gfx/tests/gtest/TestAsyncPanZoomController.cpp
@@ +1699,5 @@
>  }
>  
>  class APZOverscrollHandoffTester : public APZCTreeManagerTester {
>  protected:
> +  UniquePtr<ScopedLayerTreeRegistration> controller;

As long as you're moving this, could you rename it to something other than "controller"? We already have way too many things called controller :)

@@ +1728,5 @@
>      SetScrollableFrameMetrics(root, FrameMetrics::START_SCROLL_ID, CSSRect(0, 0, 200, 200));
>      SetScrollableFrameMetrics(layers[1], FrameMetrics::START_SCROLL_ID + 2, CSSRect(-100, -100, 200, 200));
>      SetScrollableFrameMetrics(layers[2], FrameMetrics::START_SCROLL_ID + 1, CSSRect(0, 0, 100, 100));
> +    // No ScopedLayerTreeRegistration as that just needs to be done once per test
> +    // and this is the second layer tree for a particular test.

Might be good to assert the UniquePtr is non-null? Just in case somebody tries using this layer tree on its own in the future.

@@ +1883,5 @@
> +
> +  // Pan on the child, enough to fully scroll the scrollgrab parent (20 px)
> +  // and leave some more (another 20 px) for the child.
> +  int time = 0;
> +  ApzcPan(childApzc, time, 80, 40);

Probably doesn't matter much in this case but in general I like to use numbers that don't result in similar states for multiple APZCs. In this case a scroll of 40px results in 20px scroll for both root and child, but if you did a scroll of 35px instead you'd have different scroll amounts for the root and child. In a way it's an extra level of checking to make sure the right things scrolled by the right amounts and not the other way around.

Comment 13

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #11)
> ::: gfx/layers/apz/src/APZCTreeManager.cpp
> @@ +902,5 @@
> > +  if (aHandoff) {
> > +    // Find |aPrev| in the handoff chain.
> > +    uint32_t i;
> > +    for (i = 0; i < aOverscrollHandoffChain->Length(); ++i) {
> > +      if (aOverscrollHandoffChain->GetApzcAtIndex(i) == aPrev) {
> 
> This bit to find "next" given "aPrev" could be encapsulated as a function on
> OverscrollHandoffChain. If you extract an IndexOf-type function from
> OverscrollHandoffChain::CanBePanned you can reuse it for this.

Good idea! Done.

> ::: gfx/layers/apz/src/AsyncPanZoomController.cpp
> @@ +429,5 @@
> >      // boost the velocity to be the sum of the two. Check separate axes separately
> >      // because we could have two vertical flings with small horizontal components
> >      // on the opposite side of zero, and we still want the y-fling to get accelerated.
> >      // Note that the acceleration code is only applied on the APZC that receives the
> >      // actual touch event; the accelerated velocities are then handed off using the
> 
> s/receives the actual touch event/initiates the fling/

Done.

Comment 14

[Botond Ballo [:botond]]

Attachment: Fix

Updated to address review comments. Carrying r+.

Comment 15

[Botond Ballo [:botond]]

Attachment: Gtests

Updated patch to address review comments. Carrying r+.

> >  class APZOverscrollHandoffTester : public APZCTreeManagerTester {
> >  protected:
> > +  UniquePtr<ScopedLayerTreeRegistration> controller;
> 
> As long as you're moving this, could you rename it to something other than
> "controller"? We already have way too many things called controller :)

Yeah, I was wondering why that variable was called "controller" :) Done.

> @@ +1728,5 @@
> >      SetScrollableFrameMetrics(root, FrameMetrics::START_SCROLL_ID, CSSRect(0, 0, 200, 200));
> >      SetScrollableFrameMetrics(layers[1], FrameMetrics::START_SCROLL_ID + 2, CSSRect(-100, -100, 200, 200));
> >      SetScrollableFrameMetrics(layers[2], FrameMetrics::START_SCROLL_ID + 1, CSSRect(0, 0, 100, 100));
> > +    // No ScopedLayerTreeRegistration as that just needs to be done once per test
> > +    // and this is the second layer tree for a particular test.
> 
> Might be good to assert the UniquePtr is non-null? Just in case somebody
> tries using this layer tree on its own in the future.

Yep, added.

> @@ +1883,5 @@
> > +
> > +  // Pan on the child, enough to fully scroll the scrollgrab parent (20 px)
> > +  // and leave some more (another 20 px) for the child.
> > +  int time = 0;
> > +  ApzcPan(childApzc, time, 80, 40);
> 
> Probably doesn't matter much in this case but in general I like to use
> numbers that don't result in similar states for multiple APZCs. In this case
> a scroll of 40px results in 20px scroll for both root and child, but if you
> did a scroll of 35px instead you'd have different scroll amounts for the
> root and child. In a way it's an extra level of checking to make sure the
> right things scrolled by the right amounts and not the other way around.

Fair enough :) Done.

Comment 16

[Botond Ballo [:botond]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b2326bbb0c28
https://hg.mozilla.org/integration/mozilla-inbound/rev/6797cbbf30d7

Comment 17

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/b2326bbb0c28
https://hg.mozilla.org/mozilla-central/rev/6797cbbf30d7