Bug 229037 – [GTK][WPE] CSS scroll snapping interacts badly with smooth scrolling

• Status: RESOLVED
• Resolution: FIXED
• Component: Scrolling

[Chris Lord]

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.

[Chris Lord]

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.

[Chris Lord]

Given they share very little code, I'm going to split this between the sync-scrolling path and the async.

[Chris Lord]

Created attachment 435964 [details]
Patch

[Chris Lord]

Attaching WIP patch for testing - need to write tests and verify this doesn't break anything else on EWS.

[Chris Lord]

Created attachment 436169 [details]
Patch

[Chris Lord]

Created attachment 438123 [details]
Patch

[EWS Watchlist]

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

[Chris Lord]

Created attachment 438337 [details]
Patch

[Simon Fraser (smfr)]

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.

[Chris Lord]

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.

[Chris Lord]

Created attachment 440347 [details]
Patch

[Chris Lord]

Created attachment 440348 [details]
Patch

[Simon Fraser (smfr)]

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.

[Chris Lord]

Comment on attachment 440348 [details]
Patch

Will open a bug about de-duplicating the bits in ScrollSnapAnimatorState.

[EWS]

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

[Radar WebKit Bug Importer]

<rdar://problem/83936557>