clean up hero controller scope (#60655)

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

@@ -293,18 +293,6 @@ class _CupertinoAppState extends State<CupertinoApp> {
     _heroController = CupertinoApp.createCupertinoHeroController();
   }
 
-  @override
-  void didUpdateWidget(CupertinoApp oldWidget) {
-    super.didUpdateWidget(oldWidget);
-    if (widget.navigatorKey != oldWidget.navigatorKey) {
-      // If the Navigator changes, we have to create a new observer, because the
-      // old Navigator won't be disposed (and thus won't unregister with its
-      // observers) until after the new one has been created (because the
-      // Navigator has a GlobalKey).
-      _heroController = CupertinoApp.createCupertinoHeroController();
-    }
-  }
-
   // Combine the default localization for Cupertino with the ones contributed
   // by the localizationsDelegates parameter, if any. Only the first delegate
   // of a particular LocalizationsDelegate.type is loaded so the
@@ -370,7 +358,6 @@ class _CupertinoAppState extends State<CupertinoApp> {
                   },
                   shortcuts: widget.shortcuts,
                   actions: widget.actions,
-
                 ),
               );
             },

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

@@ -586,18 +586,6 @@ class _MaterialAppState extends State<MaterialApp> {
     _heroController = MaterialApp.createMaterialHeroController();
   }
 
-  @override
-  void didUpdateWidget(MaterialApp oldWidget) {
-    super.didUpdateWidget(oldWidget);
-    if (widget.navigatorKey != oldWidget.navigatorKey) {
-      // If the Navigator changes, we have to create a new observer, because the
-      // old Navigator won't be disposed (and thus won't unregister with its
-      // observers) until after the new one has been created (because the
-      // Navigator has a GlobalKey).
-      _heroController = MaterialApp.createMaterialHeroController();
-    }
-  }
-
   // Combine the Localizations for Material with the ones contributed
   // by the localizationsDelegates parameter, if any. Only the first delegate
   // of a particular LocalizationsDelegate.type is loaded so the

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

@@ -624,26 +624,30 @@ class NavigatorObserver {
 
 /// An inherited widget to host a hero controller.
 ///
-/// This class should not be used directly. The [MaterialApp] and [CupertinoApp]
-/// use this class to host the [HeroController], and they should be the only
-/// exception to use this class. If you want to subscribe your own
-/// [HeroController], use the [Navigator.observers] instead.
-///
 /// The hosted hero controller will be picked up by the navigator in the
 /// [child] subtree. Once a navigator picks up this controller, the navigator
 /// will bar any navigator below its subtree from receiving this controller.
 ///
-/// See also:
-///
-///  * [Navigator.observers], which is the standard way of providing a
-///    [HeroController].
+/// The hero controller inside the [HeroControllerScope] can only subscribe to
+/// one navigator at a time. An assertion will be thrown if the hero controller
+/// subscribes to more than one navigators. This can happen when there are
+/// multiple navigators under the same [HeroControllerScope] in parallel.
 class HeroControllerScope extends InheritedWidget {
   /// Creates a widget to host the input [controller].
   const HeroControllerScope({
     Key key,
     this.controller,
     Widget child,
-  }) : super(key: key, child: child);
+  }) : assert(controller != null),
+       super(key: key, child: child);
+
+  /// Creates a widget to prevent the subtree from receiving the hero controller
+  /// above.
+  const HeroControllerScope.none({
+    Key key,
+    Widget child,
+  }) : controller = null,
+       super(key: key, child: child);
 
   /// The hero controller that is hosted inside this widget.
   final HeroController controller;
@@ -2803,8 +2807,30 @@ class NavigatorState extends State<Navigator> with TickerProviderStateMixin {
 
   void _updateHeroController(HeroController newHeroController) {
     if (_heroControllerFromScope != newHeroController) {
-      _heroControllerFromScope?._navigator = null;
-      newHeroController?._navigator = this;
+      if (newHeroController != null) {
+        // Makes sure the same hero controller is not shared between two navigators.
+        assert(() {
+          // It is possible that the hero controller subscribes to an existing
+          // navigator. We are fine as long as that navigator gives up the hero
+          // controller at the end of the build.
+          if (newHeroController.navigator != null) {
+            final NavigatorState previousOwner = newHeroController.navigator;
+            ServicesBinding.instance.addPostFrameCallback((Duration timestamp) {
+              // We only check if this navigator still owns the hero controller.
+              if (_heroControllerFromScope == newHeroController) {
+                assert(_heroControllerFromScope._navigator == this);
+                assert(previousOwner._heroControllerFromScope != newHeroController);
+              }
+            });
+          }
+          return true;
+        }());
+        newHeroController._navigator = this;
+      }
+      // Only unsubscribe the hero controller when it is currently subscribe to
+      // this navigator.
+      if (_heroControllerFromScope?._navigator == this)
+        _heroControllerFromScope?._navigator = null;
       _heroControllerFromScope = newHeroController;
       _updateEffectiveObservers();
     }
@@ -2865,6 +2891,7 @@ class NavigatorState extends State<Navigator> with TickerProviderStateMixin {
       _debugLocked = true;
       return true;
     }());
+    _updateHeroController(null);
     for (final NavigatorObserver observer in _effectiveObservers)
       observer._navigator = null;
     focusScopeNode.dispose();
@@ -4051,7 +4078,7 @@ class NavigatorState extends State<Navigator> with TickerProviderStateMixin {
     // Hides the HeroControllerScope for the widget subtree so that the other
     // nested navigator underneath will not pick up the hero controller above
     // this level.
-    return HeroControllerScope(
+    return HeroControllerScope.none(
       child: Listener(
         onPointerDown: _handlePointerDown,
         onPointerUp: _handlePointerUpOrCancel,

packages/flutter/test/cupertino/app_test.dart

@@ -131,4 +131,20 @@ void main() {
     expect(find.text('regular page one'), findsNothing);
     expect(find.text('regular page two'), findsNothing);
   });
+
+  testWidgets('CupertinoApp.navigatorKey can be updated', (WidgetTester tester) async {
+    final GlobalKey<NavigatorState> key1 = GlobalKey<NavigatorState>();
+    await tester.pumpWidget(CupertinoApp(
+      navigatorKey: key1,
+      home: const Placeholder(),
+    ));
+    expect(key1.currentState, isA<NavigatorState>());
+    final GlobalKey<NavigatorState> key2 = GlobalKey<NavigatorState>();
+    await tester.pumpWidget(CupertinoApp(
+      navigatorKey: key2,
+      home: const Placeholder(),
+    ));
+    expect(key2.currentState, isA<NavigatorState>());
+    expect(key1.currentState, isNull);
+  });
 }

packages/flutter/test/material/app_test.dart

@@ -843,6 +843,22 @@ void main() {
     );
     expect(tween, isA<MaterialRectArcTween>());
   });
+
+  testWidgets('MaterialApp.navigatorKey can be updated', (WidgetTester tester) async {
+    final GlobalKey<NavigatorState> key1 = GlobalKey<NavigatorState>();
+    await tester.pumpWidget(MaterialApp(
+      navigatorKey: key1,
+      home: const Placeholder(),
+    ));
+    expect(key1.currentState, isA<NavigatorState>());
+    final GlobalKey<NavigatorState> key2 = GlobalKey<NavigatorState>();
+    await tester.pumpWidget(MaterialApp(
+      navigatorKey: key2,
+      home: const Placeholder(),
+    ));
+    expect(key2.currentState, isA<NavigatorState>());
+    expect(key1.currentState, isNull);
+  });
 }
 
 class MockAccessibilityFeature implements AccessibilityFeatures {

packages/flutter/test/widgets/heroes_test.dart

@@ -304,6 +304,103 @@ Future<void> main() async {
     expect(find.byKey(thirdKey), isInCard);
   });
 
+  testWidgets('Heroes still animate after hero controller is swapped.', (WidgetTester tester) async {
+    final GlobalKey<NavigatorState> key = GlobalKey<NavigatorState>();
+    final UniqueKey heroKey = UniqueKey();
+    await tester.pumpWidget(
+      HeroControllerScope(
+        controller: HeroController(),
+        child: Directionality(
+          textDirection: TextDirection.ltr,
+          child: Navigator(
+            key: key,
+            initialRoute: 'navigator1',
+            onGenerateRoute: (RouteSettings s) {
+              return MaterialPageRoute<void>(
+                builder: (BuildContext c) {
+                  return Hero(
+                    tag: 'hero',
+                    child: Container(),
+                    flightShuttleBuilder: (
+                      BuildContext flightContext,
+                      Animation<double> animation,
+                      HeroFlightDirection flightDirection,
+                      BuildContext fromHeroContext,
+                      BuildContext toHeroContext,
+                    ) {
+                      return Container(key: heroKey);
+                    },
+                  );
+                },
+                settings: s,
+              );
+            },
+          ),
+        ),
+      ),
+    );
+    key.currentState.push(MaterialPageRoute<void>(
+      builder: (BuildContext c) {
+        return Hero(
+          tag: 'hero',
+          child: Container(),
+          flightShuttleBuilder: (
+            BuildContext flightContext,
+            Animation<double> animation,
+            HeroFlightDirection flightDirection,
+            BuildContext fromHeroContext,
+            BuildContext toHeroContext,
+            ) {
+            return Container(key: heroKey);
+          },
+        );
+      },
+    ));
+    expect(find.byKey(heroKey), findsNothing);
+    // Begins the navigation
+    await tester.pump();
+    await tester.pump(const Duration(milliseconds: 30));
+    expect(find.byKey(heroKey), isOnstage);
+    // Pumps a new hero controller.
+    await tester.pumpWidget(
+      HeroControllerScope(
+        controller: HeroController(),
+        child: Directionality(
+          textDirection: TextDirection.ltr,
+          child: Navigator(
+            key: key,
+            initialRoute: 'navigator1',
+            onGenerateRoute: (RouteSettings s) {
+              return MaterialPageRoute<void>(
+                builder: (BuildContext c) {
+                  return Hero(
+                    tag: 'hero',
+                    child: Container(),
+                    flightShuttleBuilder: (
+                      BuildContext flightContext,
+                      Animation<double> animation,
+                      HeroFlightDirection flightDirection,
+                      BuildContext fromHeroContext,
+                      BuildContext toHeroContext,
+                      ) {
+                      return Container(key: heroKey);
+                    },
+                  );
+                },
+                settings: s,
+              );
+            },
+          ),
+        ),
+      ),
+    );
+    // The original animation still flies.
+    expect(find.byKey(heroKey), isOnstage);
+    // Waits for the animation finishes.
+    await tester.pumpAndSettle();
+    expect(find.byKey(heroKey), findsNothing);
+  });
+
   testWidgets('Heroes animate should hide original hero', (WidgetTester tester) async {
     await tester.pumpWidget(MaterialApp(routes: routes));
     // Checks initial state.

packages/flutter/test/widgets/navigator_test.dart

@@ -2064,6 +2064,258 @@ void main() {
     expect(observations[1].previous, 'top1');
   });
 
+  testWidgets('hero controller can correctly transfer subscription - replacing navigator', (WidgetTester tester) async {
+    final GlobalKey<NavigatorState> key1 = GlobalKey<NavigatorState>();
+    final GlobalKey<NavigatorState> key2 = GlobalKey<NavigatorState>();
+
+    final List<NavigatorObservation> observations = <NavigatorObservation>[];
+    final HeroControllerSpy spy = HeroControllerSpy()
+      ..onPushed = (Route<dynamic> route, Route<dynamic> previousRoute) {
+        observations.add(
+          NavigatorObservation(
+            current: route?.settings?.name,
+            previous: previousRoute?.settings?.name,
+            operation: 'didPush'
+          )
+        );
+      };
+    await tester.pumpWidget(
+      HeroControllerScope(
+        controller: spy,
+        child: Directionality(
+          textDirection: TextDirection.ltr,
+          child: Navigator(
+            key: key1,
+            initialRoute: 'navigator1',
+            onGenerateRoute: (RouteSettings s) {
+              return MaterialPageRoute<void>(
+                builder: (BuildContext c) {
+                  return const Placeholder();
+                },
+                settings: s,
+              );
+            },
+          ),
+        )
+      )
+    );
+    // Transfer the subscription to another navigator
+    await tester.pumpWidget(
+      HeroControllerScope(
+        controller: spy,
+        child: Directionality(
+          textDirection: TextDirection.ltr,
+          child: Navigator(
+            key: key2,
+            initialRoute: 'navigator2',
+            onGenerateRoute: (RouteSettings s) {
+              return MaterialPageRoute<void>(
+                builder: (BuildContext c) {
+                  return const Placeholder();
+                },
+                settings: s,
+              );
+            },
+          ),
+        )
+      )
+    );
+    observations.clear();
+
+    key2.currentState.push(MaterialPageRoute<void>(
+      settings: const RouteSettings(name:'new route'),
+      builder: (BuildContext context) => const Text('new route')
+    ));
+    await tester.pumpAndSettle();
+
+    expect(find.text('new route'), findsOneWidget);
+    // It should record from the new navigator.
+    expect(observations.length, 1);
+    expect(observations[0].current, 'new route');
+    expect(observations[0].previous, 'navigator2');
+  });
+
+  testWidgets('hero controller can correctly transfer subscription - swapping navigator', (WidgetTester tester) async {
+    final GlobalKey<NavigatorState> key1 = GlobalKey<NavigatorState>();
+    final GlobalKey<NavigatorState> key2 = GlobalKey<NavigatorState>();
+
+    final List<NavigatorObservation> observations1 = <NavigatorObservation>[];
+    final HeroControllerSpy spy1 = HeroControllerSpy()
+      ..onPushed = (Route<dynamic> route, Route<dynamic> previousRoute) {
+        observations1.add(
+          NavigatorObservation(
+            current: route?.settings?.name,
+            previous: previousRoute?.settings?.name,
+            operation: 'didPush'
+          )
+        );
+      };
+    final List<NavigatorObservation> observations2 = <NavigatorObservation>[];
+    final HeroControllerSpy spy2 = HeroControllerSpy()
+      ..onPushed = (Route<dynamic> route, Route<dynamic> previousRoute) {
+        observations2.add(
+          NavigatorObservation(
+            current: route?.settings?.name,
+            previous: previousRoute?.settings?.name,
+            operation: 'didPush'
+          )
+        );
+      };
+    await tester.pumpWidget(
+      Directionality(
+        textDirection: TextDirection.ltr,
+        child: Stack(
+          children: <Widget>[
+            HeroControllerScope(
+              controller: spy1,
+              child: Navigator(
+                key: key1,
+                initialRoute: 'navigator1',
+                onGenerateRoute: (RouteSettings s) {
+                  return MaterialPageRoute<void>(
+                    builder: (BuildContext c) {
+                      return const Placeholder();
+                    },
+                    settings: s,
+                  );
+                },
+              )
+            ),
+            HeroControllerScope(
+              controller: spy2,
+              child: Navigator(
+                key: key2,
+                initialRoute: 'navigator2',
+                onGenerateRoute: (RouteSettings s) {
+                  return MaterialPageRoute<void>(
+                    builder: (BuildContext c) {
+                      return const Placeholder();
+                    },
+                    settings: s,
+                  );
+                },
+              )
+            ),
+          ],
+        ),
+      ),
+    );
+    expect(observations1.length, 1);
+    expect(observations1[0].current, 'navigator1');
+    expect(observations1[0].previous, isNull);
+    expect(observations2.length, 1);
+    expect(observations2[0].current, 'navigator2');
+    expect(observations2[0].previous, isNull);
+
+    // Swaps the spies.
+    await tester.pumpWidget(
+      Directionality(
+        textDirection: TextDirection.ltr,
+        child: Stack(
+          children: <Widget>[
+            HeroControllerScope(
+              controller: spy2,
+              child: Navigator(
+                key: key1,
+                initialRoute: 'navigator1',
+                onGenerateRoute: (RouteSettings s) {
+                  return MaterialPageRoute<void>(
+                    builder: (BuildContext c) {
+                      return const Placeholder();
+                    },
+                    settings: s,
+                  );
+                },
+              )
+            ),
+            HeroControllerScope(
+              controller: spy1,
+              child: Navigator(
+                key: key2,
+                initialRoute: 'navigator2',
+                onGenerateRoute: (RouteSettings s) {
+                  return MaterialPageRoute<void>(
+                    builder: (BuildContext c) {
+                      return const Placeholder();
+                    },
+                    settings: s,
+                  );
+                },
+              )
+            ),
+          ],
+        ),
+      ),
+    );
+
+    // Pushes a route to navigator2.
+    key2.currentState.push(MaterialPageRoute<void>(
+      settings: const RouteSettings(name:'new route2'),
+      builder: (BuildContext context) => const Text('new route2')
+    ));
+    await tester.pumpAndSettle();
+    expect(find.text('new route2'), findsOneWidget);
+    // The spy1 should record the push in navigator2.
+    expect(observations1.length, 2);
+    expect(observations1[1].current, 'new route2');
+    expect(observations1[1].previous, 'navigator2');
+    // The spy2 should not record anything.
+    expect(observations2.length, 1);
+
+    // Pushes a route to navigator1
+    key1.currentState.push(MaterialPageRoute<void>(
+      settings: const RouteSettings(name:'new route1'),
+      builder: (BuildContext context) => const Text('new route1')
+    ));
+    await tester.pumpAndSettle();
+    expect(find.text('new route1'), findsOneWidget);
+    // The spy1 should not record anything.
+    expect(observations1.length, 2);
+    // The spy2 should record the push in navigator1.
+    expect(observations2.length, 2);
+    expect(observations2[1].current, 'new route1');
+    expect(observations2[1].previous, 'navigator1');
+  });
+
+  testWidgets('hero controller subscribes to multiple navigators does throw', (WidgetTester tester) async {
+    final HeroControllerSpy spy = HeroControllerSpy();
+    await tester.pumpWidget(
+      HeroControllerScope(
+        controller: spy,
+        child: Directionality(
+          textDirection: TextDirection.ltr,
+          child: Stack(
+            children: <Widget>[
+              Navigator(
+                initialRoute: 'navigator1',
+                onGenerateRoute: (RouteSettings s) {
+                  return MaterialPageRoute<void>(
+                    builder: (BuildContext c) {
+                      return const Placeholder();
+                    },
+                    settings: s,
+                  );
+                },
+              ),
+              Navigator(
+                initialRoute: 'navigator2',
+                onGenerateRoute: (RouteSettings s) {
+                  return MaterialPageRoute<void>(
+                    builder: (BuildContext c) {
+                      return const Placeholder();
+                    },
+                    settings: s,
+                  );
+                },
+              ),
+            ],
+          ),
+        ),
+      ),
+    );
+    expect(tester.takeException(), isAssertionError);
+  });
+
   group('Page api', (){
     Widget buildNavigator({
       List<Page<dynamic>> pages,