Bug 218277

Summary:	Reconsider ScrollAnimator/ScrollController split
Product:	WebKit	Reporter:	Martin Robinson <mrobinson>
Component:	Scrolling	Assignee:	Nobody <webkit-unassigned>
Status:	NEW ---
Severity:	Normal	CC:	alex, cathiechen, fred.wang, rwlbuis, simon.fraser, 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=217709

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