Skip to content

Conversation

@TahaTesser
Copy link
Member

@TahaTesser TahaTesser commented Mar 20, 2024

fixes DropdownMenu TrailingIcon can't be styled through providing an IconButtonTheme

Code sample

expand to view the code sample
import 'package:flutter/material.dart';

void main() => runApp(const MyApp());

class MyApp extends StatelessWidget {
  const MyApp({super.key});

  @override
  Widget build(BuildContext context) {
    return MaterialApp(
      debugShowCheckedModeBanner: false,
      home: Scaffold(
        body: Center(
          child: IconButtonTheme(
            data: IconButtonThemeData(
              style: IconButton.styleFrom(
                foregroundColor: const Color(0xffff0000),
                shape: RoundedRectangleBorder(
                  borderRadius: BorderRadius.circular(10.0),
                ),
              ),
            ),
            child: SizedBox(
              width: 300,
              child: Column(
                mainAxisSize: MainAxisSize.min,
                children: [
                  const Text('IconButton'),
                  IconButton(onPressed: () {}, icon: const Icon(Icons.search)),
                  const Text('TextField'),
                  TextField(
                    decoration: InputDecoration(
                      prefixIcon: IconButton(
                        onPressed: () {},
                        icon: const Icon(Icons.search),
                      ),
                      suffixIcon: IconButton(
                        onPressed: () {},
                        icon: const Icon(Icons.search),
                      ),
                    ),
                  ),
                ],
              ),
            ),
          ),
        ),
      ),
    );
  }
}

Before

After

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@github-actions github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. labels Mar 20, 2024
@TahaTesser TahaTesser requested a review from justinmc March 20, 2024 15:53
Copy link
Contributor

@justinmc justinmc left a comment

Choose a reason for hiding this comment

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

LGTM 👍 Thanks Taha glad you're back!

@TahaTesser TahaTesser force-pushed the input_icon_button_theme branch 3 times, most recently from 06cae8e to 6d2e9bb Compare March 22, 2024 07:49
Copy link
Contributor

@Piinks Piinks left a comment

Choose a reason for hiding this comment

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

There are some customers that are broken by this. It looks like they'll need to update their code before we can land this.

Unless, could it be overriding a color value unexpectedly? Would an empty (null valued) theme here override the widget value?

Here is an example of one of the failures, the left is expected, the right is the result with this change:

Screenshot 2024-03-22 at 11 17 58 AM

@TahaTesser
Copy link
Member Author

@justinmc @Piinks

Parent iconButtonTheme isn't passed to IconButton in the prefix and suffix icons in the input decorator.

If this is a disruptive change, i could instead introduce iconButtonTheme to DropdownMenuTheme only and address the issue #145081. This shouldn't break the customer tests as it is not currently supported.

const DropdownMenuThemeData({
this.textStyle,
this.inputDecorationTheme,
this.menuStyle,
});

Copy link
Contributor

@Piinks Piinks left a comment

Choose a reason for hiding this comment

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

The affected customers may just be expecting the incorrect behavior, if so it should be relatively easy for them to be updated. We'll need to take a look at their code first. I think it make sense to inherit the theme in this way, rather than creating a special scenario for just DropdownMenu, but I'll defer to @justinmc. :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for the late response here!

I think maybe there is actually a problem that is being exposed by the Google test failures. The MaterialStateProperty colors in the iconButtonTheme here are not having the materialState of the InputDecorator applied to them. See the example below.

I would expect the icon to be purple in this example, but it's lime
import 'package:flutter/material.dart';

void main() {
  runApp(const MyApp());
}

class MyApp extends StatelessWidget {
  const MyApp({Key? key}) : super(key: key);

  @override
  Widget build(BuildContext context) {
    return MaterialApp(
      title: 'Flutter Demo',
      theme: ThemeData(
        primarySwatch: Colors.blue,
        iconButtonTheme: IconButtonThemeData(
          style: ButtonStyle(
            foregroundColor: WidgetStateProperty.resolveWith((Set<WidgetState> states) {
              if (states.contains(WidgetState.error)) {
                return Colors.deepPurple;
              }
              return Colors.limeAccent;
            }),
          ),
        ),
      ),
      home: const MyHomePage(title: 'Flutter Demo Home Page'),
    );
  }
}

class MyHomePage extends StatelessWidget {
  const MyHomePage({Key? key, required this.title}) : super(key: key);

  final String title;

  @override
  Widget build(BuildContext context) {
    return Scaffold(
      appBar: AppBar(
        title: Text(title),
      ),
      body: Center(
        child: Column(
          mainAxisAlignment: MainAxisAlignment.center,
          children: <Widget>[
            TextField(
              decoration: InputDecoration(
                errorText: 'error state',
                suffixIcon: IconButton(
                  onPressed: () {
                  },
                  icon: const Icon(Icons.phonelink_erase),
                ),
              ),
            ),
          ],
        ),
      ),
    );
  }
}

Copy link
Member Author

@TahaTesser TahaTesser Apr 4, 2024

Choose a reason for hiding this comment

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

Thanks so much for the details!

It turns out the inherited IconButtonTheme.style.foreground couldn't be overridden by _getPrefixIconColor & _getPrefixIconColor and resolve input decoration states.

I've updated both _getPrefixIconColor & _getSuffixIconColor to inherit the IconButtonTheme.style.foreground and resolve the state while placing in the order it can be superseded by inputDecorationTheme and decoration.suffixIconColor.

I think maybe there is actually a problem that is being exposed by the Google test failures

Add two new tests to verify _getPrefixIconColor & _getSuffixIconColor respects input decoration states
for IconButtonTheme.style.foreground color in the input_decorator_test.dart.

@TahaTesser TahaTesser force-pushed the input_icon_button_theme branch from 6d2e9bb to b10fad4 Compare April 4, 2024 14:07
@TahaTesser TahaTesser requested a review from justinmc April 4, 2024 15:12
Copy link
Contributor

@justinmc justinmc left a comment

Choose a reason for hiding this comment

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

LGTM with a nit 👍

Thanks for fixing that! That fixes my use case, and it looks like it fixed the Google tests too. Thanks for testing those cases as well.

@TahaTesser TahaTesser added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 4, 2024
@TahaTesser TahaTesser changed the title Fix InputDecorator suffix and prefix icons with IconButton widget ignore IconButtonTheme Fix InputDecorator suffix and prefix IconButtons ignore IconButtonTheme Apr 4, 2024
@auto-submit auto-submit bot merged commit 7c72a08 into flutter:master Apr 4, 2024
@TahaTesser TahaTesser deleted the input_icon_button_theme branch April 4, 2024 19:29
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Apr 5, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Apr 5, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Apr 5, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Apr 5, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Apr 5, 2024
auto-submit bot pushed a commit to flutter/packages that referenced this pull request Apr 5, 2024
Roll Flutter from ac2ca93 to 477ebd8 (32 revisions)

flutter/flutter@ac2ca93...477ebd8

2024-04-05 [email protected] Roll Flutter Engine from 95ffe73ac2a8 to 6974dbac35a1 (2 revisions) (flutter/flutter#146350)
2024-04-05 [email protected] Roll pub packages (flutter/flutter#146348)
2024-04-05 [email protected] Roll Packages from dce6f0c to 764d997 (4 revisions) (flutter/flutter#146344)
2024-04-05 [email protected] Roll Flutter Engine from 292c48b9af5e to 95ffe73ac2a8 (1 revision) (flutter/flutter#146343)
2024-04-05 [email protected] Roll pub packages (flutter/flutter#146331)
2024-04-05 [email protected] Roll Flutter Engine from 23a1d0d2fce6 to 292c48b9af5e (1 revision) (flutter/flutter#146330)
2024-04-05 [email protected] Roll Flutter Engine from 597d33f57183 to 23a1d0d2fce6 (1 revision) (flutter/flutter#146327)
2024-04-05 [email protected] Roll Flutter Engine from 1fa0623bb07f to 597d33f57183 (1 revision) (flutter/flutter#146325)
2024-04-05 [email protected] Roll Flutter Engine from f17d586de6f1 to 1fa0623bb07f (1 revision) (flutter/flutter#146324)
2024-04-05 [email protected] Roll Flutter Engine from d44462a42d73 to f17d586de6f1 (4 revisions) (flutter/flutter#146322)
2024-04-05 [email protected] Fix cursor is not centered when cursorHeight is set (non-Apple platforms). (flutter/flutter#145829)
2024-04-05 [email protected] refactor: Perform plugin resolution per platform (flutter/flutter#144506)
2024-04-04 [email protected] `ExpansionTile` Unable to remove right padding from title (flutter/flutter#145271)
2024-04-04 [email protected] Roll Flutter Engine from dcc99d652154 to d44462a42d73 (1 revision) (flutter/flutter#146311)
2024-04-04 49699333+dependabot[bot]@users.noreply.github.com Bump codecov/codecov-action from 4.1.1 to 4.2.0 (flutter/flutter#146310)
2024-04-04 [email protected] Roll Flutter Engine from e3104af94328 to dcc99d652154 (4 revisions) (flutter/flutter#146308)
2024-04-04 [email protected] Roll Flutter Engine from fa4d97e363ff to e3104af94328 (2 revisions) (flutter/flutter#146300)
2024-04-04 [email protected] Remove dead `compareIosVersions` function (flutter/flutter#146298)
2024-04-04 [email protected] Adds semanticsLabel to MenuItemButton (flutter/flutter#145846)
2024-04-04 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Bump to AGP 8.1/Gradle 8.3 (almost) everywhere (#146181)" (flutter/flutter#146305)
2024-04-04 [email protected] Roll Flutter Engine from 918c72b803cc to fa4d97e363ff (2 revisions) (flutter/flutter#146299)
2024-04-04 [email protected] Bump to AGP 8.1/Gradle 8.3 (almost) everywhere (flutter/flutter#146181)
2024-04-04 [email protected] Roll Flutter Engine from 03cd54d4285a to 918c72b803cc (2 revisions) (flutter/flutter#146296)
2024-04-04 [email protected] Fix InputDecorator suffix and prefix IconButtons ignore `IconButtonTheme` (flutter/flutter#145473)
2024-04-04 [email protected] Give `generate_gradle_lockfiles.dart` functionality to exclude certain subdirectories (flutter/flutter#146228)
2024-04-04 [email protected] Update documentation to discourage using the TextEditingController.text setter (flutter/flutter#146151)
2024-04-04 [email protected] Roll Flutter Engine from b2ceabf7acce to 03cd54d4285a (1 revision) (flutter/flutter#146292)
2024-04-04 [email protected] Roll Flutter Engine from b435a8a87df0 to b2ceabf7acce (1 revision) (flutter/flutter#146283)
2024-04-04 [email protected] Roll Flutter Engine from bd51ee80d74b to b435a8a87df0 (1 revision) (flutter/flutter#146279)
2024-04-04 [email protected] Roll Packages from 0e848fa to dce6f0c (4 revisions) (flutter/flutter#146276)
2024-04-04 [email protected] Flutter templates example app Gradle memory settings (flutter/flutter#146275)
2024-04-04 [email protected] Roll Flutter Engine from 4303dc0d7a73 to bd51ee80d74b (2 revisions) (flutter/flutter#146273)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages
Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
...
@felixwoestmann
Copy link

@TahaTesser Thank you for taking on the issue I created!

However, I believe my complaint wasn't entirely fixed.
I can confirm that by using the foreground color property I can now change the color of the Icon using the IconButtonThemeData. However I am not able to modify the overlayColor. The reason I want to do this is to get rid of the hover effect on the trailing Icon in favor of a hover effect on the whole component.

Is this just not intended for the Material DropdownMenu or am I using the properties wrongly?

@TahaTesser
Copy link
Member Author

@felixwoestmann
Thanks for the comment. I expected the overlay to work since we're merging the iconButtonTheme.style

  ).merge(iconButtonTheme.style),

I'll address this soon and add test that checks all iconButtonTheme.style properties.

gilnobrega pushed a commit to gilnobrega/flutter that referenced this pull request Apr 22, 2024
…eme` (flutter#145473)

fixes [DropdownMenu TrailingIcon can't be styled through providing an IconButtonTheme](flutter#145081)

### Code sample

<details>
<summary>expand to view the code sample</summary> 

```dart
import 'package:flutter/material.dart';

void main() => runApp(const MyApp());

class MyApp extends StatelessWidget {
  const MyApp({super.key});

  @OverRide
  Widget build(BuildContext context) {
    return MaterialApp(
      debugShowCheckedModeBanner: false,
      home: Scaffold(
        body: Center(
          child: IconButtonTheme(
            data: IconButtonThemeData(
              style: IconButton.styleFrom(
                foregroundColor: const Color(0xffff0000),
                shape: RoundedRectangleBorder(
                  borderRadius: BorderRadius.circular(10.0),
                ),
              ),
            ),
            child: SizedBox(
              width: 300,
              child: Column(
                mainAxisSize: MainAxisSize.min,
                children: [
                  const Text('IconButton'),
                  IconButton(onPressed: () {}, icon: const Icon(Icons.search)),
                  const Text('TextField'),
                  TextField(
                    decoration: InputDecoration(
                      prefixIcon: IconButton(
                        onPressed: () {},
                        icon: const Icon(Icons.search),
                      ),
                      suffixIcon: IconButton(
                        onPressed: () {},
                        icon: const Icon(Icons.search),
                      ),
                    ),
                  ),
                ],
              ),
            ),
          ),
        ),
      ),
    );
  }
}
```

</details>

| Before | After |
| --------------- | --------------- |
| <img src="https://github.com/flutter/flutter/assets/48603081/69b5966b-c95d-4934-b867-3262d1377f70"  /> | <img src="https://github.com/flutter/flutter/assets/48603081/0064db2b-0379-4424-a5bf-39bdc5441fe8" /> |
TecHaxter pushed a commit to TecHaxter/flutter_packages that referenced this pull request May 22, 2024
Roll Flutter from ac2ca93 to 477ebd8 (32 revisions)

flutter/flutter@ac2ca93...477ebd8

2024-04-05 [email protected] Roll Flutter Engine from 95ffe73ac2a8 to 6974dbac35a1 (2 revisions) (flutter/flutter#146350)
2024-04-05 [email protected] Roll pub packages (flutter/flutter#146348)
2024-04-05 [email protected] Roll Packages from dce6f0c to 764d997 (4 revisions) (flutter/flutter#146344)
2024-04-05 [email protected] Roll Flutter Engine from 292c48b9af5e to 95ffe73ac2a8 (1 revision) (flutter/flutter#146343)
2024-04-05 [email protected] Roll pub packages (flutter/flutter#146331)
2024-04-05 [email protected] Roll Flutter Engine from 23a1d0d2fce6 to 292c48b9af5e (1 revision) (flutter/flutter#146330)
2024-04-05 [email protected] Roll Flutter Engine from 597d33f57183 to 23a1d0d2fce6 (1 revision) (flutter/flutter#146327)
2024-04-05 [email protected] Roll Flutter Engine from 1fa0623bb07f to 597d33f57183 (1 revision) (flutter/flutter#146325)
2024-04-05 [email protected] Roll Flutter Engine from f17d586de6f1 to 1fa0623bb07f (1 revision) (flutter/flutter#146324)
2024-04-05 [email protected] Roll Flutter Engine from d44462a42d73 to f17d586de6f1 (4 revisions) (flutter/flutter#146322)
2024-04-05 [email protected] Fix cursor is not centered when cursorHeight is set (non-Apple platforms). (flutter/flutter#145829)
2024-04-05 [email protected] refactor: Perform plugin resolution per platform (flutter/flutter#144506)
2024-04-04 [email protected] `ExpansionTile` Unable to remove right padding from title (flutter/flutter#145271)
2024-04-04 [email protected] Roll Flutter Engine from dcc99d652154 to d44462a42d73 (1 revision) (flutter/flutter#146311)
2024-04-04 49699333+dependabot[bot]@users.noreply.github.com Bump codecov/codecov-action from 4.1.1 to 4.2.0 (flutter/flutter#146310)
2024-04-04 [email protected] Roll Flutter Engine from e3104af94328 to dcc99d652154 (4 revisions) (flutter/flutter#146308)
2024-04-04 [email protected] Roll Flutter Engine from fa4d97e363ff to e3104af94328 (2 revisions) (flutter/flutter#146300)
2024-04-04 [email protected] Remove dead `compareIosVersions` function (flutter/flutter#146298)
2024-04-04 [email protected] Adds semanticsLabel to MenuItemButton (flutter/flutter#145846)
2024-04-04 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Bump to AGP 8.1/Gradle 8.3 (almost) everywhere (#146181)" (flutter/flutter#146305)
2024-04-04 [email protected] Roll Flutter Engine from 918c72b803cc to fa4d97e363ff (2 revisions) (flutter/flutter#146299)
2024-04-04 [email protected] Bump to AGP 8.1/Gradle 8.3 (almost) everywhere (flutter/flutter#146181)
2024-04-04 [email protected] Roll Flutter Engine from 03cd54d4285a to 918c72b803cc (2 revisions) (flutter/flutter#146296)
2024-04-04 [email protected] Fix InputDecorator suffix and prefix IconButtons ignore `IconButtonTheme` (flutter/flutter#145473)
2024-04-04 [email protected] Give `generate_gradle_lockfiles.dart` functionality to exclude certain subdirectories (flutter/flutter#146228)
2024-04-04 [email protected] Update documentation to discourage using the TextEditingController.text setter (flutter/flutter#146151)
2024-04-04 [email protected] Roll Flutter Engine from b2ceabf7acce to 03cd54d4285a (1 revision) (flutter/flutter#146292)
2024-04-04 [email protected] Roll Flutter Engine from b435a8a87df0 to b2ceabf7acce (1 revision) (flutter/flutter#146283)
2024-04-04 [email protected] Roll Flutter Engine from bd51ee80d74b to b435a8a87df0 (1 revision) (flutter/flutter#146279)
2024-04-04 [email protected] Roll Packages from 0e848fa to dce6f0c (4 revisions) (flutter/flutter#146276)
2024-04-04 [email protected] Flutter templates example app Gradle memory settings (flutter/flutter#146275)
2024-04-04 [email protected] Roll Flutter Engine from 4303dc0d7a73 to bd51ee80d74b (2 revisions) (flutter/flutter#146273)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages
Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

autosubmit Merge PR when tree becomes green via auto submit App f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

DropdownMenu TrailingIcon can't be styled through providing an IconButtonTheme

4 participants