Bug 224326 – [css-scroll-snap] Compute proximity information while snapping

• Status: RESOLVED
• Resolution: FIXED
• Component: Scrolling

[Martin Robinson]

Currently, snap proximity is calculated during layout with the creation of ranges of offsets where snapping occurs. This change tracks moving that calculation to snap time by replacing the snap offset ranges with information about the geometry of snap areas. Not only does this simplify the code, we are going to need information about the geometry of snap areas to follow the spec as it relates to snap areas that overflow the snapport and properly dealing with masonry layouts.

[Martin Robinson]

Created attachment 425508 [details]
Patch

[Martin Robinson]

Created attachment 425591 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/76699362>

[Simon Fraser (smfr)]

Comment on attachment 425591 [details]
Patch

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

> Source/WebKit/UIProcess/RemoteLayerTree/ios/ScrollingTreeScrollingNodeDelegateIOS.mm:109
> +    WebCore::FloatSize viewportSize(static_cast<float>(CGRectGetWidth([scrollView bounds])), static_cast<float>(CGRectGetHeight([scrollView bounds])));

I think you can construct a FloatSize from a CGSize, so this could be WebCore::FloatSize viewportSize = [scrollView bounds].size;

[Martin Robinson]

Created attachment 427859 [details]
Patch

[EWS]

Committed r277083 (237386@main): <https://commits.webkit.org/237386@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 427859 [details].

[Martin Robinson]

Reopening to attach new patch.

[Martin Robinson]

Created attachment 428836 [details]
Patch

[Martin Robinson]

Comment on attachment 428836 [details]
Patch

Marking patch obsolete. This was uploaded to the incorrect bug.

[Simon Fraser (smfr)]

Comment on attachment 427859 [details]
Patch

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

> Source/WebCore/page/scrolling/ScrollSnapOffsetsInfo.cpp:157
> -    if (velocity < 0) {
> -        if (lowerSnapOffsetRangeIndex == invalidSnapOffsetIndex || lowerSnapPosition >= snapOffsetRanges[lowerSnapOffsetRangeIndex].end)
> -            return std::make_pair(lowerSnapPosition, lowerIndex);
> -        if (!originalOffsetForDirectionalSnapping.hasValue() || *originalOffsetForDirectionalSnapping > upperSnapPosition)
> -            return std::make_pair(upperSnapPosition, upperIndex);
> -    } else {
> -        if (upperSnapOffsetRangeIndex == invalidSnapOffsetIndex || snapOffsetRanges[upperSnapOffsetRangeIndex].start >= upperSnapPosition)
> +    if (velocity < 0)  {
> +        if (upperIndex != invalidSnapOffsetIndex && (!originalOffsetForDirectionalSnapping || *originalOffsetForDirectionalSnapping > upperSnapPosition))
>              return std::make_pair(upperSnapPosition, upperIndex);
> -        if (!originalOffsetForDirectionalSnapping.hasValue() || *originalOffsetForDirectionalSnapping < lowerSnapPosition)
> -            return std::make_pair(lowerSnapPosition, lowerIndex);
> +        return std::make_pair(lowerSnapPosition, lowerIndex);

This flipped the upper/lower ordering with velocity < 0. Was that intentional?

[Simon Fraser (smfr)]

(In reply to Simon Fraser (smfr) from comment #10)
> Comment on attachment 427859 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=427859&action=review
> 
> > Source/WebCore/page/scrolling/ScrollSnapOffsetsInfo.cpp:157
> > -    if (velocity < 0) {
> > -        if (lowerSnapOffsetRangeIndex == invalidSnapOffsetIndex || lowerSnapPosition >= snapOffsetRanges[lowerSnapOffsetRangeIndex].end)
> > -            return std::make_pair(lowerSnapPosition, lowerIndex);
> > -        if (!originalOffsetForDirectionalSnapping.hasValue() || *originalOffsetForDirectionalSnapping > upperSnapPosition)
> > -            return std::make_pair(upperSnapPosition, upperIndex);
> > -    } else {
> > -        if (upperSnapOffsetRangeIndex == invalidSnapOffsetIndex || snapOffsetRanges[upperSnapOffsetRangeIndex].start >= upperSnapPosition)
> > +    if (velocity < 0)  {
> > +        if (upperIndex != invalidSnapOffsetIndex && (!originalOffsetForDirectionalSnapping || *originalOffsetForDirectionalSnapping > upperSnapPosition))
> >              return std::make_pair(upperSnapPosition, upperIndex);
> > -        if (!originalOffsetForDirectionalSnapping.hasValue() || *originalOffsetForDirectionalSnapping < lowerSnapPosition)
> > -            return std::make_pair(lowerSnapPosition, lowerIndex);
> > +        return std::make_pair(lowerSnapPosition, lowerIndex);
> 
> This flipped the upper/lower ordering with velocity < 0. Was that
> intentional?

Actually it didn't, but this logic change makes it always return lowerSnapPosition even if upperSnapPosition is desired.

[Martin Robinson]

(In reply to Simon Fraser (smfr) from comment #11)
> (In reply to Simon Fraser (smfr) from comment #10)
> > Comment on attachment 427859 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=427859&action=review
> > 
> > > Source/WebCore/page/scrolling/ScrollSnapOffsetsInfo.cpp:157
> > > -    if (velocity < 0) {
> > > -        if (lowerSnapOffsetRangeIndex == invalidSnapOffsetIndex || lowerSnapPosition >= snapOffsetRanges[lowerSnapOffsetRangeIndex].end)
> > > -            return std::make_pair(lowerSnapPosition, lowerIndex);
> > > -        if (!originalOffsetForDirectionalSnapping.hasValue() || *originalOffsetForDirectionalSnapping > upperSnapPosition)
> > > -            return std::make_pair(upperSnapPosition, upperIndex);
> > > -    } else {
> > > -        if (upperSnapOffsetRangeIndex == invalidSnapOffsetIndex || snapOffsetRanges[upperSnapOffsetRangeIndex].start >= upperSnapPosition)
> > > +    if (velocity < 0)  {
> > > +        if (upperIndex != invalidSnapOffsetIndex && (!originalOffsetForDirectionalSnapping || *originalOffsetForDirectionalSnapping > upperSnapPosition))
> > >              return std::make_pair(upperSnapPosition, upperIndex);
> > > -        if (!originalOffsetForDirectionalSnapping.hasValue() || *originalOffsetForDirectionalSnapping < lowerSnapPosition)
> > > -            return std::make_pair(lowerSnapPosition, lowerIndex);
> > > +        return std::make_pair(lowerSnapPosition, lowerIndex);
> > 
> > This flipped the upper/lower ordering with velocity < 0. Was that
> > intentional?
> 
> Actually it didn't, but this logic change makes it always return
> lowerSnapPosition even if upperSnapPosition is desired.

Hrm. When scrolling with the keyboard, I am seeing all of these code paths being exercised. It's true though that this code is *very* confusing and the upper and lower snap position aren't guaranteed to be before or after the scroll destination. Sometimes they are even the same value. I have tried to address this confusing design in: 

https://bugs.webkit.org/show_bug.cgi?id=227062