Unverified Commit 7cef9661 authored by Taha Tesser's avatar Taha Tesser Committed by GitHub

Fix `NavigationDrawer` selected item has wrong icon color (#129625)

fixes [NavigationDrawer selected item has wrong icon color [Material3 spec]](https://github.com/flutter/flutter/issues/129572)

### Description
This PR fixes a mistake in the `NavigationDrawer` defaults, where generated token value returns a `null`. 
This issue can be detected when you want to customize the selected icon color for `NavigationDrawerDestination` using a custom color scheme.

### Code sample

<details> 
<summary>expanded to view the code sample</summary> 

```dart
import 'package:flutter/material.dart';

void main() => runApp(const MyApp());

class MyApp extends StatelessWidget {
  const MyApp({super.key});

  @override
  Widget build(BuildContext context) {
    return MaterialApp(
      debugShowCheckedModeBanner: false,
      themeMode: ThemeMode.light,
      theme: ThemeData(
        colorScheme: ColorScheme.fromSeed(seedColor: Colors.blue).copyWith(
          onSecondaryContainer: Colors.red,
        ),
        useMaterial3: true,
      ),
      home: const Example(),
    );
  }
}

class Example extends StatelessWidget {
  const Example({super.key});

  @override
  Widget build(BuildContext context) {
    return Scaffold(
      appBar: AppBar(
        title: const Text('NavigationDrawer Sample'),
      ),
      drawer: const NavigationDrawer(
        children: <Widget>[
          NavigationDrawerDestination(
            icon: Icon(Icons.favorite_outline_rounded),
            label: Text('Favorite'),
            selectedIcon: Icon(Icons.favorite_rounded),
          ),
          NavigationDrawerDestination(
            icon: Icon(Icons.favorite_outline_rounded),
            label: Text('Favorite'),
          ),
        ],
      ),
    );
  }
}
``` 
	
</details>

### Before
 
<img width="1053" alt="Screenshot 2023-06-27 at 13 24 38" src="https://github.com/flutter/flutter/assets/48603081/18c13a73-688f-4586-bb60-bddef45d173f">

### After

<img width="1053" alt="Screenshot 2023-06-27 at 13 24 25" src="https://github.com/flutter/flutter/assets/48603081/8a1427c6-517f-424a-b0bd-24bad7c5fbb0">
parent 3a6d6615
......@@ -423,6 +423,7 @@ md.comp.navigation-drawer.active-indicator.color,
md.comp.navigation-drawer.active-indicator.height,
md.comp.navigation-drawer.active-indicator.shape,
md.comp.navigation-drawer.active-indicator.width,
md.comp.navigation-drawer.active.icon.color,
md.comp.navigation-drawer.active.label-text.color,
md.comp.navigation-drawer.container.color,
md.comp.navigation-drawer.container.surface-tint-layer.color,
......
......@@ -43,7 +43,7 @@ class _${blockName}DefaultsM3 extends NavigationDrawerThemeData {
return IconThemeData(
size: ${getToken("md.comp.navigation-drawer.icon.size")},
color: states.contains(MaterialState.selected)
? ${componentColor("md.comp.navigation-drawer.active.icon.")}
? ${componentColor("md.comp.navigation-drawer.active.icon")}
: ${componentColor("md.comp.navigation-drawer.inactive.icon")},
);
});
......
......@@ -218,7 +218,7 @@ class NavigationDrawerDestination extends StatelessWidget {
///
/// The icon will use [NavigationDrawerThemeData.iconTheme] with
/// [MaterialState.selected]. If this is null, the default [IconThemeData]
/// would use a size of 24.0 and [ColorScheme.onSurfaceVariant].
/// would use a size of 24.0 and [ColorScheme.onSecondaryContainer].
final Widget? selectedIcon;
/// The text label that appears on the right of the icon
......@@ -702,7 +702,7 @@ class _NavigationDrawerDefaultsM3 extends NavigationDrawerThemeData {
return IconThemeData(
size: 24.0,
color: states.contains(MaterialState.selected)
? null
? _colors.onSecondaryContainer
: _colors.onSurfaceVariant,
);
});
......
......@@ -115,7 +115,7 @@ void main() {
});
testWidgets(
'M3 NavigationDrawer uses proper defaults when no parameters are given',
'NavigationDrawer uses proper defaults when no parameters are given',
(WidgetTester tester) async {
final GlobalKey<ScaffoldState> scaffoldKey = GlobalKey<ScaffoldState>();
final ThemeData theme = ThemeData(useMaterial3: true);
......@@ -125,13 +125,13 @@ void main() {
NavigationDrawer(
children: <Widget>[
Text('Headline', style: theme.textTheme.bodyLarge),
NavigationDrawerDestination(
icon: Icon(Icons.ac_unit, color: theme.iconTheme.color),
label: Text('AC', style: theme.textTheme.bodySmall),
const NavigationDrawerDestination(
icon: Icon(Icons.ac_unit),
label: Text('AC'),
),
NavigationDrawerDestination(
icon: Icon(Icons.access_alarm, color: theme.iconTheme.color),
label: Text('Alarm', style: theme.textTheme.bodySmall),
const NavigationDrawerDestination(
icon: Icon(Icons.access_alarm),
label: Text('Alarm'),
),
],
onDestinationSelected: (int i) {},
......@@ -142,11 +142,25 @@ void main() {
scaffoldKey.currentState!.openDrawer();
await tester.pump(const Duration(seconds: 1));
// Test drawer Material.
expect(_getMaterial(tester).color, theme.colorScheme.surface);
expect(_getMaterial(tester).surfaceTintColor, theme.colorScheme.surfaceTint);
expect(_getMaterial(tester).shadowColor, Colors.transparent);
expect(_getMaterial(tester).elevation, 1);
// Test indicator decoration.
expect(_getIndicatorDecoration(tester)?.color, theme.colorScheme.secondaryContainer);
expect(_getIndicatorDecoration(tester)?.shape, const StadiumBorder());
// Test selected and unselected icon colors.
expect(_iconStyle(tester, Icons.ac_unit)?.color, theme.colorScheme.onSecondaryContainer);
expect(_iconStyle(tester, Icons.access_alarm)?.color, theme.colorScheme.onSurfaceVariant);
// Test selected and unselected label colors.
expect(_labelStyle(tester, 'AC')?.color, theme.colorScheme.onSecondaryContainer);
expect(_labelStyle(tester, 'Alarm')?.color, theme.colorScheme.onSurfaceVariant);
// Test that the icon and label are the correct size.
RenderBox iconBox = tester.renderObject(find.byIcon(Icons.ac_unit));
expect(iconBox.size, const Size(24.0, 24.0));
iconBox = tester.renderObject(find.byIcon(Icons.access_alarm));
expect(iconBox.size, const Size(24.0, 24.0));
});
testWidgets('Navigation drawer is scrollable', (WidgetTester tester) async {
......@@ -418,6 +432,20 @@ ShapeDecoration? _getIndicatorDecoration(WidgetTester tester) {
.decoration as ShapeDecoration?;
}
TextStyle? _iconStyle(WidgetTester tester, IconData icon) {
final RichText iconRichText = tester.widget<RichText>(
find.descendant(of: find.byIcon(icon), matching: find.byType(RichText)),
);
return iconRichText.text.style;
}
TextStyle? _labelStyle(WidgetTester tester, String label) {
final RichText labelRichText = tester.widget<RichText>(
find.descendant(of: find.text(label), matching: find.byType(RichText)),
);
return labelRichText.text.style;
}
void widgetSetup(WidgetTester tester, double viewWidth, {double viewHeight = 1000}) {
tester.view.devicePixelRatio = 2;
final double dpi = tester.view.devicePixelRatio;
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment