Bug 230310

Summary: Unify the code paths for smooth scrolling, and scrolling to snap points
Product: WebKit Reporter: Simon Fraser (smfr) <simon.fraser>
Component: ScrollingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED CONFIGURATION CHANGED    
Severity: Normal CC: cathiechen, mrobinson, simon.fraser, webkit-bug-importer, wenson_hsieh
Priority: P2 Keywords: InRadar
Version: Safari Technology Preview   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=218857
Bug Depends on: 230385    
Bug Blocks:    

Simon Fraser (smfr)
Reported 2021-09-15 10:49:47 PDT
Smooth scrolling uses ScrollAnimator's ScrollAnimationSmooth. Snap point animations use ScrollingMomentumCalculator stuff. They should be the same.
Attachments
Simon Fraser (smfr)
Comment 1 2021-09-15 10:50:09 PDT
We have way too many scroll animators.
Simon Fraser (smfr)
Comment 2 2021-09-15 15:42:20 PDT
Maybe bug 218857 addresses this.
Martin Robinson
Comment 3 2021-09-16 03:10:02 PDT
I've done a quick inventory of all of the scroll animation classes and here's a summary: ScrollAnimator: WebProcess-only class providing operations that can scroll from one point to another and manages scroll snap / rubber banding via ScollingEffectsController. This is only used by ScrollableArea. I think it exists to avoid a layering violation, since it depends on platform-specific behavior. * ScrollAnimatorMac uses NSScrollAnimationHelper (private API) to scroll WebProcess-side scroll areas with animations. * ScrollAnimationSmooth: Generic implementation of smooth scrolling animation. This is used by ScrollAnimator for operations like "scroll to point with animation" on non-Mac platforms. On Mac this is used for CSSOM smooth scrolling (fixed by https://bugs.webkit.org/show_bug.cgi?id=218857). In addition, it's used in the UIProcess by the Nicosia scrolling tree. ScrollingEffectsCoordinator: Manages rubber banding and scroll snapping in both the WebProcess and UIProcess (scrolling tree). * ScrollSnapAnimatorState: Manages state for scroll snapping. Only used by ScrollingEffectsCoordinator. * ScrollingMomentumCalculator - Abstract base class that calculates movement for animations involving scrolling momentum. ScrollSnapAnimatorState uses this when ScrollingEffectsCoordinater sets up a momentum animation based on kinetic scrolling / scroll snap. Only Mac does this currently. * ScrollingMomentumCalculatorMac: Mac implementation of ScrollingMomentumCalculator which uses NSScrollingMomentumCalculatorSPI to calculate the momentum values. * BasicScrollingMomentumCalculator: Platform-independent implementation of ScrollingMomentumCalculator. I think this is unused, but could one day be used for kinetic scrolling on other ports. * ScrollAnimationKinetic: Kinetic scroll animation class that's only used for Nicosia Scrolling Tree kinetic animations. I'm unsure how much of this could be replaced by ScrollingEffectsCoordinator and BasicScrollingMomentumCalculator. I think there's lots of opportunities for simplification here. In particular it seems like ScrollingEffectsCoordinator / ScrollSnapAnimatorState / ScrollingMomentumCalculator could be collapsed down into a single class and greatly simplified. Another thing that could happen is that a lot of ScrollAnimator could move into ScrollableArea and the platform-dependent parts could be implemented as a delegate that only does the actual animation. These are just some initial ideas, but there's lots of room for improvement. The code is a bit convoluted, because we have to send information down to all of these helpers whenever scroll snap points change.
Simon Fraser (smfr)
Comment 4 2021-09-16 21:09:30 PDT
> a lot of ScrollAnimator could move into ScrollableArea Maybe, although it would be nice to think of snap points as a "scroll destination modifier", perhaps a ScrollableArea helper but not in the same class. We should see if there's scrolling tree code related to snap points that duplicates logic in ScrollAnimator. > ScrollAnimatorMac I intend to remove its m_scrollAnimationHelper and the delegate, and replace it with a ScrollAnimation subclass. After doing that, macOS just uses the m_scrollAnimation in the base class, and maybe ScrollAnimatorMac can mostly go away. I've love to avoid both ScrollAnimator and ScrollController having platform subclasses/implementations. That's a source of a lot of brain hurt. > ScrollAnimationKinetic See https://bugs.webkit.org/show_bug.cgi?id=230385. I want to add a macOS specific ScrollAnimationKinetic variant that uses ScrollingMomentumCalculator. Maybe ScrollAnimationKinetic can just use BasicScrollingMomentumCalculator (or move the BasicScrollingMomentumCalculator code into ScrollAnimationKinetic).
Radar WebKit Bug Importer
Comment 5 2021-09-22 10:50:17 PDT
Simon Fraser (smfr)
Comment 6 2021-10-10 10:22:01 PDT
These are both ScrollAnimation subclasses now. I think we can call this done.
Note You need to log in before you can comment on or make changes to this bug.