The `PopupMenuButton` should not steal focus from the TextField when it appears. by yiiim · Pull Request #150568 · flutter/flutter

yiiim · 2024-06-20T16:08:16Z

Fixes: #24843
Fixes: #50567

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.
I followed the breaking change policy and added Data Driven Fixes where supported.
All existing and new tests are passing.

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

justinmc

Thanks for the PR! I'm still trying to decide if this is the best approach here.

I wonder @chunhtai have you encountered this sort of thing before? Is there any other workaround?

packages/flutter/test/material/popup_menu_test.dart

justinmc · 2024-06-26T19:09:28Z

packages/flutter/lib/src/widgets/navigator.dart


+  /// When the route state is updated, request focus if the current route is at the top.
+  ///
+  /// Defaults to true.


Can you mention Navigator.requestFocus here and explain how they relate?

I made it default to Navigator.requestFocus, so it avoids needing two values to control whether to request focus.

justinmc · 2024-06-26T19:10:32Z

packages/flutter/lib/src/material/popup_menu.dart

    required this.clipBehavior,
    super.settings,
    this.popUpAnimationStyle,
+    this.requestFocus = false,


Are we sure that this is the behavior that most people want by default?

Also, am I right that this parameter will never be passed in since it's private, and it will always be false?

I've made changes here, now it defaults to the same behavior as before.

justinmc

Alright I briefly talked this over with @chunhtai and he doesn't see any reason not to take this approach. I'm on board as well; it makes sense that we should have a per-Route way to do what we already can do with Navigator.requestFocus.

Most of my comments are nits, except for one idea I had for putting this parameter on Route. I think it will clean up the code but I'd like your thoughts!

packages/flutter/lib/src/material/popup_menu.dart

packages/flutter/lib/src/widgets/navigator.dart

packages/flutter/lib/src/material/popup_menu.dart

saif-ellafi · 2024-07-27T17:35:13Z

Looking forward to this. Thank you

justinmc

LGTM with a small formatting nit 👍

packages/flutter/lib/src/widgets/navigator.dart

chunhtai

LGTM

chunhtai · 2024-08-07T21:44:06Z

packages/flutter/lib/src/material/popup_menu.dart

  Clip clipBehavior = Clip.none,
  RouteSettings? routeSettings,
  AnimationStyle? popUpAnimationStyle,
+  bool? requestFocus,


some where in the showMenu doc should mention the behavior of this parameter

Co-authored-by: Justin McCandless <[email protected]>

…ld when it appears. (flutter/flutter#150568)

Manual roll requested by [email protected] flutter/flutter@b6cd31e...76107bd 2024-08-08 [email protected] Re-enable dds for flutter drive tests that use DevTools (flutter/flutter#153129) 2024-08-08 [email protected] Roll Flutter Engine from ef820aa74f7a to 05208896830a (5 revisions) (flutter/flutter#153132) 2024-08-08 [email protected] [devicelab] opt all impeller tests to GPU tracing, opt some Android tests into merged thread mode. (flutter/flutter#153121) 2024-08-08 [email protected] Clean up .gitignore files (flutter/flutter#153060) 2024-08-08 [email protected] Roll Flutter Engine from 387f6f3c5fdb to ef820aa74f7a (4 revisions) (flutter/flutter#153124) 2024-08-08 [email protected] Shift Linux_android_emu tests from staging to prod (flutter/flutter#153110) 2024-08-08 [email protected] Move Android tests with macOS host from staging to prod (flutter/flutter#153113) 2024-08-08 [email protected] Remove -sdk for watchOS simulator in tool (flutter/flutter#152992) 2024-08-08 [email protected] Roll Flutter Engine from 3978ddd8d7a7 to 387f6f3c5fdb (3 revisions) (flutter/flutter#153111) 2024-08-08 [email protected] Roll Packages from 5cc0a01 to bb797b9 (5 revisions) (flutter/flutter#153107) 2024-08-08 [email protected] Roll pub packages [manual] (flutter/flutter#153066) 2024-08-08 [email protected] [web] Fix reading of the --local-web-sdk flag and remove the copy of useLocalWebSdk in DebuggingOptions (flutter/flutter#152642) 2024-08-08 [email protected] The `PopupMenuButton` should not steal focus from the TextField when it appears. (flutter/flutter#150568) 2024-08-08 [email protected] Fix `flutter build ipa --export-method` not accepting `enterprise` flag (flutter/flutter#153047) 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

TahaTesser · 2024-08-12T15:00:37Z

Hello everyone!

I somehow stumbled on this PR and I noticed this PR adds requestFocus parameter to classes/widgets other than related to PopupMenuButton such as bottom sheet, dialog, transition route, overlay route but doesn't test them.

It makes it possible to remove some of the requestFocus parameters from this PR without failing any tests in the framework.

If anyone is interested in writing tests, please let me know. If not, I'd be happy to write those missing tests.

yiiim · 2024-08-13T01:36:51Z

@TahaTesser I'm sorry I didn't write enough tests. It would be great if you could write the tests.

justinmc · 2024-08-15T17:32:47Z

@TahaTesser Good catch, I missed that!

…it appears. (flutter#150568) Fixes: flutter#24843 Fixes: flutter#50567

TahaTesser · 2024-08-23T16:22:59Z

Filed a PR with the missing tests #154005

…it appears. (flutter#150568) Fixes: flutter#24843 Fixes: flutter#50567

…ld when it appears. (flutter/flutter#150568)

github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. f: routes Navigator, Router, and related APIs. labels Jun 20, 2024

Piinks requested a review from justinmc June 20, 2024 20:06

justinmc reviewed Jun 26, 2024

View reviewed changes

yiiim requested a review from justinmc June 27, 2024 15:25

justinmc reviewed Jul 12, 2024

View reviewed changes

yiiim force-pushed the popupmenu_focus branch from 0e5b6ce to 3342fe1 Compare July 14, 2024 15:30

github-actions bot added the f: cupertino flutter/packages/flutter/cupertino repository label Jul 14, 2024

yiiim requested a review from justinmc July 14, 2024 15:39

route requestFocus

738e5c6

yiiim force-pushed the popupmenu_focus branch from 05f8cc6 to 738e5c6 Compare August 4, 2024 02:54

justinmc approved these changes Aug 7, 2024

View reviewed changes

packages/flutter/lib/src/widgets/navigator.dart Outdated Show resolved Hide resolved

chunhtai approved these changes Aug 7, 2024

View reviewed changes

yiiim and others added 2 commits August 8, 2024 22:14

Update packages/flutter/lib/src/widgets/navigator.dart

910a60a

Co-authored-by: Justin McCandless <[email protected]>

docs

202d572

yiiim added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 8, 2024

auto-submit bot merged commit 14cd5fa into flutter:master Aug 8, 2024

engine-flutter-autoroll mentioned this pull request Aug 8, 2024

Roll Flutter from d595e98d85ea to d23be7a07de3 (24 revisions) flutter/packages#7342

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 8, 2024

14cd5fa The PopupMenuButton should not steal focus from the TextFie…

ad0c97a

…ld when it appears. (flutter/flutter#150568)

engine-flutter-autoroll mentioned this pull request Aug 8, 2024

Roll Flutter from d595e98d85ea to a04ecb2b7234 (25 revisions) flutter/packages#7343

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 8, 2024

14cd5fa The PopupMenuButton should not steal focus from the TextFie…

2ad0a58

…ld when it appears. (flutter/flutter#150568)

engine-flutter-autoroll mentioned this pull request Aug 8, 2024

Roll Flutter from b6cd31e6b5a6 to d67f2cdebb7f (5 revisions) flutter/packages#7344

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 8, 2024

14cd5fa The PopupMenuButton should not steal focus from the TextFie…

5d04357

…ld when it appears. (flutter/flutter#150568)

engine-flutter-autoroll mentioned this pull request Aug 8, 2024

Roll Flutter from b6cd31e6b5a6 to e66ce1591f17 (6 revisions) flutter/packages#7345

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 8, 2024

14cd5fa The PopupMenuButton should not steal focus from the TextFie…

3df1901

…ld when it appears. (flutter/flutter#150568)

engine-flutter-autoroll mentioned this pull request Aug 9, 2024

Manual roll Flutter from b6cd31e6b5a6 to 76107bdf509f (14 revisions) flutter/packages#7349

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 9, 2024

14cd5fa The PopupMenuButton should not steal focus from the TextFie…

9d1e35a

…ld when it appears. (flutter/flutter#150568)

DBowen33 pushed a commit to DBowen33/flutter that referenced this pull request Aug 16, 2024

The PopupMenuButton should not steal focus from the TextField when …

375d372

…it appears. (flutter#150568) Fixes: flutter#24843 Fixes: flutter#50567

TahaTesser mentioned this pull request Aug 23, 2024

Add missing tests for requestFocus routes parameter #153999

Closed

Buchimi pushed a commit to Buchimi/flutter that referenced this pull request Sep 2, 2024

The PopupMenuButton should not steal focus from the TextField when …

9d4b091

…it appears. (flutter#150568) Fixes: flutter#24843 Fixes: flutter#50567

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 11, 2024

14cd5fa The PopupMenuButton should not steal focus from the TextFie…

1194061

…ld when it appears. (flutter/flutter#150568)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 12, 2024

14cd5fa The PopupMenuButton should not steal focus from the TextFie…

de43cc3

…ld when it appears. (flutter/flutter#150568)

Conversation

yiiim commented Jun 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

justinmc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

justinmc Jun 26, 2024

Choose a reason for hiding this comment

Uh oh!

yiiim Jun 27, 2024

Choose a reason for hiding this comment

Uh oh!

justinmc Jun 26, 2024

Choose a reason for hiding this comment

Uh oh!

justinmc Jun 26, 2024

Choose a reason for hiding this comment

Uh oh!

yiiim Jun 27, 2024

Choose a reason for hiding this comment

Uh oh!

justinmc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

saif-ellafi commented Jul 27, 2024

Uh oh!

justinmc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

chunhtai left a comment

Choose a reason for hiding this comment

Uh oh!

chunhtai Aug 7, 2024

Choose a reason for hiding this comment

Uh oh!

TahaTesser commented Aug 12, 2024

Uh oh!

yiiim commented Aug 13, 2024

Uh oh!

justinmc commented Aug 15, 2024

Uh oh!

TahaTesser commented Aug 23, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

yiiim commented Jun 20, 2024 •

edited

Loading