From 0b3ba809cc109193cd3cbb0748741f167cb4303d Mon Sep 17 00:00:00 2001 From: Bruno Leroux Date: Mon, 2 Jun 2025 10:00:46 +0200 Subject: [PATCH] Revert "Fix NavigationBar indicator overlay color (#164484)" (#169497) ## Description This PR reverts the change from [Fix NavigationBar indicator overlay color](https://github.com/flutter/flutter/pull/164484) as it leads to several regressions. The change was very small: replacing a Container (which obscured the overlay) with an Ink. Unfortunately this leads to some rendering issues which seem related to the Ink painting not being fully in sync with the animation logic implemented by `NavigationIndicator`. I investigated this but did not find an obvious solution. So I would prefer to revert https://github.com/flutter/flutter/pull/164484 as the issue it fixed has less impact than the regression. cc @justinmc for review and also in case you have some clue about why an Ink would cause such problems compared to a Container. ## Related Issue Fixes [[Flutter 3.32.0] Active NavigationBar item not selected when switching items programmatically](https://github.com/flutter/flutter/issues/169249) Fixes [NavigationDrawer active indicator offset with SvgPicture](https://github.com/flutter/flutter/issues/169436) Fixes https://github.com/flutter/flutter/pull/164484#issuecomment-2910505630 Reopens https://github.com/flutter/flutter/issues/163871 --- .../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 83217782071..9465e7ef96f 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 f7d16d5f56f..121c1ad8b0c 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 e47a3ff752f..9784a929aa0 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 06f217f94a2..667be62ebfa 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); @@ -551,8 +549,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 4a30bc8cf3b..9ad94a8e8f3 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 5e0e10631a1..b60893aa3ac 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 07236c9e633..49581b8ba90 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?;