Implement RawMenuAnchor

[davidhicks980]

This PR adds a RawMenuAnchor() widget to widgets.dart. The purpose of this widget is to provide a menu primitive for the Material and Cupertino libraries (and others) to build upon. Additionally, this PR makes MenuController an inherited widget to simplify nested access to the menu (e.g. if you want to launch a context menu from a deeply-nested widget).

This PR:

• Centralizes core menu logic to a private class, _RawMenuAnchor(),
  • Provides the internals for interacting with menus:
    • TapRegion interop
    • DismissMenuAction handler
    • Close on scroll/resize
    • Focus traversal information, if applicable
    • Subclasses override _open, _close, _isOpen, _buildAnchor, and _menuScopeNode
  • State is accessible by descendents via MenuController.maybeOf(context)._anchor
• Adds 2 public constructors, backed by a _RawMenuAnchor() that contains shared logic.
  • RawMenuAnchor()
    • Users build the overlay from scratch.
    • Provides anchor/overlay position information and TapRegionGroupId to builder
    • Does not provide FocusScope management.
  • RawMenuAnchorGroup()
    • A primitive for menus that do not have overlays (menu bars).
    • This was previously called RawMenuAnchor.node(), but @dkwingsmt made a good case for splitting out the constructor.

Documentation examples have been added, and can be viewed at https://menu-anchor.web.app/

https://github.com/user-attachments/assets/25d35f23-2aad-4d07-9172-5c3fd65d53cf

@dkwingsmt

List which issues are fixed by this PR. #143712

Some issues that need to be addressed:

Semantics:

I'm basing the menu semantics off of the comment here, but I'm unsure whether the route should be given a name. There is no menubar/menu/menuitem role in Flutter, so I'm assuming the menu should be composed of nested dialogs

Unlike the menubar pattern from W3C, the RawMenuAnchor

• does not close on tab/shift-tab. I left this behavior out of the menu so that users could customize tab behavior, but I'm not opinionated either way
• does not open on ArrowUp/ArrowDown, because this could interfere with user focus behavior in unconventional menu setups (e.g. a vertical menu).
• does not automatically focus the first item in a menu overlay when activated via enter/spacebar, but does focus the first item when horizontal traversal opens a submenu. Automatically focusing the first item whenever an overlay opens interferes with hover traversal, and I couldn't think of a good way to only focus the first item when an overlay is triggered via enter/spacebar.
• doesn't focus disabled items (I wasn't sure how to address this without editing MenuItemButton)

While it is possible to nest menus -- for example, a dropdown anchor within a full-app context menu area -- nested menus behave as a single group. I was considering adding an additional parameter that separates nested root menus from their parents, and am interested to hear your feedback.

If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.

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.

[dkwingsmt]

This PR only includes the raw_menu_anchor.dart and raw_menu_anchor_test.dart because the test suite is over 5000 LOC

Holy cow! That was quite some work! Thanks in advance (before I start to take a look)! :)

[bleroux]

Impressive work! 🤩

I just reviewed the examples as a starting point.
Noticed some formatting nits and one file is missing a newline at the end of the file which makes the 'Linux analyze' CI check red.

[davidhicks980]

Impressive work! 🤩

I just reviewed the examples as a starting point. Noticed some formatting nits and one file is missing a newline at the end of the file which makes the 'Linux analyze' CI check red.

Thanks for the fixes! I should have come back and fixed these a while ago... I appreciate you doing so.

[bleroux]

@davidhicks980
It seems the remaning CI failures are related to two failing tests from menu_anchor_test.dart (only two failing tests is impressive for such a huge PR).
Those failures look legit.
I extracted one of the test as a runnable code sample:

MenuBar with padding above MaterialApp

// 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.

import 'package:flutter/material.dart';

/// Flutter code sample for a [RawMenuAnchor] that shows a simple menu with
/// three items.
void main() => runApp(const SimpleMenuApp());

class SimpleMenuApp extends StatelessWidget {
  const SimpleMenuApp({super.key});

  @override
  Widget build(BuildContext context) {
    return Padding(
      padding: const EdgeInsets.all(10.0),
      child: MaterialApp(
        theme: ThemeData(useMaterial3: false),
        home: Material(
          child: Column(
            children: <Widget>[
              Padding(
                padding: const EdgeInsets.all(12.0),
                child: Row(
                  children: <Widget>[
                    Expanded(
                      child: MenuBar(
                        children: createTestMenus(onPressed: (TestMenu entry) {
                          print('$entry pressed!');
                        }),
                      ),
                    ),
                  ],
                ),
              ),
              const Expanded(child: Placeholder()),
            ],
          ),
        ),
      ),
    );
  }
}

List<Widget> createTestMenus({
  void Function(TestMenu)? onPressed,
  void Function(TestMenu)? onOpen,
  void Function(TestMenu)? onClose,
  Map<TestMenu, MenuSerializableShortcut> shortcuts = const <TestMenu, MenuSerializableShortcut>{},
  bool includeExtraGroups = false,
  bool accelerators = false,
}) {
  Widget submenuButton(
    TestMenu menu, {
    required List<Widget> menuChildren,
  }) {
    return SubmenuButton(
      onOpen: onOpen != null ? () => onOpen(menu) : null,
      onClose: onClose != null ? () => onClose(menu) : null,
      menuChildren: menuChildren,
      child: accelerators ? MenuAcceleratorLabel(menu.acceleratorLabel) : Text(menu.label),
    );
  }

  Widget menuItemButton(
    TestMenu menu, {
    bool enabled = true,
    Widget? leadingIcon,
    Widget? trailingIcon,
    Key? key,
  }) {
    return MenuItemButton(
      key: key,
      onPressed: enabled && onPressed != null ? () => onPressed(menu) : null,
      shortcut: shortcuts[menu],
      leadingIcon: leadingIcon,
      trailingIcon: trailingIcon,
      child: accelerators ? MenuAcceleratorLabel(menu.acceleratorLabel) : Text(menu.label),
    );
  }

  final List<Widget> result = <Widget>[
    submenuButton(
      TestMenu.mainMenu0,
      menuChildren: <Widget>[
        menuItemButton(TestMenu.subMenu00, leadingIcon: const Icon(Icons.add)),
        menuItemButton(TestMenu.subMenu01),
        menuItemButton(TestMenu.subMenu02),
      ],
    ),
    submenuButton(
      TestMenu.mainMenu1,
      menuChildren: <Widget>[
        menuItemButton(TestMenu.subMenu10),
        submenuButton(
          TestMenu.subMenu11,
          menuChildren: <Widget>[
            menuItemButton(TestMenu.subSubMenu110, key: UniqueKey()),
            menuItemButton(TestMenu.subSubMenu111),
            menuItemButton(TestMenu.subSubMenu112),
            menuItemButton(TestMenu.subSubMenu113),
          ],
        ),
        menuItemButton(TestMenu.subMenu12),
      ],
    ),
    submenuButton(
      TestMenu.mainMenu2,
      menuChildren: <Widget>[
        menuItemButton(
          TestMenu.subMenu20,
          leadingIcon: const Icon(Icons.ac_unit),
          enabled: false,
        ),
      ],
    ),
    if (includeExtraGroups)
      submenuButton(
        TestMenu.mainMenu3,
        menuChildren: <Widget>[
          menuItemButton(TestMenu.subMenu30, enabled: false),
        ],
      ),
    if (includeExtraGroups)
      submenuButton(
        TestMenu.mainMenu4,
        menuChildren: <Widget>[
          menuItemButton(TestMenu.subMenu40, enabled: false),
          menuItemButton(TestMenu.subMenu41, enabled: false),
          menuItemButton(TestMenu.subMenu42, enabled: false),
        ],
      ),
    submenuButton(TestMenu.mainMenu5, menuChildren: const <Widget>[]),
  ];
  return result;
}

enum TestMenu {
  mainMenu0('&Menu 0'),
  mainMenu1('M&enu &1'),
  mainMenu2('Me&nu 2'),
  mainMenu3('Men&u 3'),
  mainMenu4('Menu &4'),
  mainMenu5('Menu &5 && &6 &'),
  subMenu00('Sub &Menu 0&0'),
  subMenu01('Sub Menu 0&1'),
  subMenu02('Sub Menu 0&2'),
  subMenu10('Sub Menu 1&0'),
  subMenu11('Sub Menu 1&1'),
  subMenu12('Sub Menu 1&2'),
  subMenu20('Sub Menu 2&0'),
  subMenu30('Sub Menu 3&0'),
  subMenu40('Sub Menu 4&0'),
  subMenu41('Sub Menu 4&1'),
  subMenu42('Sub Menu 4&2'),
  subSubMenu110('Sub Sub Menu 11&0'),
  subSubMenu111('Sub Sub Menu 11&1'),
  subSubMenu112('Sub Sub Menu 11&2'),
  subSubMenu113('Sub Sub Menu 11&3'),
  anchorButton('Press Me'),
  outsideButton('Outside');

  const TestMenu(this.acceleratorLabel);
  final String acceleratorLabel;
  // Strip the accelerator markers.
  String get label => MenuAcceleratorLabel.stripAcceleratorMarkers(acceleratorLabel);
}

Before	After

If it can save you time, I can investigate this particular issue.

A more general question, are they some features you want to implement or issues you want to fix before marking this PR as ready for review?

[davidhicks980]

Oh actually I think I know what caused this... it's related to the _MenuPanel widget and how it handles padding. I don't want to waste your time, but it'll be a little later today before I can get to it. Otherwise, this PR should be ready! Aside from overall design concerns, all features should be implemented.

…

On Wed, Nov 13, 2024, 9:57 AM Bruno Leroux ***@***.***> wrote: @davidhicks980 <https://github.com/davidhicks980> It seems the remaning CI failures are related to two failing tests from menu_anchor_test.dart (only two failing tests is impressive for such a huge PR). Those failures look legit. I extracted one of the test as runnable code sample: MenuBar with padding above MaterialApp // 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. import 'package:flutter/material.dart'; /// Flutter code sample for a [RawMenuAnchor] that shows a simple menu with/// three items.void main() => runApp(const SimpleMenuApp()); class SimpleMenuApp extends StatelessWidget { const SimpleMenuApp({super.key}); @OverRide Widget build(BuildContext context) { return Padding( padding: const EdgeInsets.all(10.0), child: MaterialApp( theme: ThemeData(useMaterial3: false), home: Material( child: Column( children: <Widget>[ Padding( padding: const EdgeInsets.all(12.0), child: Row( children: <Widget>[ Expanded( child: MenuBar( children: createTestMenus(onPressed: (TestMenu entry) { print('$entry pressed!'); }), ), ), ], ), ), const Expanded(child: Placeholder()), ], ), ), ), ); } } List<Widget> createTestMenus({ void Function(TestMenu)? onPressed, void Function(TestMenu)? onOpen, void Function(TestMenu)? onClose, Map<TestMenu, MenuSerializableShortcut> shortcuts = const <TestMenu, MenuSerializableShortcut>{}, bool includeExtraGroups = false, bool accelerators = false, }) { Widget submenuButton( TestMenu menu, { required List<Widget> menuChildren, }) { return SubmenuButton( onOpen: onOpen != null ? () => onOpen(menu) : null, onClose: onClose != null ? () => onClose(menu) : null, menuChildren: menuChildren, child: accelerators ? MenuAcceleratorLabel(menu.acceleratorLabel) : Text(menu.label), ); } Widget menuItemButton( TestMenu menu, { bool enabled = true, Widget? leadingIcon, Widget? trailingIcon, Key? key, }) { return MenuItemButton( key: key, onPressed: enabled && onPressed != null ? () => onPressed(menu) : null, shortcut: shortcuts[menu], leadingIcon: leadingIcon, trailingIcon: trailingIcon, child: accelerators ? MenuAcceleratorLabel(menu.acceleratorLabel) : Text(menu.label), ); } final List<Widget> result = <Widget>[ submenuButton( TestMenu.mainMenu0, menuChildren: <Widget>[ menuItemButton(TestMenu.subMenu00, leadingIcon: const Icon(Icons.add)), menuItemButton(TestMenu.subMenu01), menuItemButton(TestMenu.subMenu02), ], ), submenuButton( TestMenu.mainMenu1, menuChildren: <Widget>[ menuItemButton(TestMenu.subMenu10), submenuButton( TestMenu.subMenu11, menuChildren: <Widget>[ menuItemButton(TestMenu.subSubMenu110, key: UniqueKey()), menuItemButton(TestMenu.subSubMenu111), menuItemButton(TestMenu.subSubMenu112), menuItemButton(TestMenu.subSubMenu113), ], ), menuItemButton(TestMenu.subMenu12), ], ), submenuButton( TestMenu.mainMenu2, menuChildren: <Widget>[ menuItemButton( TestMenu.subMenu20, leadingIcon: const Icon(Icons.ac_unit), enabled: false, ), ], ), if (includeExtraGroups) submenuButton( TestMenu.mainMenu3, menuChildren: <Widget>[ menuItemButton(TestMenu.subMenu30, enabled: false), ], ), if (includeExtraGroups) submenuButton( TestMenu.mainMenu4, menuChildren: <Widget>[ menuItemButton(TestMenu.subMenu40, enabled: false), menuItemButton(TestMenu.subMenu41, enabled: false), menuItemButton(TestMenu.subMenu42, enabled: false), ], ), submenuButton(TestMenu.mainMenu5, menuChildren: const <Widget>[]), ]; return result; } enum TestMenu { mainMenu0('&Menu 0'), mainMenu1('M&enu &1'), mainMenu2('Me&nu 2'), mainMenu3('Men&u 3'), mainMenu4('Menu &4'), mainMenu5('Menu &5 && &6 &'), subMenu00('Sub &Menu 0&0'), subMenu01('Sub Menu 0&1'), subMenu02('Sub Menu 0&2'), subMenu10('Sub Menu 1&0'), subMenu11('Sub Menu 1&1'), subMenu12('Sub Menu 1&2'), subMenu20('Sub Menu 2&0'), subMenu30('Sub Menu 3&0'), subMenu40('Sub Menu 4&0'), subMenu41('Sub Menu 4&1'), subMenu42('Sub Menu 4&2'), subSubMenu110('Sub Sub Menu 11&0'), subSubMenu111('Sub Sub Menu 11&1'), subSubMenu112('Sub Sub Menu 11&2'), subSubMenu113('Sub Sub Menu 11&3'), anchorButton('Press Me'), outsideButton('Outside'); const TestMenu(this.acceleratorLabel); final String acceleratorLabel; // Strip the accelerator markers. String get label => MenuAcceleratorLabel.stripAcceleratorMarkers(acceleratorLabel); } Before After image.png (view on web) <https://github.com/user-attachments/assets/c1584ec1-1e65-4d37-be54-40b3dde3e65a> image.png (view on web) <https://github.com/user-attachments/assets/5f03f3bf-f185-4f45-9044-c79cc04a6c01> If it can save you time, I can investigate this particular issue. A more general question, are they some features you want to implement or issues you want to fix before marking this PR as ready for review? — Reply to this email directly, view it on GitHub <#158255 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AODY6MOQLLECMHRJCEOK4GT2ANSFPAVCNFSM6AAAAABRIVFRKKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDINZTHA2TMNZRGA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[davidhicks980]

OH nevermind, my comment is incorrect. I think this may be something that needs to be fixed on MenuAnchor, since MenuAnchor targets the root overlay. Also, I think I need to pass the root overlay dimensions to the position parameter. I could also be completely wrong -- I'm on my phone so I'll be able to dive in later! Feel free to make any edits though.

…

On Wed, Nov 13, 2024, 10:12 AM Bruno Leroux ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In packages/flutter/lib/src/widgets/raw_menu_anchor.dart <#158255 (comment)>: > + includeSemantics: false, + canRequestFocus: false, + onFocusChange: _handleFocusChange, + child: OverlayPortal.targetsRootOverlay( + controller: _overlayController, + overlayChildBuilder: _buildOverlay, + child: child, + ), + ); + } + + Widget _buildOverlay(BuildContext context) { + final BuildContext anchorContext = _anchorKey.currentContext!; + final RenderBox overlay = Overlay.of(anchorContext).context.findRenderObject()! as RenderBox; + final RenderBox anchor = anchorContext.findRenderObject()! as RenderBox; + final Rect anchorRect = anchor.localToGlobal(Offset.zero) & anchor.size; ⬇️ Suggested change - final Rect anchorRect = anchor.localToGlobal(Offset.zero) & anchor.size; + final Rect anchorRect = anchor.localToGlobal(Offset.zero, ancestor: overlay) & anchor.size; Just after posting my comment about the failing tests, I remembered seeing this commit <763bdeb> and wondering why the ancestor parameter was removed? Looks like it was required as there are no more test failures once this parameter is restored. — Reply to this email directly, view it on GitHub <#158255 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AODY6MNOU2DCNE744XJGCIL2ANT7LAVCNFSM6AAAAABRIVFRKKVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDIMZTGU2TEOJZGY> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[davidhicks980]

@bleroux Sorry I see that you're making changes and some of the tests I added are getting in the way. The reason why I removed the ancestor overlay parameter is because of a previous PR that moved from using the default OverlayPortal() constructor to OverlayPortal.targetsRootOverlay. Because we are targeting the root overlay, I don't think it makes sense to use the closest overlay to transform coordinates. I'm not sure if that's a correct assumption though -- should we either be transforming against the root overlay?

Also, I should note that I experienced the issue with this when creating the demo website. The sidebar shrunk the nearest overlay by 300px, so the menu was misaligned with its anchor.

[bleroux]

@davidhicks980

Sorry I see that you're making changes and some of the tests I added are getting in the way.

No worry at all, I pushed my change because I thought that all checks would be green but later figured out that this change was intended (I have yet to understand the whole RawMenuAnchor structure).
Thanks for your message explaining the reasoning.
I will try to understand the positioning logic more deeply. Feel free to push more commits, I don't want to slow you down, just want to help this effort.
I just added a new MenuAnchor tests that might fail with this PR for the moment, see #158915.

[davidhicks980]

@bleroux No worries! You've helped a lot, and the test you added is important. I'm still not sure how this should be handled.

[davidhicks980]

@bleroux thank you so much for the test you added. I migrated the _MenuPanel incorrectly, and it would've completely broken any menus that used LayerLink() had the test not caught the issue.

Otherwise, it looks like everything is fixed... assuming the tree-status changes. Any thoughts before submitting for review?

Edit: One potential bug I came across. A menu using LayerLink can overflow the screen without flipping. I'm not sure if this is something to address in the future, though.

[bleroux]

Otherwise, it looks like everything is fixed... assuming the tree-status changes. Any thoughts before submitting for review?

Great! All checks are green, I think you can mark this PR as 'Ready for review'

Edit: One potential bug I came across. A menu using LayerLink can overflow the screen without flipping. I'm not sure if this is something to address in the future, though.

yes, there are issues to address with the LayerLink. DropdownMenu is the only widget to rely on it and I proposed to revert this usage in #158930.
So we can decide on either to remove it from (probably after this PR is merged) and come back to it later or keep it and try to fix it (not part of this PR, I will come back to #157921, or another solution, when RawMenuAnchor will have landed).

[flutter-dashboard]

Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change).

If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Changes reported for pull request #158255 at sha e7f31dd

[davidhicks980]

Otherwise, it looks like everything is fixed... assuming the tree-status changes. Any thoughts before submitting for review?

Great! All checks are green, I think you can mark this PR as 'Ready for review'

Edit: One potential bug I came across. A menu using LayerLink can overflow the screen without flipping. I'm not sure if this is something to address in the future, though.

yes, there are issues to address with the LayerLink. DropdownMenu is the only widget to rely on it and I proposed to revert this usage in #158930. So we can decide on either to remove it from (probably after this PR is merged) and come back to it later or keep it and try to fix it (not part of this PR, I will come back to #157921, or another solution, when RawMenuAnchor will have landed).

Very interesting -- I'm going to follow your work on this. I've been meaning to dig into the follower/target because it avoids the single-frame lag that MenuAnchor experiences when moved around the screen, and may be a better candidate for implementing nested animated menus (see #137936 for an example -- unfortunately this implementation uses Flow instead of overlays). Anyways, I appreciate the help!

[flutter-dashboard]

Golden file changes are available for triage from new commit, Click here to view.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Changes reported for pull request #158255 at sha 87c9392