From 9bc7df0177ce552f9e6baf941b34e0806e074b25 Mon Sep 17 00:00:00 2001 From: flutteractionsbot <154381524+flutteractionsbot@users.noreply.github.com> Date: Wed, 19 Feb 2025 20:49:14 -0800 Subject: [PATCH] [CP-stable]Fix buttons with icons ignore provided `foregroundColor` (#163201) 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? PR: https://github.com/flutter/flutter/pull/162880 Issue: https://github.com/flutter/flutter/issues/162839 ### 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 Fixes buttons with icons ignore `foregroundColor`. ### 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) It impacts how buttons with icons render color when using `foregroundColor`. ### Workaround: Is there a workaround for this issue? Provide additional property such as `iconColor` along with `foregroundColor` to match colors for button label and icon. ### Risk: What is the risk level of this cherry-pick? ### Test Coverage: Are you confident that your fix is well-tested by automated tests? ### Validation Steps: What are the steps to validate that this fix works? Testing and samples. --- .../lib/src/material/button_style_button.dart | 17 +++++-- .../lib/src/material/elevated_button.dart | 3 ++ .../lib/src/material/filled_button.dart | 3 ++ .../lib/src/material/outlined_button.dart | 3 ++ .../flutter/lib/src/material/text_button.dart | 3 ++ .../test/material/elevated_button_test.dart | 23 ++++++++++ .../material/elevated_button_theme_test.dart | 40 +++++++++++++++- .../test/material/filled_button_test.dart | 34 ++++++++++++++ .../material/filled_button_theme_test.dart | 46 +++++++++++++++++++ .../test/material/outlined_button_test.dart | 23 ++++++++++ .../material/outlined_button_theme_test.dart | 36 +++++++++++++++ .../test/material/text_button_test.dart | 23 ++++++++++ .../test/material/text_button_theme_test.dart | 36 +++++++++++++++ 13 files changed, 283 insertions(+), 7 deletions(-) diff --git a/packages/flutter/lib/src/material/button_style_button.dart b/packages/flutter/lib/src/material/button_style_button.dart index 1885214bfee..23c2ac57ff6 100644 --- a/packages/flutter/lib/src/material/button_style_button.dart +++ b/packages/flutter/lib/src/material/button_style_button.dart @@ -389,6 +389,16 @@ class _ButtonStyleState extends State with TickerProviderStat }); } + Color? effectiveIconColor() { + return widgetStyle?.iconColor?.resolve(statesController.value) ?? + themeStyle?.iconColor?.resolve(statesController.value) ?? + widgetStyle?.foregroundColor?.resolve(statesController.value) ?? + themeStyle?.foregroundColor?.resolve(statesController.value) ?? + defaultStyle.iconColor?.resolve(statesController.value) ?? + // Fallback to foregroundColor if iconColor is null. + defaultStyle.foregroundColor?.resolve(statesController.value); + } + final double? resolvedElevation = resolve((ButtonStyle? style) => style?.elevation); final TextStyle? resolvedTextStyle = resolve( (ButtonStyle? style) => style?.textStyle, @@ -409,7 +419,7 @@ class _ButtonStyleState extends State with TickerProviderStat final Size? resolvedMinimumSize = resolve((ButtonStyle? style) => style?.minimumSize); final Size? resolvedFixedSize = resolve((ButtonStyle? style) => style?.fixedSize); final Size? resolvedMaximumSize = resolve((ButtonStyle? style) => style?.maximumSize); - final Color? resolvedIconColor = resolve((ButtonStyle? style) => style?.iconColor); + final Color? resolvedIconColor = effectiveIconColor(); final double? resolvedIconSize = resolve((ButtonStyle? style) => style?.iconSize); final BorderSide? resolvedSide = resolve((ButtonStyle? style) => style?.side); final OutlinedBorder? resolvedShape = resolve( @@ -551,10 +561,7 @@ class _ButtonStyleState extends State with TickerProviderStat customBorder: resolvedShape!.copyWith(side: resolvedSide), statesController: statesController, child: IconTheme.merge( - data: IconThemeData( - color: resolvedIconColor ?? resolvedForegroundColor, - size: resolvedIconSize, - ), + data: IconThemeData(color: resolvedIconColor, size: resolvedIconSize), child: result, ), ); diff --git a/packages/flutter/lib/src/material/elevated_button.dart b/packages/flutter/lib/src/material/elevated_button.dart index d8aef58e145..73b572c11c1 100644 --- a/packages/flutter/lib/src/material/elevated_button.dart +++ b/packages/flutter/lib/src/material/elevated_button.dart @@ -161,6 +161,9 @@ class ElevatedButton extends ButtonStyleButton { /// [ButtonStyle.iconColor] and [iconSize] is used to construct /// [ButtonStyle.iconSize]. /// + /// If [iconColor] is null, the button icon will use [foregroundColor]. If [foregroundColor] is also + /// null, the button icon will use the default icon color. + /// /// The button's elevations are defined relative to the [elevation] /// parameter. The disabled elevation is the same as the parameter /// value, [elevation] + 2 is used when the button is hovered diff --git a/packages/flutter/lib/src/material/filled_button.dart b/packages/flutter/lib/src/material/filled_button.dart index be27cf852c6..6c776c551b6 100644 --- a/packages/flutter/lib/src/material/filled_button.dart +++ b/packages/flutter/lib/src/material/filled_button.dart @@ -232,6 +232,9 @@ class FilledButton extends ButtonStyleButton { /// [ButtonStyle.iconColor] and [iconSize] is used to construct /// [ButtonStyle.iconSize]. /// + /// If [iconColor] is null, the button icon will use [foregroundColor]. If [foregroundColor] is also + /// null, the button icon will use the default icon color. + /// /// The button's elevations are defined relative to the [elevation] /// parameter. The disabled elevation is the same as the parameter /// value, [elevation] + 2 is used when the button is hovered diff --git a/packages/flutter/lib/src/material/outlined_button.dart b/packages/flutter/lib/src/material/outlined_button.dart index 2647394201b..82191c60b73 100644 --- a/packages/flutter/lib/src/material/outlined_button.dart +++ b/packages/flutter/lib/src/material/outlined_button.dart @@ -160,6 +160,9 @@ class OutlinedButton extends ButtonStyleButton { /// [ButtonStyle.iconColor] and [iconSize] is used to construct /// [ButtonStyle.iconSize]. /// + /// If [iconColor] is null, the button icon will use [foregroundColor]. If [foregroundColor] is also + /// null, the button icon will use the default icon color. + /// /// If [overlayColor] is specified and its value is [Colors.transparent] /// then the pressed/focused/hovered highlights are effectively defeated. /// Otherwise a [WidgetStateProperty] with the same opacities as the diff --git a/packages/flutter/lib/src/material/text_button.dart b/packages/flutter/lib/src/material/text_button.dart index f863f917263..d665ec7170d 100644 --- a/packages/flutter/lib/src/material/text_button.dart +++ b/packages/flutter/lib/src/material/text_button.dart @@ -169,6 +169,9 @@ class TextButton extends ButtonStyleButton { /// [ButtonStyle.iconColor] and [iconSize] is used to construct /// [ButtonStyle.iconSize]. /// + /// If [iconColor] is null, the button icon will use [foregroundColor]. If [foregroundColor] is also + /// null, the button icon will use the default icon color. + /// /// If [overlayColor] is specified and its value is [Colors.transparent] /// then the pressed/focused/hovered highlights are effectively defeated. /// Otherwise a [WidgetStateProperty] with the same opacities as the diff --git a/packages/flutter/test/material/elevated_button_test.dart b/packages/flutter/test/material/elevated_button_test.dart index 018ce9ecc1a..282ec938637 100644 --- a/packages/flutter/test/material/elevated_button_test.dart +++ b/packages/flutter/test/material/elevated_button_test.dart @@ -2510,4 +2510,27 @@ void main() { final Offset iconTopRight = tester.getTopRight(find.byIcon(Icons.add)); expect(buttonTopRight.dx, iconTopRight.dx + 24.0); }); + + // Regression test for https://github.com/flutter/flutter/issues/162839. + testWidgets('ElevatedButton icon uses provided foregroundColor over default icon color', ( + WidgetTester tester, + ) async { + const Color foregroundColor = Color(0xFFFF1234); + + await tester.pumpWidget( + MaterialApp( + home: Material( + child: Center( + child: ElevatedButton.icon( + style: ElevatedButton.styleFrom(foregroundColor: foregroundColor), + onPressed: () {}, + icon: const Icon(Icons.add), + label: const Text('Button'), + ), + ), + ), + ), + ); + expect(iconStyle(tester, Icons.add).color, foregroundColor); + }); } diff --git a/packages/flutter/test/material/elevated_button_theme_test.dart b/packages/flutter/test/material/elevated_button_theme_test.dart index f618f706738..536aa0bee8e 100644 --- a/packages/flutter/test/material/elevated_button_theme_test.dart +++ b/packages/flutter/test/material/elevated_button_theme_test.dart @@ -6,6 +6,13 @@ import 'package:flutter/material.dart'; import 'package:flutter_test/flutter_test.dart'; void main() { + TextStyle iconStyle(WidgetTester tester, IconData icon) { + final RichText iconRichText = tester.widget( + find.descendant(of: find.byIcon(icon), matching: find.byType(RichText)), + ); + return iconRichText.text.style!; + } + test('ElevatedButtonThemeData lerp special cases', () { expect(ElevatedButtonThemeData.lerp(null, null, 0), null); const ElevatedButtonThemeData data = ElevatedButtonThemeData(); @@ -263,7 +270,7 @@ void main() { ); }); - testWidgets('Material3 - ElevatedButton repsects Theme shadowColor', (WidgetTester tester) async { + testWidgets('Material3 - ElevatedButton respects Theme shadowColor', (WidgetTester tester) async { const ColorScheme colorScheme = ColorScheme.light(); const Color shadowColor = Color(0xff000001); const Color overriddenColor = Color(0xff000002); @@ -334,7 +341,7 @@ void main() { expect(material.shadowColor, shadowColor); }); - testWidgets('Material2 - ElevatedButton repsects Theme shadowColor', (WidgetTester tester) async { + testWidgets('Material2 - ElevatedButton respects Theme shadowColor', (WidgetTester tester) async { const ColorScheme colorScheme = ColorScheme.light(); const Color shadowColor = Color(0xff000001); const Color overriddenColor = Color(0xff000002); @@ -442,4 +449,33 @@ void main() { expect(buttonTopRight.dx, iconTopRight.dx + 24.0); }); + + // Regression test for https://github.com/flutter/flutter/issues/162839. + testWidgets( + 'ElevatedButton icon uses provided ElevatedButtonTheme foregroundColor over default icon color', + (WidgetTester tester) async { + const Color foregroundColor = Color(0xFFFFA500); + + await tester.pumpWidget( + MaterialApp( + theme: ThemeData( + elevatedButtonTheme: ElevatedButtonThemeData( + style: ElevatedButton.styleFrom(foregroundColor: foregroundColor), + ), + ), + home: Material( + child: Center( + child: ElevatedButton.icon( + onPressed: () {}, + icon: const Icon(Icons.add), + label: const Text('Button'), + ), + ), + ), + ), + ); + + expect(iconStyle(tester, Icons.add).color, foregroundColor); + }, + ); } diff --git a/packages/flutter/test/material/filled_button_test.dart b/packages/flutter/test/material/filled_button_test.dart index 393c182ec17..8e36e80e099 100644 --- a/packages/flutter/test/material/filled_button_test.dart +++ b/packages/flutter/test/material/filled_button_test.dart @@ -2756,4 +2756,38 @@ void main() { final Offset iconTopRight = tester.getTopRight(find.byIcon(Icons.add)); expect(buttonTopRight.dx, iconTopRight.dx + 24.0); }); + + // Regression test for https://github.com/flutter/flutter/issues/162839. + testWidgets('FilledButton icon uses provided foregroundColor over default icon color', ( + WidgetTester tester, + ) async { + const Color foregroundColor = Color(0xFFFF1234); + + await tester.pumpWidget( + MaterialApp( + home: Material( + child: Center( + child: Column( + children: [ + FilledButton.icon( + style: FilledButton.styleFrom(foregroundColor: foregroundColor), + onPressed: () {}, + icon: const Icon(Icons.add), + label: const Text('Button'), + ), + FilledButton.tonalIcon( + style: FilledButton.styleFrom(foregroundColor: foregroundColor), + onPressed: () {}, + icon: const Icon(Icons.mail), + label: const Text('Button'), + ), + ], + ), + ), + ), + ), + ); + expect(iconStyle(tester, Icons.add).color, foregroundColor); + expect(iconStyle(tester, Icons.mail).color, foregroundColor); + }); } diff --git a/packages/flutter/test/material/filled_button_theme_test.dart b/packages/flutter/test/material/filled_button_theme_test.dart index a661cb9c2c9..a606fffbc5f 100644 --- a/packages/flutter/test/material/filled_button_theme_test.dart +++ b/packages/flutter/test/material/filled_button_theme_test.dart @@ -6,6 +6,13 @@ import 'package:flutter/material.dart'; import 'package:flutter_test/flutter_test.dart'; void main() { + TextStyle iconStyle(WidgetTester tester, IconData icon) { + final RichText iconRichText = tester.widget( + find.descendant(of: find.byIcon(icon), matching: find.byType(RichText)), + ); + return iconRichText.text.style!; + } + test('FilledButtonThemeData lerp special cases', () { expect(FilledButtonThemeData.lerp(null, null, 0), null); const FilledButtonThemeData data = FilledButtonThemeData(); @@ -360,4 +367,43 @@ void main() { expect(buttonTopRight.dx, iconTopRight.dx + 24.0); }); + + // Regression test for https://github.com/flutter/flutter/issues/162839. + testWidgets( + 'FilledButton icon uses provided FilledButtonTheme foregroundColor over default icon color', + (WidgetTester tester) async { + const Color foregroundColor = Color(0xFFFFA500); + + await tester.pumpWidget( + MaterialApp( + theme: ThemeData( + filledButtonTheme: FilledButtonThemeData( + style: FilledButton.styleFrom(foregroundColor: foregroundColor), + ), + ), + home: Material( + child: Center( + child: Column( + children: [ + FilledButton.icon( + onPressed: () {}, + icon: const Icon(Icons.add), + label: const Text('Button'), + ), + FilledButton.icon( + onPressed: () {}, + icon: const Icon(Icons.mail), + label: const Text('Button'), + ), + ], + ), + ), + ), + ), + ); + + expect(iconStyle(tester, Icons.add).color, foregroundColor); + expect(iconStyle(tester, Icons.mail).color, foregroundColor); + }, + ); } diff --git a/packages/flutter/test/material/outlined_button_test.dart b/packages/flutter/test/material/outlined_button_test.dart index 61846f87852..737926249fc 100644 --- a/packages/flutter/test/material/outlined_button_test.dart +++ b/packages/flutter/test/material/outlined_button_test.dart @@ -2854,4 +2854,27 @@ void main() { final Offset iconTopRight = tester.getTopRight(find.byIcon(Icons.add)); expect(buttonTopRight.dx, iconTopRight.dx + 24.0); }); + + // Regression test for https://github.com/flutter/flutter/issues/162839. + testWidgets('OutlinedButton icon uses provided foregroundColor over default icon color', ( + WidgetTester tester, + ) async { + const Color foregroundColor = Color(0xFFFF1234); + + await tester.pumpWidget( + MaterialApp( + home: Material( + child: Center( + child: OutlinedButton.icon( + style: OutlinedButton.styleFrom(foregroundColor: foregroundColor), + onPressed: () {}, + icon: const Icon(Icons.add), + label: const Text('Button'), + ), + ), + ), + ), + ); + expect(iconStyle(tester, Icons.add).color, foregroundColor); + }); } diff --git a/packages/flutter/test/material/outlined_button_theme_test.dart b/packages/flutter/test/material/outlined_button_theme_test.dart index e56b01aaed6..45242e6e392 100644 --- a/packages/flutter/test/material/outlined_button_theme_test.dart +++ b/packages/flutter/test/material/outlined_button_theme_test.dart @@ -6,6 +6,13 @@ import 'package:flutter/material.dart'; import 'package:flutter_test/flutter_test.dart'; void main() { + TextStyle iconStyle(WidgetTester tester, IconData icon) { + final RichText iconRichText = tester.widget( + find.descendant(of: find.byIcon(icon), matching: find.byType(RichText)), + ); + return iconRichText.text.style!; + } + test('OutlinedButtonThemeData lerp special cases', () { expect(OutlinedButtonThemeData.lerp(null, null, 0), null); const OutlinedButtonThemeData data = OutlinedButtonThemeData(); @@ -446,4 +453,33 @@ void main() { expect(buttonTopRight.dx, iconTopRight.dx + 24.0); }, ); + + // Regression test for https://github.com/flutter/flutter/issues/162839. + testWidgets( + 'OutlinedButton icon uses provided OutlinedButtonTheme foregroundColor over default icon color', + (WidgetTester tester) async { + const Color foregroundColor = Color(0xFFFFA500); + + await tester.pumpWidget( + MaterialApp( + theme: ThemeData( + outlinedButtonTheme: OutlinedButtonThemeData( + style: OutlinedButton.styleFrom(foregroundColor: foregroundColor), + ), + ), + home: Material( + child: Center( + child: OutlinedButton.icon( + onPressed: () {}, + icon: const Icon(Icons.add), + label: const Text('Button'), + ), + ), + ), + ), + ); + + expect(iconStyle(tester, Icons.add).color, foregroundColor); + }, + ); } diff --git a/packages/flutter/test/material/text_button_test.dart b/packages/flutter/test/material/text_button_test.dart index aa809651d76..5d5b38d505a 100644 --- a/packages/flutter/test/material/text_button_test.dart +++ b/packages/flutter/test/material/text_button_test.dart @@ -2547,4 +2547,27 @@ void main() { final Offset iconTopRight = tester.getTopRight(find.byIcon(Icons.add)); expect(buttonTopRight.dx, iconTopRight.dx + 16.0); }); + + // Regression test for https://github.com/flutter/flutter/issues/162839. + testWidgets('TextButton icon uses provided foregroundColor over default icon color', ( + WidgetTester tester, + ) async { + const Color foregroundColor = Color(0xFFFF1234); + + await tester.pumpWidget( + MaterialApp( + home: Material( + child: Center( + child: TextButton.icon( + style: TextButton.styleFrom(foregroundColor: foregroundColor), + onPressed: () {}, + icon: const Icon(Icons.add), + label: const Text('Button'), + ), + ), + ), + ), + ); + expect(iconStyle(tester, Icons.add).color, foregroundColor); + }); } diff --git a/packages/flutter/test/material/text_button_theme_test.dart b/packages/flutter/test/material/text_button_theme_test.dart index 04aa4e5c36f..171a45b36c4 100644 --- a/packages/flutter/test/material/text_button_theme_test.dart +++ b/packages/flutter/test/material/text_button_theme_test.dart @@ -6,6 +6,13 @@ import 'package:flutter/material.dart'; import 'package:flutter_test/flutter_test.dart'; void main() { + TextStyle iconStyle(WidgetTester tester, IconData icon) { + final RichText iconRichText = tester.widget( + find.descendant(of: find.byIcon(icon), matching: find.byType(RichText)), + ); + return iconRichText.text.style!; + } + test('TextButtonTheme lerp special cases', () { expect(TextButtonThemeData.lerp(null, null, 0), null); const TextButtonThemeData data = TextButtonThemeData(); @@ -447,4 +454,33 @@ void main() { expect(buttonTopRight.dx, iconTopRight.dx + 16.0); }); + + // Regression test for https://github.com/flutter/flutter/issues/162839. + testWidgets( + 'TextButton icon uses provided TextButtonThemeData foregroundColor over default icon color', + (WidgetTester tester) async { + const Color foregroundColor = Color(0xFFFFA500); + + await tester.pumpWidget( + MaterialApp( + theme: ThemeData( + textButtonTheme: TextButtonThemeData( + style: TextButton.styleFrom(foregroundColor: foregroundColor), + ), + ), + home: Material( + child: Center( + child: TextButton.icon( + onPressed: () {}, + icon: const Icon(Icons.add), + label: const Text('Button'), + ), + ), + ), + ), + ); + + expect(iconStyle(tester, Icons.add).color, foregroundColor); + }, + ); }