Skip to content

Commit 59e5743

Browse files
dkwingsmtdkwingsmt
authored
Allow arrow keys to navigate MenuAnchor independently of global shortcut definition (#155728)
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 flutter/flutter#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.
1 parent 500285d commit 59e5743

File tree

4 files changed

+149
-32
lines changed

4 files changed

+149
-32
lines changed

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

Lines changed: 33 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -43,12 +43,6 @@ typedef FilterCallback<T> = List<DropdownMenuEntry<T>> Function(List<DropdownMen
4343
/// Used by [DropdownMenu.searchCallback].
4444
typedef SearchCallback<T> = int? Function(List<DropdownMenuEntry<T>> entries, String query);
4545

46-
// Navigation shortcuts to move the selected menu items up or down.
47-
final Map<ShortcutActivator, Intent> _kMenuTraversalShortcuts = <ShortcutActivator, Intent> {
48-
LogicalKeySet(LogicalKeyboardKey.arrowUp): const _ArrowUpIntent(),
49-
LogicalKeySet(LogicalKeyboardKey.arrowDown): const _ArrowDownIntent(),
50-
};
51-
5246
const double _kMinimumWidth = 112.0;
5347

5448
const double _kDefaultHorizontalPadding = 12.0;
@@ -944,14 +938,22 @@ class _DropdownMenuState<T> extends State<DropdownMenu<T>> {
944938
return textField;
945939
}
946940

947-
return _DropdownMenuBody(
948-
width: widget.width,
949-
children: <Widget>[
950-
textField,
951-
..._initialMenu!.map((Widget item) => ExcludeFocus(excluding: !controller.isOpen, child: item)),
952-
trailingButton,
953-
leadingButton,
954-
],
941+
return Shortcuts(
942+
shortcuts: const <ShortcutActivator, Intent>{
943+
SingleActivator(LogicalKeyboardKey.arrowLeft): ExtendSelectionByCharacterIntent(forward: false, collapseSelection: true),
944+
SingleActivator(LogicalKeyboardKey.arrowRight): ExtendSelectionByCharacterIntent(forward: true, collapseSelection: true),
945+
SingleActivator(LogicalKeyboardKey.arrowUp): _ArrowUpIntent(),
946+
SingleActivator(LogicalKeyboardKey.arrowDown): _ArrowDownIntent(),
947+
},
948+
child: _DropdownMenuBody(
949+
width: widget.width,
950+
children: <Widget>[
951+
textField,
952+
..._initialMenu!.map((Widget item) => ExcludeFocus(excluding: !controller.isOpen, child: item)),
953+
trailingButton,
954+
leadingButton,
955+
],
956+
),
955957
);
956958
},
957959
);
@@ -977,23 +979,27 @@ class _DropdownMenuState<T> extends State<DropdownMenu<T>> {
977979
);
978980
}
979981

980-
return Shortcuts(
981-
shortcuts: _kMenuTraversalShortcuts,
982-
child: Actions(
983-
actions: <Type, Action<Intent>>{
984-
_ArrowUpIntent: CallbackAction<_ArrowUpIntent>(
985-
onInvoke: handleUpKeyInvoke,
986-
),
987-
_ArrowDownIntent: CallbackAction<_ArrowDownIntent>(
988-
onInvoke: handleDownKeyInvoke,
989-
),
990-
},
991-
child: menuAnchor,
992-
),
982+
return Actions(
983+
actions: <Type, Action<Intent>>{
984+
_ArrowUpIntent: CallbackAction<_ArrowUpIntent>(
985+
onInvoke: handleUpKeyInvoke,
986+
),
987+
_ArrowDownIntent: CallbackAction<_ArrowDownIntent>(
988+
onInvoke: handleDownKeyInvoke,
989+
),
990+
},
991+
child: menuAnchor,
993992
);
994993
}
995994
}
996995

996+
997+
// `DropdownMenu` dispatches these private intents on arrow up/down keys.
998+
// They are needed instead of the typical `DirectionalFocusIntent`s because
999+
// `DropdownMenu` does not really navigate the focus tree upon arrow up/down
1000+
// keys: the focus stays on the text field and the menu items are given fake
1001+
// highlights as if they are focused. Using `DirectionalFocusIntent`s will cause
1002+
// the action to be processed by `EditableText`.
9971003
class _ArrowUpIntent extends Intent {
9981004
const _ArrowUpIntent();
9991005
}

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

Lines changed: 16 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -432,11 +432,22 @@ class _MenuAnchorState extends State<MenuAnchor> {
432432
);
433433
}
434434

435-
return _MenuAnchorScope(
436-
anchorKey: _anchorKey,
437-
anchor: this,
438-
isOpen: _isOpen,
439-
child: child,
435+
// This `Shortcuts` is needed so that shortcuts work when the focus is on
436+
// MenuAnchor (specifically, the root menu, since submenus have their own
437+
// `Shortcuts`).
438+
return
439+
Shortcuts(
440+
shortcuts: _kMenuTraversalShortcuts,
441+
// Ignore semantics here and since the same information is typically
442+
// also provided by the children.
443+
includeSemantics: false,
444+
child:
445+
_MenuAnchorScope(
446+
anchorKey: _anchorKey,
447+
anchor: this,
448+
isOpen: _isOpen,
449+
child: child,
450+
),
440451
);
441452
}
442453

‎packages/flutter/test/material/dropdown_menu_test.dart‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1221,6 +1221,46 @@ void main() {
12211221
expect(item5material.color, Colors.transparent); // the previous item should not be highlighted.
12221222
}, variant: TargetPlatformVariant.desktop());
12231223

1224+
testWidgets('Left and right keys can move text field selection', (WidgetTester tester) async {
1225+
final TextEditingController controller = TextEditingController();
1226+
addTearDown(controller.dispose);
1227+
1228+
final ThemeData themeData = ThemeData();
1229+
await tester.pumpWidget(MaterialApp(
1230+
theme: themeData,
1231+
home: Scaffold(
1232+
body: DropdownMenu<TestMenu>(
1233+
requestFocusOnTap: true,
1234+
enableFilter: true,
1235+
filterCallback: (List<DropdownMenuEntry<TestMenu>> entries, String filter) {
1236+
return entries.where((DropdownMenuEntry<TestMenu> element) => element.label.contains(filter)).toList();
1237+
},
1238+
dropdownMenuEntries: menuChildren,
1239+
controller: controller,
1240+
),
1241+
),
1242+
));
1243+
1244+
// Open the menu.
1245+
await tester.tap(find.byType(DropdownMenu<TestMenu>));
1246+
await tester.pump();
1247+
1248+
await tester.enterText(find.byType(TextField).first, 'example');
1249+
await tester.pumpAndSettle();
1250+
expect(controller.text, 'example');
1251+
expect(controller.selection, const TextSelection.collapsed(offset: 7));
1252+
1253+
// Press left key, the caret should move left.
1254+
await tester.sendKeyEvent(LogicalKeyboardKey.arrowLeft);
1255+
await tester.pumpAndSettle();
1256+
expect(controller.selection, const TextSelection.collapsed(offset: 6));
1257+
1258+
// Press Right key, the caret should move right.
1259+
await tester.sendKeyEvent(LogicalKeyboardKey.arrowRight);
1260+
await tester.pumpAndSettle();
1261+
expect(controller.selection, const TextSelection.collapsed(offset: 7));
1262+
}, variant: TargetPlatformVariant.desktop());
1263+
12241264
// Regression test for https://github.com/flutter/flutter/issues/147253.
12251265
testWidgets('Down key and up key can navigate on desktop platforms '
12261266
'when a label text contains another label text', (WidgetTester tester) async {

‎packages/flutter/test/material/menu_anchor_test.dart‎

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2285,6 +2285,66 @@ void main() {
22852285
expect(opened, isEmpty);
22862286
expect(closed, isNotEmpty);
22872287
});
2288+
2289+
// Regression test for
2290+
// https://github.com/flutter/flutter/issues/119532#issuecomment-2274705565.
2291+
testWidgets('Shortcuts of MenuAnchor do not rely on WidgetsApp.shortcuts', (WidgetTester tester) async {
2292+
// MenuAnchor used to rely on WidgetsApp.shortcuts for menu navigation,
2293+
// which is a problem for Web because the Web uses a special set of
2294+
// default shortcuts that define arrow keys as scrolling instead of
2295+
// traversing, and therefore arrow keys won't enter submenus when the
2296+
// focus is on MenuAnchor.
2297+
//
2298+
// This test verifies that `MenuAnchor`'s shortcuts continues to work even
2299+
// when `WidgetsApp.shortcuts` contains nothing.
2300+
2301+
final FocusNode childNode = FocusNode(debugLabel: 'Dropdown Inkwell');
2302+
addTearDown(childNode.dispose);
2303+
2304+
await tester.pumpWidget(
2305+
MaterialApp(
2306+
// Clear WidgetsApp.shortcuts to make sure MenuAnchor doesn't rely on
2307+
// it.
2308+
shortcuts: const <ShortcutActivator, Intent>{},
2309+
home: Scaffold(
2310+
body: MenuAnchor(
2311+
childFocusNode: childNode,
2312+
menuChildren: List<Widget>.generate(3, (int i) =>
2313+
MenuItemButton(
2314+
child: Text('Submenu item $i'),
2315+
onPressed: () {},
2316+
)
2317+
),
2318+
builder: (BuildContext context, MenuController controller, Widget? child) {
2319+
return InkWell(
2320+
focusNode: childNode,
2321+
onTap: controller.open,
2322+
child: const Text('Main button'),
2323+
);
2324+
},
2325+
),
2326+
),
2327+
),
2328+
);
2329+
2330+
listenForFocusChanges();
2331+
2332+
// Open the drop down menu and focus on the MenuAnchor.
2333+
await tester.tap(find.text('Main button'));
2334+
await tester.pumpAndSettle();
2335+
expect(find.text('Submenu item 0'), findsOneWidget);
2336+
2337+
// Press arrowDown, and the first submenu button should be focused.
2338+
// This is the critical part. It used to not work on Web.
2339+
await tester.sendKeyEvent(LogicalKeyboardKey.arrowDown);
2340+
await tester.pump();
2341+
expect(focusedMenu, equals('MenuItemButton(Text("Submenu item 0"))'));
2342+
2343+
// Press arrowDown, and the second submenu button should be focused.
2344+
await tester.sendKeyEvent(LogicalKeyboardKey.arrowDown);
2345+
await tester.pump();
2346+
expect(focusedMenu, equals('MenuItemButton(Text("Submenu item 1"))'));
2347+
});
22882348
});
22892349

22902350
group('Accelerators', () {

0 commit comments

Comments
 (0)