-
Notifications
You must be signed in to change notification settings - Fork 29.8k
Fix DropdownMenuEntry.style not resolved when entry is highlighted #178873
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
Fix DropdownMenuEntry.style not resolved when entry is highlighted #178873
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 correctly fixes an issue where DropdownMenuEntry.style properties were not being resolved for highlighted items. The approach of using a WidgetStatesController to force the :focus state is sound, and the added regression test effectively validates the fix for textStyle. I have two main points of feedback on the implementation in dropdown_menu.dart. First, the lifecycle management of the new _highlightedItemStatesController can be improved for better performance by creating it once in initState. Second, there's a pre-existing bug in how focused styles are applied that discards other state-dependent styles (like :hover), which I've provided a suggestion to fix.
| _highlightedItemStatesController?.dispose(); | ||
| _highlightedItemStatesController = WidgetStatesController(<WidgetState>{ | ||
| WidgetState.focused, | ||
| }); |
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.
Creating and disposing a WidgetStatesController within the build method is inefficient and an anti-pattern. This leads to unnecessary object creation and disposal on every rebuild.
A better approach is to manage the controller's lifecycle within the State object. You can create a single, final _highlightedItemStatesController instance for the _DropdownMenuState, initialize it once, and dispose of it in the dispose method. This single instance can then be reused for any highlighted item.
This would involve:
- Changing the declaration in
_DropdownMenuStateto:final WidgetStatesController _highlightedItemStatesController = WidgetStatesController(<WidgetState>{WidgetState.focused});
- Updating
dispose()to call_highlightedItemStatesController.dispose();instead of_highlightedItemStatesController?.dispose();. - Removing these lines that create and dispose the controller inside
_buildButtons.
| _highlightedItemStatesController?.dispose(); | ||
| _highlightedItemStatesController = WidgetStatesController(<WidgetState>{ | ||
| WidgetState.focused, | ||
| }); |
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.
I opted for the creation of a new states controller each time because I don't think WidgetStatesController are supposed to be reused across several widgets (as they have listeners and a list of states which is updated by InkWell).
For instance, each InkWell (inside each MenuItemButton) will create a Focus widget with an onFocusChange callback that might remove the focused state. If the states controller is shared and this callback is called once then the controller will no more have the focus state turned on. Currently there is an ExcludeFocus widget surrongind each MenuItemButton, so this might not happen but it seems to me that reusing states controller for different widgets can lead to subtle issues.
@justinmc Maybe I'm too cautious here? Let me know if you encountered something similar.
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.
By "reusing it for different widgets" you just mean for each MenuItemButton that is created each time this method is called?
In the docs for statesController I see:
/// Classes based on this one can provide their own
/// [WidgetStatesController] to which they've added listeners.
/// They can also update the controller's [WidgetStatesController.value]
/// however, this may only be done when it's safe to call
/// [State.setState], like in an event handler.
It's not safe to call setState in the build phase, so I think you wouldn't be able to set the value back to focused here. So I think you are right, and you have to do it the way that you've done it here.
946b2b9 to
f5c7fd8
Compare
38582aa to
2ab74a8
Compare
justinmc
left a comment
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.
LGTM 👍 . I can't think of a better way to do the WidgetStatesController than what you've done, but if you have any other ideas let me know.
| if (entryIsSelected) { | ||
| _highlightedItemStatesController?.dispose(); |
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.
Even if the entry is not selected should you still dispose _highlightedItemStatesController and set it to null?
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.
Even if the entry is not selected should you still dispose _highlightedItemStatesController and set it to null?
It will be set to null and disposed in two cases:
- when another entry is selected.
- when the DropdownMenu will be disposed.
I think it's ok as either there is one entry selected and the controller will be properly destroyed or there is none and the controller will be destroyed when the DropdownMenu is (this second case seems similar to usual states controllers which have the same life time than the widget they are attached to).
| _highlightedItemStatesController?.dispose(); | ||
| _highlightedItemStatesController = WidgetStatesController(<WidgetState>{ | ||
| WidgetState.focused, | ||
| }); |
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.
By "reusing it for different widgets" you just mean for each MenuItemButton that is created each time this method is called?
In the docs for statesController I see:
/// Classes based on this one can provide their own
/// [WidgetStatesController] to which they've added listeners.
/// They can also update the controller's [WidgetStatesController.value]
/// however, this may only be done when it's safe to call
/// [State.setState], like in an event handler.
It's not safe to call setState in the build phase, so I think you wouldn't be able to set the value back to focused here. So I think you are right, and you have to do it the way that you've done it here.
2ab74a8 to
e0c2f37
Compare
Roll Flutter from 13b2b91 to 01d37bc (74 revisions) flutter/flutter@13b2b91...01d37bc 2026-01-07 stuartmorgan@google.com Replace Hybrid Composition wiki page with dev-facing content (flutter/flutter#180642) 2026-01-07 116356835+AbdeMohlbi@users.noreply.github.com Improve code quality in `FlutterActivityTest.java` (flutter/flutter#180585) 2026-01-07 iinozemtsev@google.com Roll Dart SDK to 3.11.0-296.1.beta (flutter/flutter#180633) 2026-01-07 vegorov@google.com Bump target Windows version to 10 (flutter/flutter#180624) 2026-01-07 goderbauer@google.com Run hook_user_defines and link_hook integration tests on CI (flutter/flutter#180622) 2026-01-07 iliyazelenkog@gmail.com Fix division by zero in RenderTable intrinsic size methods (flutter/flutter#178217) 2026-01-07 116356835+AbdeMohlbi@users.noreply.github.com Remove more `requires 24` anotations (flutter/flutter#180116) 2026-01-07 engine-flutter-autoroll@skia.org Roll Skia from 3971dbb85d45 to c5359a4d720e (1 revision) (flutter/flutter#180631) 2026-01-07 huy@nevercode.io Fix TabBar.image does not render at initialIndex for the first time (flutter/flutter#179616) 2026-01-07 engine-flutter-autoroll@skia.org Roll Skia from 1e7ad625f6f7 to 3971dbb85d45 (3 revisions) (flutter/flutter#180627) 2026-01-07 goderbauer@google.com Unpin DDS (flutter/flutter#180571) 2026-01-07 bruno.leroux@gmail.com Fix DropdownMenuEntry.style not resolved when entry is highlighted (flutter/flutter#178873) 2026-01-07 116356835+AbdeMohlbi@users.noreply.github.com Remove unnecessary `@RequiresApi24` annotations from FlutterFragment methods (flutter/flutter#180117) 2026-01-07 engine-flutter-autoroll@skia.org Roll Skia from 7fc63228056b to 1e7ad625f6f7 (1 revision) (flutter/flutter#180616) 2026-01-07 engine-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from QfR2ZFZ5kGTD3raO3... to dTvN_JVSCfGFRasvH... (flutter/flutter#180612) 2026-01-07 bkonyi@google.com [ Widget Preview ] Add support for `dart:ffi` imports (flutter/flutter#180586) 2026-01-07 engine-flutter-autoroll@skia.org Roll Skia from eec90000a899 to 7fc63228056b (2 revisions) (flutter/flutter#180608) 2026-01-07 ulisses.hen@hotmail.com Add --web-define flag for template variable injection in Flutter web builds (flutter/flutter#175805) 2026-01-07 codefu@google.com docs(engine): update rbe notes (flutter/flutter#180599) 2026-01-06 engine-flutter-autoroll@skia.org Roll Skia from b6249496c230 to eec90000a899 (3 revisions) (flutter/flutter#180602) 2026-01-06 146823759+brahim-guaali@users.noreply.github.com Forward proxy 404 responses to client (flutter/flutter#179858) 2026-01-06 engine-flutter-autoroll@skia.org Roll Dart SDK from 40a8c6188f7f to d2e3ce177270 (1 revision) (flutter/flutter#180598) 2026-01-06 104009581+Saqib198@users.noreply.github.com Restore CLI precedence for web headers and HTTPS over web_dev_config.yaml (flutter/flutter#179639) 2026-01-06 engine-flutter-autoroll@skia.org Roll Skia from f5d9da13d56d to b6249496c230 (1 revision) (flutter/flutter#180596) 2026-01-06 dkwingsmt@users.noreply.github.com Enable misc leak tracking (flutter/flutter#176992) 2026-01-06 dacoharkes@google.com [hooks] Don't require NDK for Android targets (flutter/flutter#180594) 2026-01-06 kjlubick@users.noreply.github.com [skia] Disable legacy non-const SkData APIs (flutter/flutter#179684) 2026-01-06 48625061+muradhossin@users.noreply.github.com Fix/ios share context menu (flutter/flutter#176199) 2026-01-06 engine-flutter-autoroll@skia.org Roll Skia from b970aeffa66f to f5d9da13d56d (5 revisions) (flutter/flutter#180588) 2026-01-06 goderbauer@google.com Manually bump dependencies (flutter/flutter#180509) 2026-01-06 engine-flutter-autoroll@skia.org Roll Dart SDK from 8150be8a0e48 to 40a8c6188f7f (2 revisions) (flutter/flutter#180582) 2026-01-06 engine-flutter-autoroll@skia.org Roll Packages from 12eb764 to d3f860d (2 revisions) (flutter/flutter#180581) 2026-01-06 engine-flutter-autoroll@skia.org Roll Fuchsia Test Scripts from MHF-UAfO6sVKqSEYk... to nR2ESa1Gd8yPcWo06... (flutter/flutter#180578) 2026-01-06 sokolovskyi.konstantin@gmail.com Add tooltip support to PlatformMenuItem and PlatformMenu. (flutter/flutter#180069) 2026-01-06 bruno.leroux@gmail.com Add DropdownMenuFormField.errorBuilder (flutter/flutter#179345) 2026-01-06 engine-flutter-autoroll@skia.org Roll Skia from 1845397e11ba to b970aeffa66f (2 revisions) (flutter/flutter#180566) 2026-01-06 oss@simonbinder.eu Don't embed unreferenced assets (flutter/flutter#179251) 2026-01-06 116356835+AbdeMohlbi@users.noreply.github.com Improve documentation about ValueNotifier's behavior (flutter/flutter#179870) 2026-01-06 engine-flutter-autoroll@skia.org Roll Skia from 904ba00331ca to 1845397e11ba (5 revisions) (flutter/flutter#180558) 2026-01-06 engine-flutter-autoroll@skia.org Roll Dart SDK from 2fb9ad834c4d to 8150be8a0e48 (1 revision) (flutter/flutter#180557) 2026-01-06 engine-flutter-autoroll@skia.org Roll Skia from 98c042dde68c to 904ba00331ca (3 revisions) (flutter/flutter#180550) 2026-01-06 engine-flutter-autoroll@skia.org Roll Dart SDK from ba9f7f790966 to 2fb9ad834c4d (2 revisions) (flutter/flutter#180548) 2026-01-06 engine-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from ubBGcRaAKWKihQ4ac... to QfR2ZFZ5kGTD3raO3... (flutter/flutter#180547) 2026-01-06 ahmedsameha1@gmail.com Make sure that a DraggableScrollableSheet doesn't crash in 0x0 enviro… (flutter/flutter#180433) 2026-01-06 ahmedsameha1@gmail.com Make sure that a ColorFiltered doesn't crash 0x0 environment (flutter/flutter#180307) 2026-01-06 ahmedsameha1@gmail.com Make sure that a FadeInImage doesn't crash in 0x0 environment (flutter/flutter#180495) ...
Description
This PR adds logic to resolve
DropdownMenuEntry.styleproperties with the correctWidgetStatewhen an item is highlighted.When
MenuItemButtonare highlighted the focused state is not added automatically because the focus does not move to the items (it stays on theTextFieldin order to let the user enters text to filter the items list). This PR sets theMenuItemButton.statesControllerwith a forced focused state to letMenuItemButtonknow that the focused state should be use to resolve the properties.Related Issue
Fixes DropdownMenuEntry style's text style is not resolving with states
Tests