965871 - Implement overscroll handoff for flings

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Botond Ballo [:botond]]

In bug 898478, we implemented overscroll handoff, which is a mechanism where, if an inner scrollable element is contained within an outer scrollable element, trying to scroll the inner element by more than it has room to scroll will cause the outer element to scroll by the difference. This is implemented by APZ.

Currently this is implemented for panning only. We should also implement this for flinging.

This is particularly important for the B2G dynamic toolbar, where it's the div grouping the toolbar and the content iframe that scrolls first, and the content iframe that scrolls second, and if the div doesn't hand off flings to the iframe then the user basically can't fling the webpage.

Comment 1

[Chris Lord [:cwiiis]]

I'm not sure if we want this behaviour more generally - In fennec, we purposefully don't do this, for example. It seems we really only want this for the dynamic toolbar case... Do we also need a way of hinting when this behaviour is desired?

Furthermore, for the panning case in fennec, we only hand off panning if the inner element was already at the max extent of the scrolled axis, but that's a separate issue (i.e. we only hand off panning downwards if you pan an element that's already at it's down-most y position downwards. If you pan it upwards first, then down and hit the extent, we don't hand off).

Comment 2

[Botond Ballo [:botond]]

(In reply to Chris Lord [:cwiiis] from comment #1)
> I'm not sure if we want this behaviour more generally - In fennec, we
> purposefully don't do this, for example. It seems we really only want this
> for the dynamic toolbar case... Do we also need a way of hinting when this
> behaviour is desired?

That might be nice. See also bug 951793.

> Furthermore, for the panning case in fennec, we only hand off panning if the
> inner element was already at the max extent of the scrolled axis, but that's
> a separate issue (i.e. we only hand off panning downwards if you pan an
> element that's already at it's down-most y position downwards. If you pan it
> upwards first, then down and hit the extent, we don't hand off).

I think that makes sense from a UX perspective. 

Are any of these behaviours specified (in some standard), or will they be?

Comment 3

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

I also want to stash a bit of discussion from IRC here, regarding handoff when things are axis-locked vs when they have overflow:scroll.

12:50:26 kats: or is it bad to overload mAxisLocked that way?
12:50:58 botond: kats: yeah, we evaluated this with BenWa, they are a bit different
12:51:03 kats: ok
12:51:10 botond: kats: although, it's kind of a design choice, let me run it by you
12:51:25 botond: kats: our reasoning was that we don't want to do handoff when axis locked
12:51:52 botond: kats: so let's say your scrolling an inner and you're axis locked vertically (so no horizontal), and you're in an outer that's scrollable horizontally
12:52:27 botond: kats: we don't want small horizontal displacements that naturally occur when moving your finger to be horizontally scrolling the outer, while the inner is scrolling vertically
12:52:33 botond: kats: so axis locked --> no handoff
12:52:39 kats: makes sense
12:52:40 botond: kats: but overflow:hidden --> we want handoff
12:52:46 kats: right
12:52:50 kats: ok, i agree
12:53:02 kats: i'll pass it in to UpdateWithTouchAtDevicePoint
12:53:13 botond: kats: that said, it might make sense to add an mDisableScrolling field to Axis to avoid APZC passing it in all the time
12:53:41 botond: kats: or mOverflowHidden to avoid confusion without mAxisLocked, which used to be called mDisableScrolling :)

Comment 4

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

Without this we don't hit APZ performance target for 1.4.

Comment 5

[Botond Ballo [:botond]]

I will work on this next.

Comment 6

[Botond Ballo [:botond]]

Attachment: Part 1 - Minor cleanup and refactoring

Clean up some out-of-date comments and do some very light refactoring. Should be no functionality changes.

Comment 7

[Botond Ballo [:botond]]

Attachment: [WIP] Part 2 - Implement overscroll handoff for flings

WIP patch for implementing fling handoff. Compiles but I still need to test it (b2g build acting up atm...), add some locking, and add some comments.

Comment 8

[Botond Ballo [:botond]]

Attachment: Part 1 - Minor cleanup and refactoring

The Part 1 patch contained a bug that broke overscroll handoff in general.

Comment 9

[Botond Ballo [:botond]]

Attachment: [WIP] Part 2 - Implement overscroll handoff for flings

Updated WIP patch for implementing fling handoff. It somewhat works but has some bugs I need to iron out. It also has locking now but it needs some more cleanup.

Comment 10

[Sandip Kamat]

PM Triage: stays 1.4+

Comment 11

[Botond Ballo [:botond]]

Attachment: Part 1 - Minor cleanup and refactoring

Comment 12

[Botond Ballo [:botond]]

I now have working patches for this. I'll attach them when bugzilla is cooperative.

Comment 13

[Botond Ballo [:botond]]

Attachment: Part 2 - Document APZ lock ordering

Comment 14

[Botond Ballo [:botond]]

Attachment: Part 3 - Introduce a mechanism for AsyncPanZoomAnimation::Sample() to schedule tasks for be performed after the APZC lock is released in SampleContentTransformForFrame()

Comment 15

[Botond Ballo [:botond]]

Attachment: Part 4 - Implement overscroll handoff for flings

Comment 16

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

Comment on attachment 8388722 [details] [diff] [review]
Part 1 - Minor cleanup and refactoring

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

::: gfx/layers/ipc/AsyncPanZoomController.cpp
@@ +312,5 @@
>  
> +template <typename Units>
> +static bool IsZero(const gfx::PointTyped<Units>& aPoint)
> +{
> +  return fabs(aPoint.x) < EPSILON && fabs(aPoint.y) < EPSILON;

I'd like to start using the FuzzyEquals* functions I added to mfbt and phase out uses of EPSILON defined in random parts of the code.

@@ +1161,4 @@
>      CSSPoint cssDisplacement = displacement / zoom;
>  
>      CSSPoint cssOverscroll;
> +    CSSPoint actualDisplacement(mX.AdjustDisplacement(cssDisplacement.x,

I think "allowedDisplacement" might be a better name for this, but I'll leave it to you. "actualDisplacement" is already much better than "scrollOffset"

Comment 17

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

Comment on attachment 8388746 [details] [diff] [review]
Part 2 - Document APZ lock ordering

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

::: gfx/layers/composite/APZCTreeManager.h
@@ +51,5 @@
> + *
> + *      tree lock -> APZC locks
> + *
> + * The interpretation of the lock ordering is that if you hold A and wait on B,
> + * A must precede B in the ordering.

I find this sentence a little confusing. How about:

If lock A precedes lock B in the ordering sequence, then you must NOT wait on A while holding B.

Comment 18

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

Comment on attachment 8388748 [details] [diff] [review]
Part 3 - Introduce a mechanism for AsyncPanZoomAnimation::Sample() to schedule tasks for be performed after the APZC lock is released in SampleContentTransformForFrame()

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

s/for/to/ in the commit message

Comment 19

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

Comment on attachment 8388750 [details] [diff] [review]
Part 4 - Implement overscroll handoff for flings

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

I think you're missing some tree lock holding around the pre-existing mOverscrollHandoffChain.clear() calls in the touch-end handlers. Probably good to pull those calls out into a helper function. Enough stuff here that I'd like to do a second pass on this with the changes made.

::: gfx/layers/composite/APZCTreeManager.cpp
@@ +702,5 @@
> + * @param aTarget the target APZC
> + * @param aStartPoint the start point of the displacement
> + * @param aEndPoint the end point of the displacement
> + */
> +static void TransformDisplacement(APZCTreeManager* aTreeManager,

newline after "void"

@@ +765,5 @@
> +APZCTreeManager::HandleFlingOverscroll(AsyncPanZoomController* aPrev, ScreenPoint aVelocity)
> +{
> +  // Build the overscroll handoff chain. This is necessary because it is
> +  // otherwise built on touch-start and cleared on touch-end, and a fling
> +  // happend after touch-end. Note that, unlike DispatchScroll() which is

s/happend/happens/

@@ +787,5 @@
> +        break;
> +      }
> +    }
> +
> +    // Get the next APZC in the handoff chain, in any.

s/in any/if any/

::: gfx/layers/composite/APZCTreeManager.h
@@ +281,5 @@
> +   * @param aApzc the APZC that is handing off the fling
> +   * @param aVelocity the current velocity of the fling, in |aApzc|'s screen
> +   *                  pixels per millisecond
> +   */
> +  void HandleFlingOverscroll(AsyncPanZoomController* aApzc, ScreenPoint aVelocity);

can we call this HandOffFling instead? HandleFlingOverscroll sounds like it will be called repeatedly for each frame of fling animation.

::: gfx/layers/ipc/AsyncPanZoomController.cpp
@@ +1263,5 @@
> +  // vector classes.
> +  ScreenPoint velocity(mApzc.mX.GetVelocity(), mApzc.mY.GetVelocity());
> +
> +  ScreenPoint offset(aDelta.ToMilliseconds() * velocity.x,
> +                     aDelta.ToMilliseconds() * velocity.y);

You should be able to do aDelta.ToMillseconds() * velocity here, rather than doing the individual components.

@@ +1283,5 @@
> +    // component of the fling.
> +    if (fabs(overscroll.x) < EPSILON)
> +      velocity.x = 0;
> +    else if (fabs(overscroll.y) < EPSILON)
> +      velocity.y = 0;

Is this actually needed? There should be no harm in passing along a sub-epsilon velocity.

::: gfx/layers/ipc/AsyncPanZoomController.h
@@ +305,5 @@
>    /**
> +   * Take over a fling with the given velocity from another APZC. Used for
> +   * during overscroll handoff for a fling.
> +   */
> +  void TakeoverFling(ScreenPoint aVelocity);

nit: would prefer to call this TakeOverFling so that it reads more like "verb noun"

@@ +733,5 @@
>  
>    RefPtr<AsyncPanZoomAnimation> mAnimation;
>  
>    friend class Axis;
> +  friend class FlingAnimation;

:(

Comment 20

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #19)
> @@ +1283,5 @@
> > +    // component of the fling.
> > +    if (fabs(overscroll.x) < EPSILON)
> > +      velocity.x = 0;
> > +    else if (fabs(overscroll.y) < EPSILON)
> > +      velocity.y = 0;
> 
> Is this actually needed? There should be no harm in passing along a
> sub-epsilon velocity.

The check is for a sub-epsilon _overscroll_, not a sub-epsilon velocity. We can still have a large velocity along an axis where we are not overscrolled, and we certainly don't want to hand that off.

> @@ +733,5 @@
> >  
> >    RefPtr<AsyncPanZoomAnimation> mAnimation;
> >  
> >    friend class Axis;
> > +  friend class FlingAnimation;
> 
> :(

The alternatives that I can see are:
  - make the FlingAnimation constructor take four arguments: the APZC, the tree manager, and the axes
  - give APZC public accessors for the tree managers and the axes

If you prefer for one of these over the friend approach, let me know.

Comment 21

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

(In reply to Botond Ballo [:botond] from comment #20)
> The check is for a sub-epsilon _overscroll_, not a sub-epsilon velocity. We
> can still have a large velocity along an axis where we are not overscrolled,
> and we certainly don't want to hand that off.

Ah, my mistake. But this raises an interesting question that I hadn't considered before. Say you do a diagonal fling in a subdocument in such a way that the subdocument reaches the end on one axis before the other. In such a case, we'd want to hand off the first axis as soon as it overscrolls, but continue the fling on the subdocument on the other axis. And then when the subdocument reaches the end on that second axis, that component should also get handed off. Right? I'm not sure if your code would handle that case - I suspect the second handoff would clobber the earlier handoff on the first axis.

> The alternatives that I can see are:
>   - make the FlingAnimation constructor take four arguments: the APZC, the
> tree manager, and the axes
>   - give APZC public accessors for the tree managers and the axes
> 
> If you prefer for one of these over the friend approach, let me know.

Not really, let's stick with the friend approach for now.

Comment 22

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #21)
> (In reply to Botond Ballo [:botond] from comment #20)
> > The check is for a sub-epsilon _overscroll_, not a sub-epsilon velocity. We
> > can still have a large velocity along an axis where we are not overscrolled,
> > and we certainly don't want to hand that off.
> 
> Ah, my mistake. But this raises an interesting question that I hadn't
> considered before. Say you do a diagonal fling in a subdocument in such a
> way that the subdocument reaches the end on one axis before the other. In
> such a case, we'd want to hand off the first axis as soon as it overscrolls,
> but continue the fling on the subdocument on the other axis. And then when
> the subdocument reaches the end on that second axis, that component should
> also get handed off. Right? I'm not sure if your code would handle that case
> - I suspect the second handoff would clobber the earlier handoff on the
> first axis.

Good catch! I think we can fix this by only accepting the nonzero components of the velocity in TakeOverFling().

Comment 24

[Botond Ballo [:botond]]

(Disregard the previous comment. It works if you switch the order of the operands.)

Comment 25

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #22)
> (In reply to Kartikaya Gupta (email:[e-mail]) from comment #21)
> > (In reply to Botond Ballo [:botond] from comment #20)
> > > The check is for a sub-epsilon _overscroll_, not a sub-epsilon velocity. We
> > > can still have a large velocity along an axis where we are not overscrolled,
> > > and we certainly don't want to hand that off.
> > 
> > Ah, my mistake. But this raises an interesting question that I hadn't
> > considered before. Say you do a diagonal fling in a subdocument in such a
> > way that the subdocument reaches the end on one axis before the other. In
> > such a case, we'd want to hand off the first axis as soon as it overscrolls,
> > but continue the fling on the subdocument on the other axis. And then when
> > the subdocument reaches the end on that second axis, that component should
> > also get handed off. Right? I'm not sure if your code would handle that case
> > - I suspect the second handoff would clobber the earlier handoff on the
> > first axis.
> 
> Good catch! I think we can fix this by only accepting the nonzero components
> of the velocity in TakeOverFling().

Even better, we can just add the accepted velocity to our existing velocity in TakeOverFling(). This is more general because we can have a pre-existing velocity for various reasons.

Comment 26

[Botond Ballo [:botond]]

Attachment: Part 1 - Minor cleanup and refactoring

Updated to address review comments, carrying r+.

Comment 27

[Botond Ballo [:botond]]

Attachment: Part 2 - Document APZ lock ordering

Comment 28

[Botond Ballo [:botond]]

^ Updated to address review comments, carrying r+.

Comment 29

[Botond Ballo [:botond]]

Attachment: Part 3 - Introduce a mechanism for AsyncPanZoomAnimation::Sample() to schedule tasks to be performed after the APZC lock is released in SampleContentTransformForFrame()

Updated patch to address review comments, carrying r+.

Comment 30

[Botond Ballo [:botond]]

Attachment: Part 4 - Implement overscroll handoff for flings

Updated patch to address review comments.

While testing this further I ran into an interesting issue with aDelta being negative immediately after a handoff. See the comment at the beginning of FlingAnimation::Sample() in the new patch. I'm curious to hear your thoughts on whether my solution is appropriate.

Comment 31

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

Comment on attachment 8388896 [details] [diff] [review]
Part 4 - Implement overscroll handoff for flings

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

Thanks! r=me with braces added

::: gfx/layers/ipc/AsyncPanZoomController.cpp
@@ +1262,5 @@
> +  // the negative aDelta to be processed, it will yield a displacement in the
> +  // direction opposite to the fling, which can cause us to overscroll and
> +  // hand off the fling to _our_ parent, which effectively kills the fling.
> +  if (aDelta.ToMilliseconds() <= 0)
> +    return true;

Makes sense, and I think this is fine. In normal circumstances we should never be getting a negative delta here so I think this is a good guard to have anyway. Would like braces here though.

@@ +1297,5 @@
> +    // component of the fling.
> +    if (fabs(overscroll.x) < EPSILON)
> +      velocity.x = 0;
> +    else if (fabs(overscroll.y) < EPSILON)
> +      velocity.y = 0;

braces please

Comment 32

[Botond Ballo [:botond]]

Attachment: Part 4 - Implement overscroll handoff for flings

Updated Part 4 patch to address review comments. I also replaced some more uses of EPSILON with FuzzyEquals*.

Carrying r+.

Comment 33

[Botond Ballo [:botond]]

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

Comment 34

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #33)
> Try push: https://tbpl.mozilla.org/?tree=Try&rev=be434f3e96f9

Had Werror bustage. Fixed locally, reTrying: https://tbpl.mozilla.org/?tree=Try&rev=c72998d6da24

Comment 35

[Botond Ballo [:botond]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6e71e620511d
https://hg.mozilla.org/integration/mozilla-inbound/rev/8382b9e3a551
https://hg.mozilla.org/integration/mozilla-inbound/rev/10360e4519f4
https://hg.mozilla.org/integration/mozilla-inbound/rev/b9982dcb9a00

Comment 36

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6e71e620511d
https://hg.mozilla.org/mozilla-central/rev/8382b9e3a551
https://hg.mozilla.org/mozilla-central/rev/10360e4519f4
https://hg.mozilla.org/mozilla-central/rev/b9982dcb9a00