Skip to content

Commit 9fa09ea

Browse files
authored
Fix NavigationRail hover misplaced when using large icons (flutter#134719)
## Description This PR fixes `NavigationRail` hover position when using enlarged icons whose size is specified using `NavigationRailThemeData.selectedIconTheme` and `NavigationRailThemeData.unselectedIconTheme`. ## Related Issue Fixes flutter#133799. ## Tests Adds 1 test, updates 1 test (to replace some magic numbers).
1 parent 63f25b0 commit 9fa09ea

File tree

2 files changed

+147
-7
lines changed

2 files changed

+147
-7
lines changed

packages/flutter/lib/src/material/navigation_rail.dart

Lines changed: 23 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -602,13 +602,19 @@ class _RailDestination extends StatelessWidget {
602602

603603
Widget content;
604604

605+
// The indicator height is fixed and equal to _kIndicatorHeight.
606+
// When the icon height is larger than the indicator height the indicator
607+
// vertical offset is used to vertically center the indicator.
608+
final bool isLargeIconSize = iconTheme.size != null && iconTheme.size! > _kIndicatorHeight;
609+
final double indicatorVerticalOffset = isLargeIconSize ? (iconTheme.size! - _kIndicatorHeight) / 2 : 0;
610+
605611
switch (labelType) {
606612
case NavigationRailLabelType.none:
607613
// Split the destination spacing across the top and bottom to keep the icon centered.
608614
final Widget? spacing = material3 ? const SizedBox(height: _verticalDestinationSpacingM3 / 2) : null;
609615
indicatorOffset = Offset(
610616
minWidth / 2 + destinationPadding.left,
611-
_verticalDestinationSpacingM3 / 2 + destinationPadding.top,
617+
_verticalDestinationSpacingM3 / 2 + destinationPadding.top + indicatorVerticalOffset,
612618
);
613619
final Widget iconPart = Column(
614620
children: <Widget>[
@@ -687,9 +693,15 @@ class _RailDestination extends StatelessWidget {
687693
final Widget bottomSpacing = SizedBox(height: material3 ? _verticalDestinationSpacingM3 : verticalPadding);
688694
final double indicatorHorizontalPadding = (destinationPadding.left / 2) - (destinationPadding.right / 2);
689695
final double indicatorVerticalPadding = destinationPadding.top;
690-
indicatorOffset = Offset(minWidth / 2 + indicatorHorizontalPadding, indicatorVerticalPadding);
696+
indicatorOffset = Offset(
697+
minWidth / 2 + indicatorHorizontalPadding,
698+
indicatorVerticalPadding + indicatorVerticalOffset,
699+
);
691700
if (minWidth < _NavigationRailDefaultsM2(context).minWidth!) {
692-
indicatorOffset = Offset(minWidth / 2 + _horizontalDestinationSpacingM3, indicatorVerticalPadding);
701+
indicatorOffset = Offset(
702+
minWidth / 2 + _horizontalDestinationSpacingM3,
703+
indicatorVerticalPadding + indicatorVerticalOffset,
704+
);
693705
}
694706
content = Container(
695707
constraints: BoxConstraints(
@@ -734,9 +746,15 @@ class _RailDestination extends StatelessWidget {
734746
final Widget bottomSpacing = SizedBox(height: material3 ? _verticalDestinationSpacingM3 : _verticalDestinationPaddingWithLabel);
735747
final double indicatorHorizontalPadding = (destinationPadding.left / 2) - (destinationPadding.right / 2);
736748
final double indicatorVerticalPadding = destinationPadding.top;
737-
indicatorOffset = Offset(minWidth / 2 + indicatorHorizontalPadding, indicatorVerticalPadding);
749+
indicatorOffset = Offset(
750+
minWidth / 2 + indicatorHorizontalPadding,
751+
indicatorVerticalPadding + indicatorVerticalOffset,
752+
);
738753
if (minWidth < _NavigationRailDefaultsM2(context).minWidth!) {
739-
indicatorOffset = Offset(minWidth / 2 + _horizontalDestinationSpacingM3, indicatorVerticalPadding);
754+
indicatorOffset = Offset(
755+
minWidth / 2 + _horizontalDestinationSpacingM3,
756+
indicatorVerticalPadding + indicatorVerticalOffset,
757+
);
740758
}
741759
content = Container(
742760
constraints: BoxConstraints(

packages/flutter/test/material/navigation_rail_test.dart

Lines changed: 124 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3117,6 +3117,9 @@ void main() {
31173117
const double destinationWidth = 72.0;
31183118
const double destinationHorizontalPadding = 8.0;
31193119
const double indicatorWidth = destinationWidth - 2 * destinationHorizontalPadding; // 56.0
3120+
const double verticalSpacer = 8.0;
3121+
const double verticalIconLabelSpacing = 4.0;
3122+
const double verticalDestinationSpacing = 12.0;
31203123

31213124
// The navigation rail width is larger than default because of the first destination long label.
31223125
final double railWidth = tester.getSize(find.byType(NavigationRail)).width;
@@ -3128,6 +3131,12 @@ void main() {
31283131
final Rect includedRect = indicatorRect;
31293132
final Rect excludedRect = includedRect.inflate(10);
31303133

3134+
// Compute the vertical position for the selected destination (the one with 'bookmark' icon).
3135+
const double labelHeight = 16; // fontSize is 12 and height is 1.3.
3136+
const double destinationHeight = indicatorHeight + verticalIconLabelSpacing + labelHeight + verticalDestinationSpacing;
3137+
const double secondDestinationVerticalOffset = verticalSpacer + destinationHeight;
3138+
const double secondIndicatorVerticalOffset = secondDestinationVerticalOffset;
3139+
31313140
expect(
31323141
inkFeatures,
31333142
paints
@@ -3152,7 +3161,116 @@ void main() {
31523161
color: const Color(0x0a6750a4),
31533162
)
31543163
..rrect(
3155-
rrect: RRect.fromLTRBR(indicatorLeft, 72.0, indicatorRight, 104.0, const Radius.circular(16)),
3164+
rrect: RRect.fromLTRBR(
3165+
indicatorLeft,
3166+
secondIndicatorVerticalOffset,
3167+
indicatorRight,
3168+
secondIndicatorVerticalOffset + indicatorHeight,
3169+
const Radius.circular(16),
3170+
),
3171+
color: const Color(0xffe8def8),
3172+
),
3173+
);
3174+
}, skip: kIsWeb && !isCanvasKit); // https://github.com/flutter/flutter/issues/99933
3175+
3176+
testWidgetsWithLeakTracking('NavigationRail indicator renders properly with large icon', (WidgetTester tester) async {
3177+
// This is a regression test for https://github.com/flutter/flutter/issues/133799.
3178+
const double iconSize = 50;
3179+
await _pumpNavigationRail(
3180+
tester,
3181+
navigationRailTheme: const NavigationRailThemeData(
3182+
selectedIconTheme: IconThemeData(size: iconSize),
3183+
unselectedIconTheme: IconThemeData(size: iconSize),
3184+
),
3185+
navigationRail: NavigationRail(
3186+
selectedIndex: 1,
3187+
destinations: const <NavigationRailDestination>[
3188+
NavigationRailDestination(
3189+
icon: Icon(Icons.favorite_border),
3190+
selectedIcon: Icon(Icons.favorite),
3191+
label: Text('ABC'),
3192+
),
3193+
NavigationRailDestination(
3194+
icon: Icon(Icons.bookmark_border),
3195+
selectedIcon: Icon(Icons.bookmark),
3196+
label: Text('DEF'),
3197+
),
3198+
],
3199+
labelType: NavigationRailLabelType.all,
3200+
),
3201+
);
3202+
3203+
// Hover the first destination.
3204+
final TestGesture gesture = await tester.createGesture(kind: PointerDeviceKind.mouse);
3205+
await gesture.addPointer();
3206+
await gesture.moveTo(tester.getCenter(find.byIcon(Icons.favorite_border)));
3207+
await tester.pumpAndSettle();
3208+
3209+
final RenderObject inkFeatures = tester.allRenderObjects.firstWhere((RenderObject object) => object.runtimeType.toString() == '_RenderInkFeatures');
3210+
3211+
// Default values from M3 specification.
3212+
const double railMinWidth = 80.0;
3213+
const double indicatorHeight = 32.0;
3214+
const double destinationWidth = 72.0;
3215+
const double destinationHorizontalPadding = 8.0;
3216+
const double indicatorWidth = destinationWidth - 2 * destinationHorizontalPadding; // 56.0
3217+
const double verticalSpacer = 8.0;
3218+
const double verticalIconLabelSpacing = 4.0;
3219+
const double verticalDestinationSpacing = 12.0;
3220+
3221+
// The navigation rail width is the default one because labels are short.
3222+
final double railWidth = tester.getSize(find.byType(NavigationRail)).width;
3223+
expect(railWidth, railMinWidth);
3224+
3225+
// Expected indicator position.
3226+
final double indicatorLeft = (railWidth - indicatorWidth) / 2;
3227+
final double indicatorRight = (railWidth + indicatorWidth) / 2;
3228+
const double indicatorTop = (iconSize - indicatorHeight) / 2;
3229+
const double indicatorBottom = (iconSize + indicatorHeight) / 2;
3230+
final Rect indicatorRect = Rect.fromLTRB(indicatorLeft, indicatorTop, indicatorRight, indicatorBottom);
3231+
final Rect includedRect = indicatorRect;
3232+
final Rect excludedRect = includedRect.inflate(10);
3233+
3234+
// Compute the vertical position for the selected destination (the one with 'bookmark' icon).
3235+
const double labelHeight = 16; // fontSize is 12 and height is 1.3.
3236+
const double destinationHeight = iconSize + verticalIconLabelSpacing + labelHeight + verticalDestinationSpacing;
3237+
const double secondDestinationVerticalOffset = verticalSpacer + destinationHeight;
3238+
const double indicatorOffset = (iconSize - indicatorHeight) / 2;
3239+
const double secondIndicatorVerticalOffset = secondDestinationVerticalOffset + indicatorOffset;
3240+
3241+
expect(
3242+
inkFeatures,
3243+
paints
3244+
..clipPath(
3245+
pathMatcher: isPathThat(
3246+
includes: <Offset>[
3247+
includedRect.centerLeft,
3248+
includedRect.topCenter,
3249+
includedRect.centerRight,
3250+
includedRect.bottomCenter,
3251+
],
3252+
excludes: <Offset>[
3253+
excludedRect.centerLeft,
3254+
excludedRect.topCenter,
3255+
excludedRect.centerRight,
3256+
excludedRect.bottomCenter,
3257+
],
3258+
),
3259+
)
3260+
// Hover highlight for the hovered destination (the one with 'favorite' icon).
3261+
..rect(
3262+
rect: indicatorRect,
3263+
color: const Color(0x0a6750a4),
3264+
)
3265+
// Indicator for the selected destination (the one with 'bookmark' icon).
3266+
..rrect(
3267+
rrect: RRect.fromLTRBR(
3268+
indicatorLeft,
3269+
secondIndicatorVerticalOffset,
3270+
indicatorRight,
3271+
secondIndicatorVerticalOffset + indicatorHeight,
3272+
const Radius.circular(16),
3273+
),
31563274
color: const Color(0xffe8def8),
31573275
),
31583276
);
@@ -5228,10 +5346,14 @@ Future<void> _pumpNavigationRail(
52285346
double textScaleFactor = 1.0,
52295347
required NavigationRail navigationRail,
52305348
bool useMaterial3 = true,
5349+
NavigationRailThemeData? navigationRailTheme,
52315350
}) async {
52325351
await tester.pumpWidget(
52335352
MaterialApp(
5234-
theme: ThemeData(useMaterial3: useMaterial3),
5353+
theme: ThemeData(
5354+
useMaterial3: useMaterial3,
5355+
navigationRailTheme: navigationRailTheme,
5356+
),
52355357
home: Builder(
52365358
builder: (BuildContext context) {
52375359
return MediaQuery(

0 commit comments

Comments
 (0)