From 269eaa45216d2859c76722be2da626bba3fae330 Mon Sep 17 00:00:00 2001 From: Mitchell Goodwin <58190796+MitchellGoodwin@users.noreply.github.com> Date: Mon, 2 Jun 2025 10:36:15 -0700 Subject: [PATCH] Check to see if previous page is laid out before starting hero flight (#169633) Fixes #168267 When going back to a page that never rendered with a hero transition, the app was hanging. This adds a check to see if a page needs to layout before starting the hero flight, and if so, adding a frame delay. ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [ ] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md --- packages/flutter/lib/src/widgets/heroes.dart | 21 ++-- .../flutter/test/widgets/heroes_test.dart | 102 ++++++++++++++++++ 2 files changed, 115 insertions(+), 8 deletions(-) diff --git a/packages/flutter/lib/src/widgets/heroes.dart b/packages/flutter/lib/src/widgets/heroes.dart index a12befa5e19..667d62dc050 100644 --- a/packages/flutter/lib/src/widgets/heroes.dart +++ b/packages/flutter/lib/src/widgets/heroes.dart @@ -677,12 +677,11 @@ class _HeroFlight { final HeroFlightDirection type = initialManifest.type; switch (type) { case HeroFlightDirection.pop: - return initial.value == 1.0 && initialManifest.isUserGestureTransition + return initialManifest.isUserGestureTransition // During user gesture transitions, the animation controller isn't - // driving the reverse transition, but should still be in a previously - // completed stage with the initial value at 1.0. - ? initial.status == AnimationStatus.completed - : initial.status == AnimationStatus.reverse; + // driving the reverse transition, so the status is not important. + || + initial.status == AnimationStatus.reverse; case HeroFlightDirection.push: return initial.value == 0.0 && initial.status == AnimationStatus.forward; } @@ -923,8 +922,15 @@ 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 && toRoute.maintainState) { + // immediately because their page's layout is still valid. Unless due to directly + // adding routes to the pages stack causing the route to never get laid out. + final RenderBox? fromRouteRenderBox = toRoute.subtreeContext?.findRenderObject() as RenderBox?; + final bool hasValidSize = + (fromRouteRenderBox?.hasSize ?? false) && fromRouteRenderBox!.size.isFinite; + if (isUserGestureTransition && + flightType == HeroFlightDirection.pop && + toRoute.maintainState && + hasValidSize) { _startHeroTransition(fromRoute, toRoute, flightType, isUserGestureTransition); } else { // Otherwise, delay measuring until the end of the next frame to allow @@ -934,7 +940,6 @@ class HeroController extends NavigatorObserver { // 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. toRoute.offstage = toRoute.animation!.value == 0.0; - WidgetsBinding.instance.addPostFrameCallback((Duration value) { if (fromRoute.navigator == null || toRoute.navigator == null) { return; diff --git a/packages/flutter/test/widgets/heroes_test.dart b/packages/flutter/test/widgets/heroes_test.dart index 83902f38fb8..26ab66d4744 100644 --- a/packages/flutter/test/widgets/heroes_test.dart +++ b/packages/flutter/test/widgets/heroes_test.dart @@ -216,6 +216,73 @@ class MyStatefulWidgetState extends State { Widget build(BuildContext context) => Text(widget.value); } +class DeepLinkApp extends StatefulWidget { + const DeepLinkApp({super.key}); + + static const CupertinoPage _homeScreen = CupertinoPage( + name: '/', + child: CupertinoPageScaffold( + navigationBar: CupertinoNavigationBar(middle: Text('First')), + child: Center(child: Text('Home Screen')), + ), + ); + static const CupertinoPage _middleScreen = CupertinoPage( + name: '/middle', + child: CupertinoPageScaffold( + navigationBar: CupertinoNavigationBar(middle: Text('Second')), + child: Center(child: Text('Middle Screen')), + ), + ); + static const CupertinoPage _lastScreen = CupertinoPage( + name: '/middle/last', + child: CupertinoPageScaffold( + navigationBar: CupertinoNavigationBar(middle: Text('Third')), + child: Center(child: Text('Last Screen')), + ), + ); + + @override + DeepLinkAppState createState() => DeepLinkAppState(); +} + +class DeepLinkAppState extends State { + late List> _pages; + + @override + void initState() { + super.initState(); + _pages = >[DeepLinkApp._homeScreen]; + } + + void goToDeepScreen() { + setState(() { + _pages = >[ + DeepLinkApp._homeScreen, + DeepLinkApp._middleScreen, + DeepLinkApp._lastScreen, + ]; + }); + } + + @override + Widget build(BuildContext context) { + return CupertinoApp( + builder: (BuildContext context, Widget? child) { + return Navigator( + pages: _pages, + onDidRemovePage: (Page page) { + setState(() { + if (_pages.length > 1) { + _pages = List>.from(_pages)..removeLast(); + } + }); + }, + ); + }, + ); + } +} + Future main() async { final ui.Image testImage = await createTestImage(); @@ -2984,6 +3051,41 @@ Future main() async { }), ); + // Regression test for https://github.com/flutter/flutter/issues/168267. + testWidgets('Check if previous page is laid out on backswipe gesture before flight', ( + WidgetTester tester, + ) async { + final GlobalKey appKey = GlobalKey(); + await tester.pumpWidget(DeepLinkApp(key: appKey)); + + await tester.pumpAndSettle(); + + expect(find.text('Home Screen'), findsOneWidget); + expect(find.text('Last Screen'), findsNothing); + + appKey.currentState?.goToDeepScreen(); + + await tester.pumpAndSettle(); + + expect(find.text('Home Screen'), findsNothing); + expect(find.text('Last Screen'), findsOneWidget); + + final TestGesture gesture = await tester.startGesture(const Offset(0.01, 300)); + + await gesture.moveTo(const Offset(10, 300)); + await tester.pump(); + // Should not throw an assert here for size and finite space. + await gesture.moveTo(const Offset(500, 300)); + await tester.pump(); + + await gesture.up(); + await tester.pumpAndSettle(); + + expect(find.text('Home Screen'), findsNothing); + expect(find.text('Middle Screen'), findsOneWidget); + expect(find.text('Last Screen'), findsNothing); + }); + // Regression test for https://github.com/flutter/flutter/issues/40239. testWidgets( 'In a pop transition, when fromHero is null, the to hero should eventually become visible',