Bug 227300 - Mandatory scroll snapping doesn't work correctly in tables with position:sticky
Summary: Mandatory scroll snapping doesn't work correctly in tables with position:sticky
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Scrolling (show other bugs)
Version: Safari Technology Preview
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Martin Robinson
URL:
Keywords: InRadar
Depends on:
Blocks: 228989
  Show dependency treegraph
 
Reported: 2021-06-23 09:39 PDT by Simon Fraser (smfr)
Modified: 2021-09-27 00:09 PDT (History)
14 users (show)

See Also:


Attachments
Patch (9.07 KB, patch)
2021-09-23 03:42 PDT, Martin Robinson
no flags Details | Formatted Diff | Diff
Patch (9.18 KB, patch)
2021-09-23 04:58 PDT, Martin Robinson
no flags Details | Formatted Diff | Diff
Patch (10.19 KB, patch)
2021-09-24 07:53 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 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
<rdar://problem/79971196>
Comment 8 Martin Robinson 2021-07-07 02:16:25 PDT
This seems to be the relevant specification in this case:
https://drafts.csswg.org/css-position-3/#stickypos-scroll

> 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]
Patch
Comment 10 Martin Robinson 2021-09-23 04:58:35 PDT
Created attachment 439038 [details]
Patch
Comment 11 Simon Fraser (smfr) 2021-09-23 10:01:05 PDT
Comment on attachment 439038 [details]
Patch

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