Closed Bug 965871 Opened 6 years ago Closed 6 years ago

Implement overscroll handoff for flings

Categories

(Core :: Panning and Zooming, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30
blocking-b2g 1.4+
Tracking Status
firefox28 --- wontfix
firefox29 --- wontfix
firefox30 --- fixed
b2g-v1.4 --- fixed

People

(Reporter: botond, Assigned: botond)

References

Details

Attachments

(4 files, 9 obsolete files)

5.92 KB, patch
botond
: review+
Details | Diff | Splinter Review
4.14 KB, patch
botond
: review+
Details | Diff | Splinter Review
2.70 KB, patch
botond
: review+
Details | Diff | Splinter Review
26.80 KB, patch
botond
: review+
Details | Diff | Splinter Review
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.
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).
(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?
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 :)
Without this we don't hit APZ performance target for 1.4.
blocking-b2g: --- → 1.4+
I will work on this next.
Assignee: nobody → botond
Clean up some out-of-date comments and do some very light refactoring. Should be no functionality changes.
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.
The Part 1 patch contained a bug that broke overscroll handoff in general.
Attachment #8383413 - Attachment is obsolete: true
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.
Attachment #8383414 - Attachment is obsolete: true
PM Triage: stays 1.4+
Attachment #8384036 - Attachment is obsolete: true
Attachment #8388722 - Flags: review?(bugmail.mozilla)
I now have working patches for this. I'll attach them when bugzilla is cooperative.
Attachment #8388746 - Flags: review?(bugmail.mozilla)
Attachment #8384038 - Attachment is obsolete: true
Attachment #8388750 - Flags: review?(bugmail.mozilla)
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"
Attachment #8388722 - Flags: review?(bugmail.mozilla) → review+
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.
Attachment #8388746 - Flags: review?(bugmail.mozilla) → review+
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
Attachment #8388748 - Flags: review?(bugmail.mozilla) → review+
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;

:(
Attachment #8388750 - Flags: review?(bugmail.mozilla) → review-
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
(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.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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().
(Disregard the previous comment. It works if you switch the order of the operands.)
(In reply to Botond Ballo [:botond] from comment #22)
> (In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
Updated to address review comments, carrying r+.
Attachment #8388722 - Attachment is obsolete: true
Attachment #8388883 - Flags: review+
Attachment #8388746 - Attachment is obsolete: true
Attachment #8388884 - Flags: review+
^ Updated to address review comments, carrying r+.
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.
Attachment #8388750 - Attachment is obsolete: true
Attachment #8388896 - Flags: review?(bugmail.mozilla)
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
Attachment #8388896 - Flags: review?(bugmail.mozilla) → review+
Updated Part 4 patch to address review comments. I also replaced some more uses of EPSILON with FuzzyEquals*.

Carrying r+.
Attachment #8388896 - Attachment is obsolete: true
Attachment #8389220 - Flags: review+
(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
You need to log in before you can comment on or make changes to this bug.