-
Notifications
You must be signed in to change notification settings - Fork 29.7k
Fix text input actions in DropdownMenu. #177313
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 text input actions in DropdownMenu. #177313
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 effectively addresses an issue with text input actions in DropdownMenu. The core changes involve switching from onEditingComplete to onSubmitted for handling submissions, which is the correct approach for textInputAction to function as expected. Additionally, wrapping menu items in ExcludeFocus correctly resolves focus traversal issues, particularly for TextInputAction.previous. The new regression test is comprehensive and covers all TextInputAction variants, ensuring the fix is robust. The code is clean and the changes are logical. My only suggestion is to add documentation to the newly renamed private method to align with the repository's style guide.
| } | ||
|
|
||
| void _handleEditingComplete() { | ||
| void _handleSubmitted() { |
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.
This method should have a doc comment explaining what it does, as per the Flutter Style Guide for documenting even private members.1
/// Handles the submission of the text field.
///
/// If an item is highlighted in the menu, it selects that item. Otherwise,
/// it clears the selection. Finally, it closes the menu.
void _handleSubmitted() {Style Guide References
Footnotes
-
The style guide recommends using
///for public-quality documentation, even on private members. ↩
For reference, see #174096 which proposed to make the trailing icon not focusable by default. The correct way would probably be to wrap the ExcludeFocus(
// Do not exclude focus if a focus node is explicitly provided.
excluding: trailingIconFocusNode == null,
```
|
|
Hi @justinmc, could you please take a look at this PR when you have time? |
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 as long as the Google tests pass 👍 . Maybe @bleroux should take a look as well?
Thanks for making sure this works as expected for all the different TextInputActions.
| final ValueVariant<TextInputAction> focusVariants = ValueVariant<TextInputAction>( | ||
| TextInputAction.values.toSet(), | ||
| ); |
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 can't believe I never knew you could do this for stuff other than the platform. So many unnecessary for loops around my tests. TIL.
I had a look to this change during our internal review process. But I will discuss with Kostia tomorrow because I'm thinking of some behaviors we have to verify before merging this change. |
bleroux
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!
…10408) Manual roll requested by [email protected] flutter/flutter@31a8481...ee23168 2025-11-07 [email protected] Roll Packages from f13bad3 to 3caa48b (5 revisions) (flutter/flutter#178164) 2025-11-07 [email protected] Fix text input actions in DropdownMenu. (flutter/flutter#177313) 2025-11-07 [email protected] Roll Skia from f838c4b31edb to 581d1ecd5029 (1 revision) (flutter/flutter#178157) 2025-11-07 [email protected] Roll Skia from eb3c5b280ae6 to f838c4b31edb (3 revisions) (flutter/flutter#178149) 2025-11-07 [email protected] Roll Skia from 360fe72b5bf4 to eb3c5b280ae6 (1 revision) (flutter/flutter#178147) 2025-11-07 [email protected] Roll Skia from 116f237bb39d to 360fe72b5bf4 (4 revisions) (flutter/flutter#178146) 2025-11-07 [email protected] Roll Fuchsia Linux SDK from cm88aTLui5yorSGYQ... to qDVe2mrpSgQdxra7p... (flutter/flutter#178144) 2025-11-07 [email protected] fix: findChildIndexCallback to take seperators into account for seperated named constructor in ListView and SliverList (flutter/flutter#174491) 2025-11-06 [email protected] [web] Remove unnecessary android_sdk dep (flutter/flutter#178078) 2025-11-06 [email protected] Add haptic notifications support. (flutter/flutter#177721) 2025-11-06 [email protected] Allow label to be used to compute InputDecorator Intrinsic width (flutter/flutter#178101) 2025-11-06 [email protected] Respect product flavor abiFilters by adding a `disable-abi-filtering` Android project flag. (flutter/flutter#177753) 2025-11-06 [email protected] Use aria-hidden attribute for platform view accessibility on web (flutter/flutter#177969) 2025-11-06 [email protected] [tool] Fix IP parsing by using Uri constructor (flutter/flutter#178083) 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] 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
Fixes flutter#177009 ### Description - Moves `MenuButton` submit logic from `onEditingComplete` to `onSubmitted` to allow `TextField` to handle the `textInputAction` logic - Wraps each item into `ExcludeFocus` to enable proper `TextInputAction.previous` handling. If we don't wrap each child in `ExcludeFocus`, then focus will be moved to one of them, which is not the expected behavior for `TextInputAction.previous`. | BEFORE | AFTER | | - | - | | <video alt="before" src="https://github.com/user-attachments/assets/a50d41de-7e54-409b-bf81-80dfb1db132f" /> | <video alt="after" src="https://github.com/user-attachments/assets/152e47e6-d774-481c-8478-af526b5f6749" /> | <details closed><summary>Code sample</summary> ```dart import 'package:flutter/material.dart'; void main() { runApp(const DropdownMenuExample()); } class DropdownMenuExample extends StatefulWidget { const DropdownMenuExample({super.key}); @OverRide State<DropdownMenuExample> createState() => _DropdownMenuExampleState(); } class _DropdownMenuExampleState extends State<DropdownMenuExample> { final FocusNode _previousFocusNode = FocusNode(debugLabel: 'previous'); final FocusNode _textFieldFocusNode = FocusNode(debugLabel: 'textField'); final FocusNode _nextFocusNode = FocusNode(debugLabel: 'next'); @OverRide void dispose() { _previousFocusNode.dispose(); _textFieldFocusNode.dispose(); _nextFocusNode.dispose(); super.dispose(); } @OverRide Widget build(BuildContext context) { return MaterialApp( theme: ThemeData(colorSchemeSeed: Colors.green), home: Scaffold( body: Center( child: Column( mainAxisSize: MainAxisSize.min, crossAxisAlignment: CrossAxisAlignment.center, mainAxisAlignment: MainAxisAlignment.center, spacing: 20, children: [ TextField( focusNode: _previousFocusNode, textInputAction: TextInputAction.next, decoration: InputDecoration( border: OutlineInputBorder(), labelText: 'Previous TextField', ), ), DropdownMenu<String>( label: Text('Dropdown with filter cannot close keyboard'), initialSelection: 'green', focusNode: _textFieldFocusNode, requestFocusOnTap: true, showTrailingIcon: false, textInputAction: TextInputAction.next, onSelected: (String? color) { print('SELECTED $color'); }, dropdownMenuEntries: [ DropdownMenuEntry(value: 'red', label: 'red'), DropdownMenuEntry(value: 'green', label: 'green'), DropdownMenuEntry(value: 'blue', label: 'blue'), ], ), TextField( focusNode: _nextFocusNode, textInputAction: TextInputAction.done, decoration: InputDecoration( border: OutlineInputBorder(), labelText: 'Next TextField', ), ), ], ), ), ), ); } } ``` </details> There is still one behavior I would like to discuss. When the `showTrailingIcon: true` and `textInputAction: TextInputAction.previous` are used, the focus moves not to the previous field but to the `IconButton`. If we wrap the `IconButton` with `ExcludeFocus`, then this is fixed, but I am not sure whether this is the correct way to proceed. | TextInputAction.previous and no ExcludeFocus on IconButton | TextInputAction.previous and ExcludeFocus on IconButton | | - | - | | <video alt="before" src="https://github.com/user-attachments/assets/76c90dcf-3ea1-492f-8e67-7e987b08c2ff" /> | <video alt="after" src="https://github.com/user-attachments/assets/2a1600a2-e308-430e-a12f-acdc06cbf81c" /> | ## Pre-launch Checklist - [X] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [X] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [X] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [X] I signed the [CLA]. - [X] I listed at least one issue that this PR fixes in the description above. - [ ] I updated/added relevant documentation (doc comments with `///`). - [X] I added new tests to check the change I am making, or this PR is [test-exempt]. - [X] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [X] All existing and new tests are passing. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
Fixes flutter#177009 ### Description - Moves `MenuButton` submit logic from `onEditingComplete` to `onSubmitted` to allow `TextField` to handle the `textInputAction` logic - Wraps each item into `ExcludeFocus` to enable proper `TextInputAction.previous` handling. If we don't wrap each child in `ExcludeFocus`, then focus will be moved to one of them, which is not the expected behavior for `TextInputAction.previous`. | BEFORE | AFTER | | - | - | | <video alt="before" src="https://github.com/user-attachments/assets/a50d41de-7e54-409b-bf81-80dfb1db132f" /> | <video alt="after" src="https://github.com/user-attachments/assets/152e47e6-d774-481c-8478-af526b5f6749" /> | <details closed><summary>Code sample</summary> ```dart import 'package:flutter/material.dart'; void main() { runApp(const DropdownMenuExample()); } class DropdownMenuExample extends StatefulWidget { const DropdownMenuExample({super.key}); @OverRide State<DropdownMenuExample> createState() => _DropdownMenuExampleState(); } class _DropdownMenuExampleState extends State<DropdownMenuExample> { final FocusNode _previousFocusNode = FocusNode(debugLabel: 'previous'); final FocusNode _textFieldFocusNode = FocusNode(debugLabel: 'textField'); final FocusNode _nextFocusNode = FocusNode(debugLabel: 'next'); @OverRide void dispose() { _previousFocusNode.dispose(); _textFieldFocusNode.dispose(); _nextFocusNode.dispose(); super.dispose(); } @OverRide Widget build(BuildContext context) { return MaterialApp( theme: ThemeData(colorSchemeSeed: Colors.green), home: Scaffold( body: Center( child: Column( mainAxisSize: MainAxisSize.min, crossAxisAlignment: CrossAxisAlignment.center, mainAxisAlignment: MainAxisAlignment.center, spacing: 20, children: [ TextField( focusNode: _previousFocusNode, textInputAction: TextInputAction.next, decoration: InputDecoration( border: OutlineInputBorder(), labelText: 'Previous TextField', ), ), DropdownMenu<String>( label: Text('Dropdown with filter cannot close keyboard'), initialSelection: 'green', focusNode: _textFieldFocusNode, requestFocusOnTap: true, showTrailingIcon: false, textInputAction: TextInputAction.next, onSelected: (String? color) { print('SELECTED $color'); }, dropdownMenuEntries: [ DropdownMenuEntry(value: 'red', label: 'red'), DropdownMenuEntry(value: 'green', label: 'green'), DropdownMenuEntry(value: 'blue', label: 'blue'), ], ), TextField( focusNode: _nextFocusNode, textInputAction: TextInputAction.done, decoration: InputDecoration( border: OutlineInputBorder(), labelText: 'Next TextField', ), ), ], ), ), ), ); } } ``` </details> There is still one behavior I would like to discuss. When the `showTrailingIcon: true` and `textInputAction: TextInputAction.previous` are used, the focus moves not to the previous field but to the `IconButton`. If we wrap the `IconButton` with `ExcludeFocus`, then this is fixed, but I am not sure whether this is the correct way to proceed. | TextInputAction.previous and no ExcludeFocus on IconButton | TextInputAction.previous and ExcludeFocus on IconButton | | - | - | | <video alt="before" src="https://github.com/user-attachments/assets/76c90dcf-3ea1-492f-8e67-7e987b08c2ff" /> | <video alt="after" src="https://github.com/user-attachments/assets/2a1600a2-e308-430e-a12f-acdc06cbf81c" /> | ## Pre-launch Checklist - [X] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [X] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [X] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [X] I signed the [CLA]. - [X] I listed at least one issue that this PR fixes in the description above. - [ ] I updated/added relevant documentation (doc comments with `///`). - [X] I added new tests to check the change I am making, or this PR is [test-exempt]. - [X] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [X] All existing and new tests are passing. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
Fixes #177009
Description
MenuButtonsubmit logic fromonEditingCompletetoonSubmittedto allowTextFieldto handle thetextInputActionlogicExcludeFocusto enable properTextInputAction.previoushandling. If we don't wrap each child inExcludeFocus, then focus will be moved to one of them, which is not the expected behavior forTextInputAction.previous.before.mov
after.mov
Code sample
There is still one behavior I would like to discuss. When the
showTrailingIcon: trueandtextInputAction: TextInputAction.previousare used, the focus moves not to the previous field but to theIconButton. If we wrap theIconButtonwithExcludeFocus, then this is fixed, but I am not sure whether this is the correct way to proceed.no_exclude.mov
with_exclude.mov
Pre-launch Checklist
///).