To support Bug 1022818 and Bug 1010538, the CSSOM-View smooth scrolling behavior movement model will be implemented within the APZ related classes (possibly within Axis.cpp). A separate bug will be created if this needs to be implemented for platforms that do no support APZ. Details on the smooth scrolling movement model is described in Bug 1010538.

You may find the code at https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=961280&attachment=8362635 useful as a model for one possible way to implement this. That code never landed because of other issues with wheel handling but the smooth-scrolling bits could be extracted and made useful.

While beginning implementation I noticed that there is smooth scrolling functionality already implemented within ScrollFrameHelper::AsyncScroll, using simple splines via nsSMILKeySpline. This smooth scrolling behavior is used internally and not exposed with XPCOM or WebIDL bindings. I will raise this within dev-platform to see if this same routine should be re-cycled, updated to match the proposed movement model, or kept as-is as a separate implementation for these internal use-cases.

I think the splines are decent. Can't tell if you'll find them an easier starting point than your MSD model from bug 1010538 comment 3. They're used for user input events scroll (such as mouse wheel or KB arrows). I tweaked their parameters a bit in bug 206438 - mostly making them run for a bit longer. I think they create a reasonably pleasant scroll "Feeling". If you feel like doing it, it could be nice if the algorithm was "pluggable" such that we could experiment with different algorithms to improve it further, possibly with the MSD model.

(In reply to :kip (Kearwood Gilbert) from comment #0) > A separate bug will be created if this needs to > be implemented for platforms that do no support APZ. Bug 1026023 has been created to track implementation for platforms that do not support APZ.

Bug 1022818 - EXPERIMENTAL - Implement Asynchronous Smooth Scrolling on Compositor Thread This is an experimental implementation of Smooth Scrolling for CSSOM-View Scroll-Behavior running on the Compositor thread. Patchsets from Bug 1026023 and Bug 1022818 must be applied first.

The attached "EXPERIMENTAL" patch implements smooth scorlling for CSSOM-View Scroll-Behavior, running on the compositor thread. Some issues remain before this is ready for use: - Need to cancel the keyboard, touchpad, and mousewheel events when a smooth scroll starts. Fling gestures that have been interrupted by a CSSOM-View smooth scroll should not cause additional movement after the interrupting CSSOM-View smooth scroll has finished. - CSSOM-View smooth scroll events should be interrupted by keyboard, touchpad, and mousewheel events. - On B2G, if the address bar appears or disappears during a CSSOM-View scroll animation, the final position of the scroll does not match the position requested in the DOM calls. - If the MSD is configured as an under-damped system, it can overshoot into the overscroll region. If keyboard, mouse, or touchpad events cancel the smooth scroll before it stops oscillating, the position remains outside the expected scrolling bounds until the user manually scrolls the frame.

(In reply to :kip (Kearwood Gilbert) from comment #6) > The attached "EXPERIMENTAL" patch implements smooth scorlling for CSSOM-View > Scroll-Behavior, running on the compositor thread. > > Some issues remain before this is ready for use: > - The smooth scrolling event is fired from the main thread to the compositor through the replicated FrameMetrics. This has a benefit in that it serializes with the other scroll events which may cancel or be cancelled by the smooth scrolls. Perhaps a cleaner solution would be to have a dedicated IPC message to instruct the compositor thread to begin smooth scrolling without affecting the FrameMetrics directly. (Unlike synchronous scroll DOM events, the CSSOM-View smooth scroll-behavior is asynchronous and effectively acts like other animations processed by the compositor as a response to an input gesture) I've NI'ed for advice on if it would worth / desirable to refactor my patch to use a dedicated IPC message.

I think not, but check with kats too.

I only glanced at the patch (f? me if you'd like me to look at it in more detail) but yes, I think it is better to piggyback the smooth scroll stuff on the FrameMetrics than to have a dedicated IPC message for it.

Thanks Kats and Roc. I'll fix the issues in Comment 6 then submit the patch for review. (In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #9) > I only glanced at the patch (f? me if you'd like me to look at it in more > detail) but yes, I think it is better to piggyback the smooth scroll stuff > on the FrameMetrics than to have a dedicated IPC message for it.

V2 Patch: Updated to match changes in patchset for Bug 1026023

V3 Patch: Un-bitrotted

V4 Patch: - Un-bitrotted - Now passing velocity from fling animations into smooth scrolls that interrupt them - When a SMOOTH_MSD_SCROLL scroll is being processed on a frame, mouse wheel and trackpad momentum scroll position updates will not cancel the SMOOTH_MSD_SCROLL scroll animations, enabling scripts that depend on them to be responsive without forcing the user to wait for the momentum scrolling to completely stop.

- Extended nsIScrollableFrame and nsGfxScrollFrame to return destination of smooth scrolls which are to be animated on the compositor thread. - Added apz.smooth_scroll_repaint_interval preference. - Implemented AsyncPanZoomController::PanZoomState::SMOOTH_MSD_SCROLL state and AsyncPanZoomController::SmoothScrollAnimation class to animate smooth scroll animations on the compositor thread. - Extended FrameMetrics to report requests for smooth scrolls to be animated on the compositor thread and their corresponding destination positions. - AsyncPanZoomController now checks FrameMetrics for requests to perform smooth scrolling on the compositor thread. It will ensure that they are cancelled as needed by mousewheel, touchpanel, keyboard, and CSSOM-View instant scrolling DOM methods. V5 Patch: - Tested on device with a Flame; Issues in Comment 6 have been addressed except for handoff to an overscroll animation. The overscroll animation handoff is not functioning correctly and results in invalid zoom and pan offset when CSSOM-View smooth scroll enters the overscroll.

Tests added to Bug 1022818 work when APZ is enabled and will exercise the code in this patchset when performing mochitests on B2G (or other platforms with APZ enabled).

- Extended nsIScrollableFrame and nsGfxScrollFrame to return destination of smooth scrolls which are to be animated on the compositor thread. - Added apz.smooth_scroll_repaint_interval preference. - Implemented AsyncPanZoomController::PanZoomState::SMOOTH_MSD_SCROLL state and AsyncPanZoomController::SmoothScrollAnimation class to animate smooth scroll animations on the compositor thread. - Extended FrameMetrics to report requests for smooth scrolls to be animated on the compositor thread and their corresponding destination positions. - AsyncPanZoomController now checks FrameMetrics for requests to perform smooth scrolling on the compositor thread. It will ensure that they are cancelled as needed by mousewheel, touchpanel, keyboard, and CSSOM-View instant scrolling DOM methods. V6 Patch: - Now handing off to overscroll snapback animation on B2G with correct velocity. - Screen redraws more consistently while smooth scrolling on the compositor thread.

Comment on attachment 8465034 [details] [diff] [review] Bug 1022825 - Implement Asynchronous Smooth Scrolling on Compositor Thread (V6 Patch) Review of attachment 8465034 [details] [diff] [review]: ----------------------------------------------------------------- You should get kats and/or botond to review the AsyncPanZoomController/FrameMetrics changes. The rest looks good. ::: layout/generic/nsIScrollableFrame.h @@ +337,5 @@ > + */ > + virtual bool LastScrollIsSmooth() = 0; > + /** > + * When CurrentScrollGeneration returns true, LastScrollIsSmooth returns the > + * destination of the requested smooth scroll animation. This comment seems wrong, when LastScrollIsSmooth is true? Could add an assert in the implementation to ensure this too.

Hi Kats and Botond, would one of you have some free cycles to review this?

Thanks for the review! (In reply to Matt Woodrow (:mattwoodrow) from comment #19) > Comment on attachment 8465034 [details] [diff] [review] > Bug 1022825 - Implement Asynchronous Smooth Scrolling on Compositor Thread > (V6 Patch) > > Review of attachment 8465034 [details] [diff] [review]: > ----------------------------------------------------------------- > > You should get kats and/or botond to review the > AsyncPanZoomController/FrameMetrics changes. The rest looks good. > > ::: layout/generic/nsIScrollableFrame.h > @@ +337,5 @@ > > + */ > > + virtual bool LastScrollIsSmooth() = 0; > > + /** > > + * When CurrentScrollGeneration returns true, LastScrollIsSmooth returns the > > + * destination of the requested smooth scroll animation. > > This comment seems wrong, when LastScrollIsSmooth is true? Could add an > assert in the implementation to ensure this too. Indeed, that comment was mixed up.. It should have read: * When LastScrollIsSmooth returns true, LastScrollDestination returns the * destination of the requested smooth scroll animation.

Comment on attachment 8465034 [details] [diff] [review] Bug 1022825 - Implement Asynchronous Smooth Scrolling on Compositor Thread (V6 Patch) Review of attachment 8465034 [details] [diff] [review]: ----------------------------------------------------------------- This looks pretty good. I have some comments below. I think you should also log the new FrameMetrics flags as part of the AppendToString(... FrameMetrics ...) function in gfx/layers/LayersLogging.cpp (in the detailed section). ::: gfx/layers/FrameMetrics.h @@ +92,5 @@ > , mZoom(1) > , mUpdateScrollOffset(false) > , mScrollGeneration(0) > + , mUpdateScrollOffsetSmoothly(false) > + , mSmoothScrollDestination(0, 0) I find it a little confusing to store the mSmoothScrollDestination as separate from the mScrollOffset (which is where the non-smooth scroll destination is stored, in cases where content does a scrollTo). I think we should be able to get rid of mSmoothScrollDestination and just use mScrollOffset instead. The code in RecordFrameMetrics would just populate mScrollOffset with the smooth-scroll destination, and the code in NotifyLayersUpdated would have to pick it up and start a smooth scroll to that destination, if the mUpdateScrollOffsetSmoothly flag is set. @@ +247,5 @@ > + mUpdateScrollOffsetSmoothly = aOther.mUpdateScrollOffsetSmoothly; > + } > + > + void CopySmoothScrollInfoFrom(const FrameMetrics& aOther) > + { Shouldn't need this function at all if we get rid of mSmoothScrollDestination ::: gfx/layers/apz/src/AsyncPanZoomController.cpp @@ +274,5 @@ > * will asynchronously repaint the page. It is also forced when panning stops. > * > + * "apz.smooth_scroll_repaint_interval" > + * Maximum amount of time flinging before sending a viewport change. This will > + * asynchronously repaint the page. s/flinging/doing a smooth scroll/. "Fling" means something else in APZ code. @@ +684,5 @@ > + * frame. Returns true if the smooth scroll should be advanced by one frame, > + * or false if the smooth scroll has ended. > + */ > + virtual bool Sample(FrameMetrics& aFrameMetrics, > + const TimeDuration& aDelta) MOZ_OVERRIDE; Move the function body into the class definition here @@ +766,5 @@ > + // the lock ordering. Instead we schedule HandleFlingOverscroll() to be > + // called after mMonitor is released. > + mDeferredTasks.append(NewRunnableMethod(&mApzc, > + &AsyncPanZoomController::HandleFlingOverscroll, > + velocity)); Shouldn't this return false after handing off overscroll? @@ +1520,5 @@ > APZC_LOG("%p got a pan-momentumstart in state %d\n", this, mState); > > + if (mState == SMOOTH_MSD_SCROLL) { > + // SMOOTH_MSD_SCROLL scrolls are cancelled by pan gestures. > + CancelAnimation(); According to the docs in widget/InputData.h the MOMENTUMSTART pan gesture is sent after end the of the user-controlled pan and before the momentum pan. I'm not too familiar with the events on mac os but it's not clear to me how we could possibly be in the SMOOTH_MSD_SCROLL state at this point. Could you explain? @@ +2514,5 @@ > bool scrollOffsetUpdated = aLayerMetrics.GetScrollOffsetUpdated() > && (aLayerMetrics.GetScrollGeneration() != mFrameMetrics.GetScrollGeneration()); > > + bool smoothScrollRequested = aLayerMetrics.GetScrollSmoothly() > + && (aLayerMetrics.GetScrollGeneration() != mFrameMetrics.GetScrollGeneration()); This would be better as bool smoothScrollRequested = scrollOffsetUpdated && aLayerMetrics.GetScrollSmoothly(); @@ +2569,5 @@ > + // the scroll update acknowledgement. > + mFrameMetrics.CopySmoothScrollInfoFrom(aLayerMetrics); > + CancelAnimation(); > + mLastDispatchedPaintMetrics = aLayerMetrics; > + StartSmoothMSDScroll(); If we get rid of the separate smooth-scroll destination, this code can get merged into the scrollOffsetUpdated block below. You might need a local var or two to avoid clobbering mFrameMetrics.mScrollOffset but overall I think it should end up better. @@ +2593,5 @@ > mLastDispatchedPaintMetrics = aLayerMetrics; > } > } > > + if (scrollOffsetUpdated || smoothScrollRequested) { Shouldn't smoothScrollRequested imply scrollOffsetUpdated? This change seems unnecessary. ::: gfx/layers/apz/src/AsyncPanZoomController.h @@ +337,5 @@ > PINCHING, /* nth touch-start, where n > 1. this mode allows pan and zoom */ > ANIMATING_ZOOM, /* animated zoom to a new rect */ > SNAP_BACK, /* snap-back animation to relieve overscroll */ > + SMOOTH_MSD_SCROLL, /* Smooth scrolling to destination with MSD model > + used by CSSOM-View smooth scroll-behavior */ remove MSD from the state name, and from the comment. That feels like an implementation detail that doesn't need to be baked in. Same for StartSmoothMSDScroll - rename to StartSmoothScroll @@ +798,5 @@ > > private: > friend class FlingAnimation; > friend class OverscrollSnapBackAnimation; > + friend class SmoothScrollAnimation; Update the "The functions and members in this section" comment a few lines above here to also mention "smooth scroll" in addition to fling and fling-overscroll. ::: layout/generic/nsIScrollableFrame.h @@ +325,2 @@ > */ > virtual nsIAtom* OriginOfLastScroll() = 0; For consistency it might be nice to rename OriginOfLastScroll() to be LastScrollOrigin(). If layout folks agree, then a separate patch or bug for that is fine. @@ +345,5 @@ > * Clears the "origin of last scroll" property stored in this frame, if > * the generation counter passed in matches the current scroll generation > * counter. > */ > virtual void ResetOriginIfScrollAtGeneration(uint32_t aGeneration) = 0; Likewise it might be good to rename ResetOriginIfScrollAtGeneration(...) to reflect the new expanded functionality: ResetScrollInfoIfGeneration(...) or something

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #22) Thanks for taking time to review this for me. I am making the adjustments that you have recommended. I have a couple of comments: > I find it a little confusing to store the mSmoothScrollDestination as > separate from the mScrollOffset (which is where the non-smooth scroll > destination is stored, in cases where content does a scrollTo). I think we > should be able to get rid of mSmoothScrollDestination and just use > mScrollOffset instead. > > The code in RecordFrameMetrics would just populate mScrollOffset with the > smooth-scroll destination, and the code in NotifyLayersUpdated would have to > pick it up and start a smooth scroll to that destination, if the > mUpdateScrollOffsetSmoothly flag is set. My source comments should have captured the reason for having both mScrollOffset and mSmoothScrollDestination. It is possible for both an instant scroll and smooth scroll to occur within a single framemetrics update if an instant scroll-behavior DOM call is immediately followed by a smooth scroll-behavior DOM call. In this event, mScrollOffset acts as the starting location for the smooth scroll and mSmoothScrollDestination is the destination to be animated to. > @@ +1520,5 @@ > > APZC_LOG("%p got a pan-momentumstart in state %d\n", this, mState); > > > > + if (mState == SMOOTH_MSD_SCROLL) { > > + // SMOOTH_MSD_SCROLL scrolls are cancelled by pan gestures. > > + CancelAnimation(); > > According to the docs in widget/InputData.h the MOMENTUMSTART pan gesture is > sent after end the of the user-controlled pan and before the momentum pan. > I'm not too familiar with the events on mac os but it's not clear to me how > we could possibly be in the SMOOTH_MSD_SCROLL state at this point. Could you > explain? On OSX, the momentum pan events are generated by the OS as regular scroll wheel events with an additional flag set. These events continue to fire after user input has ended until the momentum of the scroll has been consumed. The duration of this deceleration phase can reach several seconds. It is possible that a script will call a smooth scroll-behavior DOM method while these events are continuing to fire. Without the check added to the MOMENTUMSTART handler, these events would immediately cancel the smooth scroll. This has the effect of such scripts appearing to be non-responsive when activated soon after a mouse wheel or track pad scrolling gesture. Thanks for your feedback!

(In reply to :kip (Kearwood Gilbert) from comment #24) > My source comments should have captured the reason for having both > mScrollOffset and mSmoothScrollDestination. It is possible for both an > instant scroll and smooth scroll to occur within a single framemetrics > update if an instant scroll-behavior DOM call is immediately followed by a > smooth scroll-behavior DOM call. In this event, mScrollOffset acts as the > starting location for the smooth scroll and mSmoothScrollDestination is the > destination to be animated to. Ah. That makes things more complicated. I'll look over the patch again with this in mind. > > According to the docs in widget/InputData.h the MOMENTUMSTART pan gesture is > > sent after end the of the user-controlled pan and before the momentum pan. > > I'm not too familiar with the events on mac os but it's not clear to me how > > we could possibly be in the SMOOTH_MSD_SCROLL state at this point. Could you > > explain? > On OSX, the momentum pan events are generated by the OS as regular scroll > wheel events with an additional flag set. These events continue to fire > after user input has ended until the momentum of the scroll has been > consumed. The duration of this deceleration phase can reach several > seconds. It is possible that a script will call a smooth scroll-behavior > DOM method while these events are continuing to fire. Without the check > added to the MOMENTUMSTART handler, these events would immediately cancel > the smooth scroll. This is the part that confuses me. Why is the check in the MOMENTUMSTART handler rather than the MOMENTUMPAN handler? I'm assuming that when OS X generates these events, it generates a single MOMENTUMSTART event followed by a series of MOMENTUMPAN events during the deceleration phase. If a script does a smooth-scroll at this point, we won't be getting any more MOMENTUMSTART events, but we will be getting MOMENTUMPAN events. Those should be the ones checking for smooth-scroll, no?

It doesn't look to me like the patch you have deals with the "instant scroll + smooth scroll" scenario properly. Specifically, in the NotifyLayersUpdated call, if a smooth scroll is detected (which happens before the instant scroll check) it calls CopySmoothScrollInfoFrom on the metrics (which doesn't copy mScrollOffset) and then immediately starts a smooth scroll to the destination. This will use the old value of mScrollOffset as the starting position for the smooth scroll, which seems incorrect. What I would like to see here is to have the fields in FrameMetrics defined like so: mUpdateScrollOffset - Set to true iff there is an instant scroll that needs to be processed. mScrollOffset - contains the coordinates to instant scroll to. Really only used when mUpdateScrollOffset is true mDoSmoothScroll - Set to true iff there is a smooth scroll that needs to be processed. If mUpdateScrollOffset is also true, then the instant scroll is processed first. mSmoothScrollOffset - contains the coordinates to smooth-scroll to. Really only used when mDoSmoothScroll is true mScrollGeneration - bumped on the first FrameMetrics update that contains a new instant or smooth scroll (or both). In NotifyLayersUpdate, we should always check for the instant scroll first, using mUpdateScrollOffset and the generation number. If there is one, we should process it (I think almost exactly the way we do now). After that, we should check if there is a smooth scroll as well, and process that if needed. The CopyScrollInfoFrom function should only copy mScrollOffset and mScrollGeneration, I think. If you add a CopySmoothScrollInfoFrom function, it would copy mSmoothScrollOffset and mScrollGeneration. Does that sound reasonable?

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #26) > It doesn't look to me like the patch you have deals with the "instant scroll > + smooth scroll" scenario properly. Specifically, in the NotifyLayersUpdated > call, if a smooth scroll is detected (which happens before the instant > scroll check) it calls CopySmoothScrollInfoFrom on the metrics (which > doesn't copy mScrollOffset) and then immediately starts a smooth scroll to > the destination. This will use the old value of mScrollOffset as the > starting position for the smooth scroll, which seems incorrect. > > What I would like to see here is to have the fields in FrameMetrics defined > like so: > mUpdateScrollOffset - Set to true iff there is an instant scroll that needs > to be processed. > mScrollOffset - contains the coordinates to instant scroll to. Really only > used when mUpdateScrollOffset is true > mDoSmoothScroll - Set to true iff there is a smooth scroll that needs to be > processed. If mUpdateScrollOffset is also true, then the instant scroll is > processed first. > mSmoothScrollOffset - contains the coordinates to smooth-scroll to. Really > only used when mDoSmoothScroll is true > mScrollGeneration - bumped on the first FrameMetrics update that contains a > new instant or smooth scroll (or both). > > In NotifyLayersUpdate, we should always check for the instant scroll first, > using mUpdateScrollOffset and the generation number. If there is one, we > should process it (I think almost exactly the way we do now). After that, we > should check if there is a smooth scroll as well, and process that if needed. > > The CopyScrollInfoFrom function should only copy mScrollOffset and > mScrollGeneration, I think. If you add a CopySmoothScrollInfoFrom function, > it would copy mSmoothScrollOffset and mScrollGeneration. > > Does that sound reasonable? Good catch, Kats. It appears that there is a slight jank caused by this when a smooth scroll interrupts a previous smooth scroll or a fling. I'll correct it following your recommendation.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #25) > > > According to the docs in widget/InputData.h the MOMENTUMSTART pan gesture is > > > sent after end the of the user-controlled pan and before the momentum pan. > > > I'm not too familiar with the events on mac os but it's not clear to me how > > > we could possibly be in the SMOOTH_MSD_SCROLL state at this point. Could you > > > explain? > > On OSX, the momentum pan events are generated by the OS as regular scroll > > wheel events with an additional flag set. These events continue to fire > > after user input has ended until the momentum of the scroll has been > > consumed. The duration of this deceleration phase can reach several > > seconds. It is possible that a script will call a smooth scroll-behavior > > DOM method while these events are continuing to fire. Without the check > > added to the MOMENTUMSTART handler, these events would immediately cancel > > the smooth scroll. > > This is the part that confuses me. Why is the check in the MOMENTUMSTART > handler rather than the MOMENTUMPAN handler? I'm assuming that when OS X > generates these events, it generates a single MOMENTUMSTART event followed > by a series of MOMENTUMPAN events during the deceleration phase. If a script > does a smooth-scroll at this point, we won't be getting any more > MOMENTUMSTART events, but we will be getting MOMENTUMPAN events. Those > should be the ones checking for smooth-scroll, no? OSX sends a PANGESTURE_MOMENTUMSTART at the beginning of the deceleration phase. CSSOM-View scroll-behavior spec states that smooth scrolls should be interrupted by subsequent scrolls and user input. The patch includes code to ensure that such gestures will cancel a smooth scroll which is already in flight prior to the gesture / mouse wheel / track pad event starting in AsyncPanZoomController::OnPanMomentumStart. After PANGESTURE_MOMENTUMSTART, OSX sends multiple PANGESTURE_MOMENTUMPAN events. The patch includes code in AsyncPanZoomController::OnPan to ignore these events and prevent them from cancelling a smooth scroll that has started after the deceleration phase had begun. It is possible that the smooth scroll may end prior to the end of the deceleration phase synthesized by OSX. In this event, after the smooth scroll completes there may be additional movement resulting from the continued PANGESTURE_MOMENTUMPAN events. These scrolling artifacts are also reproducible without CSSOM-View Smooth scroll-behavior by using keyboard scrolling during a mousewheel or trackpad initiated momentum scroll event on OSX.

(In reply to :kip (Kearwood Gilbert) from comment #28) Ok, that makes sense now, and I think the code in the last patch to deal with this looks good. Thanks for the explanation!

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #22) > Comment on attachment 8465034 [details] [diff] [review] > @@ +766,5 @@ > > + // the lock ordering. Instead we schedule HandleFlingOverscroll() to be > > + // called after mMonitor is released. > > + mDeferredTasks.append(NewRunnableMethod(&mApzc, > > + &AsyncPanZoomController::HandleFlingOverscroll, > > + velocity)); > > Shouldn't this return false after handing off overscroll? Indeed, returning true might leave AsyncPanZoomController::mAnimation set if the overscroll is handed off to a higher level scroll frame. This also applies to AsyncPanZoomController::FlingAnimation::Sample. I will try updating both Sample() functions to return false when handing off and investigate any other unintended side effects.

- Extended nsIScrollableFrame and nsGfxScrollFrame to return destination of smooth scrolls which are to be animated on the compositor thread. - Added apz.smooth_scroll_repaint_interval preference. - Implemented AsyncPanZoomController::PanZoomState::SMOOTH_MSD_SCROLL state and AsyncPanZoomController::SmoothScrollAnimation class to animate smooth scroll animations on the compositor thread. - Extended FrameMetrics to report requests for smooth scrolls to be animated on the compositor thread and their corresponding destination positions. - AsyncPanZoomController now checks FrameMetrics for requests to perform smooth scrolling on the compositor thread. It will ensure that they are cancelled as needed by mousewheel, touchpanel, keyboard, and CSSOM-View instant scrolling DOM methods. - The layout/generic/test/test_scroll_behavior.html mochitest is no longer disabled on e10s and b2g, as 1022825 adds support for these platforms. V2 Patch: - The layout/generic/test/test_scroll_behavior.html mochitest was disabled in Bug 1022818 for e10s and b2g. The mochitest passes with this bug applied, which also re-enabled it from e10s and b2g. - Updated with feedback from Comment 21 - Comment 30.

Comment on attachment 8478564 [details] [diff] [review] Bug 1022825 - Implement Asynchronous Smooth Scrolling on Compositor Thread (V7 Patch) Review of attachment 8478564 [details] [diff] [review]: ----------------------------------------------------------------- I'm reviewing this patch incrementally in the middle of other stuff, sorry for the delay. One question I had though: ::: layout/generic/nsGfxScrollFrame.cpp @@ +2402,5 @@ > nsPoint oldScrollFramePos = mScrolledFrame->GetPosition(); > // Update frame position for scrolling > mScrolledFrame->SetPosition(mScrollPort.TopLeft() - pt); > + mLastScrollOrigin = aOrigin; > + mLastSmoothScrollOrigin = aOrigin; Doesn't setting mLastSmoothScrollOrigin here mean that every scrollTo call will do a smooth scroll? I'm not sure I understand why you're doing this. In my mental model I would expect mLastSmoothScrollOrigin to get set to null here, and then the various places where you check if the smooth scroll origin is nsGkAtoms::apz would be removed as well. It seems wrong to me to ever set mLastSmoothScrollOrigin to apz because the APZ never actually requests a smooth scroll.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #32) > Comment on attachment 8478564 [details] [diff] [review] > Bug 1022825 - Implement Asynchronous Smooth Scrolling on Compositor Thread > (V7 Patch) > > Review of attachment 8478564 [details] [diff] [review]: > ----------------------------------------------------------------- > > I'm reviewing this patch incrementally in the middle of other stuff, sorry > for the delay. One question I had though: > > ::: layout/generic/nsGfxScrollFrame.cpp > @@ +2402,5 @@ > > nsPoint oldScrollFramePos = mScrolledFrame->GetPosition(); > > // Update frame position for scrolling > > mScrolledFrame->SetPosition(mScrollPort.TopLeft() - pt); > > + mLastScrollOrigin = aOrigin; > > + mLastSmoothScrollOrigin = aOrigin; > > Doesn't setting mLastSmoothScrollOrigin here mean that every scrollTo call > will do a smooth scroll? I'm not sure I understand why you're doing this. > > In my mental model I would expect mLastSmoothScrollOrigin to get set to null > here, and then the various places where you check if the smooth scroll > origin is nsGkAtoms::apz would be removed as well. It seems wrong to me to > ever set mLastSmoothScrollOrigin to apz because the APZ never actually > requests a smooth scroll. Thanks again for the review. We reach this point when the compositor has acknowledged that it has received the last FrameMetrics update (see APZCallbackHelper.cpp). We may also hit this multiple times in the case of a smooth scroll as the compositor sends periodic FrameMetric updates to the main thread over the course of the asynchronous animation. In this case, aOrigin will equal nsGkAtoms::apz. We update both mLastScrollOrigin and mLastSmoothScrollOrigin to indicate that both types of scroll events have been received by the compositor. It is important to signal that the FrameMetrics were last updated by the compositor as these variables are used as a form of handshake to ensure that the compositor does not stomp over top of new scroll requests made by the main thread. To signal that the compositor should perform a new smooth scroll, mLastSmoothScroll needs to be set to a non-null value other than nsGkAtoms::apz. When this occurs, the compositor will not send new frame metric updates to the main thread until it has applied any instant scroll and started any smooth scroll specified in the FrameMetrics sent by the main thread.

Your comment doesn't really answer what I was trying to get at. Consider the case where everything is stationary, and some piece of Javascript does scrollTo(0, 100) for example. The code in ScrollFrameHelper::ScrollToImpl is going to set both mLastScrollOrigin and mLastSmoothScrollOrigin to a non-apz value, which means the RecordFrameMetrics call will set both the mUpdateScrollOffset and the mDoSmoothScroll flag on the FrameMetrics. The mScrollOffset will be correct and the mSmoothScrollOffset value will be some stale destination. When the NotifyLayersUpdated code runs, it'll do an instant scroll to mScrollOffset followed by a smooth scroll to the state mSmoothScrollOffset. Am I missing something here? If what I described above is accurate, I think you can make the following changes to the patch to fix it: 1) in ScrollFrameHelper::ScrollToImpl, set mLastSmoothScrollOrigin to nullptr instead of nsGkAtoms::apz 2) in APZCCallbackHelper::ScrollFrameTo, replace this: if (!aFrame->IsProcessingAsyncScroll() && (!aFrame->LastScrollOrigin() || aFrame->LastScrollOrigin() == nsGkAtoms::apz) && (!aFrame->LastSmoothScrollOrigin() || aFrame->LastSmoothScrollOrigin() == nsGkAtoms::apz)) { with this: if (!aFrame->IsProcessingAsyncScroll() && (!aFrame->LastScrollOrigin() || aFrame->LastScrollOrigin() == nsGkAtoms::apz) && (!aFrame->LastSmoothScrollOrigin())) { (Actually, even better to replace it with this): bool scrollInProgress = aFrame->IsProcessingAsyncScroll() || (aFrame->LastScrollOrigin() && aFrame->LastScrollOrigin() != nsGkAtoms::apz) || aFrame->LastSmoothScrollOrigin(); if (!scrollInProgress) { 3) in RecordFrameMetrics, replace this: if (lastSmoothScrollOrigin && lastSmoothScrollOrigin != nsGkAtoms::apz) { metrics.SetSmoothScrollOffsetUpdated(scrollableFrame->CurrentScrollGeneration()); } with this: if (lastSmoothScrollOrigin) { metrics.SetSmoothScrollOffsetUpdated(scrollableFrame->CurrentScrollGeneration()); } Does that make sense? If not let please let me know why that wouldn't work or if I made a mistake thinking through this. I'll review the rest of the patch as well in a sec.

Comment on attachment 8478564 [details] [diff] [review] Bug 1022825 - Implement Asynchronous Smooth Scrolling on Compositor Thread (V7 Patch) Review of attachment 8478564 [details] [diff] [review]: ----------------------------------------------------------------- Everything else in the patch seems fine, just some minor comments below. Minusing until we sort out the issue I described in comment 34. ::: gfx/layers/LayersLogging.cpp @@ +120,5 @@ > aStream << pfx; > AppendToString(aStream, m.mCompositionBounds, "{ cb="); > AppendToString(aStream, m.mScrollableRect, " sr="); > AppendToString(aStream, m.GetScrollOffset(), " s="); > + AppendToString(aStream, m.GetSmoothScrollOffset(), " ss="); Would be good to only print this if m.GetDoSmoothScroll() is true, as this is going to be used less frequently than m.GetScrollOffset(). ::: gfx/layers/apz/src/AsyncPanZoomController.cpp @@ +749,5 @@ > + > + aFrameMetrics.ScrollBy(adjustedOffset); > + > + // The smooth scroll may have caused us to reach the end of our scroll range. > + if (!IsZero(overscroll)) { In practice we should never actually hit this right? The MSD model slows down to stop at the destination without overshooting, so we should never enter overscroll, if I understand correctly. I don't mind leaving this code here just in case but wanted to check if my understanding was right. If so please updated the comment just above the if statement. @@ +1996,5 @@ > > +void AsyncPanZoomController::StartSmoothScroll() { > + SetState(SMOOTH_SCROLL); > + nsPoint initialPosition = CSSPoint::ToAppUnits(mFrameMetrics.GetScrollOffset()); > + // Convert velocity from ScreenPoints/ms to appunits/second Technically you're just casting from ScreenPoints/ms to CSSPoints/ms and *then* converting to appunits/second. But I do think it makes more sense to use Screen coordinates when computing the scroll because you don't want the scroll duration to be affected by zoom, for example. So this code is fine but the documentation might need a bit of correction. @@ +2611,5 @@ > bool scrollOffsetUpdated = aLayerMetrics.GetScrollOffsetUpdated() > && (aLayerMetrics.GetScrollGeneration() != mFrameMetrics.GetScrollGeneration()); > > + bool smoothScrollRequested = aLayerMetrics.GetDoSmoothScroll() > + && (aLayerMetrics.GetScrollGeneration() != mFrameMetrics.GetScrollGeneration()); Move this down to just before the use site. ::: gfx/thebes/gfxPrefs.h @@ +158,5 @@ > DECL_GFX_PREF(Live, "apz.overscroll.snap_back.spring_friction", APZOverscrollSnapBackSpringFriction, float, 0.1f); > DECL_GFX_PREF(Live, "apz.overscroll.snap_back.mass", APZOverscrollSnapBackMass, float, 1000.0f); > DECL_GFX_PREF(Live, "apz.pan_repaint_interval", APZPanRepaintInterval, int32_t, 250); > DECL_GFX_PREF(Live, "apz.printtree", APZPrintTree, bool, false); > + DECL_GFX_PREF(Live, "apz.smooth_scroll_repaint_interval", APZSmoothScrollRepaintInterval, int32_t, 75); Also add this to modules/libpref/init/all.js. As of bug 927946 we'd like to make sure all APZ prefs are entered there. ::: layout/base/nsDisplayList.cpp @@ +700,5 @@ > // If the frame was scrolled since the last layers update, and by > // something other than the APZ code, we want to tell the APZ to update > // its scroll offset. > + nsIAtom* lastScrollOrigin = scrollableFrame->LastScrollOrigin(); > + nsIAtom* lastSmoothScrollOrigin = scrollableFrame->LastSmoothScrollOrigin(); Move the lastSmoothScrollOrigin definition down to just before the use site.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #34) > Your comment doesn't really answer what I was trying to get at. Consider the > case where everything is stationary, and some piece of Javascript does > scrollTo(0, 100) for example. The code in ScrollFrameHelper::ScrollToImpl is > going to set both mLastScrollOrigin and mLastSmoothScrollOrigin to a non-apz > value, which means the RecordFrameMetrics call will set both the > mUpdateScrollOffset and the mDoSmoothScroll flag on the FrameMetrics. The > mScrollOffset will be correct and the mSmoothScrollOffset value will be some > stale destination. When the NotifyLayersUpdated code runs, it'll do an > instant scroll to mScrollOffset followed by a smooth scroll to the state > mSmoothScrollOffset. Am I missing something here? > > If what I described above is accurate, I think you can make the following > changes to the patch to fix it: > > 1) in ScrollFrameHelper::ScrollToImpl, set mLastSmoothScrollOrigin to > nullptr instead of nsGkAtoms::apz > > 2) in APZCCallbackHelper::ScrollFrameTo, replace this: > > if (!aFrame->IsProcessingAsyncScroll() && > (!aFrame->LastScrollOrigin() || aFrame->LastScrollOrigin() == > nsGkAtoms::apz) && > (!aFrame->LastSmoothScrollOrigin() || aFrame->LastSmoothScrollOrigin() > == nsGkAtoms::apz)) { > > with this: > > if (!aFrame->IsProcessingAsyncScroll() && > (!aFrame->LastScrollOrigin() || aFrame->LastScrollOrigin() == > nsGkAtoms::apz) && > (!aFrame->LastSmoothScrollOrigin())) { > > (Actually, even better to replace it with this): > > bool scrollInProgress = aFrame->IsProcessingAsyncScroll() > || (aFrame->LastScrollOrigin() && aFrame->LastScrollOrigin() != > nsGkAtoms::apz) > || aFrame->LastSmoothScrollOrigin(); > if (!scrollInProgress) { > > 3) in RecordFrameMetrics, replace this: > > if (lastSmoothScrollOrigin && lastSmoothScrollOrigin != nsGkAtoms::apz) { > > metrics.SetSmoothScrollOffsetUpdated(scrollableFrame- > >CurrentScrollGeneration()); > } > > with this: > > if (lastSmoothScrollOrigin) { > > metrics.SetSmoothScrollOffsetUpdated(scrollableFrame- > >CurrentScrollGeneration()); > } > > Does that make sense? If not let please let me know why that wouldn't work > or if I made a mistake thinking through this. I'll review the rest of the > patch as well in a sec. Thanks for your help. I see what you mean now, and will include your fix in the patch.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #35) > Comment on attachment 8478564 [details] [diff] [review] > Thanks again for your detailed review and descriptions. I will include your feedback in the patch. > ::: gfx/layers/apz/src/AsyncPanZoomController.cpp > @@ +749,5 @@ > > + > > + aFrameMetrics.ScrollBy(adjustedOffset); > > + > > + // The smooth scroll may have caused us to reach the end of our scroll range. > > + if (!IsZero(overscroll)) { > > In practice we should never actually hit this right? The MSD model slows > down to stop at the destination without overshooting, so we should never > enter overscroll, if I understand correctly. > > I don't mind leaving this code here just in case but wanted to check if my > understanding was right. If so please updated the comment just above the if > statement. > I expect this to happen very rarely; however, it can occur in two scenarios: - The layout.css.scroll-behavior.damping-ratio preference is changed to be less than 1, resulting in a under-damped MSD model for the smooth scrolls. - A smooth scroll inherits velocity from a fling gesture and the smooth scroll target position is near the edge of the page. (Critically damped MSD's only guarantee that they will not overshoot if the system starts at zero velocity)

(In reply to :kip (Kearwood Gilbert) from comment #37) > I expect this to happen very rarely; however, it can occur in two scenarios: > > - The layout.css.scroll-behavior.damping-ratio preference is changed to be > less than 1, resulting in a under-damped MSD model for the smooth scrolls. > > - A smooth scroll inherits velocity from a fling gesture and the smooth > scroll target position is near the edge of the page. (Critically damped > MSD's only guarantee that they will not overshoot if the system starts at > zero velocity) That makes sense, thanks for the explanation. It would be great to just have a quick one-sentence comment in the code at that point saying there are scenarios where the smooth scroll can go into overscroll.

Comment on attachment 8478564 [details] [diff] [review] Bug 1022825 - Implement Asynchronous Smooth Scrolling on Compositor Thread (V7 Patch) >+ >+ if (smoothScrollRequested) { >+ // A smooth scroll has been requested for animation on the compositor Oh, one other thing: please add an APZC_LOG here saying that a smooth scroll was requested and with the destination of the scroll.

- Extended nsIScrollableFrame and nsGfxScrollFrame to return destination of smooth scrolls which are to be animated on the compositor thread. - Added apz.smooth_scroll_repaint_interval preference. - Implemented AsyncPanZoomController::PanZoomState::SMOOTH_MSD_SCROLL state and AsyncPanZoomController::SmoothScrollAnimation class to animate smooth scroll animations on the compositor thread. - Extended FrameMetrics to report requests for smooth scrolls to be animated on the compositor thread and their corresponding destination positions. - AsyncPanZoomController now checks FrameMetrics for requests to perform smooth scrolling on the compositor thread. It will ensure that they are cancelled as needed by mousewheel, touchpanel, keyboard, and CSSOM-View instant scrolling DOM methods. - The layout/generic/test/test_scroll_behavior.html mochitest is no longer disabled on e10s and b2g, as 1022825 adds support for these platforms. V8 Patch: - Updated with changes from review in Comment 32 to Comment 39 - Implemented HandleSmoothScrollOverscroll() in AsyncPanZoomController.cpp to ensure that the BuildOverscrollHandoffChain() call does not deadlock on acquisition of the tree lock. (BuildOverScrollHandoffChain() was added as part of Scroll Grabbing in Bug 912666)

- Extended nsIScrollableFrame and nsGfxScrollFrame to return destination of smooth scrolls which are to be animated on the compositor thread. - Added apz.smooth_scroll_repaint_interval preference. - Implemented AsyncPanZoomController::PanZoomState::SMOOTH_MSD_SCROLL state and AsyncPanZoomController::SmoothScrollAnimation class to animate smooth scroll animations on the compositor thread. - Extended FrameMetrics to report requests for smooth scrolls to be animated on the compositor thread and their corresponding destination positions. - AsyncPanZoomController now checks FrameMetrics for requests to perform smooth scrolling on the compositor thread. It will ensure that they are cancelled as needed by mousewheel, touchpanel, keyboard, and CSSOM-View instant scrolling DOM methods. - The layout/generic/test/test_scroll_behavior.html mochitest is no longer disabled on e10s and b2g, as 1022825 adds support for these platforms. V8 Patch: - Updated with changes from review in Comment 32 to Comment 39 - Implemented HandleSmoothScrollOverscroll() in AsyncPanZoomController.cpp to ensure that the BuildOverscrollHandoffChain() call does not deadlock on acquisition of the tree lock. (BuildOverScrollHandoffChain() was added as part of Scroll Grabbing in Bug 912666) V9 Patch: - Un-bitrotted

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=582f71083988

Comment on attachment 8480827 [details] [diff] [review] Bug 1022825 - Implement Asynchronous Smooth Scrolling on Compositor Thread (V9 Patch) Review of attachment 8480827 [details] [diff] [review]: ----------------------------------------------------------------- Looks good; just a few style/docs nits. Feel free to carry the r+ and land if the try run is clean. ::: gfx/layers/LayersLogging.cpp @@ +123,5 @@ > AppendToString(aStream, m.GetScrollOffset(), " s="); > + if (m.GetDoSmoothScroll()) { > + AppendToString(aStream, m.GetSmoothScrollOffset(), " ss="); > + } > + nit: Remove blank line ::: gfx/layers/apz/src/AsyncPanZoomController.cpp @@ +691,5 @@ > + aSpringConstant, aDampingRatio) > + , mYAxisModel(aInitialPosition.y, aDestination.y, aInitialVelocity.y, > + aSpringConstant, aDampingRatio) > + { > + } nit: indent the braces to two spaces here, not three. @@ +2692,5 @@ > + APZC_LOG("%p smooth scrolling from (%f, %f) to (%f, %f)\n", this, > + mFrameMetrics.GetScrollOffset().x.value, > + mFrameMetrics.GetScrollOffset().y.value, > + aLayerMetrics.GetSmoothScrollOffset().x.value, > + GetSmoothScrollOffset.GetSmoothScrollOffset().y.value); There's been a bit of churn with the Coord class lately. On inbound right now it's backed out, so using x.value and y.value here will fail to compile. Also, the last line above says "GetSmoothScrollOffset.GetSmoothScrollOffset() which is wrong anyway. To make this code more robust so that it works with or without the Coord class change, you can do this: APZC_LOG("%p smooth scrolling from %s to %s\n", this, Stringify(mFrameMetrics.GetScrollOffset()).c_str(), Stringify(aLayerMetrics.GetSmoothScrollOffset()).c_str()); ::: layout/base/nsDisplayList.cpp @@ +709,5 @@ > + nsIAtom* lastSmoothScrollOrigin = scrollableFrame->LastSmoothScrollOrigin(); > + if (lastSmoothScrollOrigin) { > + metrics.SetSmoothScrollOffsetUpdated(scrollableFrame->CurrentScrollGeneration()); > + } > + nit: Remove blank line at the bottom here ::: layout/generic/nsIScrollableFrame.h @@ +326,4 @@ > */ > + virtual nsIAtom* LastScrollOrigin() = 0; > + /** > + * Returns the origin passed in to the last ScrollToImpl call that took Avoid referencing implementation details like ScrollToImpl in the interface documentation. And anyway this is wrong now as ScrollToImpl sets this to null. I'd change this sentence to "Returns the origin that triggered the last smooth scroll" or something similar. @@ +340,5 @@ > + * increments every time the scroll position is set. This tracks the > + * acknowledgement of the smooth scroll instantiation on the compositor > + * thread. If the smooth scroll has been stomped by an instant scroll > + * before the frame metrics have been replicated to the compositor, this > + * is set to nullptr to clear the smooth scroll. This documentation is very smooth-scroll specific but the field is used for instant scrolls as well. Also I'm not sure what "instantiation" here means. @@ +345,5 @@ > */ > virtual uint32_t CurrentScrollGeneration() = 0; > /** > + * When LastScrollDestination returns the destination of the most recently > + * requested smooth scroll animation. s/When // ::: modules/libpref/init/all.js @@ +476,5 @@ > pref("apz.x_skate_size_multiplier", "1.5"); > pref("apz.y_skate_size_multiplier", "2.5"); > #endif > > + nit: remove blank line added here

(In reply to :kip (Kearwood Gilbert) from comment #42) > Pushed to try: > > https://tbpl.mozilla.org/?tree=Try&rev=582f71083988 The layout/generic/test/test_scroll_behavior.html mochitest which was enabled for APZ by this patch failed due to limitations of reading the asynchronous animating scroll position from the main thread while the refresh driver is under test control. Bug 1062609 has been added to keep track of the issues that prevent this from being tested with APZ enabled.

- Extended nsIScrollableFrame and nsGfxScrollFrame to return destination of smooth scrolls which are to be animated on the compositor thread. - Added apz.smooth_scroll_repaint_interval preference. - Implemented AsyncPanZoomController::PanZoomState::SMOOTH_MSD_SCROLL state and AsyncPanZoomController::SmoothScrollAnimation class to animate smooth scroll animations on the compositor thread. - Extended FrameMetrics to report requests for smooth scrolls to be animated on the compositor thread and their corresponding destination positions. - AsyncPanZoomController now checks FrameMetrics for requests to perform smooth scrolling on the compositor thread. It will ensure that they are cancelled as needed by mousewheel, touchpanel, keyboard, and CSSOM-View instant scrolling DOM methods. V10 Patch: - The layout/generic/test/test_scroll_behavior.html mochitest has been commented as depending on Bug 1062609 before being enabled for APZ.

> V10 Patch: > - The layout/generic/test/test_scroll_behavior.html mochitest has been > commented as depending on Bug 1062609 before being enabled for APZ. V10 Patch also included the changes in Comment 43 and has been un-bitrotted.

(In reply to :kip (Kearwood Gilbert) from comment #44) > (In reply to :kip (Kearwood Gilbert) from comment #42) > > Pushed to try: > > > > https://tbpl.mozilla.org/?tree=Try&rev=582f71083988 > The layout/generic/test/test_scroll_behavior.html mochitest which was > enabled for APZ by this patch failed due to limitations of reading the > asynchronous animating scroll position from the main thread while the > refresh driver is under test control. > > Bug 1062609 has been added to keep track of the issues that prevent this > from being tested with APZ enabled. Should Bug 1062609 or another workaround be required before landing this?

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=25253a9b9ffa

Nope, feel free to land this first and then sort out the test issues.

- Extended nsIScrollableFrame and nsGfxScrollFrame to return destination of smooth scrolls which are to be animated on the compositor thread. - Added apz.smooth_scroll_repaint_interval preference. - Implemented AsyncPanZoomController::PanZoomState::SMOOTH_MSD_SCROLL state and AsyncPanZoomController::SmoothScrollAnimation class to animate smooth scroll animations on the compositor thread. - Extended FrameMetrics to report requests for smooth scrolls to be animated on the compositor thread and their corresponding destination positions. - AsyncPanZoomController now checks FrameMetrics for requests to perform smooth scrolling on the compositor thread. It will ensure that they are cancelled as needed by mousewheel, touchpanel, keyboard, and CSSOM-View instant scrolling DOM methods. - The layout/generic/test/test_scroll_behavior.html mochitest has been commented as depending on Bug 1062609 before being enabled for APZ. V11 Patch: - Un-bitrotted

[Blocking Requested - why for this release]: It appears that bug 1033468 has recently received blocking status, so requesting blocking status here as well.

Triage: Too risky to land on 2.1. Remove nomination from 2.1.