From 59e57437dba691e045dc417c705a9dc48db8acfb Mon Sep 17 00:00:00 2001 From: Tong Mu Date: Thu, 3 Oct 2024 13:09:19 -0700 Subject: [PATCH] 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 https://github.com/flutter/flutter/issues/119532#issuecomment-2274705565. 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. --- .../lib/src/material/dropdown_menu.dart | 60 ++++++++++--------- .../flutter/lib/src/material/menu_anchor.dart | 21 +++++-- .../test/material/dropdown_menu_test.dart | 40 +++++++++++++ .../test/material/menu_anchor_test.dart | 60 +++++++++++++++++++ 4 files changed, 149 insertions(+), 32 deletions(-) diff --git a/packages/flutter/lib/src/material/dropdown_menu.dart b/packages/flutter/lib/src/material/dropdown_menu.dart index 394a8b61e8..4b19b9b432 100644 --- a/packages/flutter/lib/src/material/dropdown_menu.dart +++ b/packages/flutter/lib/src/material/dropdown_menu.dart @@ -43,12 +43,6 @@ typedef FilterCallback = List> Function(List = int? Function(List> entries, String query); -// Navigation shortcuts to move the selected menu items up or down. -final Map _kMenuTraversalShortcuts = { - LogicalKeySet(LogicalKeyboardKey.arrowUp): const _ArrowUpIntent(), - LogicalKeySet(LogicalKeyboardKey.arrowDown): const _ArrowDownIntent(), -}; - const double _kMinimumWidth = 112.0; const double _kDefaultHorizontalPadding = 12.0; @@ -944,14 +938,22 @@ class _DropdownMenuState extends State> { return textField; } - return _DropdownMenuBody( - width: widget.width, - children: [ - textField, - ..._initialMenu!.map((Widget item) => ExcludeFocus(excluding: !controller.isOpen, child: item)), - trailingButton, - leadingButton, - ], + return Shortcuts( + shortcuts: const { + SingleActivator(LogicalKeyboardKey.arrowLeft): ExtendSelectionByCharacterIntent(forward: false, collapseSelection: true), + SingleActivator(LogicalKeyboardKey.arrowRight): ExtendSelectionByCharacterIntent(forward: true, collapseSelection: true), + SingleActivator(LogicalKeyboardKey.arrowUp): _ArrowUpIntent(), + SingleActivator(LogicalKeyboardKey.arrowDown): _ArrowDownIntent(), + }, + child: _DropdownMenuBody( + width: widget.width, + children: [ + textField, + ..._initialMenu!.map((Widget item) => ExcludeFocus(excluding: !controller.isOpen, child: item)), + trailingButton, + leadingButton, + ], + ), ); }, ); @@ -977,23 +979,27 @@ class _DropdownMenuState extends State> { ); } - return Shortcuts( - shortcuts: _kMenuTraversalShortcuts, - child: Actions( - actions: >{ - _ArrowUpIntent: CallbackAction<_ArrowUpIntent>( - onInvoke: handleUpKeyInvoke, - ), - _ArrowDownIntent: CallbackAction<_ArrowDownIntent>( - onInvoke: handleDownKeyInvoke, - ), - }, - child: menuAnchor, - ), + return Actions( + actions: >{ + _ArrowUpIntent: CallbackAction<_ArrowUpIntent>( + onInvoke: handleUpKeyInvoke, + ), + _ArrowDownIntent: CallbackAction<_ArrowDownIntent>( + onInvoke: handleDownKeyInvoke, + ), + }, + child: menuAnchor, ); } } + +// `DropdownMenu` dispatches these private intents on arrow up/down keys. +// They are needed instead of the typical `DirectionalFocusIntent`s because +// `DropdownMenu` does not really navigate the focus tree upon arrow up/down +// keys: the focus stays on the text field and the menu items are given fake +// highlights as if they are focused. Using `DirectionalFocusIntent`s will cause +// the action to be processed by `EditableText`. class _ArrowUpIntent extends Intent { const _ArrowUpIntent(); } diff --git a/packages/flutter/lib/src/material/menu_anchor.dart b/packages/flutter/lib/src/material/menu_anchor.dart index 134023ea27..b29fb447ce 100644 --- a/packages/flutter/lib/src/material/menu_anchor.dart +++ b/packages/flutter/lib/src/material/menu_anchor.dart @@ -432,11 +432,22 @@ class _MenuAnchorState extends State { ); } - return _MenuAnchorScope( - anchorKey: _anchorKey, - anchor: this, - isOpen: _isOpen, - 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`). + return + Shortcuts( + shortcuts: _kMenuTraversalShortcuts, + // Ignore semantics here and since the same information is typically + // also provided by the children. + includeSemantics: false, + child: + _MenuAnchorScope( + anchorKey: _anchorKey, + anchor: this, + isOpen: _isOpen, + child: child, + ), ); } diff --git a/packages/flutter/test/material/dropdown_menu_test.dart b/packages/flutter/test/material/dropdown_menu_test.dart index ff3c08f993..5ed70d1bd7 100644 --- a/packages/flutter/test/material/dropdown_menu_test.dart +++ b/packages/flutter/test/material/dropdown_menu_test.dart @@ -1221,6 +1221,46 @@ void main() { expect(item5material.color, Colors.transparent); // the previous item should not be highlighted. }, variant: TargetPlatformVariant.desktop()); + testWidgets('Left and right keys can move text field selection', (WidgetTester tester) async { + final TextEditingController controller = TextEditingController(); + addTearDown(controller.dispose); + + final ThemeData themeData = ThemeData(); + await tester.pumpWidget(MaterialApp( + theme: themeData, + home: Scaffold( + body: DropdownMenu( + requestFocusOnTap: true, + enableFilter: true, + filterCallback: (List> entries, String filter) { + return entries.where((DropdownMenuEntry element) => element.label.contains(filter)).toList(); + }, + dropdownMenuEntries: menuChildren, + controller: controller, + ), + ), + )); + + // Open the menu. + await tester.tap(find.byType(DropdownMenu)); + await tester.pump(); + + await tester.enterText(find.byType(TextField).first, 'example'); + await tester.pumpAndSettle(); + expect(controller.text, 'example'); + expect(controller.selection, const TextSelection.collapsed(offset: 7)); + + // Press left key, the caret should move left. + await tester.sendKeyEvent(LogicalKeyboardKey.arrowLeft); + await tester.pumpAndSettle(); + expect(controller.selection, const TextSelection.collapsed(offset: 6)); + + // Press Right key, the caret should move right. + await tester.sendKeyEvent(LogicalKeyboardKey.arrowRight); + await tester.pumpAndSettle(); + expect(controller.selection, const TextSelection.collapsed(offset: 7)); + }, variant: TargetPlatformVariant.desktop()); + // Regression test for https://github.com/flutter/flutter/issues/147253. testWidgets('Down key and up key can navigate on desktop platforms ' 'when a label text contains another label text', (WidgetTester tester) async { diff --git a/packages/flutter/test/material/menu_anchor_test.dart b/packages/flutter/test/material/menu_anchor_test.dart index 979008a10b..4e5e204141 100644 --- a/packages/flutter/test/material/menu_anchor_test.dart +++ b/packages/flutter/test/material/menu_anchor_test.dart @@ -2285,6 +2285,66 @@ void main() { expect(opened, isEmpty); expect(closed, isNotEmpty); }); + + // Regression test for + // https://github.com/flutter/flutter/issues/119532#issuecomment-2274705565. + testWidgets('Shortcuts of MenuAnchor do not rely on WidgetsApp.shortcuts', (WidgetTester tester) async { + // MenuAnchor used to rely on WidgetsApp.shortcuts for menu navigation, + // which is a problem for Web because the Web uses a special set of + // default shortcuts that define arrow keys as scrolling instead of + // traversing, and therefore arrow keys won't enter submenus when the + // focus is on MenuAnchor. + // + // This test verifies that `MenuAnchor`'s shortcuts continues to work even + // when `WidgetsApp.shortcuts` contains nothing. + + final FocusNode childNode = FocusNode(debugLabel: 'Dropdown Inkwell'); + addTearDown(childNode.dispose); + + await tester.pumpWidget( + MaterialApp( + // Clear WidgetsApp.shortcuts to make sure MenuAnchor doesn't rely on + // it. + shortcuts: const {}, + home: Scaffold( + body: MenuAnchor( + childFocusNode: childNode, + menuChildren: List.generate(3, (int i) => + MenuItemButton( + child: Text('Submenu item $i'), + onPressed: () {}, + ) + ), + builder: (BuildContext context, MenuController controller, Widget? child) { + return InkWell( + focusNode: childNode, + onTap: controller.open, + child: const Text('Main button'), + ); + }, + ), + ), + ), + ); + + listenForFocusChanges(); + + // Open the drop down menu and focus on the MenuAnchor. + await tester.tap(find.text('Main button')); + await tester.pumpAndSettle(); + expect(find.text('Submenu item 0'), findsOneWidget); + + // Press arrowDown, and the first submenu button should be focused. + // This is the critical part. It used to not work on Web. + await tester.sendKeyEvent(LogicalKeyboardKey.arrowDown); + await tester.pump(); + expect(focusedMenu, equals('MenuItemButton(Text("Submenu item 0"))')); + + // Press arrowDown, and the second submenu button should be focused. + await tester.sendKeyEvent(LogicalKeyboardKey.arrowDown); + await tester.pump(); + expect(focusedMenu, equals('MenuItemButton(Text("Submenu item 1"))')); + }); }); group('Accelerators', () {