From 1f4c6ebc972a73a60b4bdeb331c8b5137bd95a3d Mon Sep 17 00:00:00 2001 From: davidhicks980 <59215665+davidhicks980@users.noreply.github.com> Date: Wed, 7 Aug 2024 13:43:00 -0400 Subject: [PATCH] MenuAnchor hover traversal fixes (#150914) Fixes https://github.com/flutter/flutter/issues/150910 and https://github.com/flutter/flutter/issues/150911. https://github.com/flutter/flutter/issues/150910 is fixed by invalidating the focus scope whenever a hover occurs. I'm interested to hear of better fixes -- it feels a bit extreme to invalidate the focus scope so often. https://github.com/flutter/flutter/issues/150911 is fixed by replacing TextButton.onHover with MouseRegion.onHover and MouseRegion.onExit. The issue appears to be that MouseRegion.onEnter is called on scroll, whereas MouseRegion.onHover is not. I'm not confident this is a great solution, so please let me know if you all have any suggestions! @Piinks @dkwingsmt --- .../flutter/lib/src/material/menu_anchor.dart | 90 +++++++++++++---- .../test/material/menu_anchor_test.dart | 97 +++++++++++++++++++ 2 files changed, 166 insertions(+), 21 deletions(-) diff --git a/packages/flutter/lib/src/material/menu_anchor.dart b/packages/flutter/lib/src/material/menu_anchor.dart index 1fa51d3282..3db6bc1310 100644 --- a/packages/flutter/lib/src/material/menu_anchor.dart +++ b/packages/flutter/lib/src/material/menu_anchor.dart @@ -1072,6 +1072,7 @@ class _MenuItemButtonState extends State { FocusNode? _internalFocusNode; FocusNode get _focusNode => widget.focusNode ?? _internalFocusNode!; _MenuAnchorState? get _anchor => _MenuAnchorState._maybeOf(context); + bool _isHovered = false; @override void initState() { @@ -1115,7 +1116,6 @@ class _MenuItemButtonState extends State { Widget child = TextButton( onPressed: widget.enabled ? _handleSelect : null, - onHover: widget.enabled ? _handleHover : null, onFocusChange: widget.enabled ? widget.onFocusChange : null, focusNode: _focusNode, style: mergedStyle, @@ -1141,6 +1141,14 @@ class _MenuItemButtonState extends State { ); } + if (widget.onHover != null || widget.requestFocusOnHover) { + child = MouseRegion( + onHover: _handlePointerHover, + onExit: _handlePointerExit, + child: child, + ); + } + return MergeSemantics(child: child); } @@ -1151,11 +1159,29 @@ class _MenuItemButtonState extends State { } } - void _handleHover(bool hovering) { - widget.onHover?.call(hovering); - if (hovering && widget.requestFocusOnHover) { - assert(_debugMenuInfo('Requesting focus for $_focusNode from hover')); - _focusNode.requestFocus(); + void _handlePointerExit(PointerExitEvent event) { + if (_isHovered) { + widget.onHover?.call(false); + _isHovered = false; + } + } + + // TextButton.onHover and MouseRegion.onHover can't be used without triggering + // focus on scroll. + void _handlePointerHover(PointerHoverEvent event) { + if (!_isHovered) { + _isHovered = true; + widget.onHover?.call(true); + if (widget.requestFocusOnHover) { + assert(_debugMenuInfo('Requesting focus for $_focusNode from hover')); + _focusNode.requestFocus(); + + // Without invalidating the focus policy, switching to directional focus + // may not originate at this node. + FocusTraversalGroup.of(context).invalidateScopeData( + FocusScope.of(context), + ); + } } } @@ -1839,6 +1865,7 @@ class _SubmenuButtonState extends State { FocusNode? _internalFocusNode; FocusNode get _buttonFocusNode => widget.focusNode ?? _internalFocusNode!; bool get _enabled => widget.menuChildren.isNotEmpty; + bool _isHovered = false; @override void initState() { @@ -1923,7 +1950,7 @@ class _SubmenuButtonState extends State { ?? widget.defaultStyleOf(context); mergedStyle = widget.style?.merge(mergedStyle) ?? mergedStyle; - void toggleShowMenu(BuildContext context) { + void toggleShowMenu() { if (controller._anchor == null) { return; } @@ -1934,18 +1961,29 @@ class _SubmenuButtonState extends State { } } - // Called when the pointer is hovering over the menu button. - void handleHover(bool hovering, BuildContext context) { - widget.onHover?.call(hovering); - // Don't open the root menu bar menus on hover unless something else - // is already open. This means that the user has to first click to - // open a menu on the menu bar before hovering allows them to traverse - // it. - if (controller._anchor!._root._orientation == Axis.horizontal && !controller._anchor!._root._isOpen) { - return; + void handlePointerExit(PointerExitEvent event) { + if (_isHovered) { + widget.onHover?.call(false); + _isHovered = false; } + } + + // MouseRegion.onEnter and TextButton.onHover are called + // if a button is hovered after scrolling. This interferes with + // focus traversal and scroll position. MouseRegion.onHover avoids + // this issue. + void handlePointerHover(PointerHoverEvent event) { + if (!_isHovered) { + _isHovered = true; + widget.onHover?.call(true); + // Don't open the root menu bar menus on hover unless something else + // is already open. This means that the user has to first click to + // open a menu on the menu bar before hovering allows them to traverse + // it. + if (controller._anchor!._root._orientation == Axis.horizontal && !controller._anchor!._root._isOpen) { + return; + } - if (hovering) { controller.open(); controller._anchor!._focusButton(); } @@ -1957,8 +1995,7 @@ class _SubmenuButtonState extends State { style: mergedStyle, focusNode: _buttonFocusNode, onFocusChange: _enabled ? widget.onFocusChange : null, - onHover: _enabled ? (bool hovering) => handleHover(hovering, context) : null, - onPressed: _enabled ? () => toggleShowMenu(context) : null, + onPressed: _enabled ? toggleShowMenu : null, isSemanticButton: null, child: _MenuItemLabel( leadingIcon: widget.leadingIcon, @@ -1971,13 +2008,24 @@ class _SubmenuButtonState extends State { ), ); - if (_enabled && _platformSupportsAccelerators) { + if (!_enabled) { + return child; + } + + child = MouseRegion( + onHover: handlePointerHover, + onExit: handlePointerExit, + child: child, + ); + + if (_platformSupportsAccelerators) { return MenuAcceleratorCallbackBinding( - onInvoke: () => toggleShowMenu(context), + onInvoke: toggleShowMenu, hasSubmenu: true, child: child, ); } + return child; }, menuChildren: widget.menuChildren, diff --git a/packages/flutter/test/material/menu_anchor_test.dart b/packages/flutter/test/material/menu_anchor_test.dart index 694e3856f3..8476d2a38e 100644 --- a/packages/flutter/test/material/menu_anchor_test.dart +++ b/packages/flutter/test/material/menu_anchor_test.dart @@ -2021,6 +2021,103 @@ void main() { expect(focusedMenu, equals('MenuItemButton(Text("Sub Sub Menu 110"))')); }); + + testWidgets('hover traversal invalidates directional focus scope data', (WidgetTester tester) async { + // Regression test for https://github.com/flutter/flutter/issues/150910 + await tester.pumpWidget( + MaterialApp( + home: Material( + child: MenuBar( + controller: controller, + children: createTestMenus( + onPressed: onPressed, + onOpen: onOpen, + onClose: onClose, + ), + ), + ), + ), + ); + + listenForFocusChanges(); + + // Have to open a menu initially to start things going. + await tester.tap(find.text(TestMenu.mainMenu1.label)); + await tester.pump(); + expect(focusedMenu, equals('SubmenuButton(Text("Menu 1"))')); + + await hoverOver(tester, find.text(TestMenu.subMenu12.label)); + await tester.pump(); + expect(focusedMenu, equals('MenuItemButton(Text("Sub Menu 12"))')); + + // Move pointer to disabled menu + await hoverOver(tester, find.text(TestMenu.mainMenu5.label)); + await tester.pump(); + expect(focusedMenu, equals('MenuItemButton(Text("Sub Menu 12"))')); + + await tester.sendKeyEvent(LogicalKeyboardKey.arrowUp); + await tester.pump(); + expect(focusedMenu, equals('SubmenuButton(Text("Sub Menu 11"))')); + + await tester.sendKeyEvent(LogicalKeyboardKey.arrowUp); + expect(focusedMenu, equals('MenuItemButton(Text("Sub Menu 10"))')); + + await hoverOver(tester, find.text(TestMenu.subMenu12.label)); + await tester.pump(); + expect(focusedMenu, equals('MenuItemButton(Text("Sub Menu 12"))')); + + await tester.sendKeyEvent(LogicalKeyboardKey.arrowDown); + expect(focusedMenu, equals('MenuItemButton(Text("Sub Menu 12"))')); + }); + + testWidgets('scrolling does not trigger hover traversal', (WidgetTester tester) async { + // Regression test for https://github.com/flutter/flutter/issues/150911 + final GlobalKey scrolledMenuItemKey = GlobalKey(); + await tester.pumpWidget( + MaterialApp( + home: Material( + child: MenuAnchor( + style: const MenuStyle( + fixedSize: WidgetStatePropertyAll(Size.fromHeight(200)), + ), + controller: controller, + menuChildren: [ + for (int i = 0; i < 20; i++) + MenuItemButton( + key: i == 15 ? scrolledMenuItemKey : null, + onPressed: () {}, + child: Text('Item $i'), + ) + ] + ), + ), + ), + ); + + listenForFocusChanges(); + + controller.open(); + await tester.pumpAndSettle(); + + await hoverOver(tester, find.text('Item 1')); + await tester.pump(); + expect(focusedMenu, equals('MenuItemButton(Text("Item 1"))')); + + // Scroll the menu while the pointer is over a menu item. The focus should + // not change. + tester.renderObject(find.text('Item 15')).showOnScreen(); + await tester.pumpAndSettle(); + expect(focusedMenu, equals('MenuItemButton(Text("Item 1"))')); + + // Traverse with the keyboard to test that the menu scrolls without hover + // focus affecting the focused menu. + for (int i = 2; i < 20; i++) { + await tester.sendKeyEvent(LogicalKeyboardKey.arrowDown); + await tester.pump(); + expect(focusedMenu, equals('MenuItemButton(Text("Item $i"))')); + } + }); + testWidgets('menus close on ancestor scroll', (WidgetTester tester) async { final ScrollController scrollController = ScrollController(); addTearDown(scrollController.dispose);