Bug 147261

Summary: Scroll snapping on Mac should use AppKit animations
Product: WebKit Reporter: Wenson Hsieh <wenson_hsieh>
Component: WebCore Misc.Assignee: Wenson Hsieh <wenson_hsieh>
Status: RESOLVED FIXED    
Severity: Normal CC: bdakin, benjamin, bfulgham, buildbot, cdumez, cmarcelo, commit-queue, dbates, jamesr, jonlee, luiz, ossy, rniwa, tonikitoo, webkit-bug-importer, zalan
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Mac   
OS: Unspecified   
Bug Depends on: 147320    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Fix GTK/iOS build
none
Patch
none
Fix GTK build
bfulgham: review+, buildbot: commit-queue-
Archive of layout-test-results from ews106 for mac-yosemite-wk2
none
Fix iOS build
none
Attempt to gather more data about flaky tests.
none
Attempt to gather more data about flaky tests
none
Attempt to gather more data about flaky tests
none
Patch for landing none

Description Wenson Hsieh 2015-07-24 08:46:51 PDT
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.
Comment 1 Wenson Hsieh 2016-11-27 15:58:26 PST
<rdar://problem/29395293>
Comment 2 Wenson Hsieh 2016-11-28 09:43:39 PST
Created attachment 295489 [details]
Patch
Comment 3 Wenson Hsieh 2016-11-28 09:54:31 PST
Created attachment 295491 [details]
Fix GTK/iOS build
Comment 4 Sam Weinig 2016-11-28 10:56:25 PST
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.
Comment 5 Wenson Hsieh 2016-11-28 12:03:13 PST
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.
Comment 6 Wenson Hsieh 2016-11-28 12:20:38 PST
Created attachment 295512 [details]
Patch
Comment 7 Wenson Hsieh 2016-11-28 12:36:09 PST
Created attachment 295517 [details]
Fix GTK build
Comment 8 Brent Fulgham 2016-11-28 13:05:03 PST
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?
Comment 9 Brent Fulgham 2016-11-28 13:08:55 PST
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.
Comment 10 Build Bot 2016-11-28 15:48:10 PST
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
Comment 11 Build Bot 2016-11-28 15:48:15 PST
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
Comment 12 Wenson Hsieh 2016-11-28 17:00:55 PST
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!
Comment 13 Wenson Hsieh 2016-11-28 18:55:16 PST
Created attachment 295567 [details]
Fix iOS build
Comment 14 Wenson Hsieh 2016-11-28 21:25:48 PST
Created attachment 295578 [details]
Attempt to gather more data about flaky tests.
Comment 15 Wenson Hsieh 2016-11-28 21:40:57 PST
Created attachment 295580 [details]
Attempt to gather more data about flaky tests
Comment 16 Wenson Hsieh 2016-11-28 23:46:41 PST
Created attachment 295587 [details]
Attempt to gather more data about flaky tests
Comment 17 Wenson Hsieh 2016-11-29 07:33:23 PST
Created attachment 295594 [details]
Patch for landing
Comment 18 WebKit Commit Bot 2016-11-29 08:11:15 PST
Comment on attachment 295594 [details]
Patch for landing

Clearing flags on attachment: 295594

Committed r209070: <http://trac.webkit.org/changeset/209070>
Comment 19 Csaba Osztrogon√°c 2016-11-29 08:40:18 PST
(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.
Comment 20 Simon Fraser (smfr) 2016-11-29 08:40:45 PST
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?
Comment 21 Wenson Hsieh 2016-11-29 08:41:27 PST
(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.
Comment 22 Wenson Hsieh 2016-11-29 09:02:14 PST
Build fix in r209071. -calculateToReachDestination doesn't exist on El Capitan.
Comment 23 zalan 2016-11-29 09:12:32 PST
> >> 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?)
Comment 24 Wenson Hsieh 2016-11-29 09:27:58 PST
(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)
Comment 25 zalan 2016-11-29 09:34:03 PST
(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)
Comment 26 zalan 2016-11-29 09:34:53 PST
(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.
Comment 27 Csaba Osztrogon√°c 2016-11-30 02:26:31 PST
(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