Bug 135768 – Implement scroll snapping animations on Mac

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Wenson Hsieh]

Implement scroll snapping logic for use on Mac. Since there is no simple way to retarget the scroll destination on Mac (as opposed to iOS) we'll need to override momentum events and replace the animation curve with a custom one. This patch adds logic needed to compute the snap scrolling animation curve given parameters such as initial velocity and the target snap offset. Note: this doesn't actually make any elements snap -- it's just the methods needed to compute and animate offsets.

[Wenson Hsieh]

Created attachment 236352 [details]
Patch

[Wenson Hsieh]

Created attachment 236353 [details]
Patch

[Wenson Hsieh]

Created attachment 236407 [details]
Patch

[Wenson Hsieh]

Created attachment 236408 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 236408 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=236408&action=review

r- for lack of changelog. I would also like to see a fairly high-level comment somewhere that explains the states, and briefly describes how the math works.

> Source/WebCore/platform/mac/AxisScrollSnapAnimator.h:77
> +    float snapAmount() const;
> +    float glideAmount() const;

"Amount" is vague here. Maybe "distance"?

> Source/WebCore/platform/mac/AxisScrollSnapAnimator.h:80
> +    bool pushInitialMomentum(float);

What is the parameter? Velocity?

> Source/WebCore/platform/mac/AxisScrollSnapAnimator.h:81
> +    float averageInitialMomentum() const;

Again, what are the units?

> Source/WebCore/platform/mac/AxisScrollSnapAnimator.h:92
> +    static const int momentumWindowSize = 3;
> +    static const int snapMagnitudeMax;
> +    static const int snapMagnitudeMin;
> +    static const int snapThresholdHigh;
> +    static const int snapThresholdLow;
> +    static const float glideBoostMultiplier;
> +    static const float maxTargetVelocity;
> +    static const float minTargetVelocity;
> +    static const float initialToFinalMomentumFactor;

Just have these in the cpp file as non-class consts.

> Source/WebCore/platform/mac/AxisScrollSnapAnimator.h:103
> +    int m_momentumCount;

event count?

> Source/WebCore/platform/mac/AxisScrollSnapAnimator.mm:54
> +        finalVelocity = ((targetMultiplier * initialMomentum) / 2) * (1 + cos(piFloat - acos((2 - targetMultiplier) / targetMultiplier)));

You need to explain this in a comment.

> Source/WebCore/platform/mac/AxisScrollSnapAnimator.mm:61
> +        else if (finalVelocity > targetFinalVelocity + 0.5)

No blank line before else.

> Source/WebCore/platform/mac/AxisScrollSnapAnimator.mm:114
> +template<typename T> inline void clampToBounds(T& value, T min, T max)
> +{
> +    if (value < min)
> +        value = min;
> +
> +    if (value > max)
> +        value = max;
> +}

We normally use std::max(std::min(a, b), c) for this.

> Source/WebCore/platform/mac/AxisScrollSnapAnimator.mm:182
> +void AxisScrollSnapAnimator::scrollUpdate()

Name is vague. Are you updating a scroll, or are you updating as the result of a scroll?

> Source/WebCore/platform/mac/AxisScrollSnapAnimator.mm:241
> +    float rawSnapAmount = 1 + magnitude * sin(piFloat * progress);

Comment?

> Source/WebCore/platform/mac/AxisScrollSnapAnimator.mm:256
> +    float shift = ceil(((m_glideMultiplier * m_glideInitialMomentum) / 2) * (1 + cos(piFloat * progress - m_glidePhaseShift)));

Comment!

[Wenson Hsieh]

Comment on attachment 236408 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=236408&action=review

Thanks for taking a look at this! There will be a more polished version of this up soon.

>> Source/WebCore/platform/mac/AxisScrollSnapAnimator.h:77
>> +    float glideAmount() const;
> 
> "Amount" is vague here. Maybe "distance"?

Changed to compute(Snap|Glide)Delta.

>> Source/WebCore/platform/mac/AxisScrollSnapAnimator.h:80
>> +    bool pushInitialMomentum(float);
> 
> What is the parameter? Velocity?

Changed names from "Momentum" to "WheelDelta" to better reflect the fact that I'm tracking wheel deltas over time.

>> Source/WebCore/platform/mac/AxisScrollSnapAnimator.h:81
>> +    float averageInitialMomentum() const;
> 
> Again, what are the units?

Changed names from "Momentum" to "WheelDelta" to better reflect the fact that I'm tracking wheel deltas over time.

>> Source/WebCore/platform/mac/AxisScrollSnapAnimator.h:92
>> +    static const float initialToFinalMomentumFactor;
> 
> Just have these in the cpp file as non-class consts.

Fixed. (although momentumWindowSize is still declared in the header, since I'll need it to initialize the momentum window).

>> Source/WebCore/platform/mac/AxisScrollSnapAnimator.h:103
>> +    int m_momentumCount;
> 
> event count?

Changed to m_numWheelDeltasTracked.

>> Source/WebCore/platform/mac/AxisScrollSnapAnimator.mm:54
>> +        finalVelocity = ((targetMultiplier * initialMomentum) / 2) * (1 + cos(piFloat - acos((2 - targetMultiplier) / targetMultiplier)));
> 
> You need to explain this in a comment.

Got it.

>> Source/WebCore/platform/mac/AxisScrollSnapAnimator.mm:61
>> +        else if (finalVelocity > targetFinalVelocity + 0.5)
> 
> No blank line before else.

Fixed.

>> Source/WebCore/platform/mac/AxisScrollSnapAnimator.mm:182
>> +void AxisScrollSnapAnimator::scrollUpdate()
> 
> Name is vague. Are you updating a scroll, or are you updating as the result of a scroll?

Changed to scrollSnapAnimationUpdate()

>> Source/WebCore/platform/mac/AxisScrollSnapAnimator.mm:241
>> +    float rawSnapAmount = 1 + magnitude * sin(piFloat * progress);
> 
> Comment?

Will do. I'm also putting this information in the (soon to come) ChangeLog.

>> Source/WebCore/platform/mac/AxisScrollSnapAnimator.mm:256
>> +    float shift = ceil(((m_glideMultiplier * m_glideInitialMomentum) / 2) * (1 + cos(piFloat * progress - m_glidePhaseShift)));
> 
> Comment!

Adding comments here too.

[Wenson Hsieh]

Created attachment 236492 [details]
Patch

[Dean Jackson]

Comment on attachment 236492 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=236492&action=review

> Source/WebCore/page/scrolling/AxisScrollSnapOffsets.cpp:154
> +    if (scrollDestination >= snapOffsets.at(snapOffsets.size() - 1))
> +        return snapOffsets.at(snapOffsets.size() - 1);
> +
> +    size_t lowerIndex = 0;
> +    size_t upperIndex = snapOffsets.size() - 1;

If you move upperIndex to before the if, then you can use it there.

> Source/WebCore/platform/mac/AxisScrollSnapAnimator.mm:79
> +    return 16.6789 * initialWheelDelta;

Where does this come from?

[Wenson Hsieh]

Comment on attachment 236492 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=236492&action=review

>> Source/WebCore/page/scrolling/AxisScrollSnapOffsets.cpp:154
>> +    size_t upperIndex = snapOffsets.size() - 1;
> 
> If you move upperIndex to before the if, then you can use it there.

Good catch, thanks!

>> Source/WebCore/platform/mac/AxisScrollSnapAnimator.mm:79
>> +    return 16.6789 * initialWheelDelta;
> 
> Where does this come from?

Good point -- I'll add this as a named constant and leave a comment.

[Wenson Hsieh]

Created attachment 236573 [details]
Patch

[Tim Horton]

Comment on attachment 236573 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=236573&action=review

I gave up when I hit the math.

> Source/WebCore/ChangeLog:14
> +        final velocity matches a lower final velocity that is somewhat arbitrarily computed. (See the FIXME in

Isn't the "final" velocity 0 by definition? Maybe this will make sense later.

> Source/WebCore/PlatformMac.cmake:134
> +    platform/mac/AxisScrollSnapAnimator.mm

I think this file is sorted.

> Source/WebCore/page/scrolling/AxisScrollSnapOffsets.cpp:171
> +    LayoutUnit lowerSnapPosition = snapOffsets.at(lowerIndex);

We have operator[]; why .at()?

> Source/WebCore/platform/mac/AxisScrollSnapAnimator.h:90
> +    ScrollSnapState m_curState;

"cur" :( Spell it out!

[Wenson Hsieh]

Comment on attachment 236573 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=236573&action=review

>> Source/WebCore/PlatformMac.cmake:134
>> +    platform/mac/AxisScrollSnapAnimator.mm
> 
> I think this file is sorted.

Fixed.

>> Source/WebCore/page/scrolling/AxisScrollSnapOffsets.cpp:171
>> +    LayoutUnit lowerSnapPosition = snapOffsets.at(lowerIndex);
> 
> We have operator[]; why .at()?

Thanks for the tip -- changed to use [].

>> Source/WebCore/platform/mac/AxisScrollSnapAnimator.h:90
>> +    ScrollSnapState m_curState;
> 
> "cur" :( Spell it out!

Fixed.

[Wenson Hsieh]

Comment on attachment 236573 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=236573&action=review

>> Source/WebCore/ChangeLog:14
>> +        final velocity matches a lower final velocity that is somewhat arbitrarily computed. (See the FIXME in
> 
> Isn't the "final" velocity 0 by definition? Maybe this will make sense later.

I originally had that (well, a final velocity of 1) but it felt too slow at the end of the animation, so I changed it to some number between 3.5 and 7 (that seemed to give me decent results). The faster the initial velocity, the weirder it seemed to look with a very low final velocity.

[Wenson Hsieh]

Created attachment 236596 [details]
Patch

[Wenson Hsieh]

Comment on attachment 236596 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=236596&action=review

> Source/WebCore/page/scrolling/AxisScrollSnapOffsets.h:46
> +LayoutUnit closestSnapOffset(const Vector<LayoutUnit>& snapOffsets, LayoutUnit scrollDestination, float velocity)

typo: LayoutUnits -> T

[Wenson Hsieh]

Created attachment 236597 [details]
Patch

[Wenson Hsieh]

Comment on attachment 236597 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=236597&action=review

> Source/WebCore/platform/mac/AxisScrollSnapAnimator.h:60
> +    virtual void immediateScrollInAxis(ScrollEventAxis, float velocity) = 0;

>.< oops, this the second argument is supposed to be "float delta" here.

[Wenson Hsieh]

Comment on attachment 236597 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=236597&action=review

>> Source/WebCore/platform/mac/AxisScrollSnapAnimator.h:60
>> +    virtual void immediateScrollInAxis(ScrollEventAxis, float velocity) = 0;
> 
> 

^ oops, the second argument is supposed to be float delta here.

[Dean Jackson]

Comment on attachment 236597 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=236597&action=review

> Source/WebCore/ChangeLog:18
> +        Implementing the scroll snap animations required for snapping in both overflow and mainframe areas on Mac. Since we receive a series
> +        of discrete wheel events, we need to predict the distance we need to traverse, compute the appropriate snap point, and then compute
> +        an animation curve to that location. The snap animations are split into two types: snapping, which handles letting go of the trackpad
> +        with zero velocity, and gliding, which handles flick gestures. In both cases, sinusoidal curves are used to ease animation to the target
> +        location. In the former case, the initial velocity is low, and increases to a maximum value during the middle of the animation before
> +        decreasing to 0. In the latter case, the curve is computed such that the initial velocity matches the user's scroll velocity, and the
> +        final velocity matches a lower final velocity that is somewhat arbitrarily computed. (See the FIXME in
> +        AxisScrollSnapOffsets::initializeGlideParameters). How the equations and constants were chosen is described in greater detail in the
> +        comments above AxisScrollSnapOffsets::computeGlideDelta and AxisScrollSnapOffsets::computeSnapDelta. Note that the final velocity should
> +        ideally be equal to 0. However, with this particular curve, this caused the animation to feel too slow near the snap point.
> +

Could you wrap this to a smaller column width?

> Source/WebCore/platform/mac/AxisScrollSnapAnimator.mm:33
> +const float inertialScrollPredictionFactor = 16.6789;

Does this really need to have so many significant digits? It makes it look like it is some scientific value we're not allowed to change. Would 16.7 be good enough?

[Wenson Hsieh]

Created attachment 236630 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 236630 [details]
Patch

Clearing flags on attachment: 236630

Committed r172616: <http://trac.webkit.org/changeset/172616>