Bug 229037 - [GTK][WPE] CSS scroll snapping interacts badly with smooth scrolling
Summary: [GTK][WPE] CSS scroll snapping interacts badly with smooth scrolling
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Scrolling (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Lord
URL:
Keywords: InRadar
Depends on: 230541
Blocks:
  Show dependency treegraph
 
Reported: 2021-08-12 06:41 PDT by Chris Lord
Modified: 2021-10-06 09:07 PDT (History)
17 users (show)

See Also:


Attachments
Patch (17.48 KB, patch)
2021-08-20 03:04 PDT, Chris Lord
no flags Details | Formatted Diff | Diff
Patch (15.99 KB, patch)
2021-08-23 02:24 PDT, Chris Lord
no flags Details | Formatted Diff | Diff
Patch (37.13 KB, patch)
2021-09-14 05:39 PDT, Chris Lord
no flags Details | Formatted Diff | Diff
Patch (38.07 KB, patch)
2021-09-16 04:39 PDT, Chris Lord
no flags Details | Formatted Diff | Diff
Patch (16.13 KB, patch)
2021-10-06 02:29 PDT, Chris Lord
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (16.21 KB, patch)
2021-10-06 02:40 PDT, Chris Lord
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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]
Patch
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]
Patch
Comment 6 Chris Lord 2021-09-14 05:39:39 PDT
Created attachment 438123 [details]
Patch
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]
Patch
Comment 9 Simon Fraser (smfr) 2021-09-16 09:02:29 PDT
Comment on attachment 438337 [details]
Patch

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]
Patch
Comment 12 Chris Lord 2021-10-06 02:40:07 PDT
Created attachment 440348 [details]
Patch
Comment 13 Simon Fraser (smfr) 2021-10-06 08:33:46 PDT
Comment on attachment 440348 [details]
Patch

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

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
<rdar://problem/83936557>