From 731e9819e2ec30232295d4a34cca7d075d0266e0 Mon Sep 17 00:00:00 2001 From: LongCatIsLooong <31859944+LongCatIsLooong@users.noreply.github.com> Date: Thu, 13 Jun 2019 20:41:43 -0700 Subject: [PATCH] Allow "from" hero state to survive hero animation in a push transition (#32842) --- .../flutter/lib/src/cupertino/nav_bar.dart | 3 +- packages/flutter/lib/src/widgets/heroes.dart | 98 +++++-- .../flutter/test/widgets/heroes_test.dart | 258 +++++++++++++++++- 3 files changed, 324 insertions(+), 35 deletions(-) diff --git a/packages/flutter/lib/src/cupertino/nav_bar.dart b/packages/flutter/lib/src/cupertino/nav_bar.dart index d7401f2ed9..fb4b94259e 100644 --- a/packages/flutter/lib/src/cupertino/nav_bar.dart +++ b/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); diff --git a/packages/flutter/lib/src/widgets/heroes.dart b/packages/flutter/lib/src/widgets/heroes.dart index 015e6e42db..66592a08c1 100644 --- a/packages/flutter/lib/src/widgets/heroes.dart +++ b/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 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 { final GlobalKey _key = GlobalKey(); Size _placeholderSize; + // 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; - void startFlight() { + // 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 { '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 diff --git a/packages/flutter/test/widgets/heroes_test.dart b/packages/flutter/test/widgets/heroes_test.dart index d69cb446cc..4730bc3f73 100644 --- a/packages/flutter/test/widgets/heroes_test.dart +++ b/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 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 { } } +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 { Widget build(BuildContext context) => Text(widget.value); } -void main() { +Future 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: [ // 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 renderers = tester.renderObjectList(find.byType(Opacity)); - final Iterable opacities = renderers.map((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 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 navigatorKey = GlobalKey(); + final GlobalKey<_SimpleState> key1 = GlobalKey<_SimpleState>(); + final GlobalKey key2 = GlobalKey(); + + await tester.pumpWidget( + CupertinoApp( + navigatorKey: navigatorKey, + home: Row( + crossAxisAlignment: CrossAxisAlignment.start, + children: [ + Hero( + tag: 'hero', + transitionOnUserGestures: true, + child: _SimpleStatefulWidget(key: key1), + ), + const SizedBox( + width: 10, + height: 10, + child: Text('1'), + ) + ] + ) + ), + ); + + final CupertinoPageRoute route2 = CupertinoPageRoute( + 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 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: [ + 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 route2 = CupertinoPageRoute( + 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) + ); + }); }