Closed
Bug 1259296
Opened 8 years ago
Closed 8 years ago
Have wheel scrolling trigger scroll snapping directly in APZ
Categories
(Core :: Panning and Zooming, defect)
Core
Panning and Zooming
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 | ||
Updated•8 years ago
|
Assignee: nobody → botond
Assignee | ||
Comment 1•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/44197/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/44197/
Attachment #8737946 -
Flags: review?(bugmail.mozilla)
Attachment #8737947 -
Flags: review?(bugmail.mozilla)
Attachment #8737948 -
Flags: review?(bugmail.mozilla)
Assignee | ||
Comment 2•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/44199/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/44199/
Assignee | ||
Comment 3•8 years ago
|
||
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/
Assignee | ||
Comment 4•8 years ago
|
||
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.
Updated•8 years ago
|
Attachment #8737946 -
Flags: review?(bugmail.mozilla) → review+
Comment 5•8 years ago
|
||
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•8 years ago
|
||
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 7•8 years ago
|
||
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+
Assignee | ||
Comment 8•8 years ago
|
||
(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?
Comment 9•8 years ago
|
||
(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•8 years ago
|
||
(the "it" in "it doesn't make sense" referred to my comment, not the code - sorry for ambiguous wording)
Assignee | ||
Comment 11•8 years ago
|
||
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/
Assignee | ||
Comment 12•8 years ago
|
||
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/
Assignee | ||
Comment 13•8 years ago
|
||
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/
Assignee | ||
Comment 14•8 years ago
|
||
(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.
Assignee | ||
Comment 15•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7c1d514779fb
Comment 16•8 years ago
|
||
(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.
Assignee | ||
Comment 17•8 years ago
|
||
(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.
Assignee | ||
Comment 18•8 years ago
|
||
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/
Assignee | ||
Comment 19•8 years ago
|
||
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/
Assignee | ||
Comment 20•8 years ago
|
||
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•8 years ago
|
||
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•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/dc5ba1c7e59f https://hg.mozilla.org/mozilla-central/rev/4334ce2047be https://hg.mozilla.org/mozilla-central/rev/14cb56cfc385
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in
before you can comment on or make changes to this bug.
Description
•