218277 2020-10-28 03:25:33 -0700 Reconsider ScrollAnimator/ScrollController split 2020-11-12 09:29:47 -0800 1 1 1 Unclassified WebKit Scrolling WebKit Nightly Build Unspecified Unspecified NEW https://bugs.webkit.org/show_bug.cgi?id=217709 InRadar P2 Normal --- 1 mrobinson webkit-unassigned alex cathiechen fred.wang rwlbuis simon.fraser webkit-bug-importer oldest_to_newest 1702260 0 mrobinson 2020-10-28 03:25:33 -0700 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. 1702261 1 mrobinson 2020-10-28 03:29:19 -0700 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. 1702344 2 simon.fraser 2020-10-28 09:42:33 -0700 Yes, I'd love to redo this code. It's very confusing. 1703083 3 mrobinson 2020-10-30 09:21:20 -0700 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. 1703095 4 simon.fraser 2020-10-30 09:35:12 -0700 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. 1704358 5 webkit-bug-importer 2020-11-04 02:26:15 -0800 <rdar://problem/71029811> 1706880 6 simon.fraser 2020-11-12 09:27:39 -0800 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). 1706883 7 simon.fraser 2020-11-12 09:29:47 -0800 Maybe ScrollAnimationController has a delegate that allows it to delegate smooth scrolls to the platform (UIScrollView) via ScrollingTreeScrollingNodeDelegateIOS