Bug 224326

Summary: [css-scroll-snap] Compute proximity information while snapping
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: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 223021, 224380    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Description Martin Robinson 2021-04-08 05:57:48 PDT
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.
Comment 1 Martin Robinson 2021-04-08 07:57:05 PDT
Created attachment 425508 [details]
Patch
Comment 2 Martin Robinson 2021-04-09 00:31:41 PDT
Created attachment 425591 [details]
Patch
Comment 3 Radar WebKit Bug Importer 2021-04-15 05:58:13 PDT
<rdar://problem/76699362>
Comment 4 Simon Fraser (smfr) 2021-05-05 09:51:35 PDT
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;
Comment 5 Martin Robinson 2021-05-06 01:32:28 PDT
Created attachment 427859 [details]
Patch
Comment 6 EWS 2021-05-06 05:11:22 PDT
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].
Comment 7 Martin Robinson 2021-05-17 09:13:20 PDT
Reopening to attach new patch.
Comment 8 Martin Robinson 2021-05-17 09:13:23 PDT
Created attachment 428836 [details]
Patch
Comment 9 Martin Robinson 2021-05-18 03:09:20 PDT
Comment on attachment 428836 [details]
Patch

Marking patch obsolete. This was uploaded to the incorrect bug.
Comment 10 Simon Fraser (smfr) 2021-06-21 21:29:05 PDT
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?
Comment 11 Simon Fraser (smfr) 2021-06-21 21:34:45 PDT
(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.
Comment 12 Martin Robinson 2021-06-22 02:06:10 PDT
(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