Bug 227949 – [css-scroll-snap] Don't snap to offscreen snap areas in unidirectional scrolls

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

[Martin Robinson]

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.

[Radar WebKit Bug Importer]

<rdar://problem/80895783>

[Martin Robinson]

Created attachment 435192 [details]
Patch

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

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?

[Martin Robinson]

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

[Martin Robinson]

Created attachment 435765 [details]
Patch

[Martin Robinson]

Created attachment 435766 [details]
Patch

[Martin Robinson]

After chatting with Frédéric, I better understand his suggestion and the latest version of the patch contains these.

[EWS]

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