Bug 228023

Summary: [css-scroll-snap] Pass the full target point when selecting a snap offset
Product: WebKit Reporter: Martin Robinson <mrobinson>
Component: ScrollingAssignee: Martin Robinson <mrobinson>
Status: RESOLVED FIXED    
Severity: Normal CC: cmarcelo, ews-watchlist, fred.wang, jamesr, luiz, simon.fraser, tonikitoo, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: Safari Technology Preview   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 227949    
Attachments:
Description Flags
Patch
ews-feeder: commit-queue-
Patch
none
Patch none

Description Martin Robinson 2021-07-16 07:02:35 PDT
When selecting a snap offset, WebKit should pass the full target point to the selection algorithm. This will eventually be used to avoid selecting snap offsets for snap areas that are completely off the screen.
Comment 1 Martin Robinson 2021-07-16 07:14:10 PDT
Created attachment 433674 [details]
Patch
Comment 2 Martin Robinson 2021-07-16 07:42:50 PDT
Created attachment 433676 [details]
Patch
Comment 3 Frédéric Wang (:fredw) 2021-07-21 01:45:36 PDT
Comment on attachment 433676 [details]
Patch

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

LGTM

> Source/WebCore/page/scrolling/nicosia/ScrollingTreeScrollingNodeDelegateNicosia.cpp:176
> +        auto offsetY = scrollingNode().snapOffsetsInfo().closestSnapOffset(ScrollEventAxis::Vertical, scrollableAreaSize(), newOffset, deltaY, originalOffset.y()).first;

It seems before this patch we are passing LayoutUnits. Not sure that will change things much, but shouldn't you continue to pass a LayoutPoint rather than a FloatPoint? If so, the scaling/conversion matter might matter (see setNearestScrollSnapIndexForAxisAndOffset and below).

> Source/WebCore/platform/ScrollAnimator.cpp:408
> +        return axis == ScrollEventAxis::Horizontal ? newOffset.x() : newOffset.y();

This pattern seems to be repeated at several places in this patch. You may consider factoring this out in a helper function.

> Source/WebCore/platform/ScrollAnimator.cpp:417
> +        velocity = axis == ScrollEventAxis::Horizontal ? velocityInBothAxes.width() : velocityInBothAxes.height();

nit: It sounds to me more logical to rename the variables velocity = newOffset - currentOffset; and velocityOnScrollEventAxis = axis == ScrollEventAxis::Horizontal ? velocityInBothAxes.width() : velocityInBothAxes.height();

> Source/WebCore/platform/ScrollController.cpp:139
> +    layoutScrollOffset.scale(1.0 / scaleFactor);

I don't know if that makes a difference, but it looks like if we want to preserve the current logic this would be

LayoutPoint layoutScrollOffset(scrollOffset.x() / scaleFactor, scrollOffset.y() / scaleFactor)

where the scaling is performed before the conversion to LayoutUnit.

> Source/WebCore/platform/ScrollController.cpp:170
> +    layoutDestinationOffset.scale(1.0 / m_client.pageScaleFactor());

Same here, the scaling was done before the conversion to LayoutUnit in the old code.

> Source/WebCore/platform/ScrollSnapAnimatorState.cpp:51
> +    auto predictedScrollTarget = FloatPoint { m_momentumCalculator->predictedDestinationOffset() };

Why not just FloatPoint predictedScrollTarget { m_momentumCalculator->predictedDestinationOffset() }; ?

> Source/WebCore/platform/ScrollSnapAnimatorState.cpp:101
> +    predictedLayoutOffset.scale(1.0 / pageScale);

Same here, previous code was doing the scaling before LayoutUnit conversion. And you still do that at the next line for startOffset.
Comment 4 Martin Robinson 2021-07-21 09:23:09 PDT
Created attachment 433939 [details]
Patch
Comment 5 Martin Robinson 2021-07-22 01:43:00 PDT
Comment on attachment 433676 [details]
Patch

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

Thanks for the review. I've uploaded a new version of the change that addresses the issues you've raised. I'll land that now.

>> Source/WebCore/page/scrolling/nicosia/ScrollingTreeScrollingNodeDelegateNicosia.cpp:176
>> +        auto offsetY = scrollingNode().snapOffsetsInfo().closestSnapOffset(ScrollEventAxis::Vertical, scrollableAreaSize(), newOffset, deltaY, originalOffset.y()).first;
> 
> It seems before this patch we are passing LayoutUnits. Not sure that will change things much, but shouldn't you continue to pass a LayoutPoint rather than a FloatPoint? If so, the scaling/conversion matter might matter (see setNearestScrollSnapIndexForAxisAndOffset and below).

The ScrollSnapOffsets here are FloatScrollSnapOffsets, so even though the original code was creating a LayoutPoint it was silently converting the components from LayoutUnit to float for the call to closestSnapOffset. The new version removes this trip into LayoutUnits and back to floats.

>> Source/WebCore/platform/ScrollAnimator.cpp:408
>> +        return axis == ScrollEventAxis::Horizontal ? newOffset.x() : newOffset.y();
> 
> This pattern seems to be repeated at several places in this patch. You may consider factoring this out in a helper function.

I did consider this, but ultimately didn't do it because I couldn't figure out where this helper would belong.

>> Source/WebCore/platform/ScrollAnimator.cpp:417
>> +        velocity = axis == ScrollEventAxis::Horizontal ? velocityInBothAxes.width() : velocityInBothAxes.height();
> 
> nit: It sounds to me more logical to rename the variables velocity = newOffset - currentOffset; and velocityOnScrollEventAxis = axis == ScrollEventAxis::Horizontal ? velocityInBothAxes.width() : velocityInBothAxes.height();

This makes sense to me. I've done this rename.

>> Source/WebCore/platform/ScrollController.cpp:139
>> +    layoutScrollOffset.scale(1.0 / scaleFactor);
> 
> I don't know if that makes a difference, but it looks like if we want to preserve the current logic this would be
> 
> LayoutPoint layoutScrollOffset(scrollOffset.x() / scaleFactor, scrollOffset.y() / scaleFactor)
> 
> where the scaling is performed before the conversion to LayoutUnit.

Okay. I can keep this more similar to the previous version.

>> Source/WebCore/platform/ScrollController.cpp:170
>> +    layoutDestinationOffset.scale(1.0 / m_client.pageScaleFactor());
> 
> Same here, the scaling was done before the conversion to LayoutUnit in the old code.

Okay. I've changed this back.

>> Source/WebCore/platform/ScrollSnapAnimatorState.cpp:51
>> +    auto predictedScrollTarget = FloatPoint { m_momentumCalculator->predictedDestinationOffset() };
> 
> Why not just FloatPoint predictedScrollTarget { m_momentumCalculator->predictedDestinationOffset() }; ?

Oh, this is much cleaner. Thanks!

>> Source/WebCore/platform/ScrollSnapAnimatorState.cpp:101
>> +    predictedLayoutOffset.scale(1.0 / pageScale);
> 
> Same here, previous code was doing the scaling before LayoutUnit conversion. And you still do that at the next line for startOffset.

Fixed.
Comment 6 EWS 2021-07-22 02:00:36 PDT
Committed r280171 (239866@main): <https://commits.webkit.org/239866@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 433939 [details].
Comment 7 Radar WebKit Bug Importer 2021-07-22 02:01:19 PDT
<rdar://problem/80949086>