Allow "from" hero state to survive hero animation in a push transition (#32842)

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

@@ -2174,8 +2174,9 @@ CreateRectTween _linearTranslateWithLargestRectSizeTween = (Rect begin, Rect end
   );
 };
 
-final TransitionBuilder _navBarHeroLaunchPadBuilder = (
+final HeroPlaceholderBuilder _navBarHeroLaunchPadBuilder = (
   BuildContext context,
+  Size heroSize,
   Widget child,
 ) {
   assert(child is _TransitionableNavigationBar);

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

@@ -11,6 +11,7 @@ import 'navigator.dart';
 import 'overlay.dart';
 import 'pages.dart';
 import 'routes.dart';
+import 'ticker_provider.dart' show TickerMode;
 import 'transitions.dart';
 
 /// Signature for a function that takes two [Rect] instances and returns a
@@ -21,6 +22,22 @@ import 'transitions.dart';
 /// [MaterialRectArcTween].
 typedef CreateRectTween = Tween<Rect> Function(Rect begin, Rect end);
 
+/// Signature for a function that builds a [Hero] placeholder widget given a
+/// child and a [Size].
+///
+/// The child can optionally be part of the returned widget tree. The returned
+/// widget should typically be constrained to [heroSize], if it doesn't do so
+/// implicitly.
+///
+/// See also:
+///  * [TransitionBuilder], which is similar but only takes a [BuildContext]
+///    and a child widget.
+typedef HeroPlaceholderBuilder = Widget Function(
+  BuildContext context,
+  Size heroSize,
+  Widget child,
+);
+
 /// A function that lets [Hero]es self supply a [Widget] that is shown during the
 /// hero's flight from one route to another instead of default (which is to
 /// show the destination route's instance of the Hero).
@@ -189,13 +206,30 @@ class Hero extends StatefulWidget {
   ///
   /// If none is provided, the destination route's Hero child is shown in-flight
   /// by default.
+  ///
+  /// ## Limitations
+  ///
+  /// If a widget built by [flightShuttleBuilder] takes part in a [Navigator]
+  /// push transition, that widget or its descendants must not have any
+  /// [GlobalKey] that is used in the source Hero's descendant widgets. That is
+  /// because both subtrees will be included in the widget tree during the Hero
+  /// flight animation, and [GlobalKey]s must be unique across the entire widget
+  /// tree.
+  ///
+  /// If the said [GlobalKey] is essential to your application, consider providing
+  /// a custom [placeholderBuilder] for the source Hero, to avoid the [GlobalKey]
+  /// collision, such as a builder that builds an empty [SizedBox], keeping the
+  /// Hero [child]'s original size.
   final HeroFlightShuttleBuilder flightShuttleBuilder;
 
-  /// Placeholder widget left in place as the Hero's child once the flight takes off.
+  /// Placeholder widget left in place as the Hero's [child] once the flight takes
+  /// off.
   ///
-  /// By default, an empty SizedBox keeping the Hero child's original size is
-  /// left in place once the Hero shuttle has taken flight.
-  final TransitionBuilder placeholderBuilder;
+  /// By default the placeholder widget is an empty [SizedBox] keeping the Hero
+  /// child's original size, unless this Hero is a source Hero of a [Navigator]
+  /// push transition, in which case [child] will be a descendant of the placeholder
+  /// and will be kept [Offstage] during the Hero's flight.
+  final HeroPlaceholderBuilder placeholderBuilder;
 
   /// Whether to perform the hero transition if the [PageRoute] transition was
   /// triggered by a user gesture, such as a back swipe on iOS.
@@ -285,8 +319,24 @@ class Hero extends StatefulWidget {
 class _HeroState extends State<Hero> {
   final GlobalKey _key = GlobalKey();
   Size _placeholderSize;
-
-  void startFlight() {
+  // Whether the placeholder widget should wrap the hero's child widget as its
+  // own child, when `_placeholderSize` is non-null (i.e. the hero is currently
+  // in its flight animation). See `startFlight`.
+  bool _shouldIncludeChild = true;
+
+  // The `shouldIncludeChildInPlaceholder` flag dictates if the child widget of
+  // this hero should be included in the placeholder widget as a descendant.
+  //
+  // When a new hero flight animation takes place, a placeholder widget
+  // needs to be built to replace the original hero widget. When
+  // `shouldIncludeChildInPlaceholder` is set to true and `widget.placeholderBuilder`
+  // is null, the placeholder widget will include the original hero's child
+  // widget as a descendant, allowing the orignal element tree to be preserved.
+  //
+  // It is typically set to true for the *from* hero in a push transition,
+  // and false otherwise.
+  void startFlight({ bool shouldIncludedChildInPlaceholder = false }) {
+    _shouldIncludeChild = shouldIncludedChildInPlaceholder;
     assert(mounted);
     final RenderBox box = context.findRenderObject();
     assert(box != null && box.hasSize);
@@ -310,19 +360,29 @@ class _HeroState extends State<Hero> {
       'A Hero widget cannot be the descendant of another Hero widget.'
     );
 
-    if (_placeholderSize != null) {
-      if (widget.placeholderBuilder == null) {
-        return SizedBox(
-          width: _placeholderSize.width,
-          height: _placeholderSize.height,
-        );
-      } else {
-        return widget.placeholderBuilder(context, widget.child);
-      }
+    final bool isHeroInFlight = _placeholderSize != null;
+
+    if (isHeroInFlight && widget.placeholderBuilder != null) {
+      return widget.placeholderBuilder(context, _placeholderSize, widget.child);
     }
-    return KeyedSubtree(
-      key: _key,
-      child: widget.child,
+
+    if (isHeroInFlight && !_shouldIncludeChild) {
+      return SizedBox(
+        width: _placeholderSize.width,
+        height: _placeholderSize.height,
+      );
+    }
+
+    return SizedBox(
+      width: _placeholderSize?.width,
+      height: _placeholderSize?.height,
+      child: Offstage(
+        offstage: isHeroInFlight,
+        child: TickerMode(
+          enabled: !isHeroInFlight,
+          child: KeyedSubtree(key: _key, child: widget.child),
+        )
+      ),
     );
   }
 }
@@ -496,7 +556,7 @@ class _HeroFlight {
     else
       _proxyAnimation.parent = manifest.animation;
 
-    manifest.fromHero.startFlight();
+    manifest.fromHero.startFlight(shouldIncludedChildInPlaceholder: manifest.type == HeroFlightDirection.push);
     manifest.toHero.startFlight();
 
     heroRectTween = _doCreateRectTween(
@@ -574,7 +634,7 @@ class _HeroFlight {
       manifest.toHero.endFlight();
 
       // Let the heroes in each of the routes rebuild with their placeholders.
-      newManifest.fromHero.startFlight();
+      newManifest.fromHero.startFlight(shouldIncludedChildInPlaceholder: newManifest.type == HeroFlightDirection.push);
       newManifest.toHero.startFlight();
 
       // Let the transition overlay on top of the routes also rebuild since

packages/flutter/test/widgets/heroes_test.dart

@@ -2,11 +2,26 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+import 'dart:ui' as ui;
+
 import 'package:flutter_test/flutter_test.dart';
 import 'package:flutter/cupertino.dart';
 import 'package:flutter/material.dart';
 import 'package:flutter/rendering.dart';
 
+import '../painting/image_test_utils.dart' show TestImageProvider;
+
+Future<ui.Image> createTestImage() {
+  final ui.Paint paint = ui.Paint()
+    ..style = ui.PaintingStyle.stroke
+    ..strokeWidth = 1.0;
+  final ui.PictureRecorder recorder = ui.PictureRecorder();
+  final ui.Canvas pictureCanvas = ui.Canvas(recorder);
+  pictureCanvas.drawCircle(Offset.zero, 20.0, paint);
+  final ui.Picture picture = recorder.endRecording();
+  return picture.toImage(300, 300);
+}
+
 Key firstKey = const Key('first');
 Key secondKey = const Key('second');
 Key thirdKey = const Key('third');
@@ -124,6 +139,19 @@ class MutatingRoute extends MaterialPageRoute<void> {
   }
 }
 
+class _SimpleStatefulWidget extends StatefulWidget {
+  const _SimpleStatefulWidget({ Key key }) : super(key: key);
+  @override
+  _SimpleState createState() => _SimpleState();
+}
+
+class _SimpleState extends State<_SimpleStatefulWidget> {
+  int state = 0;
+
+  @override
+  Widget build(BuildContext context) => Text(state.toString());
+}
+
 class MyStatefulWidget extends StatefulWidget {
   const MyStatefulWidget({ Key key, this.value = '123' }) : super(key: key);
   final String value;
@@ -136,7 +164,9 @@ class MyStatefulWidgetState extends State<MyStatefulWidget> {
   Widget build(BuildContext context) => Text(widget.value);
 }
 
-void main() {
+Future<void> main() async {
+  final ui.Image testImage = await createTestImage();
+
   setUp(() {
     transitionFromUserGestures = false;
   });
@@ -168,16 +198,22 @@ void main() {
     // seeing them at t=16ms. The original page no longer contains the hero.
 
     expect(find.byKey(firstKey), findsNothing);
-    expect(find.byKey(secondKey), isOnstage);
+
+    expect(find.byKey(secondKey), findsOneWidget);
     expect(find.byKey(secondKey), isNotInCard);
+    expect(find.byKey(secondKey), isOnstage);
 
     await tester.pump();
 
     // t=32ms for the journey. Surely they are still at it.
 
     expect(find.byKey(firstKey), findsNothing);
-    expect(find.byKey(secondKey), isOnstage);
+
+    expect(find.byKey(secondKey), findsOneWidget);
+
+    expect(find.byKey(secondKey), findsOneWidget);
     expect(find.byKey(secondKey), isNotInCard);
+    expect(find.byKey(secondKey), isOnstage);
 
     await tester.pump(const Duration(seconds: 1));
 
@@ -883,7 +919,14 @@ void main() {
             children: <Widget>[
               // This container will appear at Y=0
               Container(
-                child: Hero(tag: 'BC', child: Container(key: heroBCKey, height: 150.0)),
+                child: Hero(
+                  tag: 'BC',
+                  child: Container(
+                    key: heroBCKey,
+                    height: 150.0,
+                    child: const Text('Hero'),
+                  )
+                ),
               ),
               const SizedBox(height: 800.0),
             ],
@@ -901,14 +944,27 @@ void main() {
               const SizedBox(height: 100.0),
               // This container will appear at Y=100
               Container(
-                child: Hero(tag: 'AB', child: Container(key: heroABKey, height: 200.0)),
+                child: Hero(
+                  tag: 'AB',
+                  child: Container(
+                    key: heroABKey,
+                    height: 200.0,
+                    child: const Text('Hero'),
+                  )
+                ),
               ),
               FlatButton(
                 child: const Text('PUSH C'),
                 onPressed: () { Navigator.push(context, routeC); },
               ),
               Container(
-                child: Hero(tag: 'BC', child: Container(height: 150.0)),
+                child: Hero(
+                  tag: 'BC',
+                  child: Container(
+                    height: 150.0,
+                    child: const Text('Hero'),
+                  )
+                ),
               ),
               const SizedBox(height: 800.0),
             ],
@@ -928,7 +984,14 @@ void main() {
                   const SizedBox(height: 200.0),
                   // This container will appear at Y=200
                   Container(
-                    child: Hero(tag: 'AB', child: Container(height: 100.0, width: 100.0)),
+                    child: Hero(
+                      tag: 'AB',
+                      child: Container(
+                        height: 100.0,
+                        width: 100.0,
+                        child: const Text('Hero'),
+                      )
+                    ),
                   ),
                   FlatButton(
                     child: const Text('PUSH B'),
@@ -966,10 +1029,22 @@ void main() {
     await tester.pump(const Duration(milliseconds: 100));
     expect(tester.getTopLeft(find.byKey(heroABKey)).dy, 100.0);
 
-    // One Opacity widget per Hero, only one now has opacity 0.0
-    final Iterable<RenderOpacity> renderers = tester.renderObjectList(find.byType(Opacity));
-    final Iterable<double> opacities = renderers.map<double>((RenderOpacity r) => r.opacity);
-    expect(opacities.singleWhere((double opacity) => opacity == 0.0), 0.0);
+    bool _isVisible(Element node) {
+      bool isVisible = true;
+      node.visitAncestorElements((Element ancestor) {
+          final RenderObject r = ancestor.renderObject;
+          if (r is RenderOpacity && r.opacity == 0) {
+            isVisible = false;
+            return false;
+          }
+          return true;
+      });
+      return isVisible;
+    }
+
+    // Of all heroes only one should be visible now.
+    final Iterable<Element> elements = find.text('Hero').evaluate();
+    expect(elements.where(_isVisible).length, 1);
 
     // Hero BC's flight finishes normally.
     await tester.pump(const Duration(milliseconds: 300));
@@ -1038,6 +1113,7 @@ void main() {
 
     // Push flight underway.
     await tester.pump(const Duration(milliseconds: 100));
+    // Visible in the hero animation.
     expect(find.text('456'), findsOneWidget);
 
     // Push flight finished.
@@ -1439,7 +1515,7 @@ void main() {
             Hero(
               tag: 'a',
               child: const Text('Batman'),
-              placeholderBuilder: (BuildContext context, Widget child) {
+              placeholderBuilder: (BuildContext context, Size heroSize, Widget child) {
                 return const Text('Venom');
               },
             ),
@@ -1452,7 +1528,7 @@ void main() {
                       child: Hero(
                         tag: 'a',
                         child: const Text('Wolverine'),
-                        placeholderBuilder: (BuildContext context, Widget child) {
+                        placeholderBuilder: (BuildContext context, Size size, Widget child) {
                           return const Text('Joker');
                         },
                       ),
@@ -1925,7 +2001,7 @@ void main() {
           // Since we're popping, only the destination route's builder is used.
           flightShuttleBuilder: shuttleBuilder,
           transitionOnUserGestures: true,
-          child: const Text('1')
+          child: const Text('1'),
         ),
       ),
     );
@@ -1936,7 +2012,7 @@ void main() {
           child: Hero(
             tag: navigatorKey,
             transitionOnUserGestures: true,
-            child: const Text('2')
+            child: const Text('2'),
           ),
         );
       }
@@ -1965,4 +2041,156 @@ void main() {
     // Still one shuttle.
     expect(shuttlesBuilt, 2);
   });
+
+  testWidgets("From hero's state should be preserved, "
+    'heroes work well with child widgets that has global keys',
+    (WidgetTester tester) async {
+      final GlobalKey<NavigatorState> navigatorKey = GlobalKey();
+      final GlobalKey<_SimpleState> key1 = GlobalKey<_SimpleState>();
+      final GlobalKey key2 = GlobalKey();
+
+      await tester.pumpWidget(
+        CupertinoApp(
+          navigatorKey: navigatorKey,
+          home: Row(
+            crossAxisAlignment: CrossAxisAlignment.start,
+            children: <Widget>[
+              Hero(
+                tag: 'hero',
+                transitionOnUserGestures: true,
+                child: _SimpleStatefulWidget(key: key1),
+              ),
+              const SizedBox(
+                width: 10,
+                height: 10,
+                child: Text('1'),
+              )
+            ]
+          )
+        ),
+      );
+
+      final CupertinoPageRoute<void> route2 = CupertinoPageRoute<void>(
+        builder: (BuildContext context) {
+          return CupertinoPageScaffold(
+            child: Hero(
+              tag: 'hero',
+              transitionOnUserGestures: true,
+              // key2 is a `GlobalKey`. The hero animation should not
+              // assert by having the same global keyed widget in more
+              // than one place in the tree.
+              child: _SimpleStatefulWidget(key: key2),
+            ),
+          );
+        }
+      );
+
+      final _SimpleState state1 = key1.currentState;
+      state1.state = 1;
+
+      navigatorKey.currentState.push(route2);
+      await tester.pump();
+
+      expect(state1.mounted, isTrue);
+
+      await tester.pumpAndSettle();
+      expect(state1.state, 1);
+      // The element should be mounted and unique.
+      expect(state1.mounted, isTrue);
+
+      expect(navigatorKey.currentState.pop(), isTrue);
+      await tester.pumpAndSettle();
+
+      // State is preserved.
+      expect(state1.state, 1);
+      // The element should be mounted and unique.
+      expect(state1.mounted, isTrue);
+  });
+
+  testWidgets("Hero works with images that don't have both width and height specified",
+    // Regression test for https://github.com/flutter/flutter/issues/32356
+    // and https://github.com/flutter/flutter/issues/31503
+    (WidgetTester tester) async {
+      final GlobalKey<NavigatorState> navigatorKey = GlobalKey();
+      const Key imageKey1 = Key('image1');
+      const Key imageKey2 = Key('image2');
+      final TestImageProvider imageProvider = TestImageProvider(testImage);
+
+      await tester.pumpWidget(
+        CupertinoApp(
+          navigatorKey: navigatorKey,
+          home: Row(
+            crossAxisAlignment: CrossAxisAlignment.start,
+            children: <Widget>[
+              Hero(
+                tag: 'hero',
+                transitionOnUserGestures: true,
+                child: Container(
+                  width: 100,
+                  child: Image(
+                    image: imageProvider,
+                    key: imageKey1,
+                  )
+                )
+              ),
+              const SizedBox(
+                width: 10,
+                height: 10,
+                child: Text('1'),
+              )
+            ]
+          )
+        ),
+      );
+
+      final CupertinoPageRoute<void> route2 = CupertinoPageRoute<void>(
+        builder: (BuildContext context) {
+          return CupertinoPageScaffold(
+            child: Hero(
+              tag: 'hero',
+              transitionOnUserGestures: true,
+              child: Container(
+                child: Image(
+                  image: imageProvider,
+                  key: imageKey2,
+                )
+              )
+            ),
+          );
+        }
+      );
+
+    // Load image before measuring the `Rect` of the `RenderImage`.
+    imageProvider.complete();
+    await tester.pump();
+    final RenderImage renderImage = tester.renderObject(
+      find.descendant(of: find.byKey(imageKey1), matching: find.byType(RawImage))
+    );
+
+    // Before push image1 should be laid out correctly.
+    expect(renderImage.size, const Size(100, 100));
+
+    navigatorKey.currentState.push(route2);
+    await tester.pump();
+
+    final TestGesture gesture = await tester.startGesture(const Offset(0.01, 300));
+    await tester.pump();
+
+    // Move (almost) across the screen, to make the animation as close to finish
+    // as possible.
+    await gesture.moveTo(const Offset(800, 200));
+    await tester.pump();
+
+    // image1 should snap to the top left corner of the Row widget.
+    expect(
+      tester.getRect(find.byKey(imageKey1, skipOffstage: false)),
+      rectMoreOrLessEquals(tester.getTopLeft(find.widgetWithText(Row, '1')) & const Size(100, 100), epsilon: 0.01)
+    );
+
+    // Text should respect the correct final size of image1.
+    expect(
+      tester.getTopRight(find.byKey(imageKey1, skipOffstage: false)).dx,
+      moreOrLessEquals(tester.getTopLeft(find.text('1')).dx, epsilon: 0.01)
+    );
+  });
 }