Summary: | [GTK][WPE] CSS scroll snapping interacts badly with smooth scrolling | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Lord <clord> | ||||||||||||||
Component: | Scrolling | Assignee: | Chris Lord <clord> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
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: | |||||||||||||||||
Attachments: |
|
Description
Chris Lord
2021-08-12 06:41:35 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. Given they share very little code, I'm going to split this between the sync-scrolling path and the async. Created attachment 435964 [details]
Patch
Attaching WIP patch for testing - need to write tests and verify this doesn't break anything else on EWS. Created attachment 436169 [details]
Patch
Created attachment 438123 [details]
Patch
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 Created attachment 438337 [details]
Patch
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. 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. Created attachment 440347 [details]
Patch
Created attachment 440348 [details]
Patch
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 on attachment 440348 [details]
Patch
Will open a bug about de-duplicating the bits in ScrollSnapAnimatorState.
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]. |