Skip to content

Fix DropdownMenu focused item styles#153159

Merged
auto-submit[bot] merged 2 commits intoflutter:masterfrom
NevercodeHQ:fix_dropdown_menu_focused_item_styles
Sep 6, 2024
Merged

Fix DropdownMenu focused item styles#153159
auto-submit[bot] merged 2 commits intoflutter:masterfrom
NevercodeHQ:fix_dropdown_menu_focused_item_styles

Conversation

@bleroux
Copy link
Contributor

@bleroux bleroux commented Aug 9, 2024 •
edited
Loading

Description

This PR fixes the style resolution for selected dropdown menu items (make it possible to provide a custom style and avoid hardcoded values when possible).

For the moment, I kept the default selected background which was previously set (onSurface.withOpacity(0.12)) to keep this PR focused on its goal which is to make it possible to overrides the defaults item style by providing a custom button style at the theme level or at the menu entry level.

Related Issue

Fixes #123736.

Tests

Adds 4 tests.

@github-actions github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. labels Aug 9, 2024
@bleroux bleroux mentioned this pull request Aug 9, 2024
Closed
14 tasks
@bleroux bleroux force-pushed the fix_dropdown_menu_focused_item_styles branch from 4bd5aee to 83d42a3 Compare August 23, 2024 13:21
@rydmike
Copy link
Contributor

rydmike commented Aug 28, 2024 •
edited
Loading

Thanks @bleroux, looks good to me. A question, what was the intention of the change on MenuAnchor and its background, that you reverted?

Nice to see that by SubmenuButton.defaultStyleOf and MenuItemButton.defaultStyleOf are available. Hopefully this makes it obvious why such getters are needed for all theme defaults 😃

@bleroux bleroux force-pushed the fix_dropdown_menu_focused_item_styles branch from 83d42a3 to 084681e Compare August 29, 2024 11:37
@bleroux bleroux changed the title [WIP] Fix DropdownMenu focused item styles Fix DropdownMenu focused item styles Aug 29, 2024
@bleroux bleroux marked this pull request as ready for review August 29, 2024 12:29
@bleroux bleroux requested review from QuncCccccc and justinmc August 29, 2024 12:29
@bleroux
Copy link
Contributor Author

bleroux commented Aug 29, 2024 •
edited
Loading

Thanks @bleroux, looks good to me.

Thanks @rydmike 🙏

A question, what was the intention of the change on MenuAnchor and its background, that you reverted?

(for others reading this, in my previous version of this PR there were some changes to MenuAnchor and a commit that revert those changes later).

At first I thought that MenuAnchor defaults were wrong for the background color (it returned Colors.transparent for all states). While reading the spec I made a quick (and false) conclusion that it should be secondaryContainer instead.

After looking deeper, I think MenuAnchor defaults (background color, foreground color, icon color) should be updated to take into account the MaterialState.selected state. Once this will be done at MenuAnchor level, we should probably use these new defaults for DropdownMenu items (for the selected item, the default background would be secondaryContainer, the default foreground color onSecondaryContainer ).

What do you think of this?

justinmc
justinmc approved these changes Aug 29, 2024
View reviewed changes
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 👍

@bleroux bleroux force-pushed the fix_dropdown_menu_focused_item_styles branch 2 times, most recently from 03057ab to 56e2dd6 Compare September 2, 2024 15:03
@bleroux
Copy link
Contributor Author

bleroux commented Sep 2, 2024

@justinmc Thanks in advance for having a look at the Google testing failure.

@justinmc
Copy link
Contributor

justinmc commented Sep 5, 2024

In the Google test failures I see some dropdowns that have become significantly more narrow with the change. Both the dropdown and the options. Could that be due to the padding change, or anything else?

@bleroux bleroux force-pushed the fix_dropdown_menu_focused_item_styles branch from 56e2dd6 to 0789402 Compare September 6, 2024 06:14
@bleroux
Copy link
Contributor Author

bleroux commented Sep 6, 2024

In the Google test failures I see some dropdowns that have become significantly more narrow with the change. Both the dropdown and the options. Could that be due to the padding change, or anything else?

Yes, that's probably related to the small change I made to the padding logic.
I restored the previous padding logic, tell me if it is better now.

justinmc
justinmc approved these changes Sep 6, 2024
View reviewed changes
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.

All green, thanks! Renewing my LGTM 👍

@bleroux bleroux added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 6, 2024
@auto-submit auto-submit bot merged commit 2e221e7 into flutter:master Sep 6, 2024
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Sep 6, 2024
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 6, 2024
@engine-flutter-autoroll
2e221e7 Fix DropdownMenu focused item styles (flutter/flutter#153159)
cc62449
@bleroux bleroux deleted the fix_dropdown_menu_focused_item_styles branch September 6, 2024 19:25
auto-submit bot pushed a commit to flutter/packages that referenced this pull request Sep 6, 2024
@engine-flutter-autoroll
Roll Flutter from 45ef8f3 to 2e221e7 (20 revisions) (#7599)
b4e0fc1 
flutter/flutter@45ef8f3...2e221e7

2024-09-06 [email protected] Fix DropdownMenu focused item styles (flutter/flutter#153159)
2024-09-06 [email protected] Support custom transition duration for `DialogRoute`, `CupertinoDialogRoute` and show dialog methods. (flutter/flutter#154048)
2024-09-06 [email protected] [tool] Add `dartFileName` setting for platform plugins  (flutter/flutter#153099)
2024-09-06 [email protected] [Conductor] Add ability to override mirror, add tests for default arg parsing and custom arg parsing (flutter/flutter#154363)
2024-09-06 [email protected] Improve CupertinoPopupSurface appearance (flutter/flutter#151430)
2024-09-06 [email protected] Roll Packages from 71e827e to 56df73e (1 revision) (flutter/flutter#154725)
2024-09-06 [email protected] Quick access to style guide (flutter/flutter#154689)
2024-09-06 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Roll Flutter Engine from c50eb8a65097 to 015f3b1dec53 (2 revisions) (#154691)" (flutter/flutter#154726)
2024-09-05 [email protected] Improve iOS unpack target's error messages (flutter/flutter#154649)
2024-09-05 [email protected] Made some pixel tests fuzzy (flutter/flutter#154680)
2024-09-05 [email protected] Roll Flutter Engine from c50eb8a65097 to 015f3b1dec53 (2 revisions) (flutter/flutter#154691)
2024-09-05 49699333+dependabot[bot]@users.noreply.github.com Bump peter-evans/create-pull-request from 7.0.0 to 7.0.1 (flutter/flutter#154690)
2024-09-05 [email protected] Normalize Dialog theme (flutter/flutter#153982)
2024-09-05 [email protected] iOS,macOS: Do not copy unsigned_binaries.txt to build outputs (flutter/flutter#154684)
2024-09-05 [email protected] Roll Flutter Engine from e042ff5df7af to c50eb8a65097 (1 revision) (flutter/flutter#154679)
2024-09-05 [email protected] Add proguard rule to keep the class for all implementations of FlutterPlugin (flutter/flutter#154677)
2024-09-05 [email protected] Fix DropdownMenu menu does not follow the text field (flutter/flutter#154667)
2024-09-05 [email protected] Roll Flutter Engine from a156e713f4dc to e042ff5df7af (1 revision) (flutter/flutter#154678)
2024-09-05 [email protected] Fix ZoomPageTransitionsBuilder hardcoded fill color (flutter/flutter#154057)
2024-09-05 [email protected] Roll Flutter Engine from 34b61eb53b99 to a156e713f4dc (1 revision) (flutter/flutter#154672)

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
@Piinks Piinks mentioned this pull request Sep 6, 2024
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 11, 2024
@engine-flutter-autoroll
2e221e7 Fix DropdownMenu focused item styles (flutter/flutter#153159)
fbb9246
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 12, 2024
@engine-flutter-autoroll
2e221e7 Fix DropdownMenu focused item styles (flutter/flutter#153159)
9bc1c0c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@justinmc justinmc justinmc approved these changes

@QuncCccccc QuncCccccc Awaiting requested review from QuncCccccc

Assignees

No one assigned

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Material3] DropdownMenu themed focused item broken

3 participants

@bleroux @rydmike @justinmc