Commit a navigator.pop as soon as the back swipe is lifted (#30422)

packages/flutter/lib/src/cupertino/route.dart

@@ -270,16 +270,10 @@ class CupertinoPageRoute<T> extends PageRoute<T> {
   static _CupertinoBackGestureController<T> _startPopGesture<T>(PageRoute<T> route) {
     assert(_isPopGestureEnabled(route));
 
-    _CupertinoBackGestureController<T> backController;
-    backController = _CupertinoBackGestureController<T>(
-      route: route,
+    return _CupertinoBackGestureController<T>(
+      navigator: route.navigator,
       controller: route.controller, // protected access
-      onEnded: () {
-        backController?.dispose();
-        backController = null;
-      },
     );
-    return backController;
   }
 
   /// Returns a [CupertinoFullscreenDialogTransition] if [route] is a full
@@ -592,18 +586,15 @@ class _CupertinoBackGestureController<T> {
   ///
   /// The [navigator] and [controller] arguments must not be null.
   _CupertinoBackGestureController({
-    @required this.route,
+    @required this.navigator,
     @required this.controller,
-    @required this.onEnded,
-  }) : assert(route != null), assert(controller != null), assert(onEnded != null) {
-    route.navigator.didStartUserGesture();
+  }) : assert(navigator != null),
+       assert(controller != null) {
+    navigator.didStartUserGesture();
   }
 
-  final PageRoute<T> route;
   final AnimationController controller;
-  final VoidCallback onEnded;
-
-  bool _animating = false;
+  final NavigatorState navigator;
 
   /// The drag gesture has changed by [fractionalDelta]. The total range of the
   /// drag should be 0.0 to 1.0.
@@ -640,36 +631,28 @@ class _CupertinoBackGestureController<T> {
       );
       controller.animateTo(1.0, duration: Duration(milliseconds: droppedPageForwardAnimationTime), curve: animationCurve);
     } else {
-      final int droppedPageBackAnimationTime = lerpDouble(0, _kMaxDroppedSwipePageForwardAnimationTime, controller.value).floor();
-      controller.animateBack(0.0, duration: Duration(milliseconds: droppedPageBackAnimationTime), curve: animationCurve);
-    }
+      // This route is destined to pop at this point. Reuse navigator's pop.
+      navigator.pop();
 
+      // The popping may have finished inline if already at the target destination.
       if (controller.isAnimating) {
-      // Don't end the gesture until the transition completes.
-      _animating = true;
-      controller.addStatusListener(_handleStatusChanged);
-    } else {
-      // Animate calls could return inline if already at the target destination
-      // value.
-      return _handleStatusChanged(controller.status);
+        // Otherwise, use a custom popping animation duration and curve.
+        final int droppedPageBackAnimationTime = lerpDouble(0, _kMaxDroppedSwipePageForwardAnimationTime, controller.value).floor();
+        controller.animateBack(0.0, duration: Duration(milliseconds: droppedPageBackAnimationTime), curve: animationCurve);
       }
-
     }
 
-  void _handleStatusChanged(AnimationStatus status) {
-    if (_animating) {
-      controller.removeStatusListener(_handleStatusChanged);
-    }
-    _animating = false;
-    onEnded();
-    if (status == AnimationStatus.dismissed)
-      route.navigator.removeRoute(route); // This also disposes the route.
+    if (controller.isAnimating) {
+      // Keep the userGestureInProgress in true state so we don't change the
+      // curve of the page transition mid-flight since CupertinoPageTransition
+      // depends on userGestureInProgress.
+      AnimationStatusListener animationStatusCallback;
+      animationStatusCallback = (AnimationStatus status) {
+        navigator.didStopUserGesture();
+        controller.removeStatusListener(animationStatusCallback);
+      };
+      controller.addStatusListener(animationStatusCallback);
     }
-
-  void dispose() {
-    if (_animating)
-      controller.removeStatusListener(_handleStatusChanged);
-    route.navigator?.didStopUserGesture();
   }
 }
 

packages/flutter/lib/src/widgets/heroes.dart

@@ -620,6 +620,9 @@ class HeroController extends NavigatorObserver {
   void didPop(Route<dynamic> route, Route<dynamic> previousRoute) {
     assert(navigator != null);
     assert(route != null);
+    // Don't trigger another flight when a pop is committed as a user gesture
+    // back swipe is snapped.
+    if (!navigator.userGestureInProgress)
       _maybeStartHeroTransition(route, previousRoute, HeroFlightDirection.pop, false);
   }
 

packages/flutter/test/cupertino/page_test.dart

@@ -462,39 +462,6 @@ void main() {
     expect(find.text('Page 1'), findsNothing);
     expect(find.text('Page 2'), isOnstage);
   });
-
-  testWidgets('test edge swipe then drop back at ending point works', (WidgetTester tester) async {
-    await tester.pumpWidget(
-      CupertinoApp(
-        onGenerateRoute: (RouteSettings settings) {
-          return CupertinoPageRoute<void>(
-            settings: settings,
-            builder: (BuildContext context) {
-              final String pageNumber = settings.name == '/' ? '1' : '2';
-              return Center(child: Text('Page $pageNumber'));
-            },
-          );
-        },
-      ),
-    );
-
-    tester.state<NavigatorState>(find.byType(Navigator)).pushNamed('/next');
-
-    await tester.pump();
-    await tester.pump(const Duration(seconds: 1));
-
-    expect(find.text('Page 1'), findsNothing);
-    expect(find.text('Page 2'), isOnstage);
-
-    final TestGesture gesture = await tester.startGesture(const Offset(5, 200));
-    // The width of the page.
-    await gesture.moveBy(const Offset(800, 0));
-    await gesture.up();
-    await tester.pump();
-
-    expect(find.text('Page 1'), isOnstage);
-    expect(find.text('Page 2'), findsNothing);
-  });
 }
 
 class RtlOverrideWidgetsDelegate extends LocalizationsDelegate<WidgetsLocalizations> {

packages/flutter/test/cupertino/route_test.dart

@@ -6,7 +6,15 @@ import 'package:flutter/cupertino.dart';
 import 'package:flutter/rendering.dart';
 import 'package:flutter_test/flutter_test.dart';
 
+import 'package:mockito/mockito.dart';
+
 void main() {
+  MockNavigatorObserver navigatorObserver;
+
+  setUp(() {
+    navigatorObserver = MockNavigatorObserver();
+  });
+
   testWidgets('Middle auto-populates with title', (WidgetTester tester) async {
     await tester.pumpWidget(
       const CupertinoApp(
@@ -579,10 +587,11 @@ void main() {
     expect(tester.getTopLeft(find.text('2')).dx, moreOrLessEquals(787, epsilon: 1));
   });
 
-  testWidgets('Snapped drags forwards and backwards should signal didStopUserGesture', (WidgetTester tester) async {
+  testWidgets('Snapped drags forwards and backwards should signal didStart/StopUserGesture', (WidgetTester tester) async {
     final GlobalKey<NavigatorState> navigatorKey = GlobalKey();
     await tester.pumpWidget(
       CupertinoApp(
+        navigatorObservers: <NavigatorObserver>[navigatorObserver],
         navigatorKey: navigatorKey,
         home: const Text('1'),
       ),
@@ -598,8 +607,10 @@ void main() {
 
     navigatorKey.currentState.push(route2);
     await tester.pumpAndSettle();
+    verify(navigatorObserver.didPush(any, any)).called(greaterThanOrEqualTo(1));
 
     await tester.dragFrom(const Offset(5, 100), const Offset(100, 0));
+    verify(navigatorObserver.didStartUserGesture(any, any)).called(1);
     await tester.pump();
     expect(tester.getTopLeft(find.text('2')).dx, moreOrLessEquals(100));
     expect(navigatorKey.currentState.userGestureInProgress, true);
@@ -610,11 +621,14 @@ void main() {
     // Back to the page covering the whole screen.
     expect(tester.getTopLeft(find.text('2')).dx, moreOrLessEquals(0));
     expect(navigatorKey.currentState.userGestureInProgress, false);
+    verify(navigatorObserver.didStopUserGesture()).called(1);
+    verifyNever(navigatorObserver.didPop(any, any));
 
     await tester.dragFrom(const Offset(5, 100), const Offset(500, 0));
     await tester.pump();
     expect(tester.getTopLeft(find.text('2')).dx, moreOrLessEquals(500));
     expect(navigatorKey.currentState.userGestureInProgress, true);
+    verify(navigatorObserver.didPop(any, any)).called(1);
 
     // Did go far enough to snap out of this route.
     await tester.pump(const Duration(milliseconds: 301));
@@ -624,4 +638,42 @@ void main() {
     expect(tester.getTopLeft(find.text('1')).dx, moreOrLessEquals(0));
     expect(navigatorKey.currentState.userGestureInProgress, false);
   });
+
+  /// Regression test for https://github.com/flutter/flutter/issues/29596.
+  testWidgets('test edge swipe then drop back at ending point works', (WidgetTester tester) async {
+    await tester.pumpWidget(
+      CupertinoApp(
+        navigatorObservers: <NavigatorObserver>[navigatorObserver],
+        onGenerateRoute: (RouteSettings settings) {
+          return CupertinoPageRoute<void>(
+            settings: settings,
+            builder: (BuildContext context) {
+              final String pageNumber = settings.name == '/' ? '1' : '2';
+              return Center(child: Text('Page $pageNumber'));
+            },
+          );
+        },
+      ),
+    );
+
+    tester.state<NavigatorState>(find.byType(Navigator)).pushNamed('/next');
+
+    await tester.pump();
+    await tester.pump(const Duration(seconds: 1));
+
+    expect(find.text('Page 1'), findsNothing);
+    expect(find.text('Page 2'), isOnstage);
+
+    final TestGesture gesture = await tester.startGesture(const Offset(5, 200));
+    // The width of the page.
+    await gesture.moveBy(const Offset(800, 0));
+    await gesture.up();
+    await tester.pump();
+
+    expect(find.text('Page 1'), isOnstage);
+    expect(find.text('Page 2'), findsNothing);
+    verify(navigatorObserver.didPop(any, any)).called(1);
+  });
 }
+
+class MockNavigatorObserver extends Mock implements NavigatorObserver {}

packages/flutter/test/widgets/heroes_test.dart

@@ -3,6 +3,7 @@
 // found in the LICENSE file.
 
 import 'package:flutter_test/flutter_test.dart';
+import 'package:flutter/cupertino.dart';
 import 'package:flutter/material.dart';
 import 'package:flutter/rendering.dart';
 
@@ -1779,4 +1780,67 @@ void main() {
     expect(find.byKey(smallContainer), isInCard);
     expect(tester.getSize(find.byKey(smallContainer)), const Size(100,100));
   });
+
+  testWidgets('On an iOS back swipe and snap, only a single flight should take place', (WidgetTester tester) async {
+    int shuttlesBuilt = 0;
+    final HeroFlightShuttleBuilder shuttleBuilder = (
+      BuildContext flightContext,
+      Animation<double> animation,
+      HeroFlightDirection flightDirection,
+      BuildContext fromHeroContext,
+      BuildContext toHeroContext,
+    ) {
+      shuttlesBuilt += 1;
+      return const Text("I'm flying in a jetplane");
+    };
+
+    final GlobalKey<NavigatorState> navigatorKey = GlobalKey();
+    await tester.pumpWidget(
+      CupertinoApp(
+        navigatorKey: navigatorKey,
+        home: Hero(
+          tag: navigatorKey,
+          // Since we're popping, only the destination route's builder is used.
+          flightShuttleBuilder: shuttleBuilder,
+          transitionOnUserGestures: true,
+          child: const Text('1')
+        ),
+      ),
+    );
+
+    final CupertinoPageRoute<void> route2 = CupertinoPageRoute<void>(
+      builder: (BuildContext context) {
+        return CupertinoPageScaffold(
+          child: Hero(
+            tag: navigatorKey,
+            transitionOnUserGestures: true,
+            child: const Text('2')
+          ),
+        );
+      }
+    );
+
+    navigatorKey.currentState.push(route2);
+    await tester.pumpAndSettle();
+
+    expect(shuttlesBuilt, 1);
+
+    final TestGesture gesture = await tester.startGesture(const Offset(5.0, 200.0));
+    await gesture.moveBy(const Offset(500.0, 0.0));
+    await tester.pump();
+    // Starting the back swipe creates a new hero shuttle.
+    expect(shuttlesBuilt, 2);
+
+    await gesture.up();
+    await tester.pump();
+    // After the lift, no additional shuttles should be created since it's the
+    // same hero flight.
+    expect(shuttlesBuilt, 2);
+
+    // Did go far enough to snap out of this route.
+    await tester.pump(const Duration(milliseconds: 301));
+    expect(find.text('2'), findsNothing);
+    // Still one shuttle.
+    expect(shuttlesBuilt, 2);
+  });
 }