Bug 227949

Summary: [css-scroll-snap] Don't snap to offscreen snap areas in unidirectional scrolls
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: 228023, 228141    
Bug Blocks: 145099    
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Martin Robinson 2021-07-14 07:08:16 PDT
Snap areas that are offscreen should not be viable snap targets. This will requiring tracking all snap areas for a particular snap offset and only snapping to that offset if one of those snap areas is within the viewport in the other axis.
Comment 1 Radar WebKit Bug Importer 2021-07-21 07:09:17 PDT
<rdar://problem/80895783>
Comment 2 Martin Robinson 2021-08-09 10:24:26 PDT
Created attachment 435192 [details]
Patch
Comment 3 Frédéric Wang (:fredw) 2021-08-17 23:01:21 PDT
Comment on attachment 435192 [details]
Patch

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

> Source/WebCore/page/scrolling/ScrollSnapOffsetsInfo.cpp:124
> +static void adjustPreviousAndNextForOnscreenSnapAreas(const InfoType& info, ScrollEventAxis axis, const SizeType& viewportSize, PointType destinationOffsetPoint, PotentialSnapPointSearchResult<UnitType>& searchResult)

nit: OnScreen?

> Source/WebCore/page/scrolling/ScrollSnapOffsetsInfo.cpp:135
> +    }

To avoid the type mismatch, I would rewrite this:

unsigned oldIndex = (*searchResult.previous).second;
searchResult.previous.reset();
for (unsigned i = 0; i <= oldIndex; i++) {
    const auto& snapOffset = snapOffsets[oldIndex - i];
    if (hasCompatibleSnapArea(info, snapOffset, axis, viewportSize, destinationOffsetPoint)) {
        searchResult.previous = std::make_pair(snapOffset.offset, oldIndex - i);
        break;
    }
}

> Source/WebCore/page/scrolling/ScrollSnapOffsetsInfo.cpp:144
> +        }

and this

unsigned oldIndex = (*searchResult.next).second;
searchResult.next.reset();
for (unsigned i = oldIndex, i < snapOffsets.size(); i++) {
    const auto& snapOffset = snapOffsets[i];
    if (hasCompatibleSnapArea(info, snapOffset, axis, viewportSize, destinationOffsetPoint)) {
        searchResult.next = std::make_pair(snapOffset.offset, i);
        break;
    }
}

> Source/WebCore/page/scrolling/ScrollSnapOffsetsInfo.cpp:162
> +    adjustPreviousAndNextForOnscreenSnapAreas<InfoType, LayoutType, PointType, SizeType>(info, axis, viewportSize, scrollDestinationOffsetPoint, searchResult);

OnScreen

> LayoutTests/imported/w3c/ChangeLog:10
> +        scrolling test no longer snaps because we don't have support for choosing between two candidates

"no longer": it was already not passing before this change right?
Comment 4 Martin Robinson 2021-08-18 06:32:48 PDT
(In reply to Frédéric Wang (:fredw) from comment #3)
> Comment on attachment 435192 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=435192&action=review
> 
> > Source/WebCore/page/scrolling/ScrollSnapOffsetsInfo.cpp:124
> > +static void adjustPreviousAndNextForOnscreenSnapAreas(const InfoType& info, ScrollEventAxis axis, const SizeType& viewportSize, PointType destinationOffsetPoint, PotentialSnapPointSearchResult<UnitType>& searchResult)
> 
> nit: OnScreen?

Okay. It does seem like "OnScreen" is more common in WebKit source than "Onscreen." I'll change these.

> 
> > Source/WebCore/page/scrolling/ScrollSnapOffsetsInfo.cpp:135
> > +    }
> 
> To avoid the type mismatch, I would rewrite this:
> 
> unsigned oldIndex = (*searchResult.previous).second;
> searchResult.previous.reset();
> for (unsigned i = 0; i <= oldIndex; i++) {
>     const auto& snapOffset = snapOffsets[oldIndex - i];
>     if (hasCompatibleSnapArea(info, snapOffset, axis, viewportSize,
> destinationOffsetPoint)) {
>         searchResult.previous = std::make_pair(snapOffset.offset, oldIndex -
> i);
>         break;
>     }
> }

Hrm. I agree that the type mismatch isn't ideal, but the issue is that I want to avoid calling hasCompatibleSnapArea as much as possible since it's possible expensive. I can add a comment to this affect though.

> > Source/WebCore/page/scrolling/ScrollSnapOffsetsInfo.cpp:144
> > +        }
> 
> and this
> 
> unsigned oldIndex = (*searchResult.next).second;
> searchResult.next.reset();
> for (unsigned i = oldIndex, i < snapOffsets.size(); i++) {
>     const auto& snapOffset = snapOffsets[i];
>     if (hasCompatibleSnapArea(info, snapOffset, axis, viewportSize,
> destinationOffsetPoint)) {
>         searchResult.next = std::make_pair(snapOffset.offset, i);
>         break;
>     }
> }

With this change, I think that snap areas that are further away would be selected over closer ones. I can adjust the code to make the oldIndex/index change you suggest though.

> 
> > Source/WebCore/page/scrolling/ScrollSnapOffsetsInfo.cpp:162
> > +    adjustPreviousAndNextForOnscreenSnapAreas<InfoType, LayoutType, PointType, SizeType>(info, axis, viewportSize, scrollDestinationOffsetPoint, searchResult);
> 
> OnScreen
> 
> > LayoutTests/imported/w3c/ChangeLog:10
> > +        scrolling test no longer snaps because we don't have support for choosing between two candidates
> 
> "no longer": it was already not passing before this change right?

It was failing, but due to the fact that it snapped to the wrong location. Now it simply does not snap.
Comment 5 Martin Robinson 2021-08-18 07:23:59 PDT
Created attachment 435765 [details]
Patch
Comment 6 Martin Robinson 2021-08-18 07:58:24 PDT
Created attachment 435766 [details]
Patch
Comment 7 Martin Robinson 2021-08-18 07:59:26 PDT
After chatting with Frédéric, I better understand his suggestion and the latest version of the patch contains these.
Comment 8 EWS 2021-08-18 08:59:55 PDT
Committed r281189 (240633@main): <https://commits.webkit.org/240633@main>

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