search

flutter / flutter

Flutter makes it easy and fast to build beautiful apps for mobile and beyond
https://flutter.dev
BSD 3-Clause "New" or "Revised" License
166.26k stars 27.51k forks source link

MenuAnchor-based menus pass through clicks that close the menu #135327

Closed Caffeinix closed 1 month ago

Caffeinix commented 1 year ago

Steps to reproduce

  1. Use MenuAnchor to show a menu.
  2. Click outside of the menu to close it, and make sure the element under your mouse at the time has its own tap event handler.
  3. Notice that although the tap does close the menu as expected, the tap event handler for the element under the mouse is also triggered.

Expected results

Tapping or clicking outside of a menu to close it should not activate whatever element is under the mouse or finger. This behavior is consistent between native menus on all operating systems and the implementation of PopupMenuButton and showMenu. In all these cases, the menu acts as a modal overlay for the purposes of these events.

Actual results

Tapping or clicking outside of a menu to close it does succeed in closing the menu, but it also triggers any tap event handlers on the element under the mouse or finger. This is contrary to convention and also general UX principles (a single tap should not trigger two different unrelated actions on two independent controls).

The bad behavior appears to be a consequence of no longer using a modal route for menus, and it seems to me that anchorTapClosesMenu may be a band-aid for other related consequences of that decision. While making the menu fully modal would make cascading menus very difficult (which I presume is why it wasn't done that way in the first place), some modal-ish behavior is likely necessary to avoid bugs like these.

Code sample

Here's a modified version of the `MenuAnchor` example code that shows a SnackBar whenever the background container receives a tap event. Open the menu and then click outside of it to close it, and you will see the SnackBar appear as the menu closes. ```dart import 'package:flutter/material.dart'; import 'package:flutter/services.dart'; /// Flutter code sample for [MenuAnchor]. void main() => runApp(const MenuApp()); /// An enhanced enum to define the available menus and their shortcuts. /// /// Using an enum for menu definition is not required, but this illustrates how /// they could be used for simple menu systems. enum MenuEntry { about('About'), showMessage( 'Show Message', SingleActivator(LogicalKeyboardKey.keyS, control: true)), hideMessage( 'Hide Message', SingleActivator(LogicalKeyboardKey.keyS, control: true)), colorMenu('Color Menu'), colorRed('Red Background', SingleActivator(LogicalKeyboardKey.keyR, control: true)), colorGreen('Green Background', SingleActivator(LogicalKeyboardKey.keyG, control: true)), colorBlue('Blue Background', SingleActivator(LogicalKeyboardKey.keyB, control: true)); const MenuEntry(this.label, [this.shortcut]); final String label; final MenuSerializableShortcut? shortcut; } class MyCascadingMenu extends StatefulWidget { const MyCascadingMenu({super.key, required this.message}); final String message; @override State createState() => _MyCascadingMenuState(); } class _MyCascadingMenuState extends State { MenuEntry? _lastSelection; final FocusNode _buttonFocusNode = FocusNode(debugLabel: 'Menu Button'); ShortcutRegistryEntry? _shortcutsEntry; Color get backgroundColor => _backgroundColor; Color _backgroundColor = Colors.red; set backgroundColor(Color value) { if (_backgroundColor != value) { setState(() { _backgroundColor = value; }); } } bool get showingMessage => _showingMessage; bool _showingMessage = false; set showingMessage(bool value) { if (_showingMessage != value) { setState(() { _showingMessage = value; }); } } @override void didChangeDependencies() { super.didChangeDependencies(); // Dispose of any previously registered shortcuts, since they are about to // be replaced. _shortcutsEntry?.dispose(); // Collect the shortcuts from the different menu selections so that they can // be registered to apply to the entire app. Menus don't register their // shortcuts, they only display the shortcut hint text. final Map shortcuts = { for (final MenuEntry item in MenuEntry.values) if (item.shortcut != null) item.shortcut!: VoidCallbackIntent(() => _activate(item)), }; // Register the shortcuts with the ShortcutRegistry so that they are // available to the entire application. _shortcutsEntry = ShortcutRegistry.of(context).addAll(shortcuts); } @override void dispose() { _shortcutsEntry?.dispose(); _buttonFocusNode.dispose(); super.dispose(); } @override Widget build(BuildContext context) { return Column( crossAxisAlignment: CrossAxisAlignment.start, children: [ MenuAnchor( childFocusNode: _buttonFocusNode, menuChildren: [ MenuItemButton( child: Text(MenuEntry.about.label), onPressed: () => _activate(MenuEntry.about), ), if (_showingMessage) MenuItemButton( onPressed: () => _activate(MenuEntry.hideMessage), shortcut: MenuEntry.hideMessage.shortcut, child: Text(MenuEntry.hideMessage.label), ), if (!_showingMessage) MenuItemButton( onPressed: () => _activate(MenuEntry.showMessage), shortcut: MenuEntry.showMessage.shortcut, child: Text(MenuEntry.showMessage.label), ), SubmenuButton( menuChildren: [ MenuItemButton( onPressed: () => _activate(MenuEntry.colorRed), shortcut: MenuEntry.colorRed.shortcut, child: Text(MenuEntry.colorRed.label), ), MenuItemButton( onPressed: () => _activate(MenuEntry.colorGreen), shortcut: MenuEntry.colorGreen.shortcut, child: Text(MenuEntry.colorGreen.label), ), MenuItemButton( onPressed: () => _activate(MenuEntry.colorBlue), shortcut: MenuEntry.colorBlue.shortcut, child: Text(MenuEntry.colorBlue.label), ), ], child: const Text('Background Color'), ), ], builder: (BuildContext context, MenuController controller, Widget? child) { return TextButton( focusNode: _buttonFocusNode, onPressed: () { if (controller.isOpen) { controller.close(); } else { controller.open(); } }, child: const Text('OPEN MENU'), ); }, ), Expanded( child: GestureDetector( onTap: () => ScaffoldMessenger.of(context).showSnackBar( const SnackBar( content: Text('Container registered a tap event'), duration: Duration(seconds: 1), ), ), child: Container( alignment: Alignment.center, color: backgroundColor, child: Column( mainAxisAlignment: MainAxisAlignment.center, children: [ Padding( padding: const EdgeInsets.all(12.0), child: Text( showingMessage ? widget.message : '', style: Theme.of(context).textTheme.headlineSmall, ), ), Text(_lastSelection != null ? 'Last Selected: ${_lastSelection!.label}' : ''), ], ), ), ), ), ], ); } void _activate(MenuEntry selection) { setState(() { _lastSelection = selection; }); switch (selection) { case MenuEntry.about: showAboutDialog( context: context, applicationName: 'MenuBar Sample', applicationVersion: '1.0.0', ); case MenuEntry.hideMessage: case MenuEntry.showMessage: showingMessage = !showingMessage; case MenuEntry.colorMenu: break; case MenuEntry.colorRed: backgroundColor = Colors.red; case MenuEntry.colorGreen: backgroundColor = Colors.green; case MenuEntry.colorBlue: backgroundColor = Colors.blue; } } } class MenuApp extends StatelessWidget { const MenuApp({super.key}); static const String kMessage = '"Talk less. Smile more." - A. Burr'; @override Widget build(BuildContext context) { return MaterialApp( theme: ThemeData(useMaterial3: true), home: const Scaffold(body: MyCascadingMenu(message: kMessage)), ); } } ```

Screenshots or Video

Screenshots / Video demonstration [cast.webm](https://github.com/flutter/flutter/assets/1880449/c174eca8-7004-45b6-9b3d-a68de2b4e628)
huycozy commented 1 year ago

Thanks for filing the issue. Reproduced this on the latest stable and master channels.

Also, I see there is a workaround for this by using IgnorePointer for the underlying widget and using a flag to trigger it by MenuAnchor's onOpen and onClose callbacks.

Workaround sample code ```dart import 'package:flutter/material.dart'; import 'package:flutter/services.dart'; /// Flutter code sample for [MenuAnchor]. void main() => runApp(const MenuApp()); /// An enhanced enum to define the available menus and their shortcuts. /// /// Using an enum for menu definition is not required, but this illustrates how /// they could be used for simple menu systems. enum MenuEntry { about('About'), showMessage('Show Message', SingleActivator(LogicalKeyboardKey.keyS, control: true)), hideMessage('Hide Message', SingleActivator(LogicalKeyboardKey.keyS, control: true)), colorMenu('Color Menu'), colorRed('Red Background', SingleActivator(LogicalKeyboardKey.keyR, control: true)), colorGreen('Green Background', SingleActivator(LogicalKeyboardKey.keyG, control: true)), colorBlue('Blue Background', SingleActivator(LogicalKeyboardKey.keyB, control: true)); const MenuEntry(this.label, [this.shortcut]); final String label; final MenuSerializableShortcut? shortcut; } class MyCascadingMenu extends StatefulWidget { const MyCascadingMenu({super.key, required this.message}); final String message; @override State createState() => _MyCascadingMenuState(); } class _MyCascadingMenuState extends State { MenuEntry? _lastSelection; final FocusNode _buttonFocusNode = FocusNode(debugLabel: 'Menu Button'); ShortcutRegistryEntry? _shortcutsEntry; Color get backgroundColor => _backgroundColor; Color _backgroundColor = Colors.red; set backgroundColor(Color value) { if (_backgroundColor != value) { setState(() { _backgroundColor = value; }); } } bool get showingMessage => _showingMessage; bool _showingMessage = false; set showingMessage(bool value) { if (_showingMessage != value) { setState(() { _showingMessage = value; }); } } @override void didChangeDependencies() { super.didChangeDependencies(); // Dispose of any previously registered shortcuts, since they are about to // be replaced. _shortcutsEntry?.dispose(); // Collect the shortcuts from the different menu selections so that they can // be registered to apply to the entire app. Menus don't register their // shortcuts, they only display the shortcut hint text. final Map shortcuts = { for (final MenuEntry item in MenuEntry.values) if (item.shortcut != null) item.shortcut!: VoidCallbackIntent(() => _activate(item)), }; // Register the shortcuts with the ShortcutRegistry so that they are // available to the entire application. _shortcutsEntry = ShortcutRegistry.of(context).addAll(shortcuts); } @override void dispose() { _shortcutsEntry?.dispose(); _buttonFocusNode.dispose(); super.dispose(); } bool _ignorePointer = false; @override Widget build(BuildContext context) { return Column( crossAxisAlignment: CrossAxisAlignment.start, children: [ MenuAnchor( onOpen: () => setState(() => _ignorePointer = true), onClose: () => setState(() => _ignorePointer = false), childFocusNode: _buttonFocusNode, menuChildren: [ MenuItemButton( child: Text(MenuEntry.about.label), onPressed: () => _activate(MenuEntry.about), ), if (_showingMessage) MenuItemButton( onPressed: () => _activate(MenuEntry.hideMessage), shortcut: MenuEntry.hideMessage.shortcut, child: Text(MenuEntry.hideMessage.label), ), if (!_showingMessage) MenuItemButton( onPressed: () => _activate(MenuEntry.showMessage), shortcut: MenuEntry.showMessage.shortcut, child: Text(MenuEntry.showMessage.label), ), SubmenuButton( menuChildren: [ MenuItemButton( onPressed: () => _activate(MenuEntry.colorRed), shortcut: MenuEntry.colorRed.shortcut, child: Text(MenuEntry.colorRed.label), ), MenuItemButton( onPressed: () => _activate(MenuEntry.colorGreen), shortcut: MenuEntry.colorGreen.shortcut, child: Text(MenuEntry.colorGreen.label), ), MenuItemButton( onPressed: () => _activate(MenuEntry.colorBlue), shortcut: MenuEntry.colorBlue.shortcut, child: Text(MenuEntry.colorBlue.label), ), ], child: const Text('Background Color'), ), ], builder: (BuildContext context, MenuController controller, Widget? child) { return TextButton( focusNode: _buttonFocusNode, onPressed: () { if (controller.isOpen) { controller.close(); } else { controller.open(); } }, child: const Text('OPEN MENU'), ); }, ), Expanded( child: IgnorePointer( ignoring: _ignorePointer, child: GestureDetector( onTap: () => ScaffoldMessenger.of(context).showSnackBar( const SnackBar( content: Text('Container registered a tap event'), duration: Duration(seconds: 1), ), ), child: Container( alignment: Alignment.center, color: backgroundColor, child: Column( mainAxisAlignment: MainAxisAlignment.center, children: [ Padding( padding: const EdgeInsets.all(12.0), child: Text( showingMessage ? widget.message : '', style: Theme.of(context).textTheme.headlineSmall, ), ), Text(_lastSelection != null ? 'Last Selected: ${_lastSelection!.label}' : ''), ], ), ), ), ), ), ], ); } void _activate(MenuEntry selection) { setState(() { _lastSelection = selection; }); switch (selection) { case MenuEntry.about: showAboutDialog( context: context, applicationName: 'MenuBar Sample', applicationVersion: '1.0.0', ); case MenuEntry.hideMessage: case MenuEntry.showMessage: showingMessage = !showingMessage; case MenuEntry.colorMenu: break; case MenuEntry.colorRed: backgroundColor = Colors.red; case MenuEntry.colorGreen: backgroundColor = Colors.green; case MenuEntry.colorBlue: backgroundColor = Colors.blue; } } } class MenuApp extends StatelessWidget { const MenuApp({super.key}); static const String kMessage = '"Talk less. Smile more." - A. Burr'; @override Widget build(BuildContext context) { return MaterialApp( theme: ThemeData(useMaterial3: true), home: const Scaffold(body: MyCascadingMenu(message: kMessage)), ); } } ```
flutter doctor -v (stable and master) ```bash [✓] Flutter (Channel stable, 3.13.5, on macOS 13.5 22G74 darwin-x64, locale en-VN) • Flutter version 3.13.5 on channel stable at /Users/huynq/Documents/GitHub/flutter • Upstream repository https://github.com/flutter/flutter.git • Framework revision 12fccda598 (5 days ago), 2023-09-19 13:56:11 -0700 • Engine revision bd986c5ed2 • Dart version 3.1.2 • DevTools version 2.25.0 [✓] Android toolchain - develop for Android devices (Android SDK version 32.0.0) • Android SDK at /Users/huynq/Library/Android/sdk • Platform android-34, build-tools 32.0.0 • ANDROID_HOME = /Users/huynq/Library/Android/sdk • Java binary at: /Applications/Android Studio.app/Contents/jbr/Contents/Home/bin/java • Java version OpenJDK Runtime Environment (build 17.0.6+0-17.0.6b802.4-9586694) • All Android licenses accepted. [✓] Xcode - develop for iOS and macOS (Xcode 14.3) • Xcode at /Applications/Xcode.app/Contents/Developer • Build 14E222b • CocoaPods version 1.11.3 [✓] Chrome - develop for the web • Chrome at /Applications/Google Chrome.app/Contents/MacOS/Google Chrome [✓] Android Studio (version 2022.2) • Android Studio at /Applications/Android Studio.app/Contents • Flutter plugin can be installed from: 🔨 https://plugins.jetbrains.com/plugin/9212-flutter • Dart plugin can be installed from: 🔨 https://plugins.jetbrains.com/plugin/6351-dart • Java version OpenJDK Runtime Environment (build 17.0.6+0-17.0.6b802.4-9586694) [✓] VS Code (version 1.82.2) • VS Code at /Applications/Visual Studio Code.app/Contents • Flutter extension version 3.72.0 [✓] Connected device (2 available) • macOS (desktop) • macos • darwin-x64 • macOS 13.5 22G74 darwin-x64 • Chrome (web) • chrome • web-javascript • Google Chrome 117.0.5938.62 [✓] Network resources • All expected network resources are available. • No issues found! ``` ```bash [!] Flutter (Channel master, 3.15.0-7.0.pre.2, on macOS 13.5 22G74 darwin-x64, locale en-VN) • Flutter version 3.15.0-7.0.pre.2 on channel master at /Users/huynq/Documents/GitHub/flutter_master ! Warning: `flutter` on your path resolves to /Users/huynq/Documents/GitHub/flutter/bin/flutter, which is not inside your current Flutter SDK checkout at /Users/huynq/Documents/GitHub/flutter_master. Consider adding /Users/huynq/Documents/GitHub/flutter_master/bin to the front of your path. ! Warning: `dart` on your path resolves to /Users/huynq/Documents/GitHub/flutter/bin/dart, which is not inside your current Flutter SDK checkout at /Users/huynq/Documents/GitHub/flutter_master. Consider adding /Users/huynq/Documents/GitHub/flutter_master/bin to the front of your path. • Upstream repository https://github.com/flutter/flutter.git • Framework revision 2f56288a4e (2 hours ago), 2023-09-24 18:49:07 -0700 • Engine revision 2daf5e7bb2 • Dart version 3.2.0 (build 3.2.0-191.0.dev) • DevTools version 2.28.0-dev.12 • If those were intentional, you can disregard the above warnings; however it is recommended to use "git" directly to perform update checks and upgrades. [✓] Android toolchain - develop for Android devices (Android SDK version 32.0.0) • Android SDK at /Users/huynq/Library/Android/sdk • Platform android-34, build-tools 32.0.0 • ANDROID_HOME = /Users/huynq/Library/Android/sdk • Java binary at: /Applications/Android Studio.app/Contents/jbr/Contents/Home/bin/java • Java version OpenJDK Runtime Environment (build 17.0.6+0-17.0.6b802.4-9586694) • All Android licenses accepted. [✓] Xcode - develop for iOS and macOS (Xcode 14.3) • Xcode at /Applications/Xcode.app/Contents/Developer • Build 14E222b • CocoaPods version 1.11.3 [✓] Chrome - develop for the web • Chrome at /Applications/Google Chrome.app/Contents/MacOS/Google Chrome [✓] Android Studio (version 2022.2) • Android Studio at /Applications/Android Studio.app/Contents • Flutter plugin can be installed from: 🔨 https://plugins.jetbrains.com/plugin/9212-flutter • Dart plugin can be installed from: 🔨 https://plugins.jetbrains.com/plugin/6351-dart • Java version OpenJDK Runtime Environment (build 17.0.6+0-17.0.6b802.4-9586694) [✓] VS Code (version 1.82.2) • VS Code at /Applications/Visual Studio Code.app/Contents • Flutter extension version 3.72.0 [✓] Connected device (2 available) • macOS (desktop) • macos • darwin-x64 • macOS 13.5 22G74 darwin-x64 • Chrome (web) • chrome • web-javascript • Google Chrome 117.0.5938.62 [✓] Network resources • All expected network resources are available. ! Doctor found issues in 1 category. ```
huycozy commented 1 year ago

Maybe this will also be resolved once https://github.com/flutter/flutter/issues/124830 is completed, you may follow it as well.

Sample code (use OverlayPortal as the upper layer - issue doesn't appear) ```dart import 'package:flutter/material.dart'; void main() => runApp(const MenuApp()); class MyCascadingMenu extends StatefulWidget { const MyCascadingMenu({super.key, required this.message}); final String message; @override State createState() => _MyCascadingMenuState(); } class _MyCascadingMenuState extends State { final OverlayPortalController _tooltipController = OverlayPortalController(); @override Widget build(BuildContext context) { return Column( crossAxisAlignment: CrossAxisAlignment.start, children: [ TextButton( onPressed: _tooltipController.toggle, child: DefaultTextStyle( style: DefaultTextStyle.of(context).style.copyWith(fontSize: 50), child: OverlayPortal( controller: _tooltipController, overlayChildBuilder: (BuildContext context) { return Positioned( right: 100, bottom: 100, child: ColoredBox( color: Colors.amberAccent, child: GestureDetector( child: const Text('tooltip'), onTap: () { _tooltipController.hide(); }), ), ); }, child: const Text('Press to show/hide tooltip'), ), ), ), Expanded( child: GestureDetector( onTap: () => ScaffoldMessenger.of(context).showSnackBar( const SnackBar( content: Text('Container registered a tap event'), duration: Duration(seconds: 1), ), ), child: Container( alignment: Alignment.center, color: Colors.red, ), ), ), ], ); } } class MenuApp extends StatelessWidget { const MenuApp({super.key}); static const String kMessage = '"Talk less. Smile more." - A. Burr'; @override Widget build(BuildContext context) { return MaterialApp( theme: ThemeData(useMaterial3: true), home: const Scaffold(body: MyCascadingMenu(message: kMessage)), ); } } ```
gspencergoog commented 1 year ago

TapRegion was actually added for menus (to replace FocusTrap), partly because it was necessary to be able to capture the previously focused node and restore it when the menu is dismissed, and a route masks that by changing the focus before notifying of the route change. Before that, we used to use a modal route, but it caused a lot of other problems. For instance, context menus on mobile shouldn't eat the key event because it makes it hard to implement a "copy" or "clear" button next to a text field that operates as expected.

It's going to be hard to stop the propagation of the pointer event that a TapRegion sees because the TapRegion doesn't (by design) participate in the gesture arena, so there's no way to indicate that the event has been handled.

I'll look and see if there's a way to stop propagation of the event, and look into adding one if there isn't a mechanism. In any case, it'll probably need to be a configuration setting as to whether or not it eats the click, since there are cases where that isn't wanted.

flutter-triage-bot[bot] commented 9 months ago

This issue is assigned to @gspencergoog but has had no recent status updates. Please consider unassigning this issue if it is not going to be addressed in the near future. This allows people to have a clearer picture of what work is actually planned. Thanks!

flutter-triage-bot[bot] commented 6 months ago

This issue was assigned to @gspencergoog but has had no status updates in a long time. To remove any ambiguity about whether the issue is being worked on, the assignee was removed.

bleroux commented 1 month ago

Closing because, since https://github.com/flutter/flutter/pull/136305, the property MenuAnchor.consumeOutsideTap is available and can be set to true to get the desired behavior.

github-actions[bot] commented 3 weeks ago

This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of flutter doctor -v and a minimal reproduction of the issue.