From c059c9b7daa10d92546ae2fbdf3fbfa16f53c533 Mon Sep 17 00:00:00 2001 From: Bruno Leroux Date: Tue, 28 Oct 2025 23:07:10 +0100 Subject: [PATCH] Fix TextButton.icon breaks focus traversal and ink effect when toggling icon (#176579) ## Description This PR changes `TextButton.icon` to avoid building a different widget. When a different widget is created the whole subtree is recreated which leads to various issues (Focus and A11y issues for instance). The change is similar to https://github.com/flutter/flutter/pull/175810 which fixed the exact same problem for `OutlinedButton.icon`. ## Related Issue Fixes [TextButton.icon breaks focus traversal and ink effect when toggling icon](https://github.com/flutter/flutter/issues/173944) ## Tests - Adds 1 test --------- Co-authored-by: chunhtai <47866232+chunhtai@users.noreply.github.com> --- .../test/demo/material/buttons_demo_test.dart | 4 +- .../flutter/lib/src/material/text_button.dart | 141 ++++++------------ .../test/material/text_button_test.dart | 30 ++++ 3 files changed, 81 insertions(+), 94 deletions(-) diff --git a/dev/integration_tests/flutter_gallery/test/demo/material/buttons_demo_test.dart b/dev/integration_tests/flutter_gallery/test/demo/material/buttons_demo_test.dart index 5304e0e1a9f..8cb984d48e3 100644 --- a/dev/integration_tests/flutter_gallery/test/demo/material/buttons_demo_test.dart +++ b/dev/integration_tests/flutter_gallery/test/demo/material/buttons_demo_test.dart @@ -24,9 +24,9 @@ void main() { { await tester.tap(find.text('TEXT')); await tester.pumpAndSettle(); - expect(find.byType(TextButton).evaluate().length, 2); + expect(find.byType(TextButton).evaluate().length, 4); final Offset topLeft1 = tester.getTopLeft(find.byType(TextButton).first); - final Offset topLeft2 = tester.getTopLeft(find.byType(TextButton).last); + final Offset topLeft2 = tester.getTopLeft(find.byType(TextButton).at(1)); expect(topLeft1.dx, 247); expect(topLeft2.dx, 425); expect(topLeft1.dy, topLeft2.dy); diff --git a/packages/flutter/lib/src/material/text_button.dart b/packages/flutter/lib/src/material/text_button.dart index c3832e0ebf0..c2400249e89 100644 --- a/packages/flutter/lib/src/material/text_button.dart +++ b/packages/flutter/lib/src/material/text_button.dart @@ -93,7 +93,7 @@ class TextButton extends ButtonStyleButton { super.statesController, super.isSemanticButton, required Widget super.child, - }); + }) : _addPadding = false; /// Create a text button from a pair of widgets that serve as the button's /// [icon] and [label]. @@ -101,56 +101,38 @@ class TextButton extends ButtonStyleButton { /// The icon and label are arranged in a row and padded by 8 logical pixels /// at the ends, with an 8 pixel gap in between. /// - /// If [icon] is null, will create a [TextButton] instead. + /// If [icon] is null, this constructor will create a [TextButton] + /// that doesn't display an icon. /// /// {@macro flutter.material.ButtonStyleButton.iconAlignment} /// - factory TextButton.icon({ - Key? key, - required VoidCallback? onPressed, - VoidCallback? onLongPress, - ValueChanged? onHover, - ValueChanged? onFocusChange, - ButtonStyle? style, - FocusNode? focusNode, - bool? autofocus, - Clip? clipBehavior, - MaterialStatesController? statesController, + TextButton.icon({ + super.key, + required super.onPressed, + super.onLongPress, + super.onHover, + super.onFocusChange, + super.style, + super.focusNode, + super.autofocus = false, + super.clipBehavior = Clip.none, + super.statesController, Widget? icon, required Widget label, IconAlignment? iconAlignment, - }) { - if (icon == null) { - return TextButton( - key: key, - onPressed: onPressed, - onLongPress: onLongPress, - onHover: onHover, - onFocusChange: onFocusChange, - style: style, - focusNode: focusNode, - autofocus: autofocus ?? false, - clipBehavior: clipBehavior ?? Clip.none, - statesController: statesController, - child: label, - ); - } - return _TextButtonWithIcon( - key: key, - onPressed: onPressed, - onLongPress: onLongPress, - onHover: onHover, - onFocusChange: onFocusChange, - style: style, - focusNode: focusNode, - autofocus: autofocus ?? false, - clipBehavior: clipBehavior ?? Clip.none, - statesController: statesController, - icon: icon, - label: label, - iconAlignment: iconAlignment, - ); - } + }) : _addPadding = icon != null, + super( + child: icon != null + ? _TextButtonWithIconChild( + label: label, + icon: icon, + buttonStyle: style, + iconAlignment: iconAlignment, + ) + : label, + ); + + final bool _addPadding; /// A static convenience method that constructs a text button /// [ButtonStyle] given simple values. @@ -397,8 +379,7 @@ class TextButton extends ButtonStyleButton { ButtonStyle defaultStyleOf(BuildContext context) { final ThemeData theme = Theme.of(context); final ColorScheme colorScheme = theme.colorScheme; - - return Theme.of(context).useMaterial3 + final ButtonStyle buttonStyle = theme.useMaterial3 ? _TextButtonDefaultsM3(context) : styleFrom( foregroundColor: colorScheme.primary, @@ -421,6 +402,27 @@ class TextButton extends ButtonStyleButton { alignment: Alignment.center, splashFactory: InkRipple.splashFactory, ); + + // Only apply padding when TextButton has an Icon. + if (_addPadding) { + final double defaultFontSize = + buttonStyle.textStyle?.resolve(const {})?.fontSize ?? 14.0; + final double effectiveTextScale = + MediaQuery.textScalerOf(context).scale(defaultFontSize) / 14.0; + final EdgeInsetsGeometry scaledPadding = ButtonStyleButton.scaledPadding( + theme.useMaterial3 + ? const EdgeInsetsDirectional.fromSTEB(12, 8, 16, 8) + : const EdgeInsets.all(8), + const EdgeInsets.symmetric(horizontal: 4), + const EdgeInsets.symmetric(horizontal: 4), + effectiveTextScale, + ); + return buttonStyle.copyWith( + padding: MaterialStatePropertyAll(scaledPadding), + ); + } + + return buttonStyle; } /// Returns the [TextButtonThemeData.style] of the closest @@ -445,51 +447,6 @@ EdgeInsetsGeometry _scaledPadding(BuildContext context) { ); } -class _TextButtonWithIcon extends TextButton { - _TextButtonWithIcon({ - super.key, - required super.onPressed, - super.onLongPress, - super.onHover, - super.onFocusChange, - super.style, - super.focusNode, - bool? autofocus, - super.clipBehavior, - super.statesController, - required Widget icon, - required Widget label, - IconAlignment? iconAlignment, - }) : super( - autofocus: autofocus ?? false, - child: _TextButtonWithIconChild( - icon: icon, - label: label, - buttonStyle: style, - iconAlignment: iconAlignment, - ), - ); - - @override - ButtonStyle defaultStyleOf(BuildContext context) { - final bool useMaterial3 = Theme.of(context).useMaterial3; - final ButtonStyle buttonStyle = super.defaultStyleOf(context); - final double defaultFontSize = - buttonStyle.textStyle?.resolve(const {})?.fontSize ?? 14.0; - final double effectiveTextScale = - MediaQuery.textScalerOf(context).scale(defaultFontSize) / 14.0; - final EdgeInsetsGeometry scaledPadding = ButtonStyleButton.scaledPadding( - useMaterial3 ? const EdgeInsetsDirectional.fromSTEB(12, 8, 16, 8) : const EdgeInsets.all(8), - const EdgeInsets.symmetric(horizontal: 4), - const EdgeInsets.symmetric(horizontal: 4), - effectiveTextScale, - ); - return buttonStyle.copyWith( - padding: MaterialStatePropertyAll(scaledPadding), - ); - } -} - class _TextButtonWithIconChild extends StatelessWidget { const _TextButtonWithIconChild({ required this.label, diff --git a/packages/flutter/test/material/text_button_test.dart b/packages/flutter/test/material/text_button_test.dart index 2b47f7cbad4..e3ac50245ba 100644 --- a/packages/flutter/test/material/text_button_test.dart +++ b/packages/flutter/test/material/text_button_test.dart @@ -2650,4 +2650,34 @@ void main() { expect(textColor(tester, buttonText), hoveredColor); expect(iconStyle(tester, buttonIcon).color, hoveredColor); }); + + // Regression test for https://github.com/flutter/flutter/issues/173944. + testWidgets('TextButton.icon does not lose focus when icon is nullified', ( + WidgetTester tester, + ) async { + Widget buildTextButton({required Widget? icon}) { + return MaterialApp( + home: Center( + child: TextButton.icon(onPressed: () {}, icon: icon, label: const Text('button')), + ), + ); + } + + // Build once with an icon. + await tester.pumpWidget(buildTextButton(icon: const Icon(Icons.abc))); + + FocusNode getButtonFocusNode() { + return Focus.of(tester.element(find.text('button'))); + } + + getButtonFocusNode().requestFocus(); + await tester.pumpAndSettle(); + expect(getButtonFocusNode().hasFocus, true); + + // Rebuild without icon. + await tester.pumpWidget(buildTextButton(icon: null)); + + // The button should still be focused. + expect(getButtonFocusNode().hasFocus, true); + }); }