Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added IconButtonTheme and apply it to IconButton in M3 #108332

Merged
merged 6 commits into from
Aug 3, 2022

Conversation

QuncCccccc
Copy link
Contributor

@QuncCccccc QuncCccccc commented Jul 26, 2022

This PR adds IconButtonTheme for IconButton
fixes #107305
fixes #84910

Actual results (on master channel)

M2 Mode with fix M3 Mode with fix
Screen Shot 2022-07-26 at 9 46 05 AM Screen Shot 2022-07-26 at 9 46 28 AM

Pre-launch Checklist

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • I read the [Tree Hygiene] wiki page, which explains my responsibilities.
  • I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement].
  • I signed the [CLA].
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is [test-exempt].
  • All existing and new tests are passing.

@flutter-dashboard flutter-dashboard bot added a: text input Entering text in a text field or keyboard related problems f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels Jul 26, 2022
@QuncCccccc QuncCccccc force-pushed the add_icon_button_theme branch 2 times, most recently from d38b273 to d419c46 Compare July 26, 2022 03:56
@QuncCccccc QuncCccccc marked this pull request as ready for review July 26, 2022 16:01
packages/flutter/lib/src/material/icon_button.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/material/icon_button_theme.dart Outdated Show resolved Hide resolved
/// overall [Theme]'s [ThemeData.iconButtonTheme].
///
/// The [IconButton] will be affected by [IconButtonTheme] and [IconButtonThemeData]
/// only if [ThemeData.useMaterial3] is set to true; otherwise, [IconTheme] will be used.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we don't factor in the icon theme when useMaterial3 is true, then this is a significant breaking change. If the IconButtonTheme overrides IconTheme, then we might avoid a breaking change here, however detecting the difference between an intentional IconTheme override and the default IconTheme created by the Theme widget will be a little bit tricky.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can avoid making a breaking change, details are here: IconTheme and IconButton defaults. The gist of the proposal is this:

IconButton’s default size and the color of its Icon descendant are currently defined by the IconTheme. We’d like them to be defined by the new IconButtonTheme instead, if an IconButtonTheme is defined with non-null properties. Otherwise we’d like them to be defined by the defaults for Material 3 rather than the current IconTheme. At least not by the current IconTheme, if the current IconTheme is the default one automatically defined by Theme.

We could check for that case:

  IconThemeData overallIconTheme = IconTheme.of(context);
  IconThemeData? iconTheme = isThemeIconTheme(overallIconTheme)
    ? null 
    : overallIconTheme;

  bool isThemeIconTheme(IconThemeData theme) {
     // Return true if theme is the same as the default 
     // value of Theme.of(context).iconTheme
  }

In this case iconTheme would only be used to define the icon button’s default size and color if it was non-null, i.e. if it wasn’t the default one defined by the overall Theme. That means that if the app has inserted its own IconTheme into the widget tree, we’d use that instead of the M3 defaults. Just to clarify, here’s how we’d resolve each IconButton property, in order of precedence:

  1. Widget properties like IconButton.color
  2. IconButtonTheme properties
  3. IconTheme properties unless they were defined by Theme.of(context).iconTheme
  4. Material 3 default IconButtonTheme properties

As always, the first non-null value is the one that’s used. As noted in the code sample, the exception for IconTheme properties is for ones defined by the default value of ThemeData.iconTheme.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CC @rydmike - thought you might be interested in this issue since it's related to the one that came up in #107305 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @HansMuller, I like this approach. It will work without breaking past M2 behavior and work as one might expect when using M3 and the new IconButtonTheme as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for the suggestions. I just pushed some changes based on the comments so both IconTheme and IconButtonTheme should work in M3 now. Please let me know if there're any questions:)

@TahaTesser
Copy link
Member

@QuncCccccc
This should close #84910

@QuncCccccc QuncCccccc force-pushed the add_icon_button_theme branch 2 times, most recently from b1c1434 to d67aefd Compare August 2, 2022 20:00
packages/flutter/lib/src/material/constants.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/material/constants.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/material/icon_button.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/material/icon_button.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/material/icon_button.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/material/icon_button.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/material/icon_button.dart Outdated Show resolved Hide resolved
@QuncCccccc QuncCccccc requested review from HansMuller and removed request for rydmike August 3, 2022 19:28
Copy link
Contributor

@HansMuller HansMuller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@QuncCccccc QuncCccccc merged commit f7b0023 into flutter:master Aug 3, 2022
hannah-hyj added a commit that referenced this pull request Aug 4, 2022
* M3 counter error style

* polish

* Update text_field_template.dart

* Roll Flutter Engine from 3cba105 to cf0db3e (1 revision) (#108716)

* resolve comments

* Roll Plugins from 257eacb1e2aa to a6d42f1e01d3 (3 revisions) (#108738)

* Override PlaceholderDimensions equality operator to avoid unnecessary TextPainter re-layouts (#108623)

* Roll Flutter Engine from cf0db3e to aa90044 (1 revision) (#108734)

* Roll Flutter Engine from aa90044 to 6d2fd23 (5 revisions) (#108744)

* Fix lerp to eccentric circle. (#108743)

* Roll Flutter Engine from 6d2fd23 to f182794 (4 revisions) (#108749)

* Roll Flutter Engine from f182794 to 25e8021 (1 revision) (#108751)

* Sync with flutter/.github#13 (#108754)

* Roll Flutter Engine from 25e8021 to e771729 (2 revisions) (#108755)

* clean-up analysis_options.yaml (#108747)

* Fix ExpansionTile shows children background when expanded (#107834)

* Create `containsSemantics` to allow for partial matching of semantics in tests. (#108573)

* Roll Flutter Engine from e771729 to 7d0f6d2 (2 revisions) (#108757)

* Enable conditional_uri_does_not_exist (#108652)

* Roll Flutter Engine from 7d0f6d2 to b257966 (3 revisions) (#108763)

* Roll Flutter Engine from b257966 to 60e5eb6 (3 revisions) (#108766)

* Reland `Linux_samsung_a02 openpay_benchmarks__scroll_perf` (#108466) (#108769)

* [SelectionOverlay]Move the debug statement to the scope of the assertion. (#108508)

* Roll Flutter Engine from 60e5eb6 to 1c3b1b3 (11 revisions) (#108780)

* Roll Flutter Engine from 1c3b1b3 to b607811 (1 revision) (#108782)

* Roll Flutter Engine from b607811 to 3b2bd24 (1 revision) (#108784)

* Roll Flutter Engine from 3b2bd24 to 0e5392c (1 revision) (#108788)

* Roll Flutter Engine from 0e5392c to 4b19256 (1 revision) (#108793)

* Roll Flutter Engine from 4b19256 to e0b5edc (2 revisions) (#108798)

* Roll Flutter Engine from e0b5edc to b164c5c (1 revision) (#108814)

* Update text_field.dart

* Roll Flutter Engine from b164c5c to eb2b57b (4 revisions) (#108821)

* Roll Plugins from a6d42f1e01d3 to 0d6d03a94ed5 (1 revision) (#108822)

* Roll Flutter Engine from eb2b57b to 978d8e2 (2 revisions) (#108825)

* Loupe Android + iOS (#107477)

* added Magnifier for iOS and Android

* Mark `Mac_ios microbenchmarks_ios_flaky` flaky (#108820)

* Deprecate `toggleableActiveColor` (#97972)

* Roll Flutter Engine from 978d8e2 to 2b31732 (4 revisions) (#108830)

* [flutter_tools] Test that DAP process terminates at the end of a session (#108301)

* Roll Flutter Engine from 2b31732 to 4e9c869 (1 revision) (#108833)

* fix noop toString() diagnostics (#108836)

* [flutter_tools] Migrate more tool tests to null-safety (#108639)

* Revert "Fix ExpansionTile shows children background when expanded" (#108844)

* Roll Flutter Engine from 4e9c869 to 6724561 (2 revisions) (#108838)

* Marks Linux_android clipper_cache_perf__e2e_summary to be unflaky (#104088)

* Update documentation to match implementation (#108843)

* Reland "Add shadowColor and surfaceTintColor to Dialog and DialogTheme." #108718

* Roll Flutter Engine from 6724561 to f3deaba (8 revisions) (#108847)

* Roll Flutter Engine from f3deaba to c07e1ac (2 revisions) (#108849)

* Roll Flutter Engine from c07e1ac to a1e77ae (5 revisions) (#108850)

* Roll Flutter Engine from a1e77ae to c456476 (2 revisions) (#108853)

* Roll Flutter Engine from c456476 to 6cd744b (1 revision) (#108857)

* Roll Flutter Engine from 6cd744b to 51296a6 (1 revision) (#108860)

* Roll Flutter Engine from 51296a6 to 05228ad (1 revision) (#108862)

* Revert "Roll Flutter Engine from 51296a6 to 05228ad (1 revision) (#108862)" (#108882)

This reverts commit a880c4e.

* Roll Plugins from 0d6d03a94ed5 to e74c42028d39 (5 revisions) (#108887)

* Roll Flutter Engine from 51296a6 to 2c28298 (6 revisions) (#108899)

* [flutter_test] Add flag to send device pointer events to the framework (#108430)

* Roll Flutter Engine from 2c28298 to adba702 (2 revisions) (#108903)

* fix flutter not finding custom device (#108884)

* Force a11y services to off for complex_layout_semantics_perf test (#108906)

* Update `equalsIgnoringHashCodes` to take a list of Strings (#108507)

* [macOS] Use editing intents from engine (#105407)

* Added `IconButtonTheme` and apply it to `IconButton` in M3 (#108332)

* Created IconButtonTheme and apply it to IconButton

* [web] Add onEntrypointLoaded to FlutterLoader. (#108776)

* Roll pub packages (#108919)

* [flutter_test] perf: find.ancestor (#108868)

* Roll Flutter Engine from adba702 to 1188a80 (4 revisions) (#108922)

* Bump github/codeql-action from 2.1.17 to 2.1.18 (#108923)

* Remove some outdated ignores from framework (#108915)

* Roll Flutter Engine from 1188a80 to 1743d1d (1 revision) (#108925)

* Clean up ScrollbarPainter (#107179)

* Remove outdated ignores (#108924)

* Add more logs to diagnose Gold flake (#108930)

* M3 counter error style

* polish

* Update text_field_template.dart

* resolve comments

* Update text_field.dart

Co-authored-by: engine-flutter-autoroll <engine-flutter-autoroll@skia.org>
Co-authored-by: Tomasz Gucio <72562119+tgucio@users.noreply.github.com>
Co-authored-by: Greg Spencer <gspencergoog@users.noreply.github.com>
Co-authored-by: Ian Hickson <ian@hixie.ch>
Co-authored-by: Michael Goderbauer <goderbauer@google.com>
Co-authored-by: Bruno Leroux <leroux_bruno@yahoo.fr>
Co-authored-by: pdblasi-google <109253501+pdblasi-google@users.noreply.github.com>
Co-authored-by: Kaushik Iska <iska.kaushik@gmail.com>
Co-authored-by: xubaolin <xubaolin@oppo.com>
Co-authored-by: Anthony Oleinik <48811365+antholeole@users.noreply.github.com>
Co-authored-by: keyonghan <54558023+keyonghan@users.noreply.github.com>
Co-authored-by: Taha Tesser <tessertaha@gmail.com>
Co-authored-by: Danny Tuppeny <danny@tuppeny.com>
Co-authored-by: Phil Quitslund <pq@users.noreply.github.com>
Co-authored-by: Christopher Fujino <christopherfujino@gmail.com>
Co-authored-by: Kate Lovett <katelovett@google.com>
Co-authored-by: Flutter GitHub Bot <fluttergithubbot@gmail.com>
Co-authored-by: parkershepherd <me@parkershepherd.com>
Co-authored-by: Darren Austin <darrenaustin@google.com>
Co-authored-by: Zachary Anderson <zanderso@users.noreply.github.com>
Co-authored-by: Jia Hao <jiahaog@users.noreply.github.com>
Co-authored-by: Hannes Winkler <hanneswinkler2000@web.de>
Co-authored-by: Matej Knopp <matej.knopp@gmail.com>
Co-authored-by: Qun Cheng <36861262+QuncCccccc@users.noreply.github.com>
Co-authored-by: David Iglesias <ditman@gmail.com>
Co-authored-by: Pascal Welsch <pascal@welsch.dev>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
@flutter-dashboard flutter-dashboard bot added the team Infra upgrades, team productivity, code health, technical debt. See also team: labels. label Aug 4, 2022
@rydmike
Copy link
Contributor

rydmike commented Aug 5, 2022

Just verified in master 3.1.0-0.0.pre.2108, that the fix has landed and works. Thanks!

camsim99 pushed a commit to camsim99/flutter that referenced this pull request Aug 10, 2022
…08332)

* Created IconButtonTheme and apply it to IconButton
camsim99 pushed a commit to camsim99/flutter that referenced this pull request Aug 10, 2022
* M3 counter error style

* polish

* Update text_field_template.dart

* Roll Flutter Engine from 3cba105 to cf0db3e (1 revision) (flutter#108716)

* resolve comments

* Roll Plugins from 257eacb1e2aa to a6d42f1e01d3 (3 revisions) (flutter#108738)

* Override PlaceholderDimensions equality operator to avoid unnecessary TextPainter re-layouts (flutter#108623)

* Roll Flutter Engine from cf0db3e to aa90044 (1 revision) (flutter#108734)

* Roll Flutter Engine from aa90044 to 6d2fd23 (5 revisions) (flutter#108744)

* Fix lerp to eccentric circle. (flutter#108743)

* Roll Flutter Engine from 6d2fd23 to f182794 (4 revisions) (flutter#108749)

* Roll Flutter Engine from f182794 to 25e8021 (1 revision) (flutter#108751)

* Sync with flutter/.github#13 (flutter#108754)

* Roll Flutter Engine from 25e8021 to e771729 (2 revisions) (flutter#108755)

* clean-up analysis_options.yaml (flutter#108747)

* Fix ExpansionTile shows children background when expanded (flutter#107834)

* Create `containsSemantics` to allow for partial matching of semantics in tests. (flutter#108573)

* Roll Flutter Engine from e771729 to 7d0f6d2 (2 revisions) (flutter#108757)

* Enable conditional_uri_does_not_exist (flutter#108652)

* Roll Flutter Engine from 7d0f6d2 to b257966 (3 revisions) (flutter#108763)

* Roll Flutter Engine from b257966 to 60e5eb6 (3 revisions) (flutter#108766)

* Reland `Linux_samsung_a02 openpay_benchmarks__scroll_perf` (flutter#108466) (flutter#108769)

* [SelectionOverlay]Move the debug statement to the scope of the assertion. (flutter#108508)

* Roll Flutter Engine from 60e5eb6 to 1c3b1b3 (11 revisions) (flutter#108780)

* Roll Flutter Engine from 1c3b1b3 to b607811 (1 revision) (flutter#108782)

* Roll Flutter Engine from b607811 to 3b2bd24 (1 revision) (flutter#108784)

* Roll Flutter Engine from 3b2bd24 to 0e5392c (1 revision) (flutter#108788)

* Roll Flutter Engine from 0e5392c to 4b19256 (1 revision) (flutter#108793)

* Roll Flutter Engine from 4b19256 to e0b5edc (2 revisions) (flutter#108798)

* Roll Flutter Engine from e0b5edc to b164c5c (1 revision) (flutter#108814)

* Update text_field.dart

* Roll Flutter Engine from b164c5c to eb2b57b (4 revisions) (flutter#108821)

* Roll Plugins from a6d42f1e01d3 to 0d6d03a94ed5 (1 revision) (flutter#108822)

* Roll Flutter Engine from eb2b57b to 978d8e2 (2 revisions) (flutter#108825)

* Loupe Android + iOS (flutter#107477)

* added Magnifier for iOS and Android

* Mark `Mac_ios microbenchmarks_ios_flaky` flaky (flutter#108820)

* Deprecate `toggleableActiveColor` (flutter#97972)

* Roll Flutter Engine from 978d8e2 to 2b31732 (4 revisions) (flutter#108830)

* [flutter_tools] Test that DAP process terminates at the end of a session (flutter#108301)

* Roll Flutter Engine from 2b31732 to 4e9c869 (1 revision) (flutter#108833)

* fix noop toString() diagnostics (flutter#108836)

* [flutter_tools] Migrate more tool tests to null-safety (flutter#108639)

* Revert "Fix ExpansionTile shows children background when expanded" (flutter#108844)

* Roll Flutter Engine from 4e9c869 to 6724561 (2 revisions) (flutter#108838)

* Marks Linux_android clipper_cache_perf__e2e_summary to be unflaky (flutter#104088)

* Update documentation to match implementation (flutter#108843)

* Reland "Add shadowColor and surfaceTintColor to Dialog and DialogTheme." flutter#108718

* Roll Flutter Engine from 6724561 to f3deaba (8 revisions) (flutter#108847)

* Roll Flutter Engine from f3deaba to c07e1ac (2 revisions) (flutter#108849)

* Roll Flutter Engine from c07e1ac to a1e77ae (5 revisions) (flutter#108850)

* Roll Flutter Engine from a1e77ae to c456476 (2 revisions) (flutter#108853)

* Roll Flutter Engine from c456476 to 6cd744b (1 revision) (flutter#108857)

* Roll Flutter Engine from 6cd744b to 51296a6 (1 revision) (flutter#108860)

* Roll Flutter Engine from 51296a6 to 05228ad (1 revision) (flutter#108862)

* Revert "Roll Flutter Engine from 51296a6 to 05228ad (1 revision) (flutter#108862)" (flutter#108882)

This reverts commit a880c4e.

* Roll Plugins from 0d6d03a94ed5 to e74c42028d39 (5 revisions) (flutter#108887)

* Roll Flutter Engine from 51296a6 to 2c28298 (6 revisions) (flutter#108899)

* [flutter_test] Add flag to send device pointer events to the framework (flutter#108430)

* Roll Flutter Engine from 2c28298 to adba702 (2 revisions) (flutter#108903)

* fix flutter not finding custom device (flutter#108884)

* Force a11y services to off for complex_layout_semantics_perf test (flutter#108906)

* Update `equalsIgnoringHashCodes` to take a list of Strings (flutter#108507)

* [macOS] Use editing intents from engine (flutter#105407)

* Added `IconButtonTheme` and apply it to `IconButton` in M3 (flutter#108332)

* Created IconButtonTheme and apply it to IconButton

* [web] Add onEntrypointLoaded to FlutterLoader. (flutter#108776)

* Roll pub packages (flutter#108919)

* [flutter_test] perf: find.ancestor (flutter#108868)

* Roll Flutter Engine from adba702 to 1188a80 (4 revisions) (flutter#108922)

* Bump github/codeql-action from 2.1.17 to 2.1.18 (flutter#108923)

* Remove some outdated ignores from framework (flutter#108915)

* Roll Flutter Engine from 1188a80 to 1743d1d (1 revision) (flutter#108925)

* Clean up ScrollbarPainter (flutter#107179)

* Remove outdated ignores (flutter#108924)

* Add more logs to diagnose Gold flake (flutter#108930)

* M3 counter error style

* polish

* Update text_field_template.dart

* resolve comments

* Update text_field.dart

Co-authored-by: engine-flutter-autoroll <engine-flutter-autoroll@skia.org>
Co-authored-by: Tomasz Gucio <72562119+tgucio@users.noreply.github.com>
Co-authored-by: Greg Spencer <gspencergoog@users.noreply.github.com>
Co-authored-by: Ian Hickson <ian@hixie.ch>
Co-authored-by: Michael Goderbauer <goderbauer@google.com>
Co-authored-by: Bruno Leroux <leroux_bruno@yahoo.fr>
Co-authored-by: pdblasi-google <109253501+pdblasi-google@users.noreply.github.com>
Co-authored-by: Kaushik Iska <iska.kaushik@gmail.com>
Co-authored-by: xubaolin <xubaolin@oppo.com>
Co-authored-by: Anthony Oleinik <48811365+antholeole@users.noreply.github.com>
Co-authored-by: keyonghan <54558023+keyonghan@users.noreply.github.com>
Co-authored-by: Taha Tesser <tessertaha@gmail.com>
Co-authored-by: Danny Tuppeny <danny@tuppeny.com>
Co-authored-by: Phil Quitslund <pq@users.noreply.github.com>
Co-authored-by: Christopher Fujino <christopherfujino@gmail.com>
Co-authored-by: Kate Lovett <katelovett@google.com>
Co-authored-by: Flutter GitHub Bot <fluttergithubbot@gmail.com>
Co-authored-by: parkershepherd <me@parkershepherd.com>
Co-authored-by: Darren Austin <darrenaustin@google.com>
Co-authored-by: Zachary Anderson <zanderso@users.noreply.github.com>
Co-authored-by: Jia Hao <jiahaog@users.noreply.github.com>
Co-authored-by: Hannes Winkler <hanneswinkler2000@web.de>
Co-authored-by: Matej Knopp <matej.knopp@gmail.com>
Co-authored-by: Qun Cheng <36861262+QuncCccccc@users.noreply.github.com>
Co-authored-by: David Iglesias <ditman@gmail.com>
Co-authored-by: Pascal Welsch <pascal@welsch.dev>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a: text input Entering text in a text field or keyboard related problems f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. team Infra upgrades, team productivity, code health, technical debt. See also team: labels.
Projects
None yet
4 participants