Bug 147261 – Scroll snapping on Mac should use AppKit animations

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

[Wenson Hsieh]

Now that AppKit has wheel event filters and a momentum scrolling calculator, the current implementation of snap scroll animation on Mac should be refactored to use AppKit.

[Wenson Hsieh]

<rdar://problem/29395293>

[Wenson Hsieh]

Created attachment 295489 [details]
Patch

[Wenson Hsieh]

Created attachment 295491 [details]
Fix GTK/iOS build

[Sam Weinig]

Comment on attachment 295491 [details]
Fix GTK/iOS build

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

> Source/WebCore/page/EventHandler.cpp:333
> +#if PLATFORM(MAC)
> +                copiedEvent.setScrollingVelocity(filteredVelocity.x(), filteredVelocity.y());
> +#else
> +                UNUSED_PARAM(filteredVelocity);
> +#endif

Even if currently unused, can we add make this setScrollingVelocity cross platform? That way, we could also pass this to copyWithDeltas and avoid the setter.

> Source/WebCore/page/WheelEventDeltaFilter.h:45
> +#if PLATFORM(COCOA)
> +    WEBCORE_EXPORT FloatPoint filteredVelocity() const;
> +#endif

Again, can we make this platform agnostic?

> Source/WebCore/page/scrolling/mac/ScrollingMomentumCalculatorMac.h:28
> +#include "NSScrollingMomentumCalculatorSPI.h"

Does this need to be in the header? Can we forward declare the types instead?

> Source/WebCore/page/scrolling/mac/ScrollingMomentumCalculatorMac.h:32
> +#if PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101100

Can we make this a HAVE(NSSCROLLING_...) or something like that?

> Source/WebCore/platform/PlatformWheelEvent.h:136
> +#if PLATFORM(MAC)
> +        void setScrollingVelocity(float velocityX, float velocityY)
> +        {
> +            this->m_scrollingVelocityX = velocityX;
> +            this->m_scrollingVelocityY = velocityY;
> +        }
> +#endif

I generally like to keep these event types immutable. Can we avoid this setter?

> Source/WebCore/platform/PlatformWheelEvent.h:197
> +        float m_scrollingVelocityX { 0 };
> +        float m_scrollingVelocityY { 0 };

Seems like this could be one of our FloatFoo types. Not sure which one fits best. Seems like we use FloatPoint for velocity elsewhere. Perhaps one day we should add a FloatVector or something.

It also seems like this should be merged with the cocoa #if below, since both are Mac specific.

[Wenson Hsieh]

Comment on attachment 295491 [details]
Fix GTK/iOS build

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

>> Source/WebCore/page/EventHandler.cpp:333
>> +#endif
> 
> Even if currently unused, can we add make this setScrollingVelocity cross platform? That way, we could also pass this to copyWithDeltas and avoid the setter.

Changed copyWithDeltas to copyWithDeltasAndVelocity, which takes in a FloatPoint velocity.

>> Source/WebCore/page/WheelEventDeltaFilter.h:45
>> +#endif
> 
> Again, can we make this platform agnostic?

Sounds good! Done.

>> Source/WebCore/page/scrolling/mac/ScrollingMomentumCalculatorMac.h:28
>> +#include "NSScrollingMomentumCalculatorSPI.h"
> 
> Does this need to be in the header? Can we forward declare the types instead?

Good catch, replaced the include with a forward declare.

>> Source/WebCore/page/scrolling/mac/ScrollingMomentumCalculatorMac.h:32
>> +#if PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101100
> 
> Can we make this a HAVE(NSSCROLLING_...) or something like that?

Added HAVE(NSSCROLLING_FILTERS).

>> Source/WebCore/platform/PlatformWheelEvent.h:136
>> +#endif
> 
> I generally like to keep these event types immutable. Can we avoid this setter?

Yes -- removed.

>> Source/WebCore/platform/PlatformWheelEvent.h:197
>> +        float m_scrollingVelocityY { 0 };
> 
> Seems like this could be one of our FloatFoo types. Not sure which one fits best. Seems like we use FloatPoint for velocity elsewhere. Perhaps one day we should add a FloatVector or something.
> 
> It also seems like this should be merged with the cocoa #if below, since both are Mac specific.

Sounds good. Changed to FloatPoint. I like the idea of a FloatVector -- something like that would be useful in the platform-invariant version of the scrolling momentum calculator, where certain vector operations like dot product and scaling are performed on the initial/target scroll offsets, but neither FloatSize nor FloatPoint know about all of these operations.

[Wenson Hsieh]

Created attachment 295512 [details]
Patch

[Wenson Hsieh]

Created attachment 295517 [details]
Fix GTK build

[Brent Fulgham]

Comment on attachment 295517 [details]
Fix GTK build

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

This is a really nice refactoring/cleanup. Thank you for tracking down the cause of the test flakiness. That has caused Simon and me a lot of head-scratching. r=me (assuming tests pass). Please clean up the minor nits I suggested.

> Source/WebCore/page/Page.cpp:1999
> +            frameView->layout();

Thank you for finding this!

We should double-check with Zalan or Simon that this is safe to do here (I think it is, as long as we are not mid-render here).

> Source/WebCore/page/scrolling/ScrollingMomentumCalculator.cpp:68
> +    FloatSize interpolatedPoint(0.0f, 0.0f);

I don't think these initializer arguments are needed. I think FloatSize() automatically zero-initializes.

> Source/WebCore/page/scrolling/ScrollingMomentumCalculator.cpp:109
> + * snap point without cubic interpolation.

I know all of this is just code moved from elsewhere, but I wonder if any of this could be shared in "platform/graphics/UnitBezier.h"?

> Source/WebCore/page/scrolling/ScrollingMomentumCalculator.cpp:134
> +    FloatSize controlVector2 = controlVector1 + (sideLength * startToEndVector / startToEndDistance);

Is there anything stopping 'startToEndDistance' being zero?

> Source/WebCore/page/scrolling/ScrollingMomentumCalculator.cpp:177
> +        initialProgress = clampTo(m_initialDelta.diagonalLength() / (m_targetScrollOffset - m_initialScrollOffset).diagonalLength(), minScrollSnapInitialProgress, maxScrollSnapInitialProgress);

Is it possible for "m_targetScrollOffset" and "m_initialScrollOffset" to be the same point?

> Source/WebCore/page/scrolling/ScrollingMomentumCalculator.cpp:185
> +        m_snapAnimationCurveMagnitude = 1.0f / (1.0f - std::pow(m_snapAnimationDecayFactor, -60.0f * scrollSnapAnimationDuration));

Please consider making "60.0f" into a named constant (e.g., 'framesPerSecond').

> Source/WebCore/page/scrolling/ScrollingMomentumCalculator.cpp:196
> +    return std::min(1.0, m_snapAnimationCurveMagnitude * (1.0 - std::pow(m_snapAnimationDecayFactor, -60.0f * scrollSnapAnimationDuration * timeProgress)));

Ditto "framesPerSecond' for 60.0f.

> Source/WebCore/page/scrolling/ScrollingMomentumCalculator.h:32
> +#include "FloatSize.h"

I think you can just forward-declare FloatPoint and FloatSize here.

> Source/WebCore/page/scrolling/ScrollingMomentumCalculator.h:61
> +    FloatPoint scrollOffsetAfterElapsedTime(double time) final;

I don't think the label 'time' is helpful here. However, it would be helpful to indicate if it is 'milliseconds' or 'ticks' or something along those lines. That could be very helpful to people wanting to use this code.

> Source/WebCore/platform/cocoa/ScrollSnapAnimatorState.h:55
> +    void setSnapOffsetsForAxis(ScrollEventAxis axis, const Vector<LayoutUnit> snapOffsets)

I think this should be "const Vector<LayoutUnit>&"

> Source/WebCore/platform/cocoa/ScrollSnapAnimatorState.h:98
> +    double m_startTime;

Why isn't this zero-initialized anymore?

[Brent Fulgham]

Comment on attachment 295517 [details]
Fix GTK build

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

>> Source/WebCore/page/Page.cpp:1999
>> +            frameView->layout();
> 
> Thank you for finding this!
> 
> We should double-check with Zalan or Simon that this is safe to do here (I think it is, as long as we are not mid-render here).

Zalan: Is it safe to trigger layout here? I think it is, since we do not hit this during rendering.

[Build Bot]

Comment on attachment 295517 [details]
Fix GTK build

Attachment 295517 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2585058

New failing tests:
fast/mediastream/delayed-permission-allowed.html

[Build Bot]

Created attachment 295542 [details]
Archive of layout-test-results from ews106 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Wenson Hsieh]

Comment on attachment 295517 [details]
Fix GTK build

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

>> Source/WebCore/page/scrolling/ScrollingMomentumCalculator.cpp:68
>> +    FloatSize interpolatedPoint(0.0f, 0.0f);
> 
> I don't think these initializer arguments are needed. I think FloatSize() automatically zero-initializes.

Good point! Done.

>> Source/WebCore/page/scrolling/ScrollingMomentumCalculator.cpp:109
>> + * snap point without cubic interpolation.
> 
> I know all of this is just code moved from elsewhere, but I wonder if any of this could be shared in "platform/graphics/UnitBezier.h"?

It's possible, but there would be more work involved in mapping from the coordinate space of the initial/target scroll positions to the unit square and back. I'll leave this as a FIXME.

>> Source/WebCore/page/scrolling/ScrollingMomentumCalculator.cpp:134
>> +    FloatSize controlVector2 = controlVector1 + (sideLength * startToEndVector / startToEndDistance);
> 
> Is there anything stopping 'startToEndDistance' being zero?

Good catch! There isn't any reason why it can't be zero here -- added an early return.

>> Source/WebCore/page/scrolling/ScrollingMomentumCalculator.cpp:177
>> +        initialProgress = clampTo(m_initialDelta.diagonalLength() / (m_targetScrollOffset - m_initialScrollOffset).diagonalLength(), minScrollSnapInitialProgress, maxScrollSnapInitialProgress);
> 
> Is it possible for "m_targetScrollOffset" and "m_initialScrollOffset" to be the same point?

Not in this branch, since alignmentVector.width() + alignmentVector.height() will be 0.

>> Source/WebCore/page/scrolling/ScrollingMomentumCalculator.cpp:185
>> +        m_snapAnimationCurveMagnitude = 1.0f / (1.0f - std::pow(m_snapAnimationDecayFactor, -60.0f * scrollSnapAnimationDuration));
> 
> Please consider making "60.0f" into a named constant (e.g., 'framesPerSecond').

Sounds good -- done!

>> Source/WebCore/page/scrolling/ScrollingMomentumCalculator.cpp:196
>> +    return std::min(1.0, m_snapAnimationCurveMagnitude * (1.0 - std::pow(m_snapAnimationDecayFactor, -60.0f * scrollSnapAnimationDuration * timeProgress)));
> 
> Ditto "framesPerSecond' for 60.0f.

Done!

>> Source/WebCore/page/scrolling/ScrollingMomentumCalculator.h:32
>> +#include "FloatSize.h"
> 
> I think you can just forward-declare FloatPoint and FloatSize here.

Done

>> Source/WebCore/page/scrolling/ScrollingMomentumCalculator.h:61
>> +    FloatPoint scrollOffsetAfterElapsedTime(double time) final;
> 
> I don't think the label 'time' is helpful here. However, it would be helpful to indicate if it is 'milliseconds' or 'ticks' or something along those lines. That could be very helpful to people wanting to use this code.

I agree -- renamed to seconds.

> Source/WebCore/page/scrolling/ScrollingMomentumCalculator.h:65
> +    float animationProgressAfterElapsedTime(double time) const;

Also renamed this to double seconds

>> Source/WebCore/platform/cocoa/ScrollSnapAnimatorState.h:55
>> +    void setSnapOffsetsForAxis(ScrollEventAxis axis, const Vector<LayoutUnit> snapOffsets)
> 
> I think this should be "const Vector<LayoutUnit>&"

Good catch -- fixed.

>> Source/WebCore/platform/cocoa/ScrollSnapAnimatorState.h:98
>> +    double m_startTime;
> 
> Why isn't this zero-initialized anymore?

Fixed!

[Wenson Hsieh]

Created attachment 295567 [details]
Fix iOS build

[Wenson Hsieh]

Created attachment 295578 [details]
Attempt to gather more data about flaky tests.

[Wenson Hsieh]

Created attachment 295580 [details]
Attempt to gather more data about flaky tests

[Wenson Hsieh]

Created attachment 295587 [details]
Attempt to gather more data about flaky tests

[Wenson Hsieh]

Created attachment 295594 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 295594 [details]
Patch for landing

Clearing flags on attachment: 295594

Committed r209070: <http://trac.webkit.org/changeset/209070>

[Csaba Osztrogonác]

(In reply to comment #18)
> Comment on attachment 295594 [details]
> Patch for landing
> 
> Clearing flags on attachment: 295594
> 
> Committed r209070: <http://trac.webkit.org/changeset/209070>

It broke the Apple Mac build, see build.webkit.org for details.

[Simon Fraser (smfr)]

Comment on attachment 295517 [details]
Fix GTK build

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

>>> Source/WebCore/page/Page.cpp:1999
>>> +            frameView->layout();
>> 
>> Thank you for finding this!
>> 
>> We should double-check with Zalan or Simon that this is safe to do here (I think it is, as long as we are not mid-render here).
> 
> Zalan: Is it safe to trigger layout here? I think it is, since we do not hit this during rendering.

I think this needs a comment to say why laying out is needed. It seems that ensureTestTrigger() is only called via script, so it's OK to do layout here.

> Source/WebCore/page/scrolling/ScrollingMomentumCalculator.cpp:34
> +static const double scrollSnapAnimationDuration = 1;

This could use the Seconds type.

> Source/WebCore/page/scrolling/ScrollingMomentumCalculator.cpp:75
> +FloatPoint BasicScrollingMomentumCalculator::scrollOffsetAfterElapsedTime(double time)

Would be better with Seconds.

> Source/WebCore/page/scrolling/ScrollingMomentumCalculator.cpp:88
> +double BasicScrollingMomentumCalculator::animationDuration()

Would be better returning Seconds.

> Source/WebCore/page/scrolling/ScrollingMomentumCalculator.cpp:193
> +float BasicScrollingMomentumCalculator::animationProgressAfterElapsedTime(double time) const

Seconds

>>> Source/WebCore/page/scrolling/ScrollingMomentumCalculator.h:61
>>> +    FloatPoint scrollOffsetAfterElapsedTime(double time) final;
>> 
>> I don't think the label 'time' is helpful here. However, it would be helpful to indicate if it is 'milliseconds' or 'ticks' or something along those lines. That could be very helpful to people wanting to use this code.
> 
> I agree -- renamed to seconds.

We have a Seconds class now.

> Source/WebCore/page/scrolling/ScrollingMomentumCalculator.h:62
> +    double animationDuration() final;

Seconds

> Source/WebCore/platform/PlatformWheelEvent.h:187
> +        FloatPoint m_scrollingVelocity;

I would use FloatSize for velocity, and state the units in a comment (pixels per second?)

> Source/WebCore/platform/cocoa/ScrollController.mm:517
> +    static const double statelessScrollSnapDelay = 0.75;

Seconds

> Source/WebCore/platform/cocoa/ScrollSnapAnimatorState.mm:60
> +    m_startTime = monotonicallyIncreasingTime();

MonotonicTime::now()

> Source/WebCore/platform/cocoa/ScrollSnapAnimatorState.mm:92
> +    double elapsedTime = monotonicallyIncreasingTime() - m_startTime;

MonotonicTime::now()

> Source/WebCore/platform/spi/mac/NSScrollingMomentumCalculatorSPI.h:39
> +@property (atomic) NSPoint destinationOrigin;

Isn't atomic the default?

[Wenson Hsieh]

(In reply to comment #19)
> (In reply to comment #18)
> > Comment on attachment 295594 [details]
> > Patch for landing
> > 
> > Clearing flags on attachment: 295594
> > 
> > Committed r209070: <http://trac.webkit.org/changeset/209070>
> 
> It broke the Apple Mac build, see build.webkit.org for details.

Thanks for the heads up -- I'm working on a fix.

[Wenson Hsieh]

Build fix in r209071. -calculateToReachDestination doesn't exist on El Capitan.

[zalan]

> >> We should double-check with Zalan or Simon that this is safe to do here (I think it is, as long as we are not mid-render here).
> > 
> > Zalan: Is it safe to trigger layout here? I think it is, since we do not hit this during rendering.
I don't see why we need to issue layout on the mainframe unconditionally. I'd rather call document::updateLayout() to ensure we also update style and call layout only if it is needed. (also is it ok to issue this only on the mainframe?)

[Wenson Hsieh]

(In reply to comment #23)
> > >> We should double-check with Zalan or Simon that this is safe to do here (I think it is, as long as we are not mid-render here).
> > > 
> > > Zalan: Is it safe to trigger layout here? I think it is, since we do not hit this during rendering.
> I don't see why we need to issue layout on the mainframe unconditionally.
> I'd rather call document::updateLayout() to ensure we also update style and
> call layout only if it is needed. (also is it ok to issue this only on the
> mainframe?)

I suppose forcing FrameView::layout might be a bit heavy-handed, but I don't think Document::updateLayout is sufficient, since I need to make sure FrameView::performPostLayoutTasks gets invoked. However, all I really need is for the mainframe scrolling node to update `expectsWheelEventTestTrigger` -- maybe I could simply add a new method that does just this? (something like FrameView::updateExpectsWheelEventTestTrigger)

[zalan]

(In reply to comment #24)
> (In reply to comment #23)
> > > >> We should double-check with Zalan or Simon that this is safe to do here (I think it is, as long as we are not mid-render here).
> > > > 
> > > > Zalan: Is it safe to trigger layout here? I think it is, since we do not hit this during rendering.
> > I don't see why we need to issue layout on the mainframe unconditionally.
> > I'd rather call document::updateLayout() to ensure we also update style and
> > call layout only if it is needed. (also is it ok to issue this only on the
> > mainframe?)
> 
> I suppose forcing FrameView::layout might be a bit heavy-handed, but I don't
> think Document::updateLayout is sufficient, since I need to make sure
> FrameView::performPostLayoutTasks gets invoked. However, all I really need
> is for the mainframe scrolling node to update `expectsWheelEventTestTrigger`
> -- maybe I could simply add a new method that does just this? (something
> like FrameView::updateExpectsWheelEventTestTrigger)
Why do you need to make sure that FrameView::performPostLayoutTasks gets invoked? What is in there that you need to run before returning from ensureTestTrigger?
calling layout just to ensure that some post layout task is getting executed seems to me a bad choice of design (also performPostLayoutTasks is not necessarily sync)

[zalan]

(In reply to comment #25)
> (In reply to comment #24)
> > (In reply to comment #23)
> > > > >> We should double-check with Zalan or Simon that this is safe to do here (I think it is, as long as we are not mid-render here).
> > > > > 
> > > > > Zalan: Is it safe to trigger layout here? I think it is, since we do not hit this during rendering.
> > > I don't see why we need to issue layout on the mainframe unconditionally.
> > > I'd rather call document::updateLayout() to ensure we also update style and
> > > call layout only if it is needed. (also is it ok to issue this only on the
> > > mainframe?)
> > 
> > I suppose forcing FrameView::layout might be a bit heavy-handed, but I don't
> > think Document::updateLayout is sufficient, since I need to make sure
> > FrameView::performPostLayoutTasks gets invoked. However, all I really need
> > is for the mainframe scrolling node to update `expectsWheelEventTestTrigger`
> > -- maybe I could simply add a new method that does just this? (something
> > like FrameView::updateExpectsWheelEventTestTrigger)
That would be a lot better.

[Csaba Osztrogonác]

(In reply to comment #18)
> Comment on attachment 295594 [details]
> Patch for landing
> 
> Clearing flags on attachment: 295594
> 
> Committed r209070: <http://trac.webkit.org/changeset/209070>

and the cmake buildfix landed in https://trac.webkit.org/changeset/209126