Notify the previous route when pushing and popping

This makes HeroController ignore StateRoutes when deciding where to
animate heroes to and from.

packages/flutter/lib/src/material/bottom_sheet.dart

@@ -223,7 +223,6 @@ Future showBottomSheet({ BuildContext context, GlobalKey<PlaceholderState> place
   return completer.future.then((_) {
     // If our overlay has been obscured by an opaque OverlayEntry then currentState
     // will have been cleared already.
-    if (placeholderKey.currentState != null)
-      placeholderKey.currentState.child = null;
+    placeholderKey.currentState?.child = null;
   });
 }

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

@@ -24,28 +24,26 @@ class HeroController extends NavigatorObserver {
 
   final List<OverlayEntry> _overlayEntries = new List<OverlayEntry>();
 
-  void didPushModal(Route route) {
+  void didPushModal(Route route, Route previousRoute) {
     assert(navigator != null);
     assert(route != null);
     if (route is ModalRoute) { // as opposed to StateRoute, say
       assert(route.performance != null);
-      Route from = navigator.currentRoute;
-      if (from is ModalRoute) // as opposed to the many other types of routes, or null
-        _from = from;
+      if (previousRoute is ModalRoute) // as opposed to the many other types of routes, or null
+        _from = previousRoute;
       _to = route;
       _performance = route.performance;
       _checkForHeroQuest();
     }
   }
 
-  void didPopModal(Route route) {
+  void didPopModal(Route route, Route previousRoute) {
     assert(navigator != null);
     assert(route != null);
     if (route is ModalRoute) { // as opposed to StateRoute, say
       assert(route.performance != null);
-      Route to = navigator.currentRoute;
-      if (to is ModalRoute) { // as opposed to the many other types of routes
-        _to = to;
+      if (previousRoute is ModalRoute) { // as opposed to the many other types of routes
+        _to = previousRoute;
         _from = route;
         _performance = route.performance;
         _checkForHeroQuest();

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

@@ -7,14 +7,24 @@ import 'overlay.dart';
 
 abstract class Route {
   List<OverlayEntry> get overlayEntries;
-
-  void didPush(OverlayState overlay, OverlayEntry insertionPoint);
-  void didPop(dynamic result);
+  void didPush(OverlayState overlay, OverlayEntry insertionPoint) { }
+  void didPop(dynamic result) { }
+
+  /// The given route has been pushed onto the navigator after this route.
+  /// Return true if the route before this one should be notified also. The
+  /// first route to return false will be the one passed to the
+  /// NavigatorObserver's didPushModal() as the previousRoute.
+  bool willPushNext(Route nextRoute) => false;
+
+  /// The given route, which came after this one, has been popped off the
+  /// navigator. Return true if the route before this one should be notified
+  /// also. The first route to return false will be the one passed to the
+  /// NavigatorObserver's didPushModal() as the previousRoute.
+  bool didPopNext(Route nextRoute) => false;
 }
 
 class NamedRouteSettings {
   const NamedRouteSettings({ this.name, this.mostValuableKeys });
-
   final String name;
   final Set<Key> mostValuableKeys;
 }
@@ -24,8 +34,8 @@ typedef Route RouteFactory(NamedRouteSettings settings);
 class NavigatorObserver {
   NavigatorState _navigator;
   NavigatorState get navigator => _navigator;
-  void didPopModal(Route route) { }
-  void didPushModal(Route route) { }
+  void didPushModal(Route route, Route previousRoute) { }
+  void didPopModal(Route route, Route previousRoute) { }
 }
 
 class Navigator extends StatefulComponent {
@@ -99,8 +109,6 @@ class NavigatorState extends State<Navigator> {
     return null;
   }
 
-  Route get currentRoute => _ephemeral.isNotEmpty ? _ephemeral.last : _modal.isNotEmpty ? _modal.last : null;
-
   void pushNamed(String name, { Set<Key> mostValuableKeys }) {
     assert(name != null);
     NamedRouteSettings settings = new NamedRouteSettings(
@@ -112,8 +120,11 @@ class NavigatorState extends State<Navigator> {
 
   void push(Route route, { Set<Key> mostValuableKeys }) {
     _popAllEphemeralRoutes();
+    int index = _modal.length-1;
+    while (index >= 0 && _modal[index].willPushNext(route))
+      index -= 1;
     route.didPush(overlay, _currentOverlay);
-    config.observer?.didPushModal(route);
+    config.observer?.didPushModal(route, index >= 0 ? _modal[index] : null);
     _modal.add(route);
   }
 
@@ -134,9 +145,13 @@ class NavigatorState extends State<Navigator> {
     if (_ephemeral.isNotEmpty) {
       _ephemeral.removeLast().didPop(result);
     } else {
+      assert(_modal.length > 1);
       Route route = _modal.removeLast();
       route.didPop(result);
-      config.observer?.didPopModal(route);
+      int index = _modal.length-1;
+      while (index >= 0 && _modal[index].didPopNext(route))
+        index -= 1;
+      config.observer?.didPopModal(route, index >= 0 ? _modal[index] : null);
     }
   }
 

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

@@ -22,11 +22,13 @@ class StateRoute extends Route {
 
   List<OverlayEntry> get overlayEntries => const <OverlayEntry>[];
 
-  void didPush(OverlayState overlay, OverlayEntry insertionPoint) { }
   void didPop(dynamic result) {
     if (onPop != null)
       onPop();
   }
+
+  bool willPushNext(Route nextRoute) => true;
+  bool didPopNext(Route nextRoute) => true;
 }
 
 class OverlayRoute extends Route {

packages/unit/test/widget/heroes_test.dart

@@ -5,117 +5,153 @@
 import 'package:flutter/material.dart';
 import 'package:test/test.dart';
 
+import 'test_matchers.dart';
 import 'widget_tester.dart';
 
-class TestOverlayRoute extends OverlayRoute {
-  List<WidgetBuilder> get builders => <WidgetBuilder>[ _build ];
-  Widget _build(BuildContext context) => new Text('Overlay');
-}
-
-bool _isOnStage(Element element) {
-  expect(element, isNotNull);
-  bool result = true;
-  element.visitAncestorElements((Element ancestor) {
-    if (ancestor.widget is OffStage) {
-      result = false;
-      return false;
-    }
-    return true;
-  });
-  return result;
-}
-
-class _IsOnStage extends Matcher {
-  const _IsOnStage();
-  bool matches(item, Map matchState) => _isOnStage(item);
-  Description describe(Description description) => description.add('onstage');
-}
-
-class _IsOffStage extends Matcher {
-  const _IsOffStage();
-  bool matches(item, Map matchState) => !_isOnStage(item);
-  Description describe(Description description) => description.add('offstage');
-}
-
-const Matcher isOnStage = const _IsOnStage();
-const Matcher isOffStage = const _IsOffStage();
+Key firstKey = new Key('first');
+Key secondKey = new Key('second');
+final Map<String, RouteBuilder> routes = <String, RouteBuilder>{
+  '/': (RouteArguments args) => new Block([
+    new Container(height: 100.0, width: 100.0),
+    new Card(child: new Hero(tag: 'a', child: new Container(height: 100.0, width: 100.0, key: firstKey))),
+    new Container(height: 100.0, width: 100.0),
+    new FlatButton(child: new Text('state route please'), onPressed: () => Navigator.of(args.context).push(new StateRoute())),
+    new FlatButton(child: new Text('button'), onPressed: () => Navigator.of(args.context).pushNamed('/two')),
+  ]),
+  '/two': (RouteArguments args) => new Block([
+    new Container(height: 150.0, width: 150.0),
+    new Card(child: new Hero(tag: 'a', child: new Container(height: 150.0, width: 150.0, key: secondKey))),
+    new Container(height: 150.0, width: 150.0),
+    new FlatButton(child: new Text('button'), onPressed: () => Navigator.of(args.context).pop()),
+  ]),
+};
 
 void main() {
-  test('Can pop ephemeral route without black flash', () {
+  test('Heroes animate', () {
     testWidgets((WidgetTester tester) {
-      GlobalKey containerKey = new GlobalKey();
-      final Map<String, RouteBuilder> routes = <String, RouteBuilder>{
-        '/': (_) => new Container(key: containerKey, child: new Text('Home')),
-        '/settings': (_) => new Container(child: new Text('Settings')),
-      };
 
       tester.pumpWidget(new MaterialApp(routes: routes));
 
-      expect(tester.findText('Home'), isOnStage);
-      expect(tester.findText('Settings'), isNull);
-      expect(tester.findText('Overlay'), isNull);
+      // the initial setup.
 
-      NavigatorState navigator = Navigator.of(containerKey.currentContext);
+      expect(tester.findElementByKey(firstKey), isOnStage);
+      expect(tester.findElementByKey(firstKey), isInCard);
+      expect(tester.findElementByKey(secondKey), isNull);
 
-      navigator.pushNamed('/settings');
+      tester.tap(tester.findText('button'));
+      tester.pump(); // begin navigation
+
+      // at this stage, the second route is off-stage, so that we can form the
+      // hero party.
+
+      expect(tester.findElementByKey(firstKey), isOnStage);
+      expect(tester.findElementByKey(firstKey), isInCard);
+      expect(tester.findElementByKey(secondKey), isOffStage);
+      expect(tester.findElementByKey(secondKey), isInCard);
 
       tester.pump();
 
-      expect(tester.findText('Home'), isOnStage);
-      expect(tester.findText('Settings'), isOffStage);
-      expect(tester.findText('Overlay'), isNull);
+      // at this stage, the heroes have just gone on their journey, we are
+      // seeing them at t=16ms. The original page no longer contains the hero.
+
+      expect(tester.findElementByKey(firstKey), isNull);
+      expect(tester.findElementByKey(secondKey), isOnStage);
+      expect(tester.findElementByKey(secondKey), isNotInCard);
+
+      tester.pump();
 
-      tester.pump(const Duration(milliseconds: 16));
+      // t=32ms for the journey. Surely they are still at it.
 
-      expect(tester.findText('Home'), isOnStage);
-      expect(tester.findText('Settings'), isOnStage);
-      expect(tester.findText('Overlay'), isNull);
+      expect(tester.findElementByKey(firstKey), isNull);
+      expect(tester.findElementByKey(secondKey), isOnStage);
+      expect(tester.findElementByKey(secondKey), isNotInCard);
 
-      tester.pump(const Duration(seconds: 1));
+      tester.pump(new Duration(seconds: 1));
 
-      expect(tester.findText('Home'), isNull);
-      expect(tester.findText('Settings'), isOnStage);
-      expect(tester.findText('Overlay'), isNull);
+      // t=1.032s for the journey. The journey has ended (it ends this frame, in
+      // fact). The hero should now be in the new page, on-stage.
 
-      navigator.push(new TestOverlayRoute());
+      expect(tester.findElementByKey(firstKey), isNull);
+      expect(tester.findElementByKey(secondKey), isOnStage);
+      expect(tester.findElementByKey(secondKey), isInCard);
 
       tester.pump();
 
-      expect(tester.findText('Home'), isNull);
-      expect(tester.findText('Settings'), isOnStage);
-      expect(tester.findText('Overlay'), isOnStage);
+      // Should not change anything.
 
-      tester.pump(const Duration(seconds: 1));
+      expect(tester.findElementByKey(firstKey), isNull);
+      expect(tester.findElementByKey(secondKey), isOnStage);
+      expect(tester.findElementByKey(secondKey), isInCard);
 
-      expect(tester.findText('Home'), isNull);
-      expect(tester.findText('Settings'), isOnStage);
-      expect(tester.findText('Overlay'), isOnStage);
+    });
+  });
+
+  test('Heroes animate even with intervening state routes', () {
+    testWidgets((WidgetTester tester) {
 
-      navigator.pop();
+      tester.pumpWidget(new Container()); // clear our memory
+
+      tester.pumpWidget(new MaterialApp(routes: routes));
+
+      // the initial setup.
+
+      expect(tester.findElementByKey(firstKey), isOnStage);
+      expect(tester.findElementByKey(firstKey), isInCard);
+      expect(tester.findElementByKey(secondKey), isNull);
+
+      // insert a state route
+
+      tester.tap(tester.findText('state route please'));
       tester.pump();
 
-      expect(tester.findText('Home'), isNull);
-      expect(tester.findText('Settings'), isOnStage);
-      expect(tester.findText('Overlay'), isNull);
+      expect(tester.findElementByKey(firstKey), isOnStage);
+      expect(tester.findElementByKey(firstKey), isInCard);
+      expect(tester.findElementByKey(secondKey), isNull);
+
+      tester.tap(tester.findText('button'));
+      tester.pump(); // begin navigation
 
-      tester.pump(const Duration(seconds: 1));
+      // at this stage, the second route is off-stage, so that we can form the
+      // hero party.
 
-      expect(tester.findText('Home'), isNull);
-      expect(tester.findText('Settings'), isOnStage);
-      expect(tester.findText('Overlay'), isNull);
+      expect(tester.findElementByKey(firstKey), isOnStage);
+      expect(tester.findElementByKey(firstKey), isInCard);
+      expect(tester.findElementByKey(secondKey), isOffStage);
+      expect(tester.findElementByKey(secondKey), isInCard);
 
-      navigator.pop();
       tester.pump();
 
-      expect(tester.findText('Home'), isOnStage);
-      expect(tester.findText('Settings'), isOnStage);
-      expect(tester.findText('Overlay'), isNull);
+      // at this stage, the heroes have just gone on their journey, we are
+      // seeing them at t=16ms. The original page no longer contains the hero.
+
+      expect(tester.findElementByKey(firstKey), isNull);
+      expect(tester.findElementByKey(secondKey), isOnStage);
+      expect(tester.findElementByKey(secondKey), isNotInCard);
+
+      tester.pump();
+
+      // t=32ms for the journey. Surely they are still at it.
+
+      expect(tester.findElementByKey(firstKey), isNull);
+      expect(tester.findElementByKey(secondKey), isOnStage);
+      expect(tester.findElementByKey(secondKey), isNotInCard);
+
+      tester.pump(new Duration(seconds: 1));
+
+      // t=1.032s for the journey. The journey has ended (it ends this frame, in
+      // fact). The hero should now be in the new page, on-stage.
+
+      expect(tester.findElementByKey(firstKey), isNull);
+      expect(tester.findElementByKey(secondKey), isOnStage);
+      expect(tester.findElementByKey(secondKey), isInCard);
+
+      tester.pump();
 
-      tester.pump(const Duration(seconds: 1));
+      // Should not change anything.
 
-      expect(tester.findText('Home'), isOnStage);
-      expect(tester.findText('Settings'), isNull);
-      expect(tester.findText('Overlay'), isNull);
+      expect(tester.findElementByKey(firstKey), isNull);
+      expect(tester.findElementByKey(secondKey), isOnStage);
+      expect(tester.findElementByKey(secondKey), isInCard);
 
     });
   });

packages/unit/test/widget/page_transitions_test.dart

+// Copyright 2015 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';
+import 'package:test/test.dart';
+
+import 'test_matchers.dart';
+import 'widget_tester.dart';
+
+class TestOverlayRoute extends OverlayRoute {
+  List<WidgetBuilder> get builders => <WidgetBuilder>[ _build ];
+  Widget _build(BuildContext context) => new Text('Overlay');
+}
+
+void main() {
+  test('Check onstage/offstage handling around transitions', () {
+    testWidgets((WidgetTester tester) {
+      GlobalKey containerKey = new GlobalKey();
+      final Map<String, RouteBuilder> routes = <String, RouteBuilder>{
+        '/': (_) => new Container(key: containerKey, child: new Text('Home')),
+        '/settings': (_) => new Container(child: new Text('Settings')),
+      };
+
+      tester.pumpWidget(new MaterialApp(routes: routes));
+
+      expect(tester.findText('Home'), isOnStage);
+      expect(tester.findText('Settings'), isNull);
+      expect(tester.findText('Overlay'), isNull);
+
+      NavigatorState navigator = Navigator.of(containerKey.currentContext);
+
+      navigator.pushNamed('/settings');
+
+      tester.pump();
+
+      expect(tester.findText('Home'), isOnStage);
+      expect(tester.findText('Settings'), isOffStage);
+      expect(tester.findText('Overlay'), isNull);
+
+      tester.pump(const Duration(milliseconds: 16));
+
+      expect(tester.findText('Home'), isOnStage);
+      expect(tester.findText('Settings'), isOnStage);
+      expect(tester.findText('Overlay'), isNull);
+
+      tester.pump(const Duration(seconds: 1));
+
+      expect(tester.findText('Home'), isNull);
+      expect(tester.findText('Settings'), isOnStage);
+      expect(tester.findText('Overlay'), isNull);
+
+      navigator.push(new TestOverlayRoute());
+
+      tester.pump();
+
+      expect(tester.findText('Home'), isNull);
+      expect(tester.findText('Settings'), isOnStage);
+      expect(tester.findText('Overlay'), isOnStage);
+
+      tester.pump(const Duration(seconds: 1));
+
+      expect(tester.findText('Home'), isNull);
+      expect(tester.findText('Settings'), isOnStage);
+      expect(tester.findText('Overlay'), isOnStage);
+
+      navigator.pop();
+      tester.pump();
+
+      expect(tester.findText('Home'), isNull);
+      expect(tester.findText('Settings'), isOnStage);
+      expect(tester.findText('Overlay'), isNull);
+
+      tester.pump(const Duration(seconds: 1));
+
+      expect(tester.findText('Home'), isNull);
+      expect(tester.findText('Settings'), isOnStage);
+      expect(tester.findText('Overlay'), isNull);
+
+      navigator.pop();
+      tester.pump();
+
+      expect(tester.findText('Home'), isOnStage);
+      expect(tester.findText('Settings'), isOnStage);
+      expect(tester.findText('Overlay'), isNull);
+
+      tester.pump(const Duration(seconds: 1));
+
+      expect(tester.findText('Home'), isOnStage);
+      expect(tester.findText('Settings'), isNull);
+      expect(tester.findText('Overlay'), isNull);
+
+    });
+  });
+}

packages/unit/test/widget/test_matchers.dart

+// Copyright 2015 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';
+import 'package:test/test.dart';
+
+bool _hasAncestorOfType(Element element, Type targetType) {
+  expect(element, isNotNull);
+  bool result = false;
+  element.visitAncestorElements((Element ancestor) {
+    if (ancestor.widget.runtimeType == targetType) {
+      result = true;
+      return false;
+    }
+    return true;
+  });
+  return result;
+}
+
+class _IsOnStage extends Matcher {
+  const _IsOnStage();
+  bool matches(item, Map matchState) => !_hasAncestorOfType(item, OffStage);
+  Description describe(Description description) => description.add('onstage');
+}
+
+class _IsOffStage extends Matcher {
+  const _IsOffStage();
+  bool matches(item, Map matchState) => _hasAncestorOfType(item, OffStage);
+  Description describe(Description description) => description.add('offstage');
+}
+
+class _IsInCard extends Matcher {
+  const _IsInCard();
+  bool matches(item, Map matchState) => _hasAncestorOfType(item, Card);
+  Description describe(Description description) => description.add('in card');
+}
+
+class _IsNotInCard extends Matcher {
+  const _IsNotInCard();
+  bool matches(item, Map matchState) => !_hasAncestorOfType(item, Card);
+  Description describe(Description description) => description.add('not in card');
+}
+
+const Matcher isOnStage = const _IsOnStage();
+const Matcher isOffStage = const _IsOffStage();
+const Matcher isInCard = const _IsInCard();
+const Matcher isNotInCard = const _IsNotInCard();