Implement CSSOM-View smooth scrolling movement for platforms that do not support APZ

RESOLVED FIXED in mozilla34

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: kip, Assigned: kip)

Tracking

(Blocks 1 bug)

Trunk
mozilla34
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

()

Attachments

(4 attachments, 11 obsolete attachments)

30.16 KB, patch
Details | Diff | Splinter Review
7.26 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
16.03 KB, patch
Details | Diff | Splinter Review
4.17 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
To support Bug 1022818 and Bug 1010538, the CSSOM-View smooth scrolling behavior movement model will be implemented on the main thread for platforms that do no support APZ (possibly within AsyncScroll.cpp).  Bug 1022825 tracks implementation for platforms that support APZ.

Details on the smooth scrolling movement model is described in Bug 1010538.
- Implemented the AxisPhysicsModel class, which encapsulates interpolation and
  integration of a 1-dimensional physics model in a frame-rate independent
  and stable manner.
- Implemented the AxisPhysicsMSDModel class, which models a generic
  1-dimensional Mass-Spring-Damper simulation.
- This physical movement simulation code has been implemented separately from
  the existing momentum code in Axis.cpp so that it can be utilized by
  both the compositor (AsyncPanZoomController) and main thread
  (nsGfxScrollFrame).
Comment on attachment 8451076 [details] [diff] [review]
Bug 1026023 - Part 1: Create Preferences

- Added layout.css.scroll-behavior.enabled preference to enable and disable
  scroll behavior and related smooth scrolling.
- Added layout.css.scroll-behavior.spring-constant preference to enable tuning
  of the smooth scroll motion.  spring-constant controls the strength of the
  simulated MSD (Mass-Spring-Damper).
- Added layout.css.scroll-behavior.damping-ratio preference to enable tuning
  of the smooth scroll motion.  damping-ratio controls the under, over, or
  critically damped behavior of the simulated MSD.
- Added nsIScrollableFrame::ScrollMode::SMOOTH_MSD to differentiate
  existing smooth scrolls used by keyboard and mousewheel events from the
  CSSOM-View scroll-behavior's MSD motion scrolling.
- Implemented ScrollFrameHelper::AsyncSmoothMSDScroll, which takes the role
  of ScrollFrameHelper::AsyncScroll when SMOOTH_MSD scrolls are requested.
- Implemented glue code to handle callbacks from AsyncSmoothMSDScroll and
  to hand off velocity between the classes when one scroll animation is
  interrupted by another.


Please note: These patches will not have any end-user effect on their own.  They provide functionality used by Bug 1022818, Bug 1022825, and Bug 1010538.
Blocks: 1022825
- Implemented the AxisPhysicsModel class, which encapsulates interpolation and
integration of a 1-dimensional physics model in a frame-rate independent
and stable manner.
- Implemented the AxisPhysicsMSDModel class, which models a generic
1-dimensional Mass-Spring-Damper simulation.
- This physical movement simulation code has been implemented separately from
the existing momentum code in Axis.cpp so that it can be utilized by
both the compositor (AsyncPanZoomController) and main thread
(nsGfxScrollFrame).

V2 Patch: Adjusted spacing and wrapping
Attachment #8451077 - Attachment is obsolete: true
Added nsIScrollableFrame::ScrollMode::SMOOTH_MSD to differentiate
existing smooth scrolls used by keyboard and mousewheel events from the
CSSOM-View scroll-behavior's MSD motion scrolling.
- Implemented ScrollFrameHelper::AsyncSmoothMSDScroll, which takes the role
of ScrollFrameHelper::AsyncScroll when SMOOTH_MSD scrolls are requested.
- Implemented glue code to handle callbacks from AsyncSmoothMSDScroll and
to hand off velocity between the classes when one scroll animation is
interrupted by another.

V2 Patch: Adjusted spacing and wrapping
Attachment #8451080 - Attachment is obsolete: true
I am looking for someone with the capacity to review these patches as well as the patches in Bug 1022818.  Would you have some extra cycles?
Flags: needinfo?(matt.woodrow)
Flags: needinfo?(m)
Flags: needinfo?(bugmail.mozilla)
I can help review, but I'm not a peer for layout/dom code so my review alone won't be enough.
Flags: needinfo?(bugmail.mozilla)
I have no experience with Gecko/DOM code, so I unfortunately can't help.
Flags: needinfo?(m)
I can do the gfx/ and layout/ parts, as could roc.
Flags: needinfo?(matt.woodrow)
- Added layout.css.scroll-behavior.enabled preference to enable and disable
scroll behavior and related smooth scrolling.
- Added layout.css.scroll-behavior.spring-constant preference to enable tuning
of the smooth scroll motion. spring-constant controls the strength of the
simulated MSD (Mass-Spring-Damper).
- Added layout.css.scroll-behavior.damping-ratio preference to enable tuning
of the smooth scroll motion. damping-ratio controls the under, over, or
critically damped behavior of the simulated MSD.

V2 Patch: Un-bitrotted
Attachment #8451076 - Attachment is obsolete: true
- Added nsIScrollableFrame::ScrollMode::SMOOTH_MSD to differentiate
existing smooth scrolls used by keyboard and mousewheel events from the
CSSOM-View scroll-behavior's MSD motion scrolling.
- Implemented ScrollFrameHelper::AsyncSmoothMSDScroll, which takes the role
of ScrollFrameHelper::AsyncScroll when SMOOTH_MSD scrolls are requested.
- Implemented glue code to handle callbacks from AsyncSmoothMSDScroll and
to hand off velocity between the classes when one scroll animation is
interrupted by another.

V3 Patch: Smooth MSD scrolling is now cancelled by mouse wheel and touchpad events, without jumping to MSD scroll destination.
Attachment #8451957 - Attachment is obsolete: true
I have updated the patch so that smooth MSD scrolling is now cancelled by mouse wheel and touchpad events, without jumping to MSD scroll destination.  This conforms with the CSSOM-View scroll-behavior spec and removes the sudden jump when it was interrupted by mouse-wheel events.

One side-effect of this is that OSX touchpad fling gestures will continuously cancel smooth scrolls while momentum is in effect.  This has the effect that "scroll to top" buttons and the like will be non-responsive until the fling-ed content comes to a stop.

Would it make sense to cancel (or ignore) the rest of a fling event when a smooth scroll is started?

Also, I noticed that when the system is set to underdamped or you rapidly change scroll directions, the overshoot is not visible due to the clamping occurring in nsGFXScrollFrame.  I'll add a "part4" patch to cover that scenario and enable us to explore the effect.
Summary: Implement CSSOM-View smooth scrolling movement for platforms that do no support APZ → Implement CSSOM-View smooth scrolling movement for platforms that do not support APZ
- Implemented the AxisPhysicsModel class, which encapsulates interpolation and
integration of a 1-dimensional physics model in a frame-rate independent
and stable manner.
- Implemented the AxisPhysicsMSDModel class, which models a generic
1-dimensional Mass-Spring-Damper simulation.
- This physical movement simulation code has been implemented separately from
the existing momentum code in Axis.cpp so that it can be utilized by
both the compositor (AsyncPanZoomController) and main thread
(nsGfxScrollFrame).

V3 Patch: Fixed type-o in AxisPhysicsModel::SetPosition method name.
Attachment #8451954 - Attachment is obsolete: true
- Added nsIScrollableFrame::ScrollMode::SMOOTH_MSD to differentiate
existing smooth scrolls used by keyboard and mousewheel events from the
CSSOM-View scroll-behavior's MSD motion scrolling.
- Implemented ScrollFrameHelper::AsyncSmoothMSDScroll, which takes the role
of ScrollFrameHelper::AsyncScroll when SMOOTH_MSD scrolls are requested.
- Implemented glue code to handle callbacks from AsyncSmoothMSDScroll and
to hand off velocity between the classes when one scroll animation is
interrupted by another.

V4 Patch:
- Intermediate animation frames are now able to overshoot the destination for under-damped MSD configurations and when changing destinations rapidly.
- If overshooting the destination and hitting the extents of the allowed scroll range for a frame (i.e. no overscroll allowed), the velocity of the MSD will be zero'ed out.  This effectively absorbs the energy of the impact and prevents bounce-back.
- Corrected indentation
Attachment #8452441 - Attachment is obsolete: true
(In reply to :kip (Kearwood Gilbert) from comment #13)
> Also, I noticed that when the system is set to underdamped or you rapidly
> change scroll directions, the overshoot is not visible due to the clamping
> occurring in nsGFXScrollFrame.  I'll add a "part4" patch to cover that
> scenario and enable us to explore the effect.

I have incorporated the overshoot changes into an updated "part3" patch, V4:
- Intermediate animation frames are now able to overshoot the destination for under-damped MSD configurations and when changing destinations rapidly.
- If overshooting the destination and hitting the extents of the allowed scroll range for a frame (i.e. no overscroll allowed), the velocity of the MSD will be zero'ed out.  This effectively absorbs the energy of the impact and prevents bounce-back.
(In reply to Matt Woodrow (:mattwoodrow) from comment #10)
> I can do the gfx/ and layout/ parts, as could roc.

Thanks Matt,

I have assigned the patches you for review.  Please advise if any should be sent to someone else.
Attachment #8452437 - Flags: review?(matt.woodrow)
Attachment #8452570 - Flags: review?(matt.woodrow)
Attachment #8452574 - Flags: review?(matt.woodrow)
Comment on attachment 8452437 [details] [diff] [review]
Bug 1026023 - Part 1: Create Preferences (V2 Patch)

Review of attachment 8452437 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsLayoutUtils.cpp
@@ +545,5 @@
> +                                 "layout.css.scroll-behavior.enabled",
> +                                 false);
> +  }
> +
> +  return sCSSScrollBehaviorEnabled;

Unrelated to this bug, but it would be nice if we could copy gfxPrefs to avoid needing all this boilerplate for each new layout pref.
Attachment #8452437 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8452570 [details] [diff] [review]
Bug 1026023 - Part 2: Implement Physically Based Movement Model (V3 Patch)

Review of attachment 8452570 [details] [diff] [review]:
-----------------------------------------------------------------

Awesome!

::: gfx/layers/AxisPhysicsMSDModel.cpp
@@ +58,5 @@
> +  // kFinishDistance while the speed is simultaneously less than
> +  // kFinishVelocity.  This enables an under-damped system to overshoot the
> +  // destination when desired without prematurely triggering the finished state.
> +  const double kFinishDistance = 30.0;
> +  const double kFinishVelocity = 60.0;

Comment about how these values were chosen in case someone wants to modify them in the future.

::: gfx/layers/AxisPhysicsMSDModel.h
@@ +16,5 @@
> + * AxisPhysicsModel encapsulates a 1-dimensional MSD (Mass-Spring-Damper) model.
> + */
> +class AxisPhysicsMSDModel : public AxisPhysicsModel {
> +public:
> +  AxisPhysicsMSDModel(double aPos, double aVelocity, double aSpringConstant,

I'd prefer aDestination and aInitialVelocity. Or a comment explaining the parameters.

::: gfx/layers/AxisPhysicsModel.cpp
@@ +8,5 @@
> +
> +namespace mozilla {
> +namespace layers {
> +
> +const double AxisPhysicsModel::kFixedTimestep = 1.0 / 120.0; // 120hz

Any specific reason to choose 120hz here? We use 60hz for our refresh rate. Please add a comment explaining why we use this rate, even if it was chosen fairly arbitrarily.

@@ +34,5 @@
> +
> +double
> +AxisPhysicsModel::GetPosition()
> +{
> +  return mPrevState.p * (1.0 - mProgress) + mNextState.p * mProgress;

Might be nice to add a static LinearInterpolate/lerp helper and implement GetVelocity/GetPosition on top of that. Just to make it super clear why we're doing this.

::: gfx/layers/moz.build
@@ +112,5 @@
>      'apz/util/ActiveElementManager.h',
>      'apz/util/APZCCallbackHelper.h',
>      'AtomicRefCountedWithFinalize.h',
> +    'AxisPhysicsModel.h',
> +    'AxisPhysicsMSDModel.h',

You shouldn't need to export these headers twice. Putting them under EXPORTS copies them to <objdir>/dist/include so you can include them via #include <header.h>, putting them under EXPORTS.mozilla.layers copies them to <objdir>/dist/include/mozilla/layers so you need to #include <mozilla/layers/header.h> (or #include "header.h" when in the same directory obviously).

Since your classes are within the mozilla/layers namespace then EXPORTS.mozilla.layers makes the most sense I think.
Attachment #8452570 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8452570 [details] [diff] [review]
Bug 1026023 - Part 2: Implement Physically Based Movement Model (V3 Patch)

Review of attachment 8452570 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/AxisPhysicsMSDModel.h
@@ +16,5 @@
> + * AxisPhysicsModel encapsulates a 1-dimensional MSD (Mass-Spring-Damper) model.
> + */
> +class AxisPhysicsMSDModel : public AxisPhysicsModel {
> +public:
> +  AxisPhysicsMSDModel(double aPos, double aVelocity, double aSpringConstant,

Also note that we're assuming a unit mass for this class.
Comment on attachment 8452574 [details] [diff] [review]
Bug 1026023 - Part 3: Integrate MSD movement with nsGfxScrollFrame (V4 Patch)

Review of attachment 8452574 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsGfxScrollFrame.cpp
@@ +1258,5 @@
> +    , mLastRefreshTime(aStartTime)
> +    , mCallee(nullptr)
> +  {}
> +
> +  NS_INLINE_DECL_REFCOUNTING(AsyncScroll)

AsyncSmoothMSDScroll

@@ +1758,5 @@
> +ScrollFrameHelper::AsyncSmoothMSDScrollCallback(void* anInstance,
> +                                                mozilla::TimeDuration aDeltaTime)
> +{
> +  ScrollFrameHelper* self = static_cast<ScrollFrameHelper*>(anInstance);
> +  if (!self || !self->mAsyncSmoothMSDScroll)

Is it actually possible to hit this? These seem like they should be assertions.

@@ +1919,5 @@
> +          Preferences::GetFloat("layout.css.scroll-behavior.spring-constant",
> +                                250.0));
> +        double dampingRatio = static_cast<double>(
> +          Preferences::GetFloat("layout.css.scroll-behavior.damping-ratio",
> +                                1.0));

Move these pref checks into the AsyncSmoothMSDScroll ctor, they're not all that relevant to this function.

@@ +1937,5 @@
> +          }
> +          // We are done scrolling, set our destination to wherever we actually
> +          // ended up scrolling to.
> +          mDestination = GetScrollPosition();
> +          return;

This block of code is repeated 3 times in this function now, probably justifies being moved into a helper.

::: layout/generic/nsGfxScrollFrame.h
@@ +173,5 @@
>    nsRect GetScrollRangeForClamping() const;
>  
>  public:
>    static void AsyncScrollCallback(void* anInstance, mozilla::TimeStamp aTime);
> +  static void AsyncSmoothMSDScrollCallback(void* anInstance,

Why is this parameter void*? Seems like we should be able to pass ScrollFrameHelper* without problems, or this could not be static.

::: layout/generic/nsIScrollableFrame.h
@@ +158,5 @@
>     * scrolling. SMOOTH will only be smooth if smooth scrolling is actually
>     * enabled. INSTANT is always synchronous, NORMAL can be asynchronous.
>     * If an INSTANT request happens while a smooth or async scroll is already in
>     * progress, the async scroll is interrupted and we instantly scroll to the
>     * destination.

Briefly define SMOOTH_MSD in this comment (and maybe the difference between it and SMOOTH).

@@ +181,5 @@
>     * aScrollPosition.x/y is different from the current CSS pixel position,
>     * makes sure we only move in the direction given by the difference.
>     * Ensures that GetScrollPositionCSSPixels (the scroll position after
>     * rounding to CSS pixels) will be exactly aScrollPosition.
>     * The scroll mode is INSTANT.

This last bit of the comment is wrong now :)
Attachment #8452574 - Flags: review?(matt.woodrow) → review+
- Added layout.css.scroll-behavior.enabled preference to enable and disable
  scroll behavior and related smooth scrolling.
- Added layout.css.scroll-behavior.spring-constant preference to enable tuning
  of the smooth scroll motion.  spring-constant controls the strength of the
  simulated MSD (Mass-Spring-Damper).
- Added layout.css.scroll-behavior.damping-ratio preference to enable tuning
  of the smooth scroll motion.  damping-ratio controls the under, over, or
  critically damped behavior of the simulated MSD.

V3 Patch: Updated to match code review in Comment 18 - Comment 21
Attachment #8452437 - Attachment is obsolete: true
- Implemented the AxisPhysicsModel class, which encapsulates interpolation and
  integration of a 1-dimensional physics model in a frame-rate independent
  and stable manner.
- Implemented the AxisPhysicsMSDModel class, which models a generic
  1-dimensional Mass-Spring-Damper simulation.
- This physical movement simulation code has been implemented separately from
  the existing momentum code in Axis.cpp so that it can be utilized by
  both the compositor (AsyncPanZoomController) and main thread
  (nsGfxScrollFrame).

V4 Patch: Updated to match code review in Comment 18 - Comment 21
Attachment #8452570 - Attachment is obsolete: true
- Added nsIScrollableFrame::ScrollMode::SMOOTH_MSD to differentiate
  existing smooth scrolls used by keyboard and mousewheel events from the
  CSSOM-View scroll-behavior's MSD motion scrolling.
- Implemented ScrollFrameHelper::AsyncSmoothMSDScroll, which takes the role
  of ScrollFrameHelper::AsyncScroll when SMOOTH_MSD scrolls are requested.
- Implemented glue code to handle callbacks from AsyncSmoothMSDScroll and
  to hand off velocity between the classes when one scroll animation is
  interrupted by another.

V5 Patch: Updated to match code review in Comment 18 - Comment 21
Attachment #8452574 - Attachment is obsolete: true
- When a smooth scroll is being processed on a frame, mouse wheel and trackpad
  momentum scroll event updates will no longer cancel the SMOOTH or SMOOTH_MSD
  scroll animations, enabling scripts that depend on them to be responsive
  without forcing the user to wait for the fling animations to completely stop.
Attachment #8456512 - Flags: review?(matt.woodrow)
Attachment #8456512 - Flags: review?(matt.woodrow) → review+
- Implemented the AxisPhysicsModel class, which encapsulates interpolation and
  integration of a 1-dimensional physics model in a frame-rate independent
  and stable manner.
- Implemented the AxisPhysicsMSDModel class, which models a generic
  1-dimensional Mass-Spring-Damper simulation.
- This physical movement simulation code has been implemented separately from
  the existing momentum code in Axis.cpp so that it can be utilized by
  both the compositor (AsyncPanZoomController) and main thread
  (nsGfxScrollFrame).

V5 Patch: Un-bitrotted
Attachment #8455713 - Attachment is obsolete: true
Blocks: 1041833
- Added layout.css.scroll-behavior.enabled preference to enable and disable
  scroll behavior and related smooth scrolling.
- Added layout.css.scroll-behavior.spring-constant preference to enable tuning
  of the smooth scroll motion.  spring-constant controls the strength of the
  simulated MSD (Mass-Spring-Damper).
- Added layout.css.scroll-behavior.damping-ratio preference to enable tuning
  of the smooth scroll motion.  damping-ratio controls the under, over, or
  critically damped behavior of the simulated MSD.

V4 Patch: Changed gfxPrefs from "Once" to "Live" so that mochitests can set the preferences.
Attachment #8455712 - Attachment is obsolete: true
Attachment #8463614 - Flags: review?(matt.woodrow)
Attachment #8463614 - Flags: review?(matt.woodrow) → review+
As this bug has no effect on its own, tests utilizing DOM API methods to access this functionality will be included in Bug 1022818.
Keywords: checkin-needed
(In reply to :kip (Kearwood Gilbert) from comment #29)
> Pushed to try:
> 
> https://tbpl.mozilla.org/?tree=Try&rev=a9fefa38e74a
Step 4 of this patchset was missed in this try push.  It has been pushed again:

https://tbpl.mozilla.org/?tree=Try&rev=2dd893edda2b
Blocks: 945584
See Also: → 1062609
You need to log in before you can comment on or make changes to this bug.