|Summary:||[GTK][WPE] CSS scroll snapping interacts badly with smooth scrolling|
|Product:||WebKit||Reporter:||Chris Lord <clord>|
|Component:||Scrolling||Assignee:||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|
|Version:||WebKit Nightly Build|
|Bug Depends on:||230541|
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 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 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 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 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].