Unverified Commit 0d6927cb authored by Luke Hutchison's avatar Luke Hutchison Committed by GitHub

Fix refresh cancelation (#139535)

Changes drag release logic so that an armed refresh is only canceled if the user has scrolled back up beyond the point where the refresh indicator was armed. (Fixes https://github.com/flutter/flutter/issues/138848.)

This is the minimal change I found could be made to restore something like the behavior that I would expect.

This may still need a bit of work, because it only masks the second issue I mentioned, that releasing a drag can cause the scroll position to be animated back up from the release point. There is actually a bug about that: https://github.com/flutter/flutter/issues/6052. I would like to see that bug fixed too. This PR doesn't address that, but makes it harder to hit that issue.

@Piinks this is a recreation of #139015 (since I couldn't figure out some issue with a git detached branch, so I fixed the PR and I'm re-submitting it). This version includes one line that was somehow accidentally dropped from the original PR. This will hopefully fix the test failures.

However, I don't have a clue how to write a test for a Flutter UI widget. I'll try to figure that out, but also I don't have a lot of time to work on this. I would appreciate at least some user testing to verify that the new behavior is much more intuitive than the old behavior.

- [?] All existing and new tests are passing.
parent c2286a76
......@@ -372,10 +372,6 @@ class RefreshIndicatorState extends State<RefreshIndicator> with TickerProviderS
}
} else if (notification is ScrollUpdateNotification) {
if (_mode == _RefreshIndicatorMode.drag || _mode == _RefreshIndicatorMode.armed) {
if ((notification.metrics.axisDirection == AxisDirection.down && notification.metrics.extentBefore > 0.0)
|| (notification.metrics.axisDirection == AxisDirection.up && notification.metrics.extentAfter > 0.0)) {
_dismiss(_RefreshIndicatorMode.canceled);
} else {
if (notification.metrics.axisDirection == AxisDirection.down) {
_dragOffset = _dragOffset! - notification.scrollDelta!;
} else if (notification.metrics.axisDirection == AxisDirection.up) {
......@@ -383,7 +379,6 @@ class RefreshIndicatorState extends State<RefreshIndicator> with TickerProviderS
}
_checkDragOffset(notification.metrics.viewportDimension);
}
}
if (_mode == _RefreshIndicatorMode.armed && notification.dragDetails == null) {
// On iOS start the refresh when the Scrollable bounces back from the
// overscroll (ScrollNotification indicating this don't have dragDetails
......@@ -402,7 +397,11 @@ class RefreshIndicatorState extends State<RefreshIndicator> with TickerProviderS
} else if (notification is ScrollEndNotification) {
switch (_mode) {
case _RefreshIndicatorMode.armed:
if (_positionController.value < 1.0) {
_dismiss(_RefreshIndicatorMode.canceled);
} else {
_show();
}
case _RefreshIndicatorMode.drag:
_dismiss(_RefreshIndicatorMode.canceled);
case _RefreshIndicatorMode.canceled:
......
......@@ -252,7 +252,7 @@ void main() {
),
);
await tester.fling(find.text('X'), const Offset(0.0, 100.0), 1000.0);
await tester.fling(find.text('X'), const Offset(0.0, 200.0), 1000.0);
await tester.pump();
await tester.pump(const Duration(seconds: 1));
await tester.pump(const Duration(seconds: 1));
......@@ -260,6 +260,76 @@ void main() {
expect(refreshCalled, true);
});
testWidgets('RefreshIndicator - drag back not far enough to cancel', (WidgetTester tester) async {
refreshCalled = false;
await tester.pumpWidget(
MaterialApp(
home: RefreshIndicator(
onRefresh: refresh,
child: ListView(
physics: const AlwaysScrollableScrollPhysics(),
children: const <Widget>[
SizedBox(
height: 200.0,
child: Text('X'),
),
SizedBox(height: 1000),
],
),
),
),
);
final Offset startLocation = tester.getCenter(find.text('X'), warnIfMissed: true, callee: 'drag');
final TestPointer testPointer = TestPointer();
await tester.sendEventToBinding(testPointer.down(startLocation));
await tester.sendEventToBinding(testPointer.move(startLocation + const Offset(0.0, 175)));
await tester.pump();
await tester.sendEventToBinding(testPointer.move(startLocation + const Offset(0.0, 150)));
await tester.pump();
await tester.sendEventToBinding(testPointer.up());
await tester.pump();
await tester.pump(const Duration(seconds: 1));
await tester.pump(const Duration(seconds: 1));
await tester.pump(const Duration(seconds: 1));
expect(refreshCalled, true);
});
testWidgets('RefreshIndicator - drag back far enough to cancel', (WidgetTester tester) async {
refreshCalled = false;
await tester.pumpWidget(
MaterialApp(
home: RefreshIndicator(
onRefresh: refresh,
child: ListView(
physics: const AlwaysScrollableScrollPhysics(),
children: const <Widget>[
SizedBox(
height: 200.0,
child: Text('X'),
),
SizedBox(height: 1000),
],
),
),
),
);
final Offset startLocation = tester.getCenter(find.text('X'), warnIfMissed: true, callee: 'drag');
final TestPointer testPointer = TestPointer();
await tester.sendEventToBinding(testPointer.down(startLocation));
await tester.sendEventToBinding(testPointer.move(startLocation + const Offset(0.0, 175)));
await tester.pump();
await tester.sendEventToBinding(testPointer.move(startLocation + const Offset(0.0, 149)));
await tester.pump();
await tester.sendEventToBinding(testPointer.up());
await tester.pump();
await tester.pump(const Duration(seconds: 1));
await tester.pump(const Duration(seconds: 1));
await tester.pump(const Duration(seconds: 1));
expect(refreshCalled, false);
});
testWidgets('RefreshIndicator - show - slow', (WidgetTester tester) async {
refreshCalled = false;
await tester.pumpWidget(
......
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