Closed Bug 1259296 Opened 4 years ago Closed 4 years ago

Have wheel scrolling trigger scroll snapping directly in APZ

Categories

(Core :: Panning and Zooming, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: botond, Assigned: botond)

References

Details

(Keywords: correctness, Whiteboard: [gfx-noted])

Attachments

(3 files)

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).
Assignee: nobody → 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.
Attachment #8737946 - Flags: review?(bugmail.mozilla) → review+
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 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/
Attachment #8737947 - Flags: review?(bugmail.mozilla) → review+
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.
Attachment #8737948 - Flags: review?(bugmail.mozilla) → review+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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?
(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! :)
(the "it" in "it doesn't make sense" referred to my comment, not the code - sorry for ambiguous wording)
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 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 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/
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
(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.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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 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 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 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/
You need to log in before you can comment on or make changes to this bug.