- Notifications
You must be signed in to change notification settings - Fork29.3k
Fix TextButton.icon breaks focus traversal and ink effect when toggling icon#176579
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Code Review
This pull request addresses an issue whereTextButton.icon loses focus when its icon is toggled. The implementation refactors theTextButton.icon factory into a named constructor. This change prevents the widget from being rebuilt, which preserves its state, including focus. The_TextButtonWithIcon subclass is removed, and its padding logic is integrated intoTextButton. A regression test is added to verify that focus is maintained when the icon is nullified. My review includes a suggestion to update a doc comment that has become inaccurate due to these changes.
Uh oh!
There was an error while loading.Please reload this page.
89921ce tod46cf10Compared46cf10 tod5f104bCompareThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
LGTM, looks like it is the same for ElevatedButton and FilledButton as well. I wish we have some way to share code without exposing additional API
@chunhtai Are there any visual diffs in the Google testing failures? Or are those failures related to tests looking up widgets by type? |
I saw a button that was previous disabled but becomes enabled, maybe some of the enabling or onpress callback is not wire up correctly? |
before |
This is their code, are we overriding the disable style somehow? final style= buttonStyle??TextButton.styleFrom( foregroundColor: theme.colors.primary, disabledForegroundColor: theme.colors.onSurfaceVariant, iconColor: theme.colors.primary, disabledIconColor: theme.colors.onSurfaceVariant, textStyle: buttonSize==ButtonSize.medium? theme.textStyles.labelLarge: theme.textStyles.bodyMedium, );returnTextButton( onPressed: onTap!=null? () {onTap!(); }:null, style: style, child:Text(label), ); |
Description
This PR changes
TextButton.iconto 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#175810 which fixed the exact same problem for
OutlinedButton.icon.Related Issue
FixesTextButton.icon breaks focus traversal and ink effect when toggling icon
Tests