Bug 218277 - Reconsider ScrollAnimator/ScrollController split
Summary: Reconsider ScrollAnimator/ScrollController split
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Scrolling (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-10-28 03:25 PDT by Martin Robinson
Modified: 2020-11-12 09:29 PST (History)
6 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Robinson 2020-10-28 03:25:33 PDT
See https://bugs.webkit.org/show_bug.cgi?id=217709

Currently ScrollAnimator is a platform-independent class used for animating scrollable areas. If rubber banding or scroll snap are enabled ScrollAnimator owns a ScrollController, which takes care of animating these particular behaviors. There are some issues with this design:

* It's unclear where the boundaries of the areas-of-concern are. Should state be stored in ScrollController or ScrollAnimator?
* Platform-specific code boundaries are a little fuzzy. There is Mac-specific code in ScrollController, ScrollAnimator, and ScrollAnimatorMac.
* We'd like to enable scroll snap support on other platforms, but probably not rubber banding. ScrollController currently manages both of those.
Comment 1 Martin Robinson 2020-10-28 03:29:19 PDT
So my spitball proposal for this is:

 1. Move state common to both rubber banding and scroll snap into ScrollAnimator.
 2. Split ScrollController into ScrollSnapController and RubberBandingController or ScrollSnapAnimator/RubberBandingAnimator.
 3. Try to push as much Mac-specific code as possible into ScrollAnimatorMac and ScrollSnapControllerMac/RubberBandingControllerMac.

There are probably other ways to do this and I'm definitely happy to hear them.
Comment 2 Simon Fraser (smfr) 2020-10-28 09:42:33 PDT
Yes, I'd love to redo this code. It's very confusing.
Comment 3 Martin Robinson 2020-10-30 09:21:20 PDT
Another thing to consider here is that scroll snap is a CSS feature and should be supported on all platforms. Perhaps we should move all scroll snap code into ScrollAnimator and rename ScrollController to RubberBandingController.
Comment 4 Simon Fraser (smfr) 2020-10-30 09:35:12 PDT
I definitely think that RubberBandingController is a good name (with platform-specific implementations).

Maybe we should have a ScrollSnapController.

It feels like we need a "coordination" class that can allow various delegates to affect the scroll, like rubber banding and scroll snap, but how those delegates interact will be hard to define.
Comment 5 Radar WebKit Bug Importer 2020-11-04 02:26:15 PST
<rdar://problem/71029811>
Comment 6 Simon Fraser (smfr) 2020-11-12 09:27:39 PST
Maybe:

ScrollAnimationController, which has as member variables
  ScrollSnapController
  RubberBandingController

ScrollAnimationController knows how to do an animated scroll to a position, via ScrollingMomentumCalculator.

ScrollAnimationController resolves conflicts between concurrent rubberbands/animated scrolls (if that happens).
Comment 7 Simon Fraser (smfr) 2020-11-12 09:29:47 PST
Maybe ScrollAnimationController has a delegate that allows it to delegate smooth scrolls to the platform (UIScrollView) via ScrollingTreeScrollingNodeDelegateIOS