From 834566f05d3609d644e637b8e113a60ae9e247b4 Mon Sep 17 00:00:00 2001 From: dy0gu <103767860+dy0gu@users.noreply.github.com> Date: Thu, 5 Sep 2024 18:57:17 +0100 Subject: [PATCH] Fix ZoomPageTransitionsBuilder hardcoded fill color (#154057) Fixes: #115275 Fixes: #116127 Fixes: #126682 Continuing on: #139078 (Credits to @LowLevelSubmarine for his initial work!) When using `ZoomPageTransitionsBuilder`, which is the default for `ThemeData` with a `MaterialApp`, dark edges would show around the exiting page that was being zoomed out in the background. Other times, a scrim (what looked like a slightly transparent dark overlay over the page) would appear. After some experimenting it was concluded that, in the first case, this was because both pages don't fully fill the enclosing scaffold area during the transition and the color for filling the remaining space was set hard coded as `Colors.black`. The second case (scrim) happens when navigating from a page with an enclosing scaffold to a nested one, without a scaffold, unlike the first case that happens when both pages have a (different) enclosing scaffold, except this time it would be the hard coded color covering the page with a slight opacity reduction. ### Changes - Replaced the hard coded color for transition filling with the current `ThemeData.colorScheme.surface` - Added a RenderBox based test to verify the correct color is being used in the transition. ## Preview **Before, notice the dark outline flash when navigating to the first page and the scrim when navigating to the second:** https://github.com/user-attachments/assets/b4cc8658-1008-49f4-8553-abd5fcc72989 **After, using the theme relative color (in this case the default white) to replace the hard coded value:** https://github.com/user-attachments/assets/b70f42d2-6246-4964-99d1-34ff8051ab06 ## 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. - [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. [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 [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md --- AUTHORS | 1 + .../src/material/page_transitions_theme.dart | 11 +- .../material/page_transitions_theme_test.dart | 107 ++++++++++++++++++ 3 files changed, 118 insertions(+), 1 deletion(-) diff --git a/AUTHORS b/AUTHORS index 8b32b71381..987103964b 100644 --- a/AUTHORS +++ b/AUTHORS @@ -128,3 +128,4 @@ Cedric Vanden Bosch Flop Dimil Kalathiya Nate Wilson +dy0gu diff --git a/packages/flutter/lib/src/material/page_transitions_theme.dart b/packages/flutter/lib/src/material/page_transitions_theme.dart index d82295fe6d..e55d9644ed 100644 --- a/packages/flutter/lib/src/material/page_transitions_theme.dart +++ b/packages/flutter/lib/src/material/page_transitions_theme.dart @@ -270,6 +270,7 @@ class _ZoomPageTransition extends StatelessWidget { @override Widget build(BuildContext context) { + final Color backgroundColor = Theme.of(context).colorScheme.surface; return DualTransitionBuilder( animation: animation, forwardBuilder: ( @@ -280,6 +281,7 @@ class _ZoomPageTransition extends StatelessWidget { return _ZoomEnterTransition( animation: animation, allowSnapshotting: allowSnapshotting && allowEnterRouteSnapshotting, + backgroundColor: backgroundColor, child: child, ); }, @@ -306,6 +308,7 @@ class _ZoomPageTransition extends StatelessWidget { animation: animation, allowSnapshotting: allowSnapshotting && allowEnterRouteSnapshotting , reverse: true, + backgroundColor: backgroundColor, child: child, ); }, @@ -331,6 +334,7 @@ class _ZoomEnterTransition extends StatefulWidget { required this.animation, this.reverse = false, required this.allowSnapshotting, + required this.backgroundColor, this.child, }); @@ -338,6 +342,7 @@ class _ZoomEnterTransition extends StatefulWidget { final Widget? child; final bool allowSnapshotting; final bool reverse; + final Color backgroundColor; @override State<_ZoomEnterTransition> createState() => _ZoomEnterTransitionState(); @@ -394,6 +399,7 @@ class _ZoomEnterTransitionState extends State<_ZoomEnterTransition> with _ZoomTr fade: fadeTransition, scale: scaleTransition, animation: widget.animation, + backgroundColor: widget.backgroundColor, ); super.initState(); } @@ -410,6 +416,7 @@ class _ZoomEnterTransitionState extends State<_ZoomEnterTransition> with _ZoomTr fade: fadeTransition, scale: scaleTransition, animation: widget.animation, + backgroundColor: widget.backgroundColor, ); } super.didUpdateWidget(oldWidget); @@ -986,6 +993,7 @@ class _ZoomEnterTransitionPainter extends SnapshotPainter { required this.scale, required this.fade, required this.animation, + required this.backgroundColor, }) { animation.addListener(notifyListeners); animation.addStatusListener(_onStatusChange); @@ -1001,6 +1009,7 @@ class _ZoomEnterTransitionPainter extends SnapshotPainter { final Animation animation; final Animation scale; final Animation fade; + final Color backgroundColor; final Matrix4 _transform = Matrix4.zero(); final LayerHandle _opacityHandle = LayerHandle(); @@ -1025,7 +1034,7 @@ class _ZoomEnterTransitionPainter extends SnapshotPainter { if (scrimOpacity > 0.0) { context.canvas.drawRect( offset & size, - Paint()..color = Colors.black.withOpacity(scrimOpacity), + Paint()..color = backgroundColor.withOpacity(scrimOpacity), ); } } diff --git a/packages/flutter/test/material/page_transitions_theme_test.dart b/packages/flutter/test/material/page_transitions_theme_test.dart index 275c310774..042f6843ad 100644 --- a/packages/flutter/test/material/page_transitions_theme_test.dart +++ b/packages/flutter/test/material/page_transitions_theme_test.dart @@ -519,4 +519,111 @@ void main() { expect(find.text('push'), findsOneWidget); expect(find.text('page b'), findsNothing); }, variant: TargetPlatformVariant.all()); + + testWidgets('ZoomPageTransitionsBuilder uses theme color during transition effects', (WidgetTester tester) async { + // Color that is being tested for presence. + const Color themeTestSurfaceColor = Color.fromARGB(255, 195, 255, 0); + + final Map routes = { + '/': (BuildContext context) => Material( + child: Scaffold( + appBar: AppBar( + title: const Text('Home Page'), + ), + body: Center( + child: Column( + mainAxisAlignment: MainAxisAlignment.center, + children: [ + ElevatedButton( + onPressed: () { + Navigator.pushNamed(context, '/scaffolded'); + }, + child: const Text('Route with scaffold!'), + ), + ElevatedButton( + onPressed: () { + Navigator.pushNamed(context, '/not-scaffolded'); + }, + child: const Text('Route with NO scaffold!'), + ), + ], + ), + ), + ), + ), + '/scaffolded': (BuildContext context) => Material( + child: Scaffold( + appBar: AppBar( + title: const Text('Scaffolded Page'), + ), + body: Center( + child: ElevatedButton( + onPressed: () { + Navigator.pop(context); + }, + child: const Text('Back to home route...'), + ), + ), + ), + ), + '/not-scaffolded': (BuildContext context) => Material( + child: Center( + child: ElevatedButton( + onPressed: () { + Navigator.pop(context); + }, + child: const Text('Back to home route...'), + ), + ), + ), + }; + + await tester.pumpWidget( + MaterialApp( + theme: ThemeData( + colorScheme: ColorScheme.fromSeed(seedColor: Colors.blue, surface: themeTestSurfaceColor), + pageTransitionsTheme: PageTransitionsTheme( + builders: { + // Force all platforms to use ZoomPageTransitionsBuilder to test each one. + for (final TargetPlatform platform in TargetPlatform.values) platform: const ZoomPageTransitionsBuilder(), + }, + ), + ), + routes: routes, + ), + ); + + // Go to scaffolded page. + await tester.tap(find.text('Route with scaffold!')); + + // Pump till animation is half-way through. + await tester.pump(); + await tester.pump(const Duration(milliseconds:75)); + + // Verify that the render box is painting the right color for scaffolded pages. + final RenderBox scaffoldedRenderBox = tester.firstRenderObject(find.byType(MaterialApp)); + // Expect the color to be at exactly 12.2% opacity at this time. + expect(scaffoldedRenderBox, paints..rect(color: themeTestSurfaceColor.withOpacity(0.122))); + + await tester.pumpAndSettle(); + + // Go back home and then go to non-scaffolded page. + await tester.tap(find.text('Back to home route...')); + await tester.pumpAndSettle(); + await tester.tap(find.text('Route with NO scaffold!')); + + // Pump till animation is half-way through. + await tester.pump(); + await tester.pump(const Duration(milliseconds:125)); + + // Verify that the render box is painting the right color for non-scaffolded pages. + final RenderBox nonScaffoldedRenderBox = tester.firstRenderObject(find.byType(MaterialApp)); + // Expect the color to be at exactly 59.6% opacity at this time. + expect(nonScaffoldedRenderBox, paints..rect(color: themeTestSurfaceColor.withOpacity(0.596))); + + await tester.pumpAndSettle(); + + // Verify that the transition successfully completed. + expect(find.text('Back to home route...'), findsOneWidget); + }, variant: TargetPlatformVariant.all()); }