Bug 135768 - Implement scroll snapping animations on Mac
Summary: Implement scroll snapping animations on Mac
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh Unspecified
: P2 Enhancement
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 134283
  Show dependency treegraph
 
Reported: 2014-08-08 14:38 PDT by Wenson Hsieh
Modified: 2014-08-15 17:30 PDT (History)
19 users (show)

See Also:


Attachments
Patch (46.43 KB, patch)
2014-08-10 22:38 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Patch (24.51 KB, patch)
2014-08-10 23:26 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Patch (21.78 KB, patch)
2014-08-11 15:33 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Patch (21.76 KB, patch)
2014-08-11 15:37 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Patch (25.69 KB, patch)
2014-08-12 18:49 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Patch (29.75 KB, patch)
2014-08-13 19:03 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Patch (29.32 KB, patch)
2014-08-14 09:52 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Patch (29.43 KB, patch)
2014-08-14 09:56 PDT, Wenson Hsieh
dino: review+
Details | Formatted Diff | Diff
Patch (29.75 KB, patch)
2014-08-14 15:39 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Wenson Hsieh 2014-08-08 14:38:15 PDT
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.
Comment 1 Wenson Hsieh 2014-08-10 22:38:38 PDT
Created attachment 236352 [details]
Patch
Comment 2 Wenson Hsieh 2014-08-10 23:26:49 PDT
Created attachment 236353 [details]
Patch
Comment 3 Wenson Hsieh 2014-08-11 15:33:52 PDT
Created attachment 236407 [details]
Patch
Comment 4 Wenson Hsieh 2014-08-11 15:37:01 PDT
Created attachment 236408 [details]
Patch
Comment 5 Simon Fraser (smfr) 2014-08-11 16:47:34 PDT
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!
Comment 6 Wenson Hsieh 2014-08-12 10:09:44 PDT
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.
Comment 7 Wenson Hsieh 2014-08-12 18:49:55 PDT
Created attachment 236492 [details]
Patch
Comment 8 Dean Jackson 2014-08-13 18:12:00 PDT
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?
Comment 9 Wenson Hsieh 2014-08-13 18:15:25 PDT
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.
Comment 10 Wenson Hsieh 2014-08-13 19:03:03 PDT
Created attachment 236573 [details]
Patch
Comment 11 Tim Horton 2014-08-13 21:50:56 PDT
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!
Comment 12 Wenson Hsieh 2014-08-14 09:21:21 PDT
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.
Comment 13 Wenson Hsieh 2014-08-14 09:24:32 PDT
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.
Comment 14 Wenson Hsieh 2014-08-14 09:52:20 PDT
Created attachment 236596 [details]
Patch
Comment 15 Wenson Hsieh 2014-08-14 09:53:22 PDT
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
Comment 16 Wenson Hsieh 2014-08-14 09:56:14 PDT
Created attachment 236597 [details]
Patch
Comment 17 Wenson Hsieh 2014-08-14 10:36:17 PDT
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.
Comment 18 Wenson Hsieh 2014-08-14 10:37:18 PDT
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.
Comment 19 Dean Jackson 2014-08-14 12:40:00 PDT
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?
Comment 20 Wenson Hsieh 2014-08-14 15:39:46 PDT
Created attachment 236630 [details]
Patch
Comment 21 WebKit Commit Bot 2014-08-14 17:21:41 PDT
Comment on attachment 236630 [details]
Patch

Clearing flags on attachment: 236630

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