Bug 228141

Summary: [css-scroll-snap] Consider all snap areas at a given snap offset when snapping
Product: WebKit Reporter: Martin Robinson <mrobinson>
Component: ScrollingAssignee: Martin Robinson <mrobinson>
Status: RESOLVED FIXED    
Severity: Normal CC: clopez, cmarcelo, ews-watchlist, fred.wang, jamesr, luiz, rego, simon.fraser, tonikitoo, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://github.com/web-platform-tests/wpt/pull/29794
Bug Depends on:    
Bug Blocks: 227949    
Attachments:
Description Flags
Patch
none
Patch none

Description Martin Robinson 2021-07-21 01:34:28 PDT
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.
Comment 1 Martin Robinson 2021-07-26 11:46:16 PDT
Created attachment 434220 [details]
Patch
Comment 2 EWS Watchlist 2021-07-26 11:47:30 PDT
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
Comment 3 Martin Robinson 2021-07-26 14:04:35 PDT
Created attachment 434235 [details]
Patch
Comment 4 Radar WebKit Bug Importer 2021-07-28 01:35:16 PDT
<rdar://problem/81205933>
Comment 5 Frédéric Wang (:fredw) 2021-07-28 02:52:54 PDT
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...)
Comment 6 Frédéric Wang (:fredw) 2021-07-28 06:43:29 PDT
Comment on attachment 434235 [details]
Patch

After talking to Martin, I can r+ this.
Comment 7 Martin Robinson 2021-07-28 10:01:23 PDT
(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.
Comment 8 EWS 2021-08-02 01:54:23 PDT
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].