Navigator pushAndRemoveUntil Fix (#35223)

* Initial work

* ++

* Updated tests

* Moved TestObserver out for access across tests.

* ++

* Added hero tests

* Review feedback

* simplified preceding route overlay

* Review feedback

* trailing doc slash

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

@@ -1154,14 +1154,14 @@ class Navigator extends StatefulWidget {
   /// The removed routes are removed without being completed, so this method
   /// does not take a return value argument.
   ///
-  /// The new route and the route below the bottommost removed route (which
-  /// becomes the route below the new route) are notified (see [Route.didPush]
-  /// and [Route.didChangeNext]). If the [Navigator] has any
-  /// [Navigator.observers], they will be notified as well (see
-  /// [NavigatorObservers.didPush] and [NavigatorObservers.didRemove]). The
-  /// removed routes are disposed, without being notified, once the new route
-  /// has finished animating. The futures that had been returned from pushing
-  /// those routes will not complete.
+  /// The newly pushed route and its preceding route are notified for
+  /// [Route.didPush]. After removal, the new route and its new preceding route,
+  /// (the route below the bottommost removed route) are notified through
+  /// [Route.didChangeNext]). If the [Navigator] has any [Navigator.observers],
+  /// they will be notified as well (see [NavigatorObservers.didPush] and
+  /// [NavigatorObservers.didRemove]). The removed routes are disposed of and
+  /// notified, once the new route has finished animating. The futures that had
+  /// been returned from pushing those routes will not complete.
   ///
   /// Ongoing gestures within the current route are canceled when a new route is
   /// pushed.
@@ -1195,7 +1195,7 @@ class Navigator extends StatefulWidget {
   /// context with a new route.
   ///
   /// {@template flutter.widgets.navigator.replace}
-  /// The old route must not be current visible, as this method skips the
+  /// The old route must not be currently visible, as this method skips the
   /// animations and therefore the removal would be jarring if it was visible.
   /// To replace the top-most route, consider [pushReplacement] instead, which
   /// _does_ animate the new route, and delays removing the old route until the
@@ -1881,6 +1881,12 @@ class NavigatorState extends State<Navigator> with TickerProviderStateMixin {
   Future<T> pushAndRemoveUntil<T extends Object>(Route<T> newRoute, RoutePredicate predicate) {
     assert(!_debugLocked);
     assert(() { _debugLocked = true; return true; }());
+
+    // The route that is being pushed on top of
+    final Route<dynamic> precedingRoute = _history.isNotEmpty ? _history.last : null;
+    final OverlayEntry precedingRouteOverlay = _currentOverlayEntry;
+
+    // Routes to remove
     final List<Route<dynamic>> removedRoutes = <Route<dynamic>>[];
     while (_history.isNotEmpty && !predicate(_history.last)) {
       final Route<dynamic> removedRoute = _history.removeLast();
@@ -1888,26 +1894,33 @@ class NavigatorState extends State<Navigator> with TickerProviderStateMixin {
       assert(removedRoute.overlayEntries.isNotEmpty);
       removedRoutes.add(removedRoute);
     }
+
+    // Push new route
     assert(newRoute._navigator == null);
     assert(newRoute.overlayEntries.isEmpty);
-    final Route<dynamic> oldRoute = _history.isNotEmpty ? _history.last : null;
+    final Route<dynamic> newPrecedingRoute = _history.isNotEmpty ? _history.last : null;
     newRoute._navigator = this;
-    newRoute.install(_currentOverlayEntry);
+    newRoute.install(precedingRouteOverlay);
     _history.add(newRoute);
+
     newRoute.didPush().whenCompleteOrCancel(() {
       if (mounted) {
-        for (Route<dynamic> route in removedRoutes)
-          route.dispose();
+        for (Route<dynamic> removedRoute in removedRoutes) {
+          for (NavigatorObserver observer in widget.observers)
+            observer.didRemove(removedRoute, newPrecedingRoute);
+          removedRoute.dispose();
+        }
+
+        if (newPrecedingRoute != null)
+          newPrecedingRoute.didChangeNext(newRoute);
       }
     });
+
+    // Notify for newRoute
     newRoute.didChangeNext(null);
-    if (oldRoute != null)
-      oldRoute.didChangeNext(newRoute);
-    for (NavigatorObserver observer in widget.observers) {
-      observer.didPush(newRoute, oldRoute);
-      for (Route<dynamic> removedRoute in removedRoutes)
-        observer.didRemove(removedRoute, oldRoute);
-    }
+    for (NavigatorObserver observer in widget.observers)
+      observer.didPush(newRoute, precedingRoute);
+
     assert(() { _debugLocked = false; return true; }());
     _afterNavigation(newRoute);
     return newRoute.popped;

packages/flutter/test/widgets/navigator_replacement_test.dart

@@ -5,6 +5,8 @@
 import 'package:flutter_test/flutter_test.dart';
 import 'package:flutter/material.dart';
 
+import 'observer_tester.dart';
+
 void main() {
   testWidgets('Back during pushReplacement', (WidgetTester tester) async {
     await tester.pumpWidget(MaterialApp(
@@ -39,42 +41,185 @@ void main() {
     expect(find.text('home'), findsOneWidget);
   });
 
-  testWidgets('pushAndRemoveUntil', (WidgetTester tester) async {
-    await tester.pumpWidget(MaterialApp(
-      home: const Material(child: Text('home')),
-      routes: <String, WidgetBuilder>{
-        '/a': (BuildContext context) => const Material(child: Text('a')),
-        '/b': (BuildContext context) => const Material(child: Text('b')),
-      },
-    ));
-
-    final NavigatorState navigator = tester.state(find.byType(Navigator));
-    navigator.pushNamed('/a');
-    await tester.pumpAndSettle();
-
-    expect(find.text('home', skipOffstage: false), findsOneWidget);
-    expect(find.text('a', skipOffstage: false), findsOneWidget);
-    expect(find.text('b', skipOffstage: false), findsNothing);
-
-    navigator.pushNamedAndRemoveUntil('/b', (Route<dynamic> route) => false);
-    await tester.pumpAndSettle();
-
-    expect(find.text('home', skipOffstage: false), findsNothing);
-    expect(find.text('a', skipOffstage: false), findsNothing);
-    expect(find.text('b', skipOffstage: false), findsOneWidget);
-
-    navigator.pushNamed('/');
-    await tester.pumpAndSettle();
-
-    expect(find.text('home', skipOffstage: false), findsOneWidget);
-    expect(find.text('a', skipOffstage: false), findsNothing);
-    expect(find.text('b', skipOffstage: false), findsOneWidget);
-
-    navigator.pushNamedAndRemoveUntil('/a', ModalRoute.withName('/b'));
-    await tester.pumpAndSettle();
-
-    expect(find.text('home', skipOffstage: false), findsNothing);
-    expect(find.text('a', skipOffstage: false), findsOneWidget);
-    expect(find.text('b', skipOffstage: false), findsOneWidget);
+  group('pushAndRemoveUntil', () {
+    testWidgets('notifies appropriately', (WidgetTester tester) async {
+      final TestObserver observer = TestObserver();
+      final Widget myApp = MaterialApp(
+        home: const Material(child: Text('home')),
+        routes: <String, WidgetBuilder>{
+          '/a': (BuildContext context) => const Material(child: Text('a')),
+          '/b': (BuildContext context) => const Material(child: Text('b')),
+        },
+        navigatorObservers: <NavigatorObserver>[observer],
+      );
+
+      await tester.pumpWidget(myApp);
+
+      final NavigatorState navigator = tester.state(find.byType(Navigator));
+      final List<String> log = <String>[];
+      observer
+        ..onPushed = (Route<dynamic> route, Route<dynamic> previousRoute) {
+          log.add('${route.settings.name} pushed, previous route: ${previousRoute.settings.name}');
+        }
+        ..onRemoved = (Route<dynamic> route, Route<dynamic> previousRoute) {
+          log.add('${route.settings.name} removed, previous route: ${previousRoute?.settings?.name}');
+        };
+
+
+      navigator.pushNamed('/a');
+      await tester.pumpAndSettle();
+
+      expect(find.text('home', skipOffstage: false), findsOneWidget);
+      expect(find.text('a', skipOffstage: false), findsOneWidget);
+      expect(find.text('b', skipOffstage: false), findsNothing);
+
+      // Remove all routes below
+      navigator.pushNamedAndRemoveUntil('/b', (Route<dynamic> route) => false);
+      await tester.pumpAndSettle();
+
+      expect(find.text('home', skipOffstage: false), findsNothing);
+      expect(find.text('a', skipOffstage: false), findsNothing);
+      expect(find.text('b', skipOffstage: false), findsOneWidget);
+      expect(log, equals(<String>[
+        '/a pushed, previous route: /',
+        '/b pushed, previous route: /a',
+        '/a removed, previous route: null',
+        '/ removed, previous route: null',
+      ]));
+
+      log.clear();
+
+      navigator.pushNamed('/');
+      await tester.pumpAndSettle();
+
+      expect(find.text('home', skipOffstage: false), findsOneWidget);
+      expect(find.text('a', skipOffstage: false), findsNothing);
+      expect(find.text('b', skipOffstage: false), findsOneWidget);
+
+      // Remove only some routes below
+      navigator.pushNamedAndRemoveUntil('/a', ModalRoute.withName('/b'));
+      await tester.pumpAndSettle();
+
+      expect(find.text('home', skipOffstage: false), findsNothing);
+      expect(find.text('a', skipOffstage: false), findsOneWidget);
+      expect(find.text('b', skipOffstage: false), findsOneWidget);
+      expect(log, equals(<String>[
+        '/ pushed, previous route: /b',
+        '/a pushed, previous route: /',
+        '/ removed, previous route: /b',
+      ]));
+    });
+
+    testWidgets('triggers page transition animation for pushed route', (WidgetTester tester) async {
+      final Widget myApp = MaterialApp(
+        home: const Material(child: Text('home')),
+        routes: <String, WidgetBuilder>{
+          '/a': (BuildContext context) => const Material(child: Text('a')),
+          '/b': (BuildContext context) => const Material(child: Text('b')),
+        },
+      );
+
+      await tester.pumpWidget(myApp);
+      final NavigatorState navigator = tester.state(find.byType(Navigator));
+
+      navigator.pushNamed('/a');
+      await tester.pumpAndSettle();
+
+      navigator.pushNamedAndRemoveUntil('/b', (Route<dynamic> route) => false);
+      await tester.pump();
+      await tester.pump(const Duration(milliseconds: 100));
+
+      // We are mid-transition, both pages are onstage
+      expect(find.text('a'), findsOneWidget);
+      expect(find.text('b'), findsOneWidget);
+
+      // Complete transition
+      await tester.pumpAndSettle();
+      expect(find.text('a'), findsNothing);
+      expect(find.text('b'), findsOneWidget);
+    });
+
+    testWidgets('Hero transition triggers when preceding route contains hero, and predicate route does not', (WidgetTester tester) async {
+      const String kHeroTag = 'hero';
+      final Widget myApp = MaterialApp(
+        initialRoute: '/',
+        routes: <String, WidgetBuilder>{
+          '/': (BuildContext context) => const Material(child: Text('home')),
+          '/a': (BuildContext context) => const Material(child: Hero(
+            tag: kHeroTag,
+            child: Text('a'),
+          )),
+          '/b': (BuildContext context) => const Material(child: Padding(
+            padding: EdgeInsets.all(100.0),
+            child: Hero(
+              tag: kHeroTag,
+              child: Text('b'),
+            ),
+          )),
+        },
+      );
+
+      await tester.pumpWidget(myApp);
+      final NavigatorState navigator = tester.state(find.byType(Navigator));
+
+      navigator.pushNamed('/a');
+      await tester.pumpAndSettle();
+
+      navigator.pushNamedAndRemoveUntil('/b', ModalRoute.withName('/'));
+      await tester.pump();
+      await tester.pump(const Duration(milliseconds: 16));
+
+      expect(find.text('b'), isOnstage);
+
+      // 'b' text is heroing to its new location
+      final Offset bOffset = tester.getTopLeft(find.text('b'));
+      expect(bOffset.dx, greaterThan(0.0));
+      expect(bOffset.dx, lessThan(100.0));
+      expect(bOffset.dy, greaterThan(0.0));
+      expect(bOffset.dy, lessThan(100.0));
+
+      await tester.pump(const Duration(seconds: 1));
+
+      expect(find.text('a'), findsNothing);
+      expect(find.text('b'), isOnstage);
+    });
+
+    testWidgets('Hero transition does not trigger when preceding route does not contain hero, but predicate route does', (WidgetTester tester) async {
+      const String kHeroTag = 'hero';
+      final Widget myApp = MaterialApp(
+        initialRoute: '/',
+        routes: <String, WidgetBuilder>{
+          '/': (BuildContext context) => const Material(child: Hero(
+            tag:kHeroTag,
+            child: Text('home'),
+          )),
+          '/a': (BuildContext context) => const Material(child: Text('a')),
+          '/b': (BuildContext context) => const Material(child: Padding(
+            padding: EdgeInsets.all(100.0),
+            child: Hero(
+              tag: kHeroTag,
+              child: Text('b'),
+            ),
+          )),
+        },
+      );
+
+      await tester.pumpWidget(myApp);
+      final NavigatorState navigator = tester.state(find.byType(Navigator));
+
+      navigator.pushNamed('/a');
+      await tester.pumpAndSettle();
+
+      navigator.pushNamedAndRemoveUntil('/b', ModalRoute.withName('/'));
+      await tester.pump();
+      await tester.pump(const Duration(milliseconds: 16));
+
+      expect(find.text('b'), isOnstage);
+
+      // 'b' text is sliding in from the right, no hero transition
+      final Offset bOffset = tester.getTopLeft(find.text('b'));
+      expect(bOffset.dx, 100.0);
+      expect(bOffset.dy, greaterThan(100.0));
+    });
   });
 }

packages/flutter/test/widgets/navigator_test.dart

@@ -8,6 +8,7 @@ import 'package:flutter/foundation.dart';
 import 'package:flutter_test/flutter_test.dart';
 import 'package:flutter/material.dart';
 
+import 'observer_tester.dart';
 import 'semantics_tester.dart';
 
 class FirstWidget extends StatelessWidget {
@@ -90,48 +91,6 @@ class OnTapPage extends StatelessWidget {
   }
 }
 
-typedef OnObservation = void Function(Route<dynamic> route, Route<dynamic> previousRoute);
-
-class TestObserver extends NavigatorObserver {
-  OnObservation onPushed;
-  OnObservation onPopped;
-  OnObservation onRemoved;
-  OnObservation onReplaced;
-  OnObservation onStartUserGesture;
-
-  @override
-  void didPush(Route<dynamic> route, Route<dynamic> previousRoute) {
-    if (onPushed != null) {
-      onPushed(route, previousRoute);
-    }
-  }
-
-  @override
-  void didPop(Route<dynamic> route, Route<dynamic> previousRoute) {
-    if (onPopped != null) {
-      onPopped(route, previousRoute);
-    }
-  }
-
-  @override
-  void didRemove(Route<dynamic> route, Route<dynamic> previousRoute) {
-    if (onRemoved != null)
-      onRemoved(route, previousRoute);
-  }
-
-  @override
-  void didReplace({ Route<dynamic> oldRoute, Route<dynamic> newRoute }) {
-    if (onReplaced != null)
-      onReplaced(newRoute, oldRoute);
-  }
-
-  @override
-  void didStartUserGesture(Route<dynamic> route, Route<dynamic> previousRoute) {
-    if (onStartUserGesture != null)
-      onStartUserGesture(route, previousRoute);
-  }
-}
-
 void main() {
   testWidgets('Can navigator navigate to and from a stateful widget', (WidgetTester tester) async {
     final Map<String, WidgetBuilder> routes = <String, WidgetBuilder>{

packages/flutter/test/widgets/observer_tester.dart

+// Copyright 2019 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+import 'package:flutter/material.dart';
+
+typedef OnObservation = void Function(Route<dynamic> route, Route<dynamic> previousRoute);
+
+/// A trivial observer for testing the navigator.
+class TestObserver extends NavigatorObserver {
+  OnObservation onPushed;
+  OnObservation onPopped;
+  OnObservation onRemoved;
+  OnObservation onReplaced;
+  OnObservation onStartUserGesture;
+
+  @override
+  void didPush(Route<dynamic> route, Route<dynamic> previousRoute) {
+    if (onPushed != null) {
+      onPushed(route, previousRoute);
+    }
+  }
+
+  @override
+  void didPop(Route<dynamic> route, Route<dynamic> previousRoute) {
+    if (onPopped != null) {
+      onPopped(route, previousRoute);
+    }
+  }
+
+  @override
+  void didRemove(Route<dynamic> route, Route<dynamic> previousRoute) {
+    if (onRemoved != null)
+      onRemoved(route, previousRoute);
+  }
+
+  @override
+  void didReplace({ Route<dynamic> oldRoute, Route<dynamic> newRoute }) {
+    if (onReplaced != null)
+      onReplaced(newRoute, oldRoute);
+  }
+
+  @override
+  void didStartUserGesture(Route<dynamic> route, Route<dynamic> previousRoute) {
+    if (onStartUserGesture != null)
+      onStartUserGesture(route, previousRoute);
+  }
+}