Allow arrow keys to navigate `MenuAnchor` independently of global shortcut definition #155728

dkwingsmt · 2024-09-26T00:11:59Z

This PR adjusts the implementation of handling navigational shortcuts (i.e. arrow keys) on MenuAnchor and DropdownMenu.

Motivation

The direct outcome of this PR is to allow keyboard to enter submenus on Web: When the focus is on a MenuAnchor while the menu is open, pressing arrow keys should move the focus to the menu item.

Before the PR, this works for all platforms but Web, a problem described in Allow MenuAnchor created menus to traverse the submenu via keyboard. #119532 (comment).

It is caused by the fact that MenuAnchor does not wrap itself with a Shortcuts, and therefore key events when the focus is on a MenuAnchor has been working only because the event falls back to the Shortcuts widget defined by WidgetsApp, whose default value happens to satisfy MenuAnchor's needs - except on Web where arrow keys are defined to scroll instead of traverse.

Instead of defining this problem as "just a patch for Web", I think it's better to define it as a problem of all platforms: MenuAnchor's shortcuts should be independent of WidgetsApp.shortcuts. Because even if WidgetsApp.shortcuts is redefined as something else, people should probably still expect arrow keys to work on MenuAnchor.

Therefore this PR makes MenuAnchor produce a Shortcuts by itself.

Dropdown menu

The fix above breaks DropdownMenu. DropdownMenu uses MenuAnchor, while defining its own shortcuts because, when filter is enabled:

The left and right arrow keys need to move the text carets instead
The up and down arrow keys need to "fake" directional navigation - the focus needs to stay on the text field, while some menu item is highlighted as if it is focused.

Before the PR, DropdownMenu defines these shortcuts out of MenuAnchor. In order for the DropdownMenu's shortcuts to take priority, these shortcuts are moved to between MenuAnchor and the Textfield.

A test is added to verify that the left/right keys move text carets.

Below are psuedo-widget-trees after the PR:

MenuAnchor
 |- Shortcuts(arrows->DirectionalFocusIntent)
    |- MenuAnchor.child
    |- menu

DropdownMenu
 |- Actions(DirectionalFocusIntent->_dropdownMenuNavigation)
    |- MenuAnchor
        |- Shortcuts(arrows->DirectionalFocusIntent)
            |- Shortcuts(leftright->ExtendSelectionByCharacterIntent, updown->_dropdownMenuArrowIntent)
            |  |- TextField
            |     |- EditableText
            |        |- Actions(DirectionalFocusIntent->DirectionalFocusAction.forTextField)
            |- menu

Known issues

After this PR, traversing the menu still have quite a few problems, which are left for other PRs.

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.

dkwingsmt · 2024-09-26T07:12:49Z

cc @davidhicks980

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

…-key-web

gspencergoog

bleroux

Looking at the test failures, the problem is that DropdownMenu reuses MenuAnchor and defines its own Shortcuts which are superceeded by the Shortcuts added by this PR.

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

Lines 972 to 985 in 7bb9352

    
           return Shortcuts( 
        
             shortcuts: _kMenuTraversalShortcuts, 
        
             child: Actions( 
        
               actions: <Type, Action<Intent>>{ 
        
                 _ArrowUpIntent: CallbackAction<_ArrowUpIntent>( 
        
                   onInvoke: handleUpKeyInvoke, 
        
                 ), 
        
                 _ArrowDownIntent: CallbackAction<_ArrowDownIntent>( 
        
                   onInvoke: handleDownKeyInvoke, 
        
                 ), 
        
               }, 
        
               child: menuAnchor, 
        
             ), 
        
           );

bleroux · 2024-09-27T05:53:16Z

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

-      child: child,
+    // This `Shortcuts` is needed so that shortcuts work when the focus is on
+    // MenuAnchor (specifically, the root menu, since submenus have their own
+    // `Shortcuts`.)


Suggested change

// `Shortcuts`.)

// `Shortcuts`).

In french it would be that way, not sure if it is the same in english. 😅

Period outside the parenthesis is correct in English too 👍

justinmc

Good catch by @bleroux on the Dropdown thing. I'm not sure if it's helpful here but things like Action.overridable also exist.

justinmc · 2024-09-27T23:05:16Z

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

-      child: child,
+    // This `Shortcuts` is needed so that shortcuts work when the focus is on
+    // MenuAnchor (specifically, the root menu, since submenus have their own
+    // `Shortcuts`.)


Period outside the parenthesis is correct in English too 👍

dkwingsmt · 2024-09-27T23:10:52Z

Yeah I was unsure how to fix the drop down menu, both to make it functional and semantically correct. Thank you for letting me know overridable. I'll take a look soon.

packages/flutter/test/material/menu_anchor_test.dart

…obal shortcut definition (flutter/flutter#155728)

Manual roll requested by [email protected] flutter/flutter@ec2e12b...0917e9d 2024-10-05 [email protected] Roll Flutter Engine from 2068577e510b to 01de9dcf6bbf (1 revision) (flutter/flutter#156274) 2024-10-05 [email protected] Roll Flutter Engine from 6d6bc39fe8b1 to 2068577e510b (1 revision) (flutter/flutter#156267) 2024-10-05 [email protected] Roll Flutter Engine from 92b5b318190b to 6d6bc39fe8b1 (4 revisions) (flutter/flutter#156266) 2024-10-05 [email protected] Roll Flutter Engine from fd6082ab6693 to 92b5b318190b (1 revision) (flutter/flutter#156261) 2024-10-05 [email protected] Roll Flutter Engine from bc5f9fc80b06 to fd6082ab6693 (4 revisions) (flutter/flutter#156258) 2024-10-04 [email protected] Roll Flutter Engine from eece6c3ec63a to bc5f9fc80b06 (2 revisions) (flutter/flutter#156252) 2024-10-04 [email protected] Roll pub packages (flutter/flutter#156254) 2024-10-04 [email protected] Roll Flutter Engine from 9fa363bdde20 to eece6c3ec63a (1 revision) (flutter/flutter#156250) 2024-10-04 [email protected] Roll Flutter Engine from d38f5e560a98 to 9fa363bdde20 (1 revision) (flutter/flutter#156249) 2024-10-04 [email protected] remove bringup from Windows tool_integration_tests_* (flutter/flutter#156179) 2024-10-04 [email protected] Roll Flutter Engine from 205484009711 to d38f5e560a98 (4 revisions) (flutter/flutter#156239) 2024-10-04 [email protected] Normalize `ThemeData.cardTheme` (flutter/flutter#153254) 2024-10-04 [email protected] Roll Packages from 429650f to 05bf1d4 (5 revisions) (flutter/flutter#156225) 2024-10-04 [email protected] Roll Flutter Engine from e0109f0fd5b2 to 205484009711 (1 revision) (flutter/flutter#156216) 2024-10-04 [email protected] Roll pub packages (flutter/flutter#156215) 2024-10-04 [email protected] Roll Flutter Engine from c2a9fb051bd0 to e0109f0fd5b2 (1 revision) (flutter/flutter#156213) 2024-10-04 [email protected] Roll Flutter Engine from 66d397dff87a to c2a9fb051bd0 (5 revisions) (flutter/flutter#156206) 2024-10-04 [email protected] Roll Flutter Engine from 20369c5d2b93 to 66d397dff87a (5 revisions) (flutter/flutter#156188) 2024-10-03 [email protected] Roll Flutter Engine from de1762dbc5cc to 20369c5d2b93 (7 revisions) (flutter/flutter#156174) 2024-10-03 [email protected] Refactor devfs_web_ddc_modules_test.dart (flutter/flutter#155609) 2024-10-03 [email protected] Allow arrow keys to navigate `MenuAnchor` independently of global shortcut definition (flutter/flutter#155728) 2024-10-03 [email protected] Inherited Theme: zero rebuilds (flutter/flutter#155699) 2024-10-03 [email protected] Roll Flutter Engine from 247bc68c578e to de1762dbc5cc (4 revisions) (flutter/flutter#156160) 2024-10-03 [email protected] Roll pub packages (flutter/flutter#156159) 2024-10-03 [email protected] pattern-matching refactor (flutter/flutter#154753) 2024-10-03 [email protected] Add autocorrect and enableSuggestions to SearchDelegate (flutter/flutter#154932) 2024-10-03 [email protected] Roll Packages from 7c97c88 to 429650f (6 revisions) (flutter/flutter#156155) 2024-10-03 [email protected] Move some tools test ownership to Ben (flutter/flutter#156123) 2024-10-03 [email protected] further shard the Windows tool_integration_tests* targets (flutter/flutter#156121) 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

…obal shortcut definition (flutter/flutter#155728)

Impl

7e75e19

github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. labels Sep 26, 2024

Add test

8ecade8

dkwingsmt marked this pull request as ready for review September 26, 2024 07:02

dkwingsmt added 2 commits September 26, 2024 00:10

Remove a comment

aece1a9

Improve comment

d5f28dd

dkwingsmt requested review from TahaTesser, gspencergoog and justinmc September 26, 2024 07:13

TahaTesser requested a review from bleroux September 26, 2024 09:07

Merge branch 'master' into menu-key-web

4206ab0

gspencergoog reviewed Sep 26, 2024

View reviewed changes

packages/flutter/lib/src/material/menu_anchor.dart Outdated Show resolved Hide resolved

dkwingsmt added 2 commits September 26, 2024 12:36

Analyse

8f243c7

Merge branch 'menu-key-web' of github.com:dkwingsmt/flutter into menu…

6c1ec0e

…-key-web

gspencergoog approved these changes Sep 26, 2024

View reviewed changes

bleroux reviewed Sep 27, 2024

View reviewed changes

justinmc reviewed Sep 27, 2024

View reviewed changes

dkwingsmt added 2 commits September 30, 2024 20:47

Merge remote-tracking branch 'origin/main' into menu-key-web

ccb4e05

Seems to work?

62eaffa

github-actions bot added the a: text input Entering text in a text field or keyboard related problems label Oct 1, 2024

dkwingsmt added 4 commits October 1, 2024 11:13

Add left/right test

97e4af9

Remove useless _ArrowUp/DownIntent

54d0f50

Revert back to only testing desktop (shouldn't need to)

e1f79c6

Seems to all work?

506efc7

github-actions bot removed the a: text input Entering text in a text field or keyboard related problems label Oct 1, 2024

dkwingsmt changed the title ~~[MenuAnchor] Allow keyboard to enter submenu on Web~~ Adjust navigational shortcut handling of MenuAnchor and DropdownMenu Oct 1, 2024

dkwingsmt changed the title ~~Adjust navigational shortcut handling of MenuAnchor and DropdownMenu~~ Allow arrow keys to navigate MenuAnchor independently of global shortcut definition Oct 1, 2024

davidhicks980 reviewed Oct 2, 2024

View reviewed changes

packages/flutter/test/material/menu_anchor_test.dart Show resolved Hide resolved

davidhicks980 reviewed Oct 2, 2024

View reviewed changes

packages/flutter/test/material/menu_anchor_test.dart Outdated Show resolved Hide resolved

davidhicks980 reviewed Oct 2, 2024

View reviewed changes

packages/flutter/test/material/menu_anchor_test.dart Outdated Show resolved Hide resolved

dkwingsmt added 4 commits October 2, 2024 10:10

Merge remote-tracking branch 'origin/main' into menu-key-web

ebc1894

Simplify test

83dae77

Merge remote-tracking branch 'origin/main' into menu-key-web

f75c0c3

Inline custom button

93933fb

dkwingsmt added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 3, 2024

auto-submit bot merged commit 59e5743 into flutter:master Oct 3, 2024

engine-flutter-autoroll mentioned this pull request Oct 4, 2024

Roll Flutter from fa402c8057a1 to 195f81e025de (122 revisions) flutter/packages#7789

Closed

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

59e5743 Allow arrow keys to navigate MenuAnchor independently of gl…

3243486

…obal shortcut definition (flutter/flutter#155728)

engine-flutter-autoroll mentioned this pull request Oct 4, 2024

Roll Flutter from fa402c8057a1 to 85abc1a3a469 (123 revisions) flutter/packages#7791

Closed

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

59e5743 Allow arrow keys to navigate MenuAnchor independently of gl…

2019011

…obal shortcut definition (flutter/flutter#155728)

engine-flutter-autoroll mentioned this pull request Oct 4, 2024

Roll Flutter from fa402c8057a1 to 85abc1a3a469 (123 revisions) flutter/packages#7793

Closed

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

59e5743 Allow arrow keys to navigate MenuAnchor independently of gl…

fc00a02

…obal shortcut definition (flutter/flutter#155728)

Piinks mentioned this pull request Oct 4, 2024

Flutter Framework Team: Notable Commits #121415

Closed

engine-flutter-autoroll mentioned this pull request Oct 5, 2024

Roll Flutter from fa402c8057a1 to 0917e9dfdce0 (135 revisions) flutter/packages#7797

Closed

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

59e5743 Allow arrow keys to navigate MenuAnchor independently of gl…

1231d9c

…obal shortcut definition (flutter/flutter#155728)

engine-flutter-autoroll mentioned this pull request Oct 5, 2024

Roll Flutter from fa402c8057a1 to 0917e9dfdce0 (135 revisions) flutter/packages#7798

Closed

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

59e5743 Allow arrow keys to navigate MenuAnchor independently of gl…

4639331

…obal shortcut definition (flutter/flutter#155728)

engine-flutter-autoroll mentioned this pull request Oct 6, 2024

Roll Flutter from fa402c8057a1 to bae797143174 (138 revisions) flutter/packages#7799

Closed

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

59e5743 Allow arrow keys to navigate MenuAnchor independently of gl…

0e4b458

…obal shortcut definition (flutter/flutter#155728)

engine-flutter-autoroll mentioned this pull request Oct 6, 2024

Roll Flutter from fa402c8057a1 to bae797143174 (138 revisions) flutter/packages#7800

Closed

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

59e5743 Allow arrow keys to navigate MenuAnchor independently of gl…

68f9793

…obal shortcut definition (flutter/flutter#155728)

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

Manual roll Flutter from ec2e12ba5099 to 0917e9dfdce0 (29 revisions) flutter/packages#7816

Merged

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

59e5743 Allow arrow keys to navigate MenuAnchor independently of gl…

f5857eb

…obal shortcut definition (flutter/flutter#155728)

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

59e5743 Allow arrow keys to navigate MenuAnchor independently of gl…

de056da

…obal shortcut definition (flutter/flutter#155728)

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

59e5743 Allow arrow keys to navigate MenuAnchor independently of gl…

98ec4a8

…obal shortcut definition (flutter/flutter#155728)

huycozy mentioned this pull request Apr 21, 2025

DropdownMenu up/down arrow keys don't work right when enableSearch is disabled #151855

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Allow arrow keys to navigate `MenuAnchor` independently of global shortcut definition #155728

Allow arrow keys to navigate `MenuAnchor` independently of global shortcut definition #155728

Uh oh!

dkwingsmt commented Sep 26, 2024 •

edited

Loading

Uh oh!

dkwingsmt commented Sep 26, 2024

Uh oh!

Uh oh!

gspencergoog left a comment

Uh oh!

bleroux left a comment

Uh oh!

bleroux Sep 27, 2024 •

edited

Loading

Uh oh!

justinmc Sep 27, 2024

Uh oh!

justinmc left a comment

Uh oh!

justinmc Sep 27, 2024

Uh oh!

dkwingsmt commented Sep 27, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

	return Shortcuts(
	shortcuts: _kMenuTraversalShortcuts,
	child: Actions(
	actions: <Type, Action<Intent>>{
	_ArrowUpIntent: CallbackAction<_ArrowUpIntent>(
	onInvoke: handleUpKeyInvoke,
	),
	_ArrowDownIntent: CallbackAction<_ArrowDownIntent>(
	onInvoke: handleDownKeyInvoke,
	),
	},
	child: menuAnchor,
	),
	);

Allow arrow keys to navigate MenuAnchor independently of global shortcut definition #155728

Allow arrow keys to navigate MenuAnchor independently of global shortcut definition #155728

Uh oh!

Conversation

dkwingsmt commented Sep 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Dropdown menu

Known issues

Pre-launch Checklist

Uh oh!

dkwingsmt commented Sep 26, 2024

Uh oh!

Uh oh!

gspencergoog left a comment

Choose a reason for hiding this comment

Uh oh!

bleroux left a comment

Choose a reason for hiding this comment

Uh oh!

bleroux Sep 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justinmc Sep 27, 2024

Choose a reason for hiding this comment

Uh oh!

justinmc left a comment

Choose a reason for hiding this comment

Uh oh!

justinmc Sep 27, 2024

Choose a reason for hiding this comment

Uh oh!

dkwingsmt commented Sep 27, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Allow arrow keys to navigate `MenuAnchor` independently of global shortcut definition #155728

Allow arrow keys to navigate `MenuAnchor` independently of global shortcut definition #155728

dkwingsmt commented Sep 26, 2024 •

edited

Loading

bleroux Sep 27, 2024 •

edited

Loading