Bug 228023 – [css-scroll-snap] Pass the full target point when selecting a snap offset

• Component: Scrolling
• Status: RESOLVED FIXED

[Martin Robinson]

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.

[Martin Robinson]

Created attachment 433674 [details]
Patch

[Martin Robinson]

Created attachment 433676 [details]
Patch

[Frédéric Wang (:fredw)]

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.

[Martin Robinson]

Created attachment 433939 [details]
Patch

[Martin Robinson]

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.

[EWS]

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].

[Radar WebKit Bug Importer]

<rdar://problem/80949086>