Bug 227949 - [css-scroll-snap] Don't snap to offscreen snap areas in unidirectional scrolls
Summary: [css-scroll-snap] Don't snap to offscreen snap areas in unidirectional scrolls
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Scrolling (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Martin Robinson
URL:
Keywords: InRadar
Depends on: 228023 228141
Blocks: 145099
  Show dependency treegraph
 
Reported: 2021-07-14 07:08 PDT by Martin Robinson
Modified: 2021-08-18 08:59 PDT (History)
8 users (show)

See Also:


Attachments
Patch (10.02 KB, patch)
2021-08-09 10:24 PDT, Martin Robinson
no flags Details | Formatted Diff | Diff
Patch (10.35 KB, patch)
2021-08-18 07:23 PDT, Martin Robinson
no flags Details | Formatted Diff | Diff
Patch (10.44 KB, patch)
2021-08-18 07:58 PDT, Martin Robinson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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].