mirror of
https://github.com/flutter/flutter.git
synced 2026-02-20 02:29:02 +08:00
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>
This commit is contained in:
parent
7f90a3be6f
commit
c059c9b7da
@ -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);
|
||||
|
||||
@ -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<bool>? onHover,
|
||||
ValueChanged<bool>? 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 <WidgetState>{})?.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<EdgeInsetsGeometry>(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 <WidgetState>{})?.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<EdgeInsetsGeometry>(scaledPadding),
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
class _TextButtonWithIconChild extends StatelessWidget {
|
||||
const _TextButtonWithIconChild({
|
||||
required this.label,
|
||||
|
||||
@ -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);
|
||||
});
|
||||
}
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user