Bug 227300

Summary: Mandatory scroll snapping doesn't work correctly in tables with position:sticky
Product: WebKit Reporter: Simon Fraser (smfr) <simon.fraser>
Component: ScrollingAssignee: Martin Robinson <mrobinson>
Severity: Normal CC: changseok, cmarcelo, esprehn+autocc, ews-watchlist, fred.wang, glenn, jamesr, kondapallykalyan, luiz, mrobinson, pdr, simon.fraser, tonikitoo, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: Safari Technology Preview   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 228989    
Description Flags
Patch none

Description Simon Fraser (smfr) 2021-06-23 09:39:03 PDT
In this testcase https://codepen.io/snewcomer/pen/wvJLyzj the snapping in X doesn't behave like mandatory, when it should.
Comment 1 Martin Robinson 2021-06-24 01:24:32 PDT
This seems broken in STP, but works for me in trunk. Perhaps this was fixed by a recent change?
Comment 2 Simon Fraser (smfr) 2021-06-24 11:14:56 PDT
I can reproduce at r279166.
Comment 3 Martin Robinson 2021-06-24 11:58:29 PDT
Ah, after a bit more debugging, I think the key thing here is asynchronous overflow scrolling. When I have it turned off, snapping works as expected. I suspect that the code is not snapping where it should be at some point.
Comment 4 Martin Robinson 2021-06-25 07:19:46 PDT
Similarly to https://bugs.chromium.org/p/chromium/issues/detail?id=835301 what seems to be happening here is that the `position: sticky` table headers are always at the current scroll offset. This means that these cells are always chosen as the destination of the snap operation, which means that we always snap to the current snap offset!

I think that the solution here is to ensure that when we calculate snap points, we should not take into account sticky positioning of children.
Comment 5 Martin Robinson 2021-06-25 07:20:55 PDT
It seems what might be happening in this case is that a layout is happening in between the scroll operation and the snap operation, which seems a bit wrong.
Comment 6 Martin Robinson 2021-06-25 07:22:15 PDT
(In reply to Martin Robinson from comment #5)
> It seems what might be happening in this case is that a layout is happening
> in between the scroll operation and the snap operation, which seems a bit
> wrong.

Ah, no this is correct. The scroll position / layout will happen while fingers are down on the touchpad, but the snap won't happen until fingers come up (ie after sticky repositioning).
Comment 7 Radar WebKit Bug Importer 2021-06-30 09:40:18 PDT
Comment 8 Martin Robinson 2021-07-07 02:16:25 PDT
This seems to be the relevant specification in this case:

> For the purposes of any operation targeting the scroll position
> of a sticky positioned element (or one of its descendants), the sticky 
> positioned element must be considered to be positioned at its initial
> (non-offsetted) position

My reading of this is that regardless of the whether asynchronous overflow scrolling is on, we should use the original (non-offsetted) position of the sticky-positioned element for the snap point.
Comment 9 Martin Robinson 2021-09-23 03:42:25 PDT
Created attachment 439031 [details]
Comment 10 Martin Robinson 2021-09-23 04:58:35 PDT
Created attachment 439038 [details]
Comment 11 Simon Fraser (smfr) 2021-09-23 10:01:05 PDT
Comment on attachment 439038 [details]

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

You'll need to skip the test on iOS.

> Source/WebCore/page/scrolling/ScrollSnapOffsetsInfo.cpp:334
> +        OptionSet<MapCoordinatesMode> options;
> +        options.add(UseTransforms);
> +        options.add(IgnoreStickyOffsets);

OptionSet<MapCoordinatesMode> options = { UseTransforms, IgnoreStickyOffsets };
Comment 12 Martin Robinson 2021-09-24 07:53:04 PDT
Created attachment 439146 [details]
Comment 13 EWS 2021-09-27 00:08:55 PDT
Committed r283100 (242158@main): <https://commits.webkit.org/242158@main>

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