From 88bee39f54220e0b6f2731d4e3704f5b496e8bd6 Mon Sep 17 00:00:00 2001 From: chunhtai <47866232+chunhtai@users.noreply.github.com> Date: Tue, 27 Aug 2024 18:53:24 -0700 Subject: [PATCH] Refactored HeroController logic to handle complex cases (#150027) previous pr https://github.com/flutter/flutter/pull/140675 fixes https://github.com/flutter/flutter/issues/115358 fixes https://github.com/flutter/flutter/issues/110426 fixes https://github.com/flutter/flutter/issues/88578 most of the issue here is that herocontroller try to deal with multiple push/pops in one frame. This pr introduce a new navigator observer api didChangeTop, which fired when the top most route change regardless the sequence of the operations. One of the uncertainty is how to infer the hero flight direction, but the current logic in this PR seems to be cover all the test cases --- packages/flutter/lib/src/widgets/heroes.dart | 73 +++--- .../flutter/lib/src/widgets/navigator.dart | 29 ++- .../flutter/test/widgets/heroes_test.dart | 238 ++++++++++++++++++ .../flutter/test/widgets/navigator_test.dart | 8 +- 4 files changed, 303 insertions(+), 45 deletions(-) diff --git a/packages/flutter/lib/src/widgets/heroes.dart b/packages/flutter/lib/src/widgets/heroes.dart index 12fc059462..b4015d1456 100644 --- a/packages/flutter/lib/src/widgets/heroes.dart +++ b/packages/flutter/lib/src/widgets/heroes.dart @@ -834,34 +834,31 @@ class HeroController extends NavigatorObserver { final Map _flights = {}; @override - void didPush(Route route, Route? previousRoute) { - assert(navigator != null); - _maybeStartHeroTransition(previousRoute, route, HeroFlightDirection.push, false); - } - - @override - void didPop(Route route, Route? previousRoute) { + void didChangeTop(Route topRoute, Route? previousTopRoute) { + assert(topRoute.isCurrent); assert(navigator != null); + if (previousTopRoute == null) { + return; + } // Don't trigger another flight when a pop is committed as a user gesture // back swipe is snapped. if (!navigator!.userGestureInProgress) { - _maybeStartHeroTransition(route, previousRoute, HeroFlightDirection.pop, false); - } - } - - @override - void didReplace({ Route? newRoute, Route? oldRoute }) { - assert(navigator != null); - if (newRoute?.isCurrent ?? false) { - // Only run hero animations if the top-most route got replaced. - _maybeStartHeroTransition(oldRoute, newRoute, HeroFlightDirection.push, false); + _maybeStartHeroTransition( + fromRoute: previousTopRoute, + toRoute: topRoute, + isUserGestureTransition: false, + ); } } @override void didStartUserGesture(Route route, Route? previousRoute) { assert(navigator != null); - _maybeStartHeroTransition(route, previousRoute, HeroFlightDirection.pop, true); + _maybeStartHeroTransition( + fromRoute: route, + toRoute: previousRoute, + isUserGestureTransition: true, + ); } @override @@ -894,29 +891,37 @@ class HeroController extends NavigatorObserver { // If we're transitioning between different page routes, start a hero transition // after the toRoute has been laid out with its animation's value at 1.0. - void _maybeStartHeroTransition( - Route? fromRoute, - Route? toRoute, - HeroFlightDirection flightType, - bool isUserGestureTransition, - ) { + void _maybeStartHeroTransition({ + required Route? fromRoute, + required Route? toRoute, + required bool isUserGestureTransition, + }) { if (toRoute == fromRoute || toRoute is! PageRoute || fromRoute is! PageRoute) { return; } - - final PageRoute from = fromRoute; - final PageRoute to = toRoute; + final Animation newRouteAnimation = toRoute.animation!; + final Animation oldRouteAnimation = fromRoute.animation!; + final HeroFlightDirection flightType; + switch ((isUserGestureTransition, oldRouteAnimation.status, newRouteAnimation.status)) { + case (true, _, _): + case (_, AnimationStatus.reverse, _): + flightType = HeroFlightDirection.pop; + case (_, _, AnimationStatus.forward): + flightType = HeroFlightDirection.push; + default: + return; + } // A user gesture may have already completed the pop, or we might be the initial route switch (flightType) { case HeroFlightDirection.pop: - if (from.animation!.value == 0.0) { + if (fromRoute.animation!.value == 0.0) { return; } case HeroFlightDirection.push: - if (to.animation!.value == 1.0) { + if (toRoute.animation!.value == 1.0) { return; } } @@ -924,8 +929,8 @@ class HeroController extends NavigatorObserver { // For pop transitions driven by a user gesture: if the "to" page has // maintainState = true, then the hero's final dimensions can be measured // immediately because their page's layout is still valid. - if (isUserGestureTransition && flightType == HeroFlightDirection.pop && to.maintainState) { - _startHeroTransition(from, to, flightType, isUserGestureTransition); + if (isUserGestureTransition && flightType == HeroFlightDirection.pop && toRoute.maintainState) { + _startHeroTransition(fromRoute, toRoute, flightType, isUserGestureTransition); } else { // Otherwise, delay measuring until the end of the next frame to allow // the 'to' route to build and layout. @@ -933,13 +938,13 @@ class HeroController extends NavigatorObserver { // Putting a route offstage changes its animation value to 1.0. Once this // frame completes, we'll know where the heroes in the `to` route are // going to end up, and the `to` route will go back onstage. - to.offstage = to.animation!.value == 0.0; + toRoute.offstage = toRoute.animation!.value == 0.0; WidgetsBinding.instance.addPostFrameCallback((Duration value) { - if (from.navigator == null || to.navigator == null) { + if (fromRoute.navigator == null || toRoute.navigator == null) { return; } - _startHeroTransition(from, to, flightType, isUserGestureTransition); + _startHeroTransition(fromRoute, toRoute, flightType, isUserGestureTransition); }, debugLabel: 'HeroController.startTransition'); } } diff --git a/packages/flutter/lib/src/widgets/navigator.dart b/packages/flutter/lib/src/widgets/navigator.dart index c3ccc0f69d..cdbe527242 100644 --- a/packages/flutter/lib/src/widgets/navigator.dart +++ b/packages/flutter/lib/src/widgets/navigator.dart @@ -793,6 +793,17 @@ class NavigatorObserver { /// The [Navigator] replaced `oldRoute` with `newRoute`. void didReplace({ Route? newRoute, Route? oldRoute }) { } + /// The top most route has changed. + /// + /// The `topRoute` is the new top most route. This can be a new route pushed + /// on top of the screen, or an existing route that becomes the new top-most + /// route because the previous top-most route has been popped. + /// + /// The `previousTopRoute` was the top most route before the change. This can + /// be a route that was popped off the screen, or a route that will be covered + /// by the `topRoute`. This can also be null if this is the first build. + void didChangeTop(Route topRoute, Route? previousTopRoute) { } + /// The [Navigator]'s routes are being moved by a user gesture. /// /// For example, this is called when an iOS back gesture starts, and is used @@ -3962,12 +3973,14 @@ class NavigatorState extends State with TickerProviderStateMixin, Res for (final NavigatorObserver observer in _effectiveObservers) { NavigatorObserver._navigators[observer] = null; } + _effectiveObservers = []; super.deactivate(); } @override void activate() { super.activate(); + _updateEffectiveObservers(); for (final NavigatorObserver observer in _effectiveObservers) { assert(observer.navigator == null); NavigatorObserver._navigators[observer] = this; @@ -3981,12 +3994,7 @@ class NavigatorState extends State with TickerProviderStateMixin, Res _debugLocked = true; return true; }()); - assert(() { - for (final NavigatorObserver observer in _effectiveObservers) { - assert(observer.navigator != this); - } - return true; - }()); + assert(_effectiveObservers.isEmpty); _updateHeroController(null); focusNode.dispose(); _forcedDisposeAllRouteEntries(); @@ -4011,6 +4019,7 @@ class NavigatorState extends State with TickerProviderStateMixin, Res ]; } + _RouteEntry? _lastTopmostRoute; String? _lastAnnouncedRouteName; bool _debugUpdatingPage = false; @@ -4437,9 +4446,15 @@ class NavigatorState extends State with TickerProviderStateMixin, Res // notifications. _flushRouteAnnouncement(); + final _RouteEntry? lastEntry = _lastRouteEntryWhereOrNull(_RouteEntry.isPresentPredicate); + if (lastEntry != null && _lastTopmostRoute != lastEntry) { + for (final NavigatorObserver observer in _effectiveObservers) { + observer.didChangeTop(lastEntry.route, _lastTopmostRoute?.route); + } + } + _lastTopmostRoute = lastEntry; // Announce route name changes. if (widget.reportsRouteUpdateToEngine) { - final _RouteEntry? lastEntry = _lastRouteEntryWhereOrNull(_RouteEntry.isPresentPredicate); final String? routeName = lastEntry?.route.settings.name; if (routeName != null && routeName != _lastAnnouncedRouteName) { SystemNavigator.routeInformationUpdated(uri: Uri.parse(routeName)); diff --git a/packages/flutter/test/widgets/heroes_test.dart b/packages/flutter/test/widgets/heroes_test.dart index d9bd88e1fc..ea6a4ad3f2 100644 --- a/packages/flutter/test/widgets/heroes_test.dart +++ b/packages/flutter/test/widgets/heroes_test.dart @@ -2347,6 +2347,244 @@ Future main() async { expect(tester.getSize(find.byKey(smallContainer)), const Size(100,100)); }); + testWidgets('Can add two page with heroes simultaneously using page API.', (WidgetTester tester) async { + // Regression test for https://github.com/flutter/flutter/issues/115358. + + const String heroTag = 'foo'; + final GlobalKey navigator = GlobalKey(); + final Key smallContainer = UniqueKey(); + final Key largeContainer = UniqueKey(); + final MaterialPage page1 = MaterialPage( + child: Center( + child: Card( + child: Hero( + tag: heroTag, + child: Container( + key: largeContainer, + color: Colors.red, + height: 200.0, + width: 200.0, + ), + ), + ), + ), + ); + final MaterialPage page2 = MaterialPage( + child: Center( + child: Card( + child: Hero( + tag: heroTag, + child: Container( + color: Colors.red, + height: 1000.0, + width: 1000.0, + ), + ), + ), + ), + ); + final MaterialPage page3 = MaterialPage( + child: Center( + child: Card( + child: Hero( + tag: heroTag, + child: Container( + key: smallContainer, + color: Colors.red, + height: 100.0, + width: 100.0, + ), + ), + ), + ), + ); + final HeroController controller = HeroController(); + + await tester.pumpWidget( + MaterialApp( + navigatorKey: navigator, + home: Navigator( + observers: [controller], + pages: >[page1], + onPopPage: (_, __) => false, + ), + ) + ); + + // The initial setup. + expect(find.byKey(largeContainer), isOnstage); + expect(find.byKey(largeContainer), isInCard); + expect(find.byKey(smallContainer, skipOffstage: false), findsNothing); + + await tester.pumpWidget( + MaterialApp( + navigatorKey: navigator, + home: Navigator( + observers: [controller], + pages: >[page1, page2, page3], + onPopPage: (_, __) => false, + ), + ), + ); + + expect(find.byKey(largeContainer), isOnstage); + expect(find.byKey(largeContainer), isInCard); + expect(find.byKey(smallContainer, skipOffstage: false), isOffstage); + expect(find.byKey(smallContainer, skipOffstage: false), isInCard); + + await tester.pump(); + + // The hero started flying. + expect(find.byKey(largeContainer), findsNothing); + expect(find.byKey(smallContainer), isOnstage); + expect(find.byKey(smallContainer), isNotInCard); + + await tester.pump(const Duration(milliseconds: 100)); + + // The hero is in-flight. + expect(find.byKey(largeContainer), findsNothing); + expect(find.byKey(smallContainer), isOnstage); + expect(find.byKey(smallContainer), isNotInCard); + final Size size = tester.getSize(find.byKey(smallContainer)); + expect(size.height, greaterThan(100)); + expect(size.width, greaterThan(100)); + expect(size.height, lessThan(200)); + expect(size.width, lessThan(200)); + + await tester.pumpAndSettle(); + + // The transition has ended. + expect(find.byKey(largeContainer), findsNothing); + expect(find.byKey(smallContainer), isOnstage); + expect(find.byKey(smallContainer), isInCard); + expect(tester.getSize(find.byKey(smallContainer)), const Size(100,100)); + }); + + testWidgets('Can still trigger hero even if page underneath changes', (WidgetTester tester) async { + // Regression test for https://github.com/flutter/flutter/issues/88578. + + const String heroTag = 'foo'; + final GlobalKey navigator = GlobalKey(); + final Key smallContainer = UniqueKey(); + final Key largeContainer = UniqueKey(); + final MaterialPage unrelatedPage1 = MaterialPage( + key: UniqueKey(), + child: Center( + child: Card( + child: Container( + color: Colors.red, + height: 1000.0, + width: 1000.0, + ), + ), + ), + ); + final MaterialPage unrelatedPage2 = MaterialPage( + key: UniqueKey(), + child: Center( + child: Card( + child: Container( + color: Colors.red, + height: 1000.0, + width: 1000.0, + ), + ), + ), + ); + final MaterialPage page1 = MaterialPage( + key: UniqueKey(), + child: Center( + child: Card( + child: Hero( + tag: heroTag, + child: Container( + key: largeContainer, + color: Colors.red, + height: 200.0, + width: 200.0, + ), + ), + ), + ), + ); + final MaterialPage page2 = MaterialPage( + key: UniqueKey(), + child: Center( + child: Card( + child: Hero( + tag: heroTag, + child: Container( + key: smallContainer, + color: Colors.red, + height: 100.0, + width: 100.0, + ), + ), + ), + ), + ); + final HeroController controller = HeroController(); + + await tester.pumpWidget( + MaterialApp( + navigatorKey: navigator, + home: Navigator( + observers: [controller], + pages: >[unrelatedPage1, page1], + onPopPage: (_, __) => false, + ), + ) + ); + + // The initial setup. + expect(find.byKey(largeContainer), isOnstage); + expect(find.byKey(largeContainer), isInCard); + expect(find.byKey(smallContainer, skipOffstage: false), findsNothing); + + await tester.pumpWidget( + MaterialApp( + navigatorKey: navigator, + home: Navigator( + observers: [controller], + pages: >[unrelatedPage2, page2], + onPopPage: (_, __) => false, + ), + ), + ); + + expect(find.byKey(largeContainer), isOnstage); + expect(find.byKey(largeContainer), isInCard); + expect(find.byKey(smallContainer, skipOffstage: false), isOffstage); + expect(find.byKey(smallContainer, skipOffstage: false), isInCard); + + await tester.pump(); + + // The hero started flying. + expect(find.byKey(largeContainer), findsNothing); + expect(find.byKey(smallContainer), isOnstage); + expect(find.byKey(smallContainer), isNotInCard); + + await tester.pump(const Duration(milliseconds: 100)); + + // The hero is in-flight. + expect(find.byKey(largeContainer), findsNothing); + expect(find.byKey(smallContainer), isOnstage); + expect(find.byKey(smallContainer), isNotInCard); + final Size size = tester.getSize(find.byKey(smallContainer)); + expect(size.height, greaterThan(100)); + expect(size.width, greaterThan(100)); + expect(size.height, lessThan(200)); + expect(size.width, lessThan(200)); + + await tester.pumpAndSettle(); + + // The transition has ended. + expect(find.byKey(largeContainer), findsNothing); + expect(find.byKey(smallContainer), isOnstage); + expect(find.byKey(smallContainer), isInCard); + expect(tester.getSize(find.byKey(smallContainer)), const Size(100,100)); + }); + testWidgets('On an iOS back swipe and snap, only a single flight should take place', (WidgetTester tester) async { int shuttlesBuilt = 0; Widget shuttleBuilder( diff --git a/packages/flutter/test/widgets/navigator_test.dart b/packages/flutter/test/widgets/navigator_test.dart index 3f1bb6a991..3b2560d2d6 100644 --- a/packages/flutter/test/widgets/navigator_test.dart +++ b/packages/flutter/test/widgets/navigator_test.dart @@ -4350,14 +4350,14 @@ void main() { } await tester.pumpWidget(build()); - observer._checkInvocations([#navigator, #didPush]); + observer._checkInvocations([#navigator, #didPush, #didChangeTop]); await tester.pumpWidget(Container(child: build())); - observer._checkInvocations([#navigator, #didPush, #navigator]); + observer._checkInvocations([#navigator, #didPush, #didChangeTop]); await tester.pumpWidget(Container()); - observer._checkInvocations([#navigator]); + observer._checkInvocations([]); final GlobalKey key = GlobalKey(); await tester.pumpWidget(build(key)); - observer._checkInvocations([#navigator, #didPush]); + observer._checkInvocations([#navigator, #didPush, #didChangeTop]); await tester.pumpWidget(Container(child: build(key))); observer._checkInvocations([#navigator, #navigator]); });