Bug 228141 – [css-scroll-snap] Consider all snap areas at a given snap offset when snapping

• Component: Scrolling
• Status: RESOLVED FIXED

[Martin Robinson]

When we process snap offsets we only record data about the first snap area at a given snap stop. Instead we should keep information about all snap areas because any of them might be one which is larger than the viewport and provides a range of possible valid snap offsets.

[Martin Robinson]

Created attachment 434220 [details]
Patch

[EWS Watchlist]

This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess

[Martin Robinson]

Created attachment 434235 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/81205933>

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

Comment on attachment 434235 [details]
Patch

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

> Source/WebCore/ChangeLog:12
> +        the viewport. This will avoid extra work on the most common usecase of large sets

"use case" in two words?

> Source/WebCore/ChangeLog:24
> +        (WebCore::convertOffsetInfo): Convert the list of snap ares.

areas*

> Source/WebCore/page/scrolling/ScrollSnapOffsetsInfo.cpp:232
> +    auto addOrUpdateStopForSnapOffset = [](HashMap<float, SnapOffset<LayoutUnit>>& offsets, LayoutUnit newOffset, ScrollSnapStop stop, bool hasSnapAreaLargerThanViewport, size_t snapAreaIndices)

s/snapAreaIndices/snapAreaIndex/

BTW, this is already present before you patch and could be done separately, but it looks like we can use a HashMap<LayoutUnit, SnapOffset<LayoutUnit>> here, and in the horizontalSnapOffsetsMap/verticalSnapOffsetsMap below.

> Source/WebCore/page/scrolling/ScrollSnapOffsetsInfo.cpp:234
> +        auto offset = offsets.ensure(newOffset, [&] {

Does HashMap::ensure have the same semantic as HashMap::add, except that it uses a functor? https://webkit-search.igalia.com/webkit/source/Source/WTF/wtf/HashMap.h#158

It's not clear to me why you can't keep add? Is it just to make init more convenient?

Also it seems the variable should still be called addResult.

> Source/WebCore/page/scrolling/ScrollSnapOffsetsInfo.cpp:243
> +        offset.iterator->value.snapAreaIndices.append(snapAreaIndices);

I'm think it's a bit confusing that some values are immediately initialized with the passed parameters and some are not. Instead, I would do

Try and add { newOffset, stop, hasSnapAreaLargerThanViewport, { snapAreaIndex } } to the HashMap
If there was already an entry (addResult.isNewEntry is false) then update stop, hasSnapAreaLargerThanViewport and snapAreaIndices accordingly.

Then the comment above can be updated to say we updated too to mention the new variables. 

IIUC, this assumes snapAreaIndices will always grow at each call and so hasSnapAreaLargerThanViewport can never become false again. Is it correct? In the past, snapAreaIndex was never updated after the first call.

> Source/WebCore/page/scrolling/ScrollSnapOffsetsInfo.h:80
> +    return a.offset == b.offset && a.stop == b.stop && a.snapAreaIndices == b.snapAreaIndices;

I guess this is implied by a.snapAreaIndices == b.snapAreaIndices, but shouldn't use also compare a.hasSnapAreaLargerThanViewport == b.hasSnapAreaLargerThanViewport ? (That can also make comparison slightly faster...)

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

Comment on attachment 434235 [details]
Patch

After talking to Martin, I can r+ this.

[Martin Robinson]

(In reply to Frédéric Wang (:fredw) from comment #5)
> Comment on attachment 434235 [details]
> Patch

Thanks for the review!

> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=434235&action=review
> 
> > Source/WebCore/ChangeLog:12
> > +        the viewport. This will avoid extra work on the most common usecase of large sets
> 
> "use case" in two words?

Fixed.


> > Source/WebCore/ChangeLog:24
> > +        (WebCore::convertOffsetInfo): Convert the list of snap ares.
> 
> areas*

Fixed.

> > Source/WebCore/page/scrolling/ScrollSnapOffsetsInfo.cpp:232
> > +    auto addOrUpdateStopForSnapOffset = [](HashMap<float, SnapOffset<LayoutUnit>>& offsets, LayoutUnit newOffset, ScrollSnapStop stop, bool hasSnapAreaLargerThanViewport, size_t snapAreaIndices)
> 
> s/snapAreaIndices/snapAreaIndex/

Fixed.

> Also it seems the variable should still be called addResult.

Fixed.

 
> Try and add { newOffset, stop, hasSnapAreaLargerThanViewport, {
> snapAreaIndex } } to the HashMap
> If there was already an entry (addResult.isNewEntry is false) then update
> stop, hasSnapAreaLargerThanViewport and snapAreaIndices accordingly.
> 
> Then the comment above can be updated to say we updated too to mention the
> new variables. 

In the end, I decided to remove this comment because it's a bit redundant.
 
> IIUC, this assumes snapAreaIndices will always grow at each call and so
> hasSnapAreaLargerThanViewport can never become false again. Is it correct?
> In the past, snapAreaIndex was never updated after the first call.
> 
> > Source/WebCore/page/scrolling/ScrollSnapOffsetsInfo.h:80
> > +    return a.offset == b.offset && a.stop == b.stop && a.snapAreaIndices == b.snapAreaIndices;
> 
> I guess this is implied by a.snapAreaIndices == b.snapAreaIndices, but
> shouldn't use also compare a.hasSnapAreaLargerThanViewport ==
> b.hasSnapAreaLargerThanViewport ? (That can also make comparison slightly
> faster...)

Fixed.

[EWS]

Committed r280527 (240159@main): <https://commits.webkit.org/240159@main>

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