1259296 - Have wheel scrolling trigger scroll snapping directly in APZ

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Botond Ballo [:botond]]

With bug 1219296 implemented, APZ has the ability to trigger scroll snapping directly, without going through the main thread.

This is currently hooked up for touch events and trackpad gestures, but not wheel events. This bug tracks hooking it up for wheel events.

This is a prerequisite for fixing bug 1249040 (or it might fix it altogether).

Comment 1

[Botond Ballo [:botond]]

Attachment: MozReview Request: Bug 1259296 - Scroll snap in the compositor in response to wheel events. r=kats

Review commit: https://reviewboard.mozilla.org/r/44197/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/44197/

Comment 2

[Botond Ballo [:botond]]

Attachment: MozReview Request: Bug 1259296 - Do not scroll snap on the main thread for wheel events handled by APZ. r=kats

Review commit: https://reviewboard.mozilla.org/r/44199/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/44199/

Comment 3

[Botond Ballo [:botond]]

Attachment: MozReview Request: Bug 1259296 - Make sure APZ smooth scroll animations end at their exact destination scroll offset. r=kats

This avoids getting "stuck" at a scroll snap point with wheel scrolling.

Review commit: https://reviewboard.mozilla.org/r/44201/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/44201/

Comment 4

[Botond Ballo [:botond]]

Some notes on the approach taken here:

  - For instant wheel scrolls, I adjust the destination of the instant
    scroll to be the snap point. This matches the main-thread behaviour.

  - For smooth wheel scrolls, I use a SmoothScrollAnimation rather than
    a WheelScrollAnimation. The rationale is that this is what the main
    thread code path used to do.

  - I realize that we're doing a fair bit of shuffling between deltas
    and destinations. I did it this way to reuse the existing code as
    much as possible, but if it bothers you I can try to rework it to
    avoid some of these conversions.

  - There's one main thread behaviour that this patch doesn't emulate:
    On OS X 10.6, trackpad scrolling creates ScrollWheelInputs with
    PIXEL delta type, for which we don't snap. However, the widget
    code generates an eWheelOperationEnd event at the end of the
    gesture, which triggers scroll snapping in the main thread code.
    I didn't implement an APZ counterpart, because Markus said we're
    unlikely to support e10s and APZ on 10.6.

Comment 5

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

Comment on attachment 8737946 [details]
MozReview Request: Bug 1259296 - Scroll snap in the compositor in response to wheel events. r=kats

https://reviewboard.mozilla.org/r/44197/#review40987

::: gfx/layers/apz/src/AsyncPanZoomController.h:650
(Diff revision 1)
>    void OnTouchEndOrCancel();
>  
> +  // If |aEvent| should trigger scroll snapping, adjust |aDelta| to reflect
> +  // the snapping (that is, make it a delta that will take us to the desired
> +  // snap point). Returns true iff. the delta was so adjusted.
> +  bool MaybeAdjustDeltaForScrollSnapping(ParentLayerPoint& aDelta,

Now that we have a bunch of snap-related functions it might be nice to create a section in AsyncPanZoomController.h for these functions. Next time this code is touched, maybe.

::: gfx/layers/apz/src/AsyncPanZoomController.cpp:3987
(Diff revision 1)
> +
> +  ReentrantMonitorAutoEnter lock(mMonitor);
> +  CSSPoint scrollOffset = mFrameMetrics.GetScrollOffset();
> +  CSSToParentLayerScale2D zoom = mFrameMetrics.GetZoom();
> +  CSSPoint destination = mFrameMetrics.CalculateScrollRange().ClampPoint(
> +      mFrameMetrics.GetScrollOffset() + (aDelta / zoom));

Presumably you meant to use the local |scrollOffset| variable you extracted above?

Comment 6

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

Comment on attachment 8737947 [details]
MozReview Request: Bug 1259296 - Do not scroll snap on the main thread for wheel events handled by APZ. r=kats

https://reviewboard.mozilla.org/r/44199/#review40991

\o/

Comment 7

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

Comment on attachment 8737948 [details]
MozReview Request: Bug 1259296 - Make sure APZ smooth scroll animations end at their exact destination scroll offset. r=kats

https://reviewboard.mozilla.org/r/44201/#review40989

::: gfx/layers/apz/src/AsyncPanZoomController.cpp:744
(Diff revision 1)
> +      aFrameMetrics.SetScrollOffset(
> +          aFrameMetrics.CalculateScrollRange().ClampPoint(
> +              CSSPoint::FromAppUnits(nsPoint(mXAxisModel.GetDestination(),
> +                                             mYAxisModel.GetDestination()))));

Do we need to run the destination from the axis model through ClampPoint here? The destination should already be the exact snap position, I think.

My concern is that there might be codepaths where we do a smooth scroll to a destination but don't want snapping, and this will affect those codepaths. I'm not sure if any such cases exist though, I can't think of any at the moment.

Comment 8

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #7)
> Comment on attachment 8737948 [details]
> MozReview Request: Bug 1259296 - Make sure APZ smooth scroll animations end
> at their exact destination scroll offset. r=kats
> 
> https://reviewboard.mozilla.org/r/44201/#review40989
> 
> ::: gfx/layers/apz/src/AsyncPanZoomController.cpp:744
> (Diff revision 1)
> > +      aFrameMetrics.SetScrollOffset(
> > +          aFrameMetrics.CalculateScrollRange().ClampPoint(
> > +              CSSPoint::FromAppUnits(nsPoint(mXAxisModel.GetDestination(),
> > +                                             mYAxisModel.GetDestination()))));
> 
> Do we need to run the destination from the axis model through ClampPoint
> here? The destination should already be the exact snap position, I think.

It should. I just clamped to be extra sure, as I'm calling SetScrollOffset() directly (so there's nothing else that would check / clamp), and setting an out-of-bounds scroll offset is likely to cause Bad Things to happen.

> My concern is that there might be codepaths where we do a smooth scroll to a
> destination but don't want snapping, and this will affect those codepaths.
> I'm not sure if any such cases exist though, I can't think of any at the
> moment.

Is this related to your first point, or an independent concern?

Comment 9

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

(In reply to Botond Ballo [:botond] from comment #8)
> > Do we need to run the destination from the axis model through ClampPoint
> > here? The destination should already be the exact snap position, I think.
> 
> It should. I just clamped to be extra sure, as I'm calling SetScrollOffset()
> directly (so there's nothing else that would check / clamp), and setting an
> out-of-bounds scroll offset is likely to cause Bad Things to happen.

Ah, makes sense

> > My concern is that there might be codepaths where we do a smooth scroll to a
> > destination but don't want snapping, and this will affect those codepaths.
> > I'm not sure if any such cases exist though, I can't think of any at the
> > moment.
> 
> Is this related to your first point, or an independent concern?

It was related, but it was based on a misunderstanding of the code. After looking at the code again, it doesn't make sense, ignore it! :)

Comment 10

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

(the "it" in "it doesn't make sense" referred to my comment, not the code - sorry for ambiguous wording)

Comment 11

[Botond Ballo [:botond]]

Comment on attachment 8737946 [details]
MozReview Request: Bug 1259296 - Scroll snap in the compositor in response to wheel events. r=kats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44197/diff/1-2/

Comment 12

[Botond Ballo [:botond]]

Comment on attachment 8737947 [details]
MozReview Request: Bug 1259296 - Do not scroll snap on the main thread for wheel events handled by APZ. r=kats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44199/diff/1-2/

Comment 13

[Botond Ballo [:botond]]

Comment on attachment 8737948 [details]
MozReview Request: Bug 1259296 - Make sure APZ smooth scroll animations end at their exact destination scroll offset. r=kats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44201/diff/1-2/

Comment 14

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #5)
> ::: gfx/layers/apz/src/AsyncPanZoomController.h:650
> >    void OnTouchEndOrCancel();
> >  
> > +  // If |aEvent| should trigger scroll snapping, adjust |aDelta| to reflect
> > +  // the snapping (that is, make it a delta that will take us to the desired
> > +  // snap point). Returns true iff. the delta was so adjusted.
> > +  bool MaybeAdjustDeltaForScrollSnapping(ParentLayerPoint& aDelta,
> 
> Now that we have a bunch of snap-related functions it might be nice to
> create a section in AsyncPanZoomController.h for these functions. Next time
> this code is touched, maybe.

Done. Let me know if the formatting is up to your standards.

> 
> ::: gfx/layers/apz/src/AsyncPanZoomController.cpp:3987
> (Diff revision 1)
> > +
> > +  ReentrantMonitorAutoEnter lock(mMonitor);
> > +  CSSPoint scrollOffset = mFrameMetrics.GetScrollOffset();
> > +  CSSToParentLayerScale2D zoom = mFrameMetrics.GetZoom();
> > +  CSSPoint destination = mFrameMetrics.CalculateScrollRange().ClampPoint(
> > +      mFrameMetrics.GetScrollOffset() + (aDelta / zoom));
> 
> Presumably you meant to use the local |scrollOffset| variable you extracted
> above?

Yup, fixed.

Comment 15

[Botond Ballo [:botond]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7c1d514779fb

Comment 16

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

(In reply to Botond Ballo [:botond] from comment #14)
> 
> Done. Let me know if the formatting is up to your standards.
> 

The formatting is fine, but it's not clear where the "section" ends. Usually when I create a new section I move it to the bottom, so that the thing after it is either another section or the end of the class.

Comment 17

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #16)
> The formatting is fine, but it's not clear where the "section" ends. Usually
> when I create a new section I move it to the bottom, so that the thing after
> it is either another section or the end of the class.

I added some blank lines at the bottom, but I agree that's not necessarily clear. I'll adjust it before landing.

Comment 18

[Botond Ballo [:botond]]

Comment on attachment 8737946 [details]
MozReview Request: Bug 1259296 - Scroll snap in the compositor in response to wheel events. r=kats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44197/diff/2-3/

Comment 19

[Botond Ballo [:botond]]

Comment on attachment 8737947 [details]
MozReview Request: Bug 1259296 - Do not scroll snap on the main thread for wheel events handled by APZ. r=kats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44199/diff/2-3/

Comment 20

[Botond Ballo [:botond]]

Comment on attachment 8737948 [details]
MozReview Request: Bug 1259296 - Make sure APZ smooth scroll animations end at their exact destination scroll offset. r=kats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44201/diff/2-3/

Comment 21

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dc5ba1c7e59f
https://hg.mozilla.org/integration/mozilla-inbound/rev/4334ce2047be
https://hg.mozilla.org/integration/mozilla-inbound/rev/14cb56cfc385

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/dc5ba1c7e59f
https://hg.mozilla.org/mozilla-central/rev/4334ce2047be
https://hg.mozilla.org/mozilla-central/rev/14cb56cfc385