feat: Add autofocus for MenuItemButton
#139396
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie or stuartmorgan on the #hackers channel in Chat (don't just cc them here, they won't see it! Use Discord!). If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
github-actions
bot
added
framework
|
Thanks for the PR! In order to review of this PR, however, we'll need you to add some tests, as the bot message describes above. |
|
This pull request executed golden file tests, but it has not been updated in a while (20+ days). Test results from Gold expire after as many days, so this pull request will need to be updated with a fresh commit in order to get results from Gold. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
|
Hi @Fernthedev would you like to continue working on this PR and add some tests? |
Hi, yes! I apologize for my absence since I've been very busy and forgot about this. I don't have any experience with test development in Flutter so I'm not sure how to do so. If you're offering to add them yourself or take over the PR, I endorse it. If not I'll try to do it this Friday as I think I'll have time by then. Thanks for your interest! |
Fernthedev
force-pushed
the
feat/menu-item-button/autofocus
branch
from
a14169e
to
02bd50f
Compare
Fernthedev
force-pushed
the
feat/menu-item-button/autofocus
branch
from
02bd50f
to
9a1c37a
Compare
|
Hi @Piinks (hope you don't mind the ping) I have added the tests as requested. However, I'd like to note that during testing, I noticed that I hope the test adequately conforms to the Flutter repo's requirements but in the case it does not feel free to ping me. |
There was a problem hiding this comment.
Thanks for adding tests! I'll defer to @gspencergoog for this, I don't know if the focus issue you mentioned in the menu is intentional or a separate issue.
There was a problem hiding this comment.
Hi @Fernthedev, this came PR came up in PR triage this week. Can you let us know that status of this change? It looks like you mentioned this is a partial fix, is there something missing here? Thanks!
Hey thanks! So basically when I originally wrote this I was listing 3 specific issues in the original issue. That's no longer the case, this PR addresses the issue specifically mentioned in the original GitHub post. Here's a video referencing the problem I tried to resolve with this. #139395 (comment) I apologize for the confusion, I wasn't thinking well when I wrote these posts. 😅 tl;dr this should resolve the problem where opening the menu would not select an item and therefore require extra navigation steps. Example being opening the menu with keyboard and attempt an action. |
There was a problem hiding this comment.
Thanks for the contribution! Could you provide some more details about the question that @Piinks asked?
|
Hi @Fernthedev would you like to continue working on this PR and address the above feedback? |
|
Hi, I left a response to a review left by @QuncCccccc. Did I miss something else you needed clarified? |
Co-authored-by: Qun Cheng <[email protected]>
|
Thanks for the quick fixes. |
QuncCccccc
added
the
autosubmit
Manual roll Flutter from e726eb4 to 15f95ce (48 revisions) Manual roll requested by [email protected] flutter/flutter@e726eb4...15f95ce 2024-06-28 [email protected] Roll Flutter Engine from ddd4814b9d40 to 94591ffb20df (5 revisions) (flutter/flutter#150968) 2024-06-27 [email protected] Manual engine roll to ddd4814 (flutter/flutter#150952) 2024-06-27 [email protected] local lint copy gradle task config (flutter/flutter#150957) 2024-06-27 [email protected] Roll Flutter Engine from b42c80460538 to d1506c12808e (3 revisions) (flutter/flutter#150951) 2024-06-27 [email protected] [tool] make the `systemTempDirectory` getter on `ErrorHandlingFileSystem` wrap the underlying filesystem's temp directory in a`ErrorHandlingDirectory` (flutter/flutter#150876) 2024-06-27 [email protected] Have flutter.js load local canvaskit instead of the CDN when appropriate (flutter/flutter#150806) 2024-06-27 [email protected] Roll Flutter Engine from a9194f0f01f4 to b42c80460538 (10 revisions) (flutter/flutter#150940) 2024-06-27 [email protected] [a11y] Reland [#149375 ] Update semantics in dropdown.dart (flutter/flutter#150578) 2024-06-27 [email protected] Bump dartdoc to 8.0.9+1 (flutter/flutter#150935) 2024-06-27 [email protected] add onFocus to text fields (flutter/flutter#150648) 2024-06-27 [email protected] Fixes `flutter build ipa` failure: Command line name "app-store" is deprecated. Use "app-store-connect" (flutter/flutter#150407) 2024-06-27 [email protected] Copy any previous `IconThemeData` instead of overwriting it in CupertinoButton (flutter/flutter#149777) 2024-06-27 [email protected] Improve the behavior of scrollbar drag-scrolls triggered by the trackpad (flutter/flutter#150275) 2024-06-27 [email protected] feat: Add autofocus for `MenuItemButton` (flutter/flutter#139396) 2024-06-27 [email protected] Roll Flutter Engine from 1d5e3cc55a75 to a9194f0f01f4 (7 revisions) (flutter/flutter#150888) 2024-06-26 [email protected] Reland "Remove dual_screen from new_gallery integration test" (flutter/flutter#150873) 2024-06-26 [email protected] Switch to more reliable flutter.dev link destinations in the tool (flutter/flutter#150587) 2024-06-26 [email protected] Adding `@docImport`s to the `animation` library (flutter/flutter#150798) 2024-06-26 [email protected] Remove CODEOWNERS trailing whitespace (flutter/flutter#150882) 2024-06-26 [email protected] Roll Flutter Engine from e03cf86c4170 to 1d5e3cc55a75 (3 revisions) (flutter/flutter#150875) 2024-06-26 [email protected] Remind folks we are moving. (flutter/flutter#150872) 2024-06-26 [email protected] Remove `bringup: true` from web test shard. (flutter/flutter#150785) 2024-06-26 [email protected] Roll Flutter Engine from c0017bed42c2 to e03cf86c4170 (1 revision) (flutter/flutter#150867) 2024-06-26 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Remove `dual_screen` from `new_gallery` integration test (#150808)" (flutter/flutter#150871) 2024-06-26 [email protected] Remove `dual_screen` from `new_gallery` integration test (flutter/flutter#150808) 2024-06-26 [email protected] Roll Flutter Engine from d4624a36712b to c0017bed42c2 (4 revisions) (flutter/flutter#150865) 2024-06-26 [email protected] Fixes for Style Guide for Flutter Repo (flutter/flutter#150167) 2024-06-26 [email protected] Roll Flutter Engine from da62c629ed5c to d4624a36712b (3 revisions) (flutter/flutter#150852) 2024-06-26 [email protected] Use `Isolate.packageConfigSync! to locate the packageconfig of flutter tools (flutter/flutter#150340) 2024-06-26 [email protected] Roll Flutter Engine from 25af762ffbb3 to da62c629ed5c (2 revisions) (flutter/flutter#150829) 2024-06-26 [email protected] Fix leaky tests. (flutter/flutter#150817) 2024-06-26 [email protected] Roll Flutter Engine from 94023d711db3 to 25af762ffbb3 (4 revisions) (flutter/flutter#150818) 2024-06-26 [email protected] Roll pub packages (flutter/flutter#150810) 2024-06-25 [email protected] Remove reference to `MaterialApp` and `showCupertinoModalPopup` from `CupertinoAlertDialog` (flutter/flutter#150725) 2024-06-25 [email protected] Read `AndroidManifest.xml` and emit `manifest-impeller-(enabled|disabled)` analytics (flutter/flutter#150791) 2024-06-25 [email protected] [flutter_tools] Shut down Chromium cleanly using a command sent through the debug protocol (flutter/flutter#150645) 2024-06-25 [email protected] Reland fix inputDecorator hint color on M3 (flutter/flutter#150278) 2024-06-25 [email protected] Roll Flutter Engine from 62e0b5f9c340 to 94023d711db3 (7 revisions) (flutter/flutter#150797) 2024-06-25 [email protected] Fix collapsed InputDecorator minimum height (flutter/flutter#150770) 2024-06-25 [email protected] Add more warm up frame docs (flutter/flutter#150464) 2024-06-25 [email protected] Roll pub packages (flutter/flutter#150739) 2024-06-25 [email protected] Add `focusNode`, `focusColor`, `onFocusChange`, `autofocus` to `CupertinoButton` (flutter/flutter#150721) 2024-06-25 [email protected] Document RenderObject._relayoutBoundary and its invariant; small refactors (flutter/flutter#150527) 2024-06-25 [email protected] Roll Flutter Engine from 6313b1e5afd7 to 62e0b5f9c340 (1 revision) (flutter/flutter#150790) ...
MenuAnchor for Material 3 is great for contextual menus but there are some minor issues related to accessibility. This PR aims to close the gap by adding `autofocus` to the menu item button so keyboard navigation is more intuitive. Otherwise, it becomes a mess to navigate through just keyboards. Partially resolves flutter#139395 ## Additional Notes I should mention, I have not written tests for this due to it's trivial nature. I also lack the experience of writing Flutter tests in general, so if someone feels inclined to take over this PR and add it they're welcome to.
MenuAnchor for Material 3 is great for contextual menus but there are some minor issues related to accessibility.
This PR aims to close the gap by adding
autofocusto the menu item button so keyboard navigation is more intuitive. Otherwise, it becomes a mess to navigate through just keyboards.Partially resolves #139395
Pre-launch Checklist
///).Additional Notes
I should mention, I have not written tests for this due to it's trivial nature. I also lack the experience of writing Flutter tests in general, so if someone feels inclined to take over this PR and add it they're welcome to.
If you need help, consider asking for advice on the #hackers-new channel on Discord.