From c8e34e067c4ad2df3b726db32d7480222960115a Mon Sep 17 00:00:00 2001 From: Chikamatsu Kazuya <43089218+chika3742@users.noreply.github.com> Date: Thu, 23 Jan 2025 04:13:01 +0900 Subject: [PATCH] Match CupertinoPageTransitionsBuilder animation duration to CupertinoPageRoute (2) (#161577) Original PR: #160241 (reverted in #161555 due to regression ), closes #29068 In addition to the original PR, this includes a fix for the integration test failure of [flutter_gallery/transitions_perf_e2e](https://github.com/flutter/flutter/tree/1b0441c18ae0266ec62a66d846b6b38debc8f772/dev/integration_tests/flutter_gallery/test_driver). Error log is [here](https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8725826401212583777/+/u/run_flutter_gallery__transition_perf_e2e_ios/stdout). Tested locally on iOS real device and confirmed it passes. It seems that the next operation of the test was performed before the page animation was finished because the PR makes the page transition duration longer. ## 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. - [x] 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 --- .../test_driver/run_demos.dart | 4 +- packages/flutter/lib/src/cupertino/route.dart | 8 +++- .../src/material/page_transitions_theme.dart | 3 ++ .../material/page_transitions_theme_test.dart | 42 +++++++++++++++++++ .../flutter/test/widgets/heroes_test.dart | 4 +- 5 files changed, 55 insertions(+), 6 deletions(-) diff --git a/dev/integration_tests/flutter_gallery/test_driver/run_demos.dart b/dev/integration_tests/flutter_gallery/test_driver/run_demos.dart index 475e0eeb80..ca39b31ff1 100644 --- a/dev/integration_tests/flutter_gallery/test_driver/run_demos.dart +++ b/dev/integration_tests/flutter_gallery/test_driver/run_demos.dart @@ -73,8 +73,8 @@ Future runDemos(List demos, WidgetController controller) async { if (kUnsynchronizedDemos.contains(demo)) { // These tests have animation, pumpAndSettle cannot be used. - // This time is questionable. 400ms is the tested reasonable result. - await controller.pump(const Duration(milliseconds: 400)); + // This time is questionable. 600ms is the tested reasonable result. + await controller.pump(const Duration(milliseconds: 600)); await controller.pump(); await pageBack(); } else { diff --git a/packages/flutter/lib/src/cupertino/route.dart b/packages/flutter/lib/src/cupertino/route.dart index f0c37a036e..0206f7604c 100644 --- a/packages/flutter/lib/src/cupertino/route.dart +++ b/packages/flutter/lib/src/cupertino/route.dart @@ -148,9 +148,13 @@ mixin CupertinoRouteTransitionMixin on PageRoute { super.didChangePrevious(previousRoute); } + /// The duration of the page transition. + /// + /// A relatively rigorous eyeball estimation. + static const Duration kTransitionDuration = Duration(milliseconds: 500); + @override - // A relatively rigorous eyeball estimation. - Duration get transitionDuration => const Duration(milliseconds: 500); + Duration get transitionDuration => kTransitionDuration; @override Color? get barrierColor => fullscreenDialog ? null : _kCupertinoPageTransitionBarrierColor; diff --git a/packages/flutter/lib/src/material/page_transitions_theme.dart b/packages/flutter/lib/src/material/page_transitions_theme.dart index d8d4fc9570..b6e5b71679 100644 --- a/packages/flutter/lib/src/material/page_transitions_theme.dart +++ b/packages/flutter/lib/src/material/page_transitions_theme.dart @@ -1019,6 +1019,9 @@ class CupertinoPageTransitionsBuilder extends PageTransitionsBuilder { /// Constructs a page transition animation that matches the iOS transition. const CupertinoPageTransitionsBuilder(); + @override + Duration get transitionDuration => CupertinoRouteTransitionMixin.kTransitionDuration; + @override DelegatedTransitionBuilder? get delegatedTransition => CupertinoPageTransition.delegatedTransition; diff --git a/packages/flutter/test/material/page_transitions_theme_test.dart b/packages/flutter/test/material/page_transitions_theme_test.dart index 9dfff0ac47..1a5cebb28c 100644 --- a/packages/flutter/test/material/page_transitions_theme_test.dart +++ b/packages/flutter/test/material/page_transitions_theme_test.dart @@ -286,6 +286,48 @@ void main() { variant: TargetPlatformVariant.only(TargetPlatform.android), ); + testWidgets( + 'CupertinoPageTransitionsBuilder default duration is 500ms', + (WidgetTester tester) async { + final Map routes = { + '/': + (BuildContext context) => Material( + child: TextButton( + child: const Text('push'), + onPressed: () { + Navigator.of(context).pushNamed('/b'); + }, + ), + ), + '/b': (BuildContext context) => const Text('page b'), + }; + + await tester.pumpWidget( + MaterialApp( + theme: ThemeData( + pageTransitionsTheme: const PageTransitionsTheme( + builders: { + TargetPlatform.iOS: CupertinoPageTransitionsBuilder(), + }, + ), + ), + routes: routes, + ), + ); + + expect(find.byType(CupertinoPageTransition), findsOneWidget); + + await tester.tap(find.text('push')); + await tester.pump(); + await tester.pump(const Duration(milliseconds: 499)); + expect(tester.hasRunningAnimations, isTrue); + + await tester.pump(const Duration(milliseconds: 10)); + expect(tester.hasRunningAnimations, isFalse); + }, + variant: TargetPlatformVariant.only(TargetPlatform.iOS), + ); + testWidgets( 'Animation duration changes accordingly when page transition builder changes', (WidgetTester tester) async { diff --git a/packages/flutter/test/widgets/heroes_test.dart b/packages/flutter/test/widgets/heroes_test.dart index baa46161ec..e0d9d37b79 100644 --- a/packages/flutter/test/widgets/heroes_test.dart +++ b/packages/flutter/test/widgets/heroes_test.dart @@ -1975,7 +1975,7 @@ Future main() async { await tester.tap(find.text('two')); await tester.pump(); - await tester.pump(const Duration(milliseconds: 500)); + await tester.pump(const Duration(milliseconds: 501)); expect(find.byKey(firstKey), findsNothing); expect(find.byKey(secondKey), isOnstage); @@ -2016,7 +2016,7 @@ Future main() async { await tester.tap(find.text('two')); await tester.pump(); - await tester.pump(const Duration(milliseconds: 500)); + await tester.pump(const Duration(milliseconds: 501)); expect(find.byKey(firstKey), findsNothing); expect(find.byKey(secondKey), isOnstage);