Bug 141973 – Scroll Snap Points are not supported on the main frame

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Brent Fulgham]

We should be able to support Scroll Snap Points on the main frame, and stay within our fast scrolling thread while doing so.

[Radar WebKit Bug Importer]

<rdar://problem/19938393>

[Brent Fulgham]

Created attachment 247280 [details]
Patch

[Brent Fulgham]

Created attachment 247281 [details]
Example horizontal main-frame scroll snap points

[Brent Fulgham]

Created attachment 247282 [details]
Example vertical main-frame scroll snap points

[Brent Fulgham]

Created attachment 247283 [details]
Example overflow scroll snap points

[Brent Fulgham]

Attached a set of manual tests for this code. Full-fledged snap point tests will be part of a future update.

[Simon Fraser (smfr)]

Comment on attachment 247280 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        Reviewed by NOBODY (OOPS!).
> +
> +        * page/scrolling/mac/ScrollingTreeFrameScrollingNodeMac.h: Implement the AxisScrollAnimatorClient interface.

I would like a summary of what you changed.

> Source/WebCore/page/scrolling/mac/ScrollingTreeFrameScrollingNodeMac.h:46
> +#if !ENABLE(CSS_SCROLL_SNAP)
> +class AxisScrollSnapAnimatorClient { };
> +#endif

This is a bit odd. You can just wrap the class declaration and surround the public AxisScrollSnapAnimatorClient with the #ifdefs.

> Source/WebCore/page/scrolling/mac/ScrollingTreeFrameScrollingNodeMac.h:109
> +    RetainPtr<CFRunLoopTimerRef> m_horizontalScrollSnapTimer;
> +    RetainPtr<CFRunLoopTimerRef> m_verticalScrollSnapTimer;

You should use a std::unique_ptr<Timer> like ScrollAnimator does.

Is there a reason the AxisScrollSnapAnimator doesn't manage the timer?

> Source/WebCore/page/scrolling/mac/ScrollingTreeFrameScrollingNodeMac.mm:87
> +static inline Vector<LayoutUnit> convertFloatToLayoutUnits(const Vector<float>& snapOffsetsAsFloat)
> +{
> +    Vector<LayoutUnit> snapOffsets;
> +    snapOffsets.reserveInitialCapacity(snapOffsetsAsFloat.size());
> +    for (size_t i = 0; i < snapOffsetsAsFloat.size(); ++i)
> +        snapOffsets.append(LayoutUnit(snapOffsetsAsFloat[i]));
> +    
> +    return snapOffsets;
> +}
> +#endif

I'm not sure we need to do this conversion. We should just treat scroll offsets as floats on the scrolling thread.

> Source/WebCore/page/scrolling/mac/ScrollingTreeFrameScrollingNodeMac.mm:143
> +        m_horizontalScrollSnapAnimator = std::make_unique<AxisScrollSnapAnimator>(this, convertFloatToLayoutUnits(scrollingStateNode.horizontalSnapOffsets()), ScrollEventAxis::Horizontal);

Is there a reason we have to make a new animator when the offsets change? Won't that destroy snapping state if we commit a scrolling tree while animating to a snap point is in progress?

> Source/WebCore/platform/mac/ScrollAnimatorMac.h:182
> +    virtual void immediateScrollInAxis(ScrollEventAxis, float delta) override;

I think "OnAxis" makes more sense than "InAxis".

> Source/WebCore/platform/mac/ScrollAnimatorMac.h:184
> +    virtual LayoutUnit scrollOffsetInAxis(ScrollEventAxis) override;

Why can't it just get the scrollOffset as a point or size, then choose the axis itself?

> Source/WebCore/platform/mac/ScrollAnimatorMac.h:186
> +    virtual void startScrollSnapTimer(ScrollEventAxis) override;
> +    virtual void stopScrollSnapTimer(ScrollEventAxis) override;

Manage the timer itself?

> Source/WebCore/platform/mac/ScrollAnimatorMac.h:187
> +    virtual void updateScrollAnimatorsAndTimers() override;

Pretty vague for a client message.

[Sam Weinig]

Comment on attachment 247280 [details]
Patch

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

> Source/WebCore/platform/mac/ScrollAnimatorMac.h:82
> +    // Trivial wrappers around the actual update loop in AxisScrollSnapAnimator, since WebCore Timer requires a Timer argument.

This comment doesn't seem correct.

[Darin Adler]

Comment on attachment 247280 [details]
Patch

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

> Source/WebCore/page/scrolling/mac/ScrollingTreeFrameScrollingNodeMac.h:48
> +class ScrollingTreeFrameScrollingNodeMac : public ScrollingTreeFrameScrollingNode, private ScrollControllerClient, public AxisScrollSnapAnimatorClient  {

Can this be private inheritance instead of public?

> Source/WebCore/page/scrolling/mac/ScrollingTreeFrameScrollingNodeMac.h:80
> +    virtual LayoutUnit scrollOffsetInAxis(ScrollEventAxis) override;
> +    virtual void immediateScrollInAxis(ScrollEventAxis, float velocity) override;
> +    virtual void startScrollSnapTimer(ScrollEventAxis) override;
> +    virtual void stopScrollSnapTimer(ScrollEventAxis) override;

Aren’t we moving to just “override” rather than virtual plus override?

> Source/WebCore/page/scrolling/mac/ScrollingTreeFrameScrollingNodeMac.mm:78
> +static inline Vector<LayoutUnit> convertFloatToLayoutUnits(const Vector<float>& snapOffsetsAsFloat)

If we do need to keep this function, we don’t need to mention “float” in the function name.

> Source/WebCore/page/scrolling/mac/ScrollingTreeFrameScrollingNodeMac.mm:83
> +    for (size_t i = 0; i < snapOffsetsAsFloat.size(); ++i)
> +        snapOffsets.append(LayoutUnit(snapOffsetsAsFloat[i]));

If we keep this function, we should use a modern for loop rather than an old-style one.

Also, can we omit the explicit cast to LayoutUnit?

> Source/WebCore/page/scrolling/mac/ScrollingTreeFrameScrollingNodeMac.mm:85
> +    return snapOffsets;

Might be able to just write:

    return Vector<LayoutUnit>(snapOffsetsAsFloat.begin(), snapOffsetsAsFloat.end());

Or maybe that won’t work.

> Source/WebCore/platform/mac/ScrollAnimatorMac.h:61
> +class ScrollAnimatorMac : public ScrollAnimator, private ScrollControllerClient, public AxisScrollSnapAnimatorClient {

Can this be private inheritance?

> Source/WebCore/platform/mac/ScrollAnimatorMac.mm:1481
> +    if (scrollSnapTimer && scrollSnapTimer->isActive())
> +        scrollSnapTimer->stop();

Is the check for isActive an important optimization here? Not sure why we have a pattern of checking isActive before calling stop.

[Brent Fulgham]

Comment on attachment 247280 [details]
Patch

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

>> Source/WebCore/page/scrolling/mac/ScrollingTreeFrameScrollingNodeMac.h:46
>> +#endif
> 
> This is a bit odd. You can just wrap the class declaration and surround the public AxisScrollSnapAnimatorClient with the #ifdefs.

This is how Beth did it in her ScrollElasticityController (now ScrollController) class, and I thought it looked cleaner than doing the #ifdef thing around the class declaration.

>> Source/WebCore/page/scrolling/mac/ScrollingTreeFrameScrollingNodeMac.h:48
>> +class ScrollingTreeFrameScrollingNodeMac : public ScrollingTreeFrameScrollingNode, private ScrollControllerClient, public AxisScrollSnapAnimatorClient  {
> 
> Can this be private inheritance instead of public?

No; I tried that first. The compiler complained when passing a ScrollingTreeFrameScrollingNodeMac* as a constructor argument for AxisScrollSnapAnimator (which wants an AxisScrollSnapAnimatorClient) due to the 'private' inheritance. When I changed it to public it worked properly.

>> Source/WebCore/page/scrolling/mac/ScrollingTreeFrameScrollingNodeMac.h:80
>> +    virtual void stopScrollSnapTimer(ScrollEventAxis) override;
> 
> Aren’t we moving to just “override” rather than virtual plus override?

I wasn't sure if that was agreed on or not. I'm happy to switch to just override.

>> Source/WebCore/page/scrolling/mac/ScrollingTreeFrameScrollingNodeMac.h:109
>> +    RetainPtr<CFRunLoopTimerRef> m_verticalScrollSnapTimer;
> 
> You should use a std::unique_ptr<Timer> like ScrollAnimator does.
> 
> Is there a reason the AxisScrollSnapAnimator doesn't manage the timer?

Because of threading differences, we have to use WebCore::Timer timers in our main thread scrolling logic, and we use CFRunLoopTimer in our scroll thread logic.

I could convert the main thread code to use CFRunLoopTimers, too, then put them all in AxisScrollSnapAnimator.

Eventually, all of this is getting moved to ScrollController so consolidating some of this now is probably a good idea.

>> Source/WebCore/page/scrolling/mac/ScrollingTreeFrameScrollingNodeMac.mm:85
>> +    return snapOffsets;
> 
> Might be able to just write:
> 
>     return Vector<LayoutUnit>(snapOffsetsAsFloat.begin(), snapOffsetsAsFloat.end());
> 
> Or maybe that won’t work.

I'll try it. But maybe I can do away with the whole thing...

>> Source/WebCore/page/scrolling/mac/ScrollingTreeFrameScrollingNodeMac.mm:87
>> +#endif
> 
> I'm not sure we need to do this conversion. We should just treat scroll offsets as floats on the scrolling thread.

The AxisScrollSnapAnimator expects to work in LayoutUnits. I haven't dug to much into how that class works to see if this can be easily changed; I was just trying to honor the existing API.

>> Source/WebCore/page/scrolling/mac/ScrollingTreeFrameScrollingNodeMac.mm:143
>> +        m_horizontalScrollSnapAnimator = std::make_unique<AxisScrollSnapAnimator>(this, convertFloatToLayoutUnits(scrollingStateNode.horizontalSnapOffsets()), ScrollEventAxis::Horizontal);
> 
> Is there a reason we have to make a new animator when the offsets change? Won't that destroy snapping state if we commit a scrolling tree while animating to a snap point is in progress?

Wenson left a FIXME in the ScrollAnimator code about this. I think it will destroy snapping state, but if the snap points are changing I'm not sure if that's such a problem. If the target snap points have changed from our current animation, it seems like we do want to tear it down and start over. But this obviously creates a visual discrepancy, which will probably look bad.

I need to write another test that plays with the snap points on-the-fly to see what happens.

>> Source/WebCore/platform/mac/ScrollAnimatorMac.h:61
>> +class ScrollAnimatorMac : public ScrollAnimator, private ScrollControllerClient, public AxisScrollSnapAnimatorClient {
> 
> Can this be private inheritance?

I don't think so (see my above response to your earlier comment). But maybe the constructor for AxisScrollSnapAnimator is just doing something wrong, and it should work... I'll ask Anders about this tomorrow.

>> Source/WebCore/platform/mac/ScrollAnimatorMac.h:82
>> +    // Trivial wrappers around the actual update loop in AxisScrollSnapAnimator, since WebCore Timer requires a Timer argument.
> 
> This comment doesn't seem correct.

Yes. I'll get rid of this comment.

>> Source/WebCore/platform/mac/ScrollAnimatorMac.h:182
>> +    virtual void immediateScrollInAxis(ScrollEventAxis, float delta) override;
> 
> I think "OnAxis" makes more sense than "InAxis".

OK!

>> Source/WebCore/platform/mac/ScrollAnimatorMac.h:184
>> +    virtual LayoutUnit scrollOffsetInAxis(ScrollEventAxis) override;
> 
> Why can't it just get the scrollOffset as a point or size, then choose the axis itself?

It probably could. I'll audit the callers of this method and see.

>> Source/WebCore/platform/mac/ScrollAnimatorMac.h:186
>> +    virtual void stopScrollSnapTimer(ScrollEventAxis) override;
> 
> Manage the timer itself?

Ditto my above explanation for ScrollAnimatorMac.

>> Source/WebCore/platform/mac/ScrollAnimatorMac.h:187
>> +    virtual void updateScrollAnimatorsAndTimers() override;
> 
> Pretty vague for a client message.

Yeah. But that's pretty much what it does... I'll see if I can come up with a better name.

>> Source/WebCore/platform/mac/ScrollAnimatorMac.mm:1481
>> +        scrollSnapTimer->stop();
> 
> Is the check for isActive an important optimization here? Not sure why we have a pattern of checking isActive before calling stop.

Hmm. I'm not sure! I'll review the 'stop' logic to see if there's any reason to worry about it.

[Brent Fulgham]

I'll revise this some more tomorrow and upload a new patch.

[Brent Fulgham]

Comment on attachment 247280 [details]
Patch

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

>> Source/WebCore/page/scrolling/mac/ScrollingTreeFrameScrollingNodeMac.mm:78
>> +static inline Vector<LayoutUnit> convertFloatToLayoutUnits(const Vector<float>& snapOffsetsAsFloat)
> 
> If we do need to keep this function, we don’t need to mention “float” in the function name.

OK!

>>> Source/WebCore/page/scrolling/mac/ScrollingTreeFrameScrollingNodeMac.mm:85
>>> +    return snapOffsets;
>> 
>> Might be able to just write:
>> 
>>     return Vector<LayoutUnit>(snapOffsetsAsFloat.begin(), snapOffsetsAsFloat.end());
>> 
>> Or maybe that won’t work.
> 
> I'll try it. But maybe I can do away with the whole thing...

This didn't work (compiler did not agree this was valid).

>>> Source/WebCore/platform/mac/ScrollAnimatorMac.h:184
>>> +    virtual LayoutUnit scrollOffsetInAxis(ScrollEventAxis) override;
>> 
>> Why can't it just get the scrollOffset as a point or size, then choose the axis itself?
> 
> It probably could. I'll audit the callers of this method and see.

I don't think it makes sense to return a point or size; Callers currently expect a float return value, and use it as part of calculations for scroll handling. I would have to modify the four or five places that call this with a new check for axis and retrieve the correct value (x or y) depending on axis.

Since this method basically encapsulates that logic, I think it's better as it stands.

[Brent Fulgham]

Created attachment 247356 [details]
Patch v2 (Revised based on comments)

[Brent Fulgham]

Comment on attachment 247356 [details]
Patch v2 (Revised based on comments)

The code is moving from ScrollAnimator down to ScrollAnimatorMac because it is only currently used on the Mac port. I wanted to consolidate the code in one place before moving it to ScrollController in a future patch.

[Brent Fulgham]

Created attachment 247476 [details]
Patch - Updated from earlier review comments.

[Brent Fulgham]

Comment on attachment 247476 [details]
Patch - Updated from earlier review comments.

Note: I still would like to move the snap implementation down to ScrollAnimatorMac as a prior step to moving it into ScrollController.

[Brent Fulgham]

Created attachment 247744 [details]
Updated patch after refactoring in Bug 142102.

[Brent Fulgham]

*** Bug 135967 has been marked as a duplicate of this bug. ***

[Brent Fulgham]

Created attachment 247822 [details]
Patch v5. Updated after refactoring.

[Simon Fraser (smfr)]

Comment on attachment 247822 [details]
Patch v5. Updated after refactoring.

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

> Source/WebCore/ChangeLog:18
> +        so that these values can be used on the main and scrolling threads. Simply storing a pointer to a set of snap points will not
> +        work on the scrolling thread.

I think would be better written as "we have to make a copy of the snap points vector to send to the scrolling thread".

[Brent Fulgham]

(In reply to comment #20)
> Comment on attachment 247822 [details]
> Patch v5. Updated after refactoring.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=247822&action=review
> 
> > Source/WebCore/ChangeLog:18
> > +        so that these values can be used on the main and scrolling threads. Simply storing a pointer to a set of snap points will not
> > +        work on the scrolling thread.
> 
> I think would be better written as "we have to make a copy of the snap
> points vector to send to the scrolling thread".

OK!

[Brent Fulgham]

Committed r180987: <http://trac.webkit.org/changeset/180987>