The PopupMenuButton should not steal focus from the TextField when it appears.
#150568
Conversation
github-actions
bot
added
framework
|
|
||
| /// When the route state is updated, request focus if the current route is at the top. | ||
| /// | ||
| /// Defaults to true. |
There was a problem hiding this comment.
Can you mention Navigator.requestFocus here and explain how they relate?
There was a problem hiding this comment.
I made it default to Navigator.requestFocus, so it avoids needing two values to control whether to request focus.
| required this.clipBehavior, | ||
| super.settings, | ||
| this.popUpAnimationStyle, | ||
| this.requestFocus = false, |
There was a problem hiding this comment.
Are we sure that this is the behavior that most people want by default?
There was a problem hiding this comment.
Also, am I right that this parameter will never be passed in since it's private, and it will always be false?
There was a problem hiding this comment.
I've made changes here, now it defaults to the same behavior as before.
justinmc
left a comment
justinmc
left a comment
There was a problem hiding this comment.
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!
github-actions
bot
added
the
f: cupertino
|
Looking forward to this. Thank you |
justinmc
left a comment
justinmc
left a comment
There was a problem hiding this comment.
LGTM with a small formatting nit 👍
| Clip clipBehavior = Clip.none, | ||
| RouteSettings? routeSettings, | ||
| AnimationStyle? popUpAnimationStyle, | ||
| bool? requestFocus, |
There was a problem hiding this comment.
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)
…ld when it appears. (flutter/flutter#150568)
…ld when it appears. (flutter/flutter#150568)
…ld when it appears. (flutter/flutter#150568)
…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
|
Hello everyone! I somehow stumbled on this PR and I noticed this PR adds It makes it possible to remove some of the If anyone is interested in writing tests, please let me know. If not, I'd be happy to write those missing tests. |
|
@TahaTesser I'm sorry I didn't write enough tests. It would be great if you could write the tests. |
|
@TahaTesser Good catch, I missed that! |
…it appears. (flutter#150568) Fixes: flutter#24843 Fixes: flutter#50567
|
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)
…ld when it appears. (flutter/flutter#150568)
5 participants
Fixes: #24843
Fixes: #50567
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.