From 46a4c05aceaa636a07c5b1d89175ce51169e7778 Mon Sep 17 00:00:00 2001 From: flutteractionsbot <154381524+flutteractionsbot@users.noreply.github.com> Date: Thu, 5 Jun 2025 08:39:08 -0700 Subject: [PATCH] [CP-stable]Revert "Fix NavigationBar indicator overlay color (#164484)" (#170052) This pull request is created by [automatic cherry pick workflow](https://github.com/flutter/flutter/blob/main/docs/releases/Flutter-Cherrypick-Process.md#automatically-creates-a-cherry-pick-request) Please fill in the form below, and a flutter domain expert will evaluate this cherry pick request. ### Issue Link: What is the link to the issue this cherry-pick is addressing? https://github.com/flutter/flutter/issues/169249 https://github.com/flutter/flutter/issues/169436 ### Changelog Description: Explain this cherry pick in one line that is accessible to most Flutter developers. See [best practices](https://github.com/flutter/flutter/blob/main/docs/releases/Hotfix-Documentation-Best-Practices.md) for examples NavigationBar active indicator animation gets stucked. ### Impact Description: What is the impact (ex. visual jank on Samsung phones, app crash, cannot ship an iOS app)? Does it impact development (ex. flutter doctor crashes when Android Studio is installed), or the shipping production app (the app crashes on launch) Visual glitch visible by users. In several scenarios, the active indicator for NavigationBar and NavigationDrawer is not properly painted (animation stucked half way or active color not reflecting the current state). ### Workaround: Is there a workaround for this issue? No ### Risk: What is the risk level of this cherry-pick? - [ x ] Low ### Test Coverage: Are you confident that your fix is well-tested by automated tests? - [ x ] Yes ### Validation Steps: What are the steps to validate that this fix works? Run the code sample from https://github.com/flutter/flutter/issues/169249. This fix is a revert to a PR which landed in the current stable. --- .../lib/src/material/navigation_bar.dart | 3 +- .../test/material/navigation_bar_test.dart | 4 +- .../material/navigation_bar_theme_test.dart | 4 +- .../test/material/navigation_drawer_test.dart | 10 ++-- .../navigation_drawer_theme_test.dart | 4 +- .../test/material/navigation_rail_test.dart | 58 +++++++++++++------ .../material/navigation_rail_theme_test.dart | 4 +- 7 files changed, 52 insertions(+), 35 deletions(-) diff --git a/packages/flutter/lib/src/material/navigation_bar.dart b/packages/flutter/lib/src/material/navigation_bar.dart index 8321778207..9465e7ef96 100644 --- a/packages/flutter/lib/src/material/navigation_bar.dart +++ b/packages/flutter/lib/src/material/navigation_bar.dart @@ -16,7 +16,6 @@ import 'package:flutter/widgets.dart'; import 'color_scheme.dart'; import 'colors.dart'; import 'elevation_overlay.dart'; -import 'ink_decoration.dart'; import 'ink_well.dart'; import 'material.dart'; import 'material_localizations.dart'; @@ -869,7 +868,7 @@ class NavigationIndicator extends StatelessWidget { builder: (BuildContext context, Animation fadeAnimation) { return FadeTransition( opacity: fadeAnimation, - child: Ink( + child: Container( width: width, height: height, decoration: ShapeDecoration( diff --git a/packages/flutter/test/material/navigation_bar_test.dart b/packages/flutter/test/material/navigation_bar_test.dart index f7d16d5f56..121c1ad8b0 100644 --- a/packages/flutter/test/material/navigation_bar_test.dart +++ b/packages/flutter/test/material/navigation_bar_test.dart @@ -1716,8 +1716,8 @@ Material _getMaterial(WidgetTester tester) { ShapeDecoration? _getIndicatorDecoration(WidgetTester tester) { return tester - .firstWidget( - find.descendant(of: find.byType(FadeTransition), matching: find.byType(Ink)), + .firstWidget( + find.descendant(of: find.byType(FadeTransition), matching: find.byType(Container)), ) .decoration as ShapeDecoration?; diff --git a/packages/flutter/test/material/navigation_bar_theme_test.dart b/packages/flutter/test/material/navigation_bar_theme_test.dart index e47a3ff752..9784a929aa 100644 --- a/packages/flutter/test/material/navigation_bar_theme_test.dart +++ b/packages/flutter/test/material/navigation_bar_theme_test.dart @@ -367,8 +367,8 @@ Material _barMaterial(WidgetTester tester) { ShapeDecoration? _indicator(WidgetTester tester) { return tester - .firstWidget( - find.descendant(of: find.byType(FadeTransition), matching: find.byType(Ink)), + .firstWidget( + find.descendant(of: find.byType(FadeTransition), matching: find.byType(Container)), ) .decoration as ShapeDecoration?; diff --git a/packages/flutter/test/material/navigation_drawer_test.dart b/packages/flutter/test/material/navigation_drawer_test.dart index fde25e676a..78b67436a5 100644 --- a/packages/flutter/test/material/navigation_drawer_test.dart +++ b/packages/flutter/test/material/navigation_drawer_test.dart @@ -123,16 +123,14 @@ void main() { await tester.tap(find.text('AC')); await tester.pump(); - // When no background color is set, only the non-visible indicator Ink is expected. - expect(findDestinationInk('AC'), findsOne); + expect(findDestinationInk('AC'), findsNothing); // Destination with a custom background color. await tester.tap(find.byIcon(Icons.access_alarm)); await tester.pump(); // A Material is added with the custom color. - expect(findDestinationInk('Alarm'), findsNWidgets(2)); - // The drawer destination Ink is the first one, the second is the indicator's one. + expect(findDestinationInk('Alarm'), findsOne); final BoxDecoration destinationDecoration = tester.firstWidget(findDestinationInk('Alarm')).decoration! as BoxDecoration; expect(destinationDecoration.color, color); @@ -511,8 +509,8 @@ InkWell? _getInkWell(WidgetTester tester) { ShapeDecoration? _getIndicatorDecoration(WidgetTester tester) { return tester - .firstWidget( - find.descendant(of: find.byType(FadeTransition), matching: find.byType(Ink)), + .firstWidget( + find.descendant(of: find.byType(FadeTransition), matching: find.byType(Container)), ) .decoration as ShapeDecoration?; diff --git a/packages/flutter/test/material/navigation_drawer_theme_test.dart b/packages/flutter/test/material/navigation_drawer_theme_test.dart index 4a30bc8cf3..9ad94a8e8f 100644 --- a/packages/flutter/test/material/navigation_drawer_theme_test.dart +++ b/packages/flutter/test/material/navigation_drawer_theme_test.dart @@ -285,8 +285,8 @@ Material _getMaterial(WidgetTester tester) { ShapeDecoration? _getIndicatorDecoration(WidgetTester tester) { return tester - .firstWidget( - find.descendant(of: find.byType(FadeTransition), matching: find.byType(Ink)), + .firstWidget( + find.descendant(of: find.byType(FadeTransition), matching: find.byType(Container)), ) .decoration as ShapeDecoration?; diff --git a/packages/flutter/test/material/navigation_rail_test.dart b/packages/flutter/test/material/navigation_rail_test.dart index 5e0e10631a..b60893aa3a 100644 --- a/packages/flutter/test/material/navigation_rail_test.dart +++ b/packages/flutter/test/material/navigation_rail_test.dart @@ -3076,7 +3076,6 @@ void main() { final RenderObject inkFeatures = tester.allRenderObjects.firstWhere( (RenderObject object) => object.runtimeType.toString() == '_RenderInkFeatures', ); - const Rect indicatorRect = Rect.fromLTRB(12.0, 0.0, 68.0, 32.0); const Rect includedRect = indicatorRect; final Rect excludedRect = includedRect.inflate(10); @@ -3102,7 +3101,7 @@ void main() { ) ..rect(rect: indicatorRect, color: const Color(0x0a6750a4)) ..rrect( - rrect: RRect.fromLTRBR(12.0, 0.0, 68.0, 32.0, const Radius.circular(16)), + rrect: RRect.fromLTRBR(12.0, 72.0, 68.0, 104.0, const Radius.circular(16)), color: const Color(0xffe8def8), ), ); @@ -3164,7 +3163,7 @@ void main() { ) ..rect(rect: indicatorRect, color: const Color(0x0a6750a4)) ..rrect( - rrect: RRect.fromLTRBR(12.0, 6.0, 68.0, 38.0, const Radius.circular(16)), + rrect: RRect.fromLTRBR(12.0, 58.0, 68.0, 90.0, const Radius.circular(16)), color: const Color(0xffe8def8), ), ); @@ -3230,7 +3229,7 @@ void main() { ) ..rect(rect: indicatorRect, color: const Color(0x0a6750a4)) ..rrect( - rrect: RRect.fromLTRBR(30.0, 24.0, 86.0, 56.0, const Radius.circular(16)), + rrect: RRect.fromLTRBR(30.0, 96.0, 86.0, 128.0, const Radius.circular(16)), color: const Color(0xffe8def8), ), ); @@ -3295,7 +3294,7 @@ void main() { ) ..rect(rect: indicatorRect, color: const Color(0x0a6750a4)) ..rrect( - rrect: RRect.fromLTRBR(0.0, 6.0, 50.0, 38.0, const Radius.circular(16)), + rrect: RRect.fromLTRBR(0.0, 58.0, 50.0, 90.0, const Radius.circular(16)), color: const Color(0xffe8def8), ), ); @@ -3362,7 +3361,7 @@ void main() { ) ..rect(rect: indicatorRect, color: const Color(0x0a6750a4)) ..rrect( - rrect: RRect.fromLTRBR(140.0, 24.0, 196.0, 56.0, const Radius.circular(16)), + rrect: RRect.fromLTRBR(140.0, 96.0, 196.0, 128.0, const Radius.circular(16)), color: const Color(0xffe8def8), ), ); @@ -3402,11 +3401,13 @@ void main() { ); // Default values from M3 specification. - const double railMinWidth = 80.0; const double indicatorHeight = 32.0; const double destinationWidth = 72.0; const double destinationHorizontalPadding = 8.0; const double indicatorWidth = destinationWidth - 2 * destinationHorizontalPadding; // 56.0 + const double verticalSpacer = 8.0; + const double verticalIconLabelSpacing = 4.0; + const double verticalDestinationSpacing = 12.0; // The navigation rail width is larger than default because of the first destination long label. final double railWidth = tester.getSize(find.byType(NavigationRail)).width; @@ -3417,7 +3418,13 @@ void main() { final Rect indicatorRect = Rect.fromLTRB(indicatorLeft, 0.0, indicatorRight, indicatorHeight); final Rect includedRect = indicatorRect; final Rect excludedRect = includedRect.inflate(10); - const double indicatorHorizontalPadding = (railMinWidth - indicatorWidth) / 2; // 12.0 + + // Compute the vertical position for the selected destination (the one with 'bookmark' icon). + const double labelHeight = 16; // fontSize is 12 and height is 1.3. + const double destinationHeight = + indicatorHeight + verticalIconLabelSpacing + labelHeight + verticalDestinationSpacing; + const double secondDestinationVerticalOffset = verticalSpacer + destinationHeight; + const double secondIndicatorVerticalOffset = secondDestinationVerticalOffset; expect( inkFeatures, @@ -3441,10 +3448,10 @@ void main() { ..rect(rect: indicatorRect, color: const Color(0x0a6750a4)) ..rrect( rrect: RRect.fromLTRBR( - indicatorHorizontalPadding, - 0.0, - indicatorHorizontalPadding + indicatorWidth, - indicatorHeight, + indicatorLeft, + secondIndicatorVerticalOffset, + indicatorRight, + secondIndicatorVerticalOffset + indicatorHeight, const Radius.circular(16), ), color: const Color(0xffe8def8), @@ -3497,6 +3504,9 @@ void main() { const double destinationWidth = 72.0; const double destinationHorizontalPadding = 8.0; const double indicatorWidth = destinationWidth - 2 * destinationHorizontalPadding; // 56.0 + const double verticalSpacer = 8.0; + const double verticalIconLabelSpacing = 4.0; + const double verticalDestinationSpacing = 12.0; // The navigation rail width is the default one because labels are short. final double railWidth = tester.getSize(find.byType(NavigationRail)).width; @@ -3516,8 +3526,13 @@ void main() { final Rect includedRect = indicatorRect; final Rect excludedRect = includedRect.inflate(10); - // Icon height is greater than indicator height so the indicator has a vertical offset. - const double secondIndicatorVerticalOffset = (iconSize - indicatorHeight) / 2; + // Compute the vertical position for the selected destination (the one with 'bookmark' icon). + const double labelHeight = 16; // fontSize is 12 and height is 1.3. + const double destinationHeight = + iconSize + verticalIconLabelSpacing + labelHeight + verticalDestinationSpacing; + const double secondDestinationVerticalOffset = verticalSpacer + destinationHeight; + const double indicatorOffset = (iconSize - indicatorHeight) / 2; + const double secondIndicatorVerticalOffset = secondDestinationVerticalOffset + indicatorOffset; expect( inkFeatures, @@ -3596,6 +3611,7 @@ void main() { const double destinationWidth = 72.0; const double destinationHorizontalPadding = 8.0; const double indicatorWidth = destinationWidth - 2 * destinationHorizontalPadding; // 56.0 + const double verticalSpacer = 8.0; const double verticalDestinationSpacingM3 = 12.0; // The navigation rail width is the default one because labels are short. @@ -3615,7 +3631,11 @@ void main() { final Rect excludedRect = includedRect.inflate(10); // Compute the vertical position for the selected destination (the one with 'bookmark' icon). - const double secondIndicatorVerticalOffset = verticalDestinationSpacingM3 / 2; + const double destinationHeight = indicatorHeight + verticalDestinationSpacingM3; + const double secondDestinationVerticalOffset = verticalSpacer + destinationHeight; + const double secondIndicatorVerticalOffset = + secondDestinationVerticalOffset + verticalDestinationSpacingM3 / 2; + const double secondDestinationHorizontalOffset = 800 - railMinExtendedWidth; // RTL. expect( inkFeatures, @@ -3641,9 +3661,9 @@ void main() { // Indicator for the selected destination (the one with 'bookmark' icon). ..rrect( rrect: RRect.fromLTRBR( - indicatorLeft, + secondDestinationHorizontalOffset + indicatorLeft, secondIndicatorVerticalOffset, - indicatorRight, + secondDestinationHorizontalOffset + indicatorRight, secondIndicatorVerticalOffset + indicatorHeight, const Radius.circular(16), ), @@ -6150,8 +6170,8 @@ Widget _buildWidget(Widget child, {bool useMaterial3 = true, bool isRTL = false} ShapeDecoration? _getIndicatorDecoration(WidgetTester tester) { return tester - .firstWidget( - find.descendant(of: find.byType(FadeTransition), matching: find.byType(Ink)), + .firstWidget( + find.descendant(of: find.byType(FadeTransition), matching: find.byType(Container)), ) .decoration as ShapeDecoration?; diff --git a/packages/flutter/test/material/navigation_rail_theme_test.dart b/packages/flutter/test/material/navigation_rail_theme_test.dart index 07236c9e63..49581b8ba9 100644 --- a/packages/flutter/test/material/navigation_rail_theme_test.dart +++ b/packages/flutter/test/material/navigation_rail_theme_test.dart @@ -327,8 +327,8 @@ Material _railMaterial(WidgetTester tester) { ShapeDecoration? _indicatorDecoration(WidgetTester tester) { return tester - .firstWidget( - find.descendant(of: find.byType(NavigationIndicator), matching: find.byType(Ink)), + .firstWidget( + find.descendant(of: find.byType(NavigationIndicator), matching: find.byType(Container)), ) .decoration as ShapeDecoration?;