Unverified Commit fffbbf27 authored by Chip Weinberger's avatar Chip Weinberger Committed by GitHub

[Velocity Tracker] Fix: Issue 97761: Flutter Scrolling does not match iOS;...

[Velocity Tracker] Fix: Issue 97761: Flutter Scrolling does not match iOS; inadvertent scrolling when user lifts up finger (#132291)

## 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
parent 52914c76
......@@ -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<double> x = <double>[];
final List<double> y = <double>[];
final List<double> w = <double>[];
......@@ -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.
......
......@@ -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<void>.delayed(const Duration(milliseconds: 50));
expect(tracker.getVelocity().pixelsPerSecond, Offset.zero);
});
}
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment