-
Notifications
You must be signed in to change notification settings - Fork 29.7k
Add DropdownMenu.focusNode
#142516
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
Add DropdownMenu.focusNode
#142516
Conversation
|
Still needs a test, see discussion in #142384. |
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.
canRequestFocus allows the text input field to be focused only on desktop unless DropdownMenu.requestFocusOnTap is set to true.
This can be handled two ways:
- A. focusable
FocusNodeoverrides therequestFocusOnTapvalue. (I added this) - Explicitly set
requestFocusOnTaptotruewhen providing aFocusNode.
cc: @QuncCccccc if you've any input here.
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.
Thanks for adding this! I think the first option makes more sense:)
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.
We should update the docs as well right? (To say which one wins.)
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.
Yep!
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.
pushed an update to the docs
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.
The analyzer is probably failing due to missing {@tool snippet}.
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.
And maybe we can create a {@macro ...} for this documentation. Seems it is the same as the TextField.focusNode:)
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.
In general I think @macro is a bit of a mistake, because it makes it hard to read the documentation inline, and makes it very easy for us to write incorrect documentation (e.g. when we're updating docs in a template, it's easy to add changes that don't apply to all the places that use that template). I'm currently in the middle of rewriting a bunch of documentation around text fields and the magnifier in particular that are suboptimal because of over-eager use of macros.
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 see. That makes sense:)
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.
oh, of course, i forgot to advance the animation to see the changed rendering. d'oh.
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.
Minor detail, easy to forget since a lot of tests simply use pumpAndSettle.
This and widget.focusNode!.canRequestFocus gets the expected test results for all target platforms.
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.
Yeah I regret ever creating pumpAndSettle. :-)
|
Thanks @TahaTesser! |
b92e7c4 to
aa6137d
Compare
aa6137d to
a75d246
Compare
|
@TahaTesser's changes LGTM, but someone will have to review my part of the changes before we can land it. :-) |
Taking a look! |
QuncCccccc
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:) Thanks for adding this.
|
Thanks! |
This comment was marked as outdated.
This comment was marked as outdated.
Manual roll requested by [email protected] flutter/flutter@75a2e5b...c65ab4d 2024-01-31 [email protected] Roll Flutter Engine from 2d2d88068a23 to 8e7df85f7d11 (2 revisions) (flutter/flutter#142583) 2024-01-31 [email protected] Add `DropdownMenu.focusNode` (flutter/flutter#142516) 2024-01-31 [email protected] Marks Windows_android native_assets_android to be unflaky (flutter/flutter#140042) 2024-01-31 [email protected] Marks Linux_pixel_7pro complex_layout_scroll_perf_impeller_gles__timeline_summary to be unflaky (flutter/flutter#140039) 2024-01-31 [email protected] Marks Linux_android native_assets_android to be unflaky (flutter/flutter#140040) 2024-01-31 [email protected] Marks Mac_android native_assets_android to be unflaky (flutter/flutter#140041) 2024-01-31 [email protected] Marks Mac flavors_test_macos to be unflaky (flutter/flutter#140647) 2024-01-31 [email protected] Roll Flutter Engine from 588f06b02664 to 2d2d88068a23 (1 revision) (flutter/flutter#142569) 2024-01-31 [email protected] Roll Flutter Engine from e6e1d6b3d035 to 588f06b02664 (1 revision) (flutter/flutter#142566) 2024-01-31 [email protected] M3 - Fix Chip icon and label colors (flutter/flutter#140573) 2024-01-31 [email protected] Roll Flutter Engine from d20ed240ace9 to e6e1d6b3d035 (3 revisions) (flutter/flutter#142564) 2024-01-30 [email protected] Be less verbose in CI for customer_testing, and revert old timeout hack (flutter/flutter#139611) 2024-01-30 [email protected] use PDI to end the isolated scope for RLI (flutter/flutter#141345) 2024-01-30 [email protected] Style correctness improvements for toStrings and related fixes (flutter/flutter#142485) 2024-01-30 [email protected] Roll Flutter Engine from 500ae6c4d947 to d20ed240ace9 (1 revision) (flutter/flutter#142554) 2024-01-30 49699333+dependabot[bot]@users.noreply.github.com Bump codecov/codecov-action from 3.1.5 to 3.1.6 (flutter/flutter#142553) 2024-01-30 [email protected] Roll Flutter Engine from cd12316a0883 to 500ae6c4d947 (1 revision) (flutter/flutter#142551) 2024-01-30 [email protected] Roll Flutter Engine from 0e4342c5a1f2 to cd12316a0883 (6 revisions) (flutter/flutter#142548) 2024-01-30 [email protected] Roll Flutter Engine from 438e9b4d7d4e to 0e4342c5a1f2 (11 revisions) (flutter/flutter#142543) 2024-01-30 [email protected] Fix: selection handles do not inherit color from local `Theme` widget (flutter/flutter#142476) 2024-01-30 [email protected] Marks Windows_arm64 plugin_test_windows to be unflaky (flutter/flutter#137675) 2024-01-30 [email protected] Revert "Roll Flutter Engine from 0e586d1c28c8 to f02a4a80a77e (3 revisions)" (flutter/flutter#142533) 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
DropdownMenudoesn't have a focusNodeCode sample
expand to view the code sample