Bug 229037

Summary: [GTK][WPE] CSS scroll snapping interacts badly with smooth scrolling
Product: WebKit Reporter: Chris Lord <clord>
Component: ScrollingAssignee: Chris Lord <clord>
Severity: Normal CC: achristensen, berto, cdumez, cgarcia, cmarcelo, darin, ews-watchlist, fred.wang, gustavo, jamesr, kbr, koivisto, luiz, mrobinson, simon.fraser, tonikitoo, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=231294
Bug Depends on: 230541    
Bug Blocks:    
Description Flags
ews-feeder: commit-queue-
Patch none

Description Chris Lord 2021-08-12 06:41:35 PDT
There are some issues with the combination of smooth scrolling and scroll-snapping;

With sync scrolling, smooth scrolling is disabled when scroll-snapping happens.
With sync scrolling and the touchpad, even the smallest movement immediately snaps to an element in that direction (making scrolling practically unusable).
With async scrolling, if you interrupt a scroll, snapping often doesn't work.
With async scrolling and touchpad scrolling, movement is erratic (perhaps fighting between snapping and respecting the input).

Both sets of issues seem to stem from not considering touch scrolling or animated/smooth scrolling, but may involve separate fixes.
Comment 1 Chris Lord 2021-08-12 06:44:29 PDT
Similarly, with synchronous scrolling and smooth scrolling enabled, sometimes mouse-wheel and keyboard scrolling stops working altogether. Touchpad scrolling behaves badly with sync scrolling, regardless of whether smooth scrolling is enabled.
Comment 2 Chris Lord 2021-08-20 02:52:06 PDT
Given they share very little code, I'm going to split this between the sync-scrolling path and the async.
Comment 3 Chris Lord 2021-08-20 03:04:16 PDT
Created attachment 435964 [details]
Comment 4 Chris Lord 2021-08-20 03:04:55 PDT
Attaching WIP patch for testing - need to write tests and verify this doesn't break anything else on EWS.
Comment 5 Chris Lord 2021-08-23 02:24:48 PDT
Created attachment 436169 [details]
Comment 6 Chris Lord 2021-09-14 05:39:39 PDT
Created attachment 438123 [details]
Comment 7 EWS Watchlist 2021-09-14 05:40:46 PDT
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See https://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Comment 8 Chris Lord 2021-09-16 04:39:25 PDT
Created attachment 438337 [details]
Comment 9 Simon Fraser (smfr) 2021-09-16 09:02:29 PDT
Comment on attachment 438337 [details]

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

Can you separate the setMouseWheelIsPrecise stuff into a patch and land that first? I'm in the middle of refactoring ScrollAnimator and would prefer no changes there for a week or so.

> Source/WebCore/platform/ScrollAnimator.cpp:408
> +    if (m_scrollAnimation->isActive())
> +        return true;

This is an example of how badly factored this code is. Ideally you would not need this and ScrollController would have the same behavior across platforms.
Comment 10 Chris Lord 2021-10-05 09:51:51 PDT
After the refactor, the fix for this fixes both platforms in both sync and async paths (nice!) Got it working locally, but need to alter/fix the related test, will finish tomorrow hopefully.
Comment 11 Chris Lord 2021-10-06 02:29:29 PDT
Created attachment 440347 [details]
Comment 12 Chris Lord 2021-10-06 02:40:07 PDT
Created attachment 440348 [details]
Comment 13 Simon Fraser (smfr) 2021-10-06 08:33:46 PDT
Comment on attachment 440348 [details]

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

> Source/WebCore/platform/ScrollingEffectsController.cpp:289
> +        float scale = m_client.pageScaleFactor();
> +        auto scrollOffset = m_client.scrollOffset();
> +        auto extents = m_client.scrollExtents();
> +
> +        auto originalOffset = LayoutPoint(scrollOffset.x() / scale, scrollOffset.y() / scale);
> +        auto newOffset = LayoutPoint((scrollOffset.x() + deltaX) / scale, (scrollOffset.y() + deltaY) / scale);
> +
> +        auto offsetX = snapOffsetsInfo()->closestSnapOffset(ScrollEventAxis::Horizontal, LayoutSize(extents.contentsSize), newOffset, deltaX, originalOffset.x()).first;
> +        auto offsetY = snapOffsetsInfo()->closestSnapOffset(ScrollEventAxis::Vertical, LayoutSize(extents.contentsSize), newOffset, deltaY, originalOffset.y()).first;
> +
> +        deltaX = (offsetX - originalOffset.x()) * scale;
> +        deltaY = (offsetY - originalOffset.y()) * scale;

Would be nice to push as much of this code into ScrollSnapAnimatorState as possible.

> Source/WebCore/platform/ScrollingEffectsController.cpp:367
> +    if (m_inScrollGesture || (m_currentAnimation && m_currentAnimation->isActive()))
> +        return true;

At some point I want a single "current state" value in ScrollingEffectsController so we know what the current animation is for.
Comment 14 Chris Lord 2021-10-06 08:45:04 PDT
Comment on attachment 440348 [details]

Will open a bug about de-duplicating the bits in ScrollSnapAnimatorState.
Comment 15 EWS 2021-10-06 09:06:44 PDT
Committed r283626 (242578@main): <https://commits.webkit.org/242578@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 440348 [details].
Comment 16 Radar WebKit Bug Importer 2021-10-06 09:07:16 PDT