[WIP] Action based menu handling #167537

dkwingsmt · 2025-04-22T07:50:24Z

This is a refactor that manipulates menus via intents and actions. The goal is to incorporate with the design for animated menus that uses dual menu controllers and no decorators or delegates, as described in this comment.

Key points:

First, let's only look at RawMenuAnchors:
- The MenuController no longer directly calls _RawMenuAnchorBaseMixin's methods. Instead, it dispatches OpenMenuIntent, CloseMenuIntent, etc.
  - The intents will be dispatched to a context that is assigned when the controller is attached, which is the context within the Actions widget of the raw menu anchor.
- The raw menu anchor builds an Actions widget, which handles the intents with callback actions that call private methods, such as open() and close().
- This solves the problem of how the MenuController's methods affect the raw menu anchor.
As for themed menus:
- As said above, the themed menu is given an external menu controller, which builds another (internal) menu controller for its raw menu anchor.
- The themed menu will build its own Actions widget and handles opening and closing.
  - Opening will making the internal menu controller open and play animations, while closing will play animations before making the internal menu controller close.
  - Dismissing will be discussed later.
  - Similarly, the context just within this Actions widget is assigned to the external menu controller. Therefore when calling the methods of the external menu controller will eventually call private methods of the themed menu.
How dismissing works:
- The goal is that DismissIntent, the standard way of dismissing, such as pressing Esc, must be handled by the root themed menu.
- This consists of two steps:
  - The DismissIntent is handled by any raw menu anchor, which finds its root raw menu anchor and call its _dismiss().
  - The _dismiss() actually dispatches a DismissMenuIntent, which is by default handled by the raw menu anchor in an overridable way. The themed menu also handles it in its Actions by calling the animated close().
  - We can't let DismissIntent be handled overridable because it would leak to the next widget surrounding it (such as a dialog barrier).

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.

davidhicks980 · 2025-04-25T15:19:57Z

I'll post the same comment on this PR and the delegate PR so that viewers of the other PR have some context.

I spent some time playing with this PR last night. It's fundamentally similar to the decorator, but I liked your idea of themed menus overriding the actions of their RawMenuAnchor, because it feels like it fits with Flutter paradigms. I ended up with something completely different, but in the spirit of this PR. Here's my thought process:

What I like about the PR:

It's more understandable than the decorator.
It avoids the animationStatus errors that I introduced
I like the concept of placing an intent at a different layer of the tree. I'm used to moving actions around, so this was a cool solution.
Intent/Actions made the code a lot more explicit

A few issues that I came across:

Passing BuildContext is usually discouraged because a BuildContext can be changed if a widget is reparented, and there's a risk of memory leaks. Also, exposing the attachment/detachment process increases the risk of memory leaks
It's a lot of code to reimplement for each animated menu, and it's somewhat easy to mess up (e.g. what if you forget an action handler)
Having MenuController.dismiss(), MenuController.close(), and MenuController.detach() would probably be confusing to someone who is first using MenuController.
Existing menus (e.g. PopupMenuButton, context menus, etc) should probably be wired up to handle the new intents, otherwise users may be confused.
Intent bubbling
- For example, given a root menu and a submenu, if the internal submenu emits a DismissMenuIntent that is not overridden by the themed submenu, but is overridden by the root menu, wouldn't the root menu animate closed instead of the submenu synchronously closing? I'm not sure, but it's a bit confusing for me to think about.
Less important because I think we were going to remove the animationStatus, but making the animation status mutable by everyone could lead to bugs.

So, I tried a couple different changes to simplify things. For example, we could pass a "inner" MenuController inside of the OpenMenuIntent/CloseMenuIntent that could be used by actions to perform a different opening or closing action. This would remove the need for attachment/detachment and BuildContext references.

However, after thinking about it more, I realized our solutions are basically trying to accomplish the following:

We want MenuController.open() and MenuController.close() to animate by default
We want to provide a way to show or hide the overlay before and after animations using the MenuController
Allowing end users to skip menu animations could be useful (e.g. if you wanted to hide the menu immediately when transitioning to a new page)

I'm curious of your thoughts on this but what if skipped actions and decorators, and just added a flag to MenuController.open() and MenuController.close() that chose whether to trigger animation callbacks on RawMenuAnchor. For example, if transition was the flag and it was true by default, MenuController.open() would trigger onOpenRequested and MenuController.open(transition: false) would hide the overlay. The PR is here: #167806.

final MenuController menuController = MenuController;
late final AnimationController animationController = AnimationController(vsync: this, duration: Duration(milliseconds: 200));

void _handleCloseRequest() {
    animationController.reverse().whenComplete(() {
      menuController.close(transition: false); // Hides the overlay without calling onCloseRequest
    });
  }

  void _handleOpenRequest() {
    menuController.open(transition: false); // Shows the overlay without calling onOpenRequested
    animationController.forward();
  }

  // Example function called by the anchor
  void _handleAnchorTapped() {
     if (animationController.isForwardOrCompleted) {
        menuController.close();
      } else {
        menuController.open();
      }
   }

  @override
  Widget build(BuildContext context) {
    return RawMenuAnchor(
      controller: menuController,
      onCloseRequested: _handleCloseRequest,  // Called when MenuController.close() is called
      onOpenRequested: _handleOpenRequest.   // Called when MenuController.open() is called
     ...
  )
)

Let me know! Sorry, I realize this is has been a drawn out discussion.

dkwingsmt · 2025-05-27T17:37:25Z

Closing this PR in favor of #167806.

@dkwingsmt

Alternative to #163481, #167537, #163481 that uses callbacks. @dkwingsmt - you inspired me to simplify the menu behavior. I didn't end up using Actions, mainly because nested behavior was unwieldy and capturing BuildContext has drawbacks. This uses a basic callback mechanism to animate the menu open and closed. Check out the examples. <hr /> ### The problem RawMenuAnchor synchronously shows or hides an overlay menu in response to `MenuController.open()` and `MenuController.close`, respectively. Because animations cannot be run on a hidden overlay, there currently is no way for developers to add animations to RawMenuAnchor and its subclasses (MenuAnchor, DropdownMenuButton, etc). ### The solution This PR: - Adds two callbacks -- `onOpenRequested` and `onCloseRequested` -- to RawMenuAnchor. - onOpenRequested is called with a position and a showOverlay callback, which opens the menu when called. - onCloseRequested is called with a hideOverlay callback, which hides the menu when called. When `MenuController.open()` and `MenuController.close()` are called, onOpenRequested and onCloseRequested are invoked, respectively. Precursor for #143416, #135025, #143712 ## Demo https://github.com/user-attachments/assets/bb14abca-af26-45fe-8d45-289b5d07dab2 ```dart // Copyright 2014 The Flutter Authors. All rights reserved. // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. // ignore_for_file: public_member_api_docs import 'dart:ui' as ui; import 'package:flutter/material.dart' hide MenuController, RawMenuAnchor, RawMenuOverlayInfo; import 'raw_menu_anchor.dart'; /// Flutter code sample for a [RawMenuAnchor] that animates a simple menu using /// [RawMenuAnchor.onOpenRequested] and [RawMenuAnchor.onCloseRequested]. void main() { runApp(const App()); } class Menu extends StatefulWidget { const Menu({super.key}); @OverRide State<Menu> createState() => _MenuState(); } class _MenuState extends State<Menu> with SingleTickerProviderStateMixin { late final AnimationController animationController; final MenuController menuController = MenuController(); @OverRide void initState() { super.initState(); animationController = AnimationController( vsync: this, duration: const Duration(milliseconds: 300), ); } @OverRide void dispose() { animationController.dispose(); super.dispose(); } void _handleMenuOpenRequest(Offset? position, void Function({Offset? position}) showOverlay) { // Mount or reposition the menu before animating the menu open. showOverlay(position: position); if (animationController.isForwardOrCompleted) { // If the menu is already open or opening, the animation is already // running forward. return; } // Animate the menu into view. This will cancel the closing animation. animationController.forward(); } void _handleMenuCloseRequest(VoidCallback hideOverlay) { if (!animationController.isForwardOrCompleted) { // If the menu is already closed or closing, do nothing. return; } // Animate the menu out of view. // // Be sure to use `whenComplete` so that the closing animation // can be interrupted by an opening animation. animationController.reverse().whenComplete(() { if (mounted) { // Hide the menu after the menu has closed hideOverlay(); } }); } @OverRide Widget build(BuildContext context) { return RawMenuAnchor( controller: menuController, onOpenRequested: _handleMenuOpenRequest, onCloseRequested: _handleMenuCloseRequest, overlayBuilder: (BuildContext context, RawMenuOverlayInfo info) { final ui.Offset position = info.anchorRect.bottomLeft; return Positioned( top: position.dy + 5, left: position.dx, child: TapRegion( groupId: info.tapRegionGroupId, child: Material( color: ColorScheme.of(context).primaryContainer, shape: RoundedRectangleBorder(borderRadius: BorderRadius.circular(8)), elevation: 3, child: SizeTransition( sizeFactor: animationController, child: const SizedBox( height: 200, width: 150, child: Center(child: Text('Howdy', textAlign: TextAlign.center)), ), ), ), ), ); }, builder: (BuildContext context, MenuController menuController, Widget? child) { return FilledButton( onPressed: () { if (animationController.isForwardOrCompleted) { menuController.close(); } else { menuController.open(); } }, child: const Text('Toggle Menu'), ); }, ); } } class App extends StatelessWidget { const App({super.key}); @OverRide Widget build(BuildContext context) { return MaterialApp( theme: ThemeData(colorScheme: ColorScheme.fromSeed(seedColor: Colors.blue)), home: const Scaffold(body: Center(child: Menu())), ); } } ``` ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord].  [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md --------- Co-authored-by: Tong Mu <[email protected]>

@dkwingsmt

Alternative to flutter#163481, flutter#167537, flutter#163481 that uses callbacks. @dkwingsmt - you inspired me to simplify the menu behavior. I didn't end up using Actions, mainly because nested behavior was unwieldy and capturing BuildContext has drawbacks. This uses a basic callback mechanism to animate the menu open and closed. Check out the examples. <hr /> ### The problem RawMenuAnchor synchronously shows or hides an overlay menu in response to `MenuController.open()` and `MenuController.close`, respectively. Because animations cannot be run on a hidden overlay, there currently is no way for developers to add animations to RawMenuAnchor and its subclasses (MenuAnchor, DropdownMenuButton, etc). ### The solution This PR: - Adds two callbacks -- `onOpenRequested` and `onCloseRequested` -- to RawMenuAnchor. - onOpenRequested is called with a position and a showOverlay callback, which opens the menu when called. - onCloseRequested is called with a hideOverlay callback, which hides the menu when called. When `MenuController.open()` and `MenuController.close()` are called, onOpenRequested and onCloseRequested are invoked, respectively. Precursor for flutter#143416, flutter#135025, flutter#143712 ## Demo https://github.com/user-attachments/assets/bb14abca-af26-45fe-8d45-289b5d07dab2 ```dart // Copyright 2014 The Flutter Authors. All rights reserved. // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. // ignore_for_file: public_member_api_docs import 'dart:ui' as ui; import 'package:flutter/material.dart' hide MenuController, RawMenuAnchor, RawMenuOverlayInfo; import 'raw_menu_anchor.dart'; /// Flutter code sample for a [RawMenuAnchor] that animates a simple menu using /// [RawMenuAnchor.onOpenRequested] and [RawMenuAnchor.onCloseRequested]. void main() { runApp(const App()); } class Menu extends StatefulWidget { const Menu({super.key}); @OverRide State<Menu> createState() => _MenuState(); } class _MenuState extends State<Menu> with SingleTickerProviderStateMixin { late final AnimationController animationController; final MenuController menuController = MenuController(); @OverRide void initState() { super.initState(); animationController = AnimationController( vsync: this, duration: const Duration(milliseconds: 300), ); } @OverRide void dispose() { animationController.dispose(); super.dispose(); } void _handleMenuOpenRequest(Offset? position, void Function({Offset? position}) showOverlay) { // Mount or reposition the menu before animating the menu open. showOverlay(position: position); if (animationController.isForwardOrCompleted) { // If the menu is already open or opening, the animation is already // running forward. return; } // Animate the menu into view. This will cancel the closing animation. animationController.forward(); } void _handleMenuCloseRequest(VoidCallback hideOverlay) { if (!animationController.isForwardOrCompleted) { // If the menu is already closed or closing, do nothing. return; } // Animate the menu out of view. // // Be sure to use `whenComplete` so that the closing animation // can be interrupted by an opening animation. animationController.reverse().whenComplete(() { if (mounted) { // Hide the menu after the menu has closed hideOverlay(); } }); } @OverRide Widget build(BuildContext context) { return RawMenuAnchor( controller: menuController, onOpenRequested: _handleMenuOpenRequest, onCloseRequested: _handleMenuCloseRequest, overlayBuilder: (BuildContext context, RawMenuOverlayInfo info) { final ui.Offset position = info.anchorRect.bottomLeft; return Positioned( top: position.dy + 5, left: position.dx, child: TapRegion( groupId: info.tapRegionGroupId, child: Material( color: ColorScheme.of(context).primaryContainer, shape: RoundedRectangleBorder(borderRadius: BorderRadius.circular(8)), elevation: 3, child: SizeTransition( sizeFactor: animationController, child: const SizedBox( height: 200, width: 150, child: Center(child: Text('Howdy', textAlign: TextAlign.center)), ), ), ), ), ); }, builder: (BuildContext context, MenuController menuController, Widget? child) { return FilledButton( onPressed: () { if (animationController.isForwardOrCompleted) { menuController.close(); } else { menuController.open(); } }, child: const Text('Toggle Menu'), ); }, ); } } class App extends StatelessWidget { const App({super.key}); @OverRide Widget build(BuildContext context) { return MaterialApp( theme: ThemeData(colorScheme: ColorScheme.fromSeed(seedColor: Colors.blue)), home: const Scaffold(body: Center(child: Menu())), ); } } ``` ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord].  [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md --------- Co-authored-by: Tong Mu <[email protected]>

Impl prototype

c310fd4

github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. labels Apr 22, 2025

dkwingsmt added 4 commits April 22, 2025 01:22

New way of dismissing

fd9d5e7

Fix raw tests

be4ff84

Fix material menu anchor test

c5b9365

Remove dead code

748d311

dkwingsmt mentioned this pull request Apr 23, 2025

Implement RawMenuAnchorAnimationDelegate for animating menus. #165556

Closed

9 tasks

Add example

09ba11e

github-actions bot added a: animation Animation APIs d: api docs Issues with https://api.flutter.dev/ d: examples Sample code and demos labels Apr 24, 2025

davidhicks980 mentioned this pull request Apr 25, 2025

Add RawMenuAnchor animation callbacks #167806

Merged

9 tasks

dkwingsmt closed this May 27, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[WIP] Action based menu handling #167537

[WIP] Action based menu handling #167537

Uh oh!

dkwingsmt commented Apr 22, 2025 •

edited

Loading

Uh oh!

davidhicks980 commented Apr 25, 2025

Uh oh!

dkwingsmt commented May 27, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

[WIP] Action based menu handling #167537

[WIP] Action based menu handling #167537

Uh oh!

Conversation

dkwingsmt commented Apr 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

davidhicks980 commented Apr 25, 2025

Uh oh!

dkwingsmt commented May 27, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

dkwingsmt commented Apr 22, 2025 •

edited

Loading