From fffbbf279edd44c0e6ee297d4bb3b4d4dec22199 Mon Sep 17 00:00:00 2001 From: Chip Weinberger Date: Mon, 25 Sep 2023 12:09:51 -0700 Subject: [PATCH] [Velocity Tracker] Fix: Issue 97761: Flutter Scrolling does not match iOS; inadvertent scrolling when user lifts up finger (#132291) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Issue **Issue:** https://github.com/flutter/flutter/issues/97761 https://github.com/flutter/flutter/assets/1863934/53c5e0df-b85a-483c-a17d-bddd18db3aa9 ## The Cause: The bug is very simple to understand - `velocity_tracker.dart` **only adds new samples while your finger is moving**. **Therefore**, if you move your finger quickly & (important) stop suddenly with no extra movement, the last 3 samples will all be > 0 dy. Regardless of how long you wait, you will get movement when you lift up your finger. **Logs from velocity_tracker.dart:** Notice: all 3 `_previousVelocityAt` are `dy > 0` despite a 2 second delay since the last scroll ``` // start moving finger flutter: addPosition dy:-464.0 flutter: addPosition dy:-465.0 flutter: addPosition dy:-466.0 flutter: addPosition dy:-467.0 flutter: addPosition dy:-468.0 flutter: addPosition dy:-469.0 flutter: addPosition dy:-470.0 // stop moving finger here, keep it still for 2 seconds & lift it up flutter: _previousVelocityAt(-2) samples(-467.0, -468.0)) dy:-176.772140710624 flutter: _previousVelocityAt(-1) samples(-468.0, -469.0)) dy:-375.0937734433609 flutter: _previousVelocityAt(0) samples(-469.0, -470.0)) dy:-175.71604287471447 flutter: primaryVelocity DragEndDetails(Velocity(0.0, -305.5)).primaryVelocity flutter: createBallisticSimulation pixels 464.16666666666663 velocity 305.4699824197211 ``` ## The Fix **There are 3 options to fix it:** A. sample uniformly *per unit time* (a larger more risky change, hurts battery life) B. consider elapsed time since the last sample. If greater than X, assume no more velocity. (easy & just as valid) C. similar to B, but instead add "ghost samples" of velocity zero, and run calculations as normal (a bit tricker, of dubious benefit imo) **For Option B I considered two approaches:** 1. _get the current timestamp and compare to event timestamp._ This is tricky because events are documented to use an arbitrary timescale & I wasn't able to find the code that generates the timestamps. This approach could be considered more. 2. _get a new timestamp using Stopwatch and compare now vs when the last sample was added._ This is the solution implemented here. There is a limitation in that we don't know when addSamples is called relative to the event. But, this estimation is already on a very low latency path & still it gives us a *minimum* time bound which is sufficient for comparison. **This PR chooses the simplest of the all solutions. Please try it our yourself, it completely solves the problem 😀** Option _B.1_ would be a nice alternative as well, if we can define and access the same timesource as the pointer tracker in a maintainable simple way. ## After Fix https://github.com/flutter/flutter/assets/1863934/be50d8e7-d5da-495a-a4af-c71bc541cbe3 --- .../lib/src/gestures/velocity_tracker.dart | 39 ++++++++++++++++++- .../test/gestures/velocity_tracker_test.dart | 18 +++++++++ 2 files changed, 56 insertions(+), 1 deletion(-) diff --git a/packages/flutter/lib/src/gestures/velocity_tracker.dart b/packages/flutter/lib/src/gestures/velocity_tracker.dart index ca7aa468a0..f761a1be23 100644 --- a/packages/flutter/lib/src/gestures/velocity_tracker.dart +++ b/packages/flutter/lib/src/gestures/velocity_tracker.dart @@ -149,12 +149,17 @@ class VelocityTracker { /// The kind of pointer this tracker is for. final PointerDeviceKind kind; + // Time difference since the last sample was added + final Stopwatch _sinceLastSample = Stopwatch(); + // Circular buffer; current sample at _index. final List<_PointAtTime?> _samples = List<_PointAtTime?>.filled(_historySize, null); int _index = 0; /// Adds a position as the given time to the tracker. void addPosition(Duration time, Offset position) { + _sinceLastSample.start(); + _sinceLastSample.reset(); _index += 1; if (_index == _historySize) { _index = 0; @@ -169,6 +174,16 @@ class VelocityTracker { /// /// Returns null if there is no data on which to base an estimate. VelocityEstimate? getVelocityEstimate() { + // no recent user movement? + if (_sinceLastSample.elapsedMilliseconds > VelocityTracker._assumePointerMoveStoppedMilliseconds) { + return const VelocityEstimate( + pixelsPerSecond: Offset.zero, + confidence: 1.0, + duration: Duration.zero, + offset: Offset.zero, + ); + } + final List x = []; final List y = []; final List w = []; @@ -195,7 +210,7 @@ class VelocityTracker { final double age = (newestSample.time - sample.time).inMicroseconds.toDouble() / 1000; final double delta = (sample.time - previousSample.time).inMicroseconds.abs().toDouble() / 1000; previousSample = sample; - if (age > _horizonMilliseconds || delta > _assumePointerMoveStoppedMilliseconds) { + if (age > _horizonMilliseconds || delta > VelocityTracker._assumePointerMoveStoppedMilliseconds) { break; } @@ -288,6 +303,8 @@ class IOSScrollViewFlingVelocityTracker extends VelocityTracker { @override void addPosition(Duration time, Offset position) { + _sinceLastSample.start(); + _sinceLastSample.reset(); assert(() { final _PointAtTime? previousPoint = _touchSamples[_index]; if (previousPoint == null || previousPoint.time <= time) { @@ -326,6 +343,16 @@ class IOSScrollViewFlingVelocityTracker extends VelocityTracker { @override VelocityEstimate getVelocityEstimate() { + // no recent user movement? + if (_sinceLastSample.elapsedMilliseconds > VelocityTracker._assumePointerMoveStoppedMilliseconds) { + return const VelocityEstimate( + pixelsPerSecond: Offset.zero, + confidence: 1.0, + duration: Duration.zero, + offset: Offset.zero, + ); + } + // The velocity estimated using this expression is an approximation of the // scroll velocity of an iOS scroll view at the moment the user touch was // released, not the final velocity of the iOS pan gesture recognizer @@ -387,6 +414,16 @@ class MacOSScrollViewFlingVelocityTracker extends IOSScrollViewFlingVelocityTrac @override VelocityEstimate getVelocityEstimate() { + // no recent user movement? + if (_sinceLastSample.elapsedMilliseconds > VelocityTracker._assumePointerMoveStoppedMilliseconds) { + return const VelocityEstimate( + pixelsPerSecond: Offset.zero, + confidence: 1.0, + duration: Duration.zero, + offset: Offset.zero, + ); + } + // The velocity estimated using this expression is an approximation of the // scroll velocity of a macOS scroll view at the moment the user touch was // released. diff --git a/packages/flutter/test/gestures/velocity_tracker_test.dart b/packages/flutter/test/gestures/velocity_tracker_test.dart index 3e6b94534b..61eebc13fe 100644 --- a/packages/flutter/test/gestures/velocity_tracker_test.dart +++ b/packages/flutter/test/gestures/velocity_tracker_test.dart @@ -144,4 +144,22 @@ void main() { } } }); + + test('Assume zero velocity when there are no recent samples', () async { + final IOSScrollViewFlingVelocityTracker tracker = IOSScrollViewFlingVelocityTracker(PointerDeviceKind.touch); + Offset position = Offset.zero; + Duration time = Duration.zero; + const Offset positionDelta = Offset(0, -1); + const Duration durationDelta = Duration(seconds: 1); + + for (int i = 0; i < 10; i+=1) { + position += positionDelta; + time += durationDelta; + tracker.addPosition(time, position); + } + + await Future.delayed(const Duration(milliseconds: 50)); + + expect(tracker.getVelocity().pixelsPerSecond, Offset.zero); + }); }