Add support for nested submenus to ActionMenu

[iansan5653]

ActionMenu has Primer designs and docs for nesting, but has never actually supported it, reading:

Looking for contributors to add submenus to the React and Rails implementation, as they are currently not available in this component.

👋 I am a contributor! And huge kudos to @cheshire137 as well since this PR is mostly stealing our work from the internal NestableActionMenu recipe.

---

Based on my understanding of the W3 guidance on menus, submenus should meet the following requirements:

1. The anchor should match surrounding menu items but render a visual indicator of a submenu
2. When activated, the anchor menu item should open a submenu and keep the root menu open
3. Menus should be navigable by left/right arrow keys, opening submenus with right and closing them with left
4. The entire stack of menus should close when the user presses Tab or activates a menu item
5. Only the top menu should close when the user presses Escape
6. The anchor menu item should have aria-haspopup and aria-expanded (when expanded)

Fortunately meeting these requirements with the existing ActionMenu architecture is actually pretty straightforward:

• A new component ActionMenu.MenuItemAnchor renders an ActionList.Item with a trailing visual as the menu's anchor Automatic application of a trailing visual and right arrow event handler when the submenu anchor is ActionList.Item
• Using context, ActionMenu internally determines if it is a submenu in order to close parent menus when necessary and to bind left/right navigation
• Everything else is pretty much taken care of by existing logic - ie, AnchoredOverlay already assigns the right ARIA attributes to the menu item anchor

The result is an intuitive submenu API that 'just works':

<ActionMenu>
  <ActionMenu.Button>Edit</ActionMenu.Button>
  <ActionMenu.Overlay>
    <ActionList>
      <ActionList.Item>Cut</ActionList.Item>
      <ActionList.Item>Copy</ActionList.Item>
      <ActionList.Item>Paste</ActionList.Item>
      <ActionMenu>
        <ActionMenu.Anchor>
          <ActionList.Item>Paste special</ActionList.Item>
        </ActionMenu.Anchor>
        <ActionMenu.Overlay>
          <ActionList>
            <ActionList.Item>Paste plain text</ActionList.Item>
            <ActionList.Item>Paste formulas</ActionList.Item>
            <ActionList.Item>Paste with formatting</ActionList.Item>
            <ActionMenu>
              <ActionMenu.Anchor>
                <ActionList.Item>Paste from</ActionList.Item>
              </ActionMenu.Anchor>
              <ActionMenu.Overlay>
                <ActionList>
                  <ActionList.Item>Current clipboard</ActionList.Item>
                  <ActionList.Item>History</ActionList.Item>
                  <ActionList.Item>Another device</ActionList.Item>
                </ActionList>
              </ActionMenu.Overlay>
            </ActionMenu>
          </ActionList>
        </ActionMenu.Overlay>
      </ActionMenu>
    </ActionList>
  </ActionMenu.Overlay>
</ActionMenu>

Screen.Recording.2024-03-13.at.12.50.45.PM.mov

The only thing I'm not so sure of is the naming of ActionMenu.MenuItemAnchor. It's not consistent with ActionMenu.Button, but I figured just calling it ActionMenu.MenuItem would be confusing. Open to suggestions here.

• Related: https://github.com/github/primer/issues/3120

Changelog

New

• Adds support for nested submenus in ActionMenu

Rollout strategy

• Patch release
• Minor release
• Major release; if selected, include a written rollout or migration plan
• None; if selected, include a brief description as to why

Testing & Reviewing

Merge checklist

• Added/updated tests
• Added/updated documentation
• Added/updated previews (Storybook)
• Changes are SSR compatible
• Tested in Chrome
• Tested in Firefox
• Tested in Safari
• Tested in Edge
• (GitHub staff only) Integration tests pass at github/github (Learn more about how to run integration tests)

[changeset-bot]

🦋 Changeset detected

Latest commit: d207bf7

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@primer/react	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[github-actions]

size-limit report 📦

Path	Size
packages/react/dist/browser.esm.js	88.95 KB (+0.27% 🔺)
packages/react/dist/browser.umd.js	89.28 KB (+0.33% 🔺)

[broccolinisoup]

Hey @iansan5653 👋🏻 Thanks so much for creating this PR 🙏🏻

I am not sure if we have had any discussions around the API, please let me know if I miss any context on that but I wonder if we have explored other options before settling on creating a new sub components. I think the functionality works great, thanks so much again, I just wanted to make sure we are not overloading the component with sub components if we have other feasible alternatives. Like using an existing anchor component maybe 🤷🏻‍♀️

<ActionMenu.Anchor>
     <ActionList.Item>
         Paste special
         <ActionList.TrailingVisual>
              <ChevronRightIcon />
         </ActionList.TrailingVisual>
     </ActionList.Item>
  </ActionMenu.Anchor>

vs

<ActionMenu.MenuItemAnchor>Paste special</ActionMenu.MenuItemAnchor>

Especially since we are relying on context to determine if the menu is a submenu. This option is more manually constructing it but I wonder if it feels intuitive enough to folks without introducing more sub components to ActionMenu. Or maybe introducing a new sub component reduces complexity under the hood, it is just food for thoughts to talk about the api options.

Tagging @siddharthkp to get his thoughts around the API

Thanks again for pushing this PR, super super helpful! 🙌🏻

[broccolinisoup]

Should we also check if the sub sub menu is not in the documents? like expect(component.queryByRole('menu')).not.toBeInTheDocument() or not really needed?

[iansan5653]

Unlike ActionMenu.Button, ActionMenu.MenuItemAnchor is actually doing two things:

1. Rendering an ActionList.Item with a trailing visual by default
2. Wiring up keyboard navigation (opening the menu on → press)

For (1) I agree it's pretty straightforward for consumers to just build it themselves - we're not gaining much by providing a component for that. Although I do prefer to provide the trailing visual by default because I'm concerned we might end up with inconsistent iconography otherwise. It's important to have a consistent visual here so we can intuitively indicate (to sighted users) that a submenu is available. If some apps use KebabHorizontalIcon and some use ArrowRightIcon and some use TriangleRightIcon and finally others use ChevronRightIcon, it's going to be a poor experience.

For (2) I think we definitely have to do this internally somehow - we can't expect consumers to always wire up the correct keyboard event in a consistent manner.

---

💭 We definitely could explore alternative APIs that meet those requirements. Since we do have the context isSubmenu value, it's easy for Anchor to know whether or know it is the anchor for a submenu. This would easily allow us to wire the keyboard event handling (2) to any submenu anchor. So (2) is an easy problem to solve.

The tricky part then is actually just the trailing visual (1). How do we render that by default?

One option might be for Anchor to detect if its child is an ActionList.Item and clone it to add the icon. I really want to avoid this because it's fragile and generally not a good idea to manipulate children (in React or IRL 😄).

using an existing anchor component maybe

This could work - we could easily change ActionMenu.Button to return an ActionList.Item instead of a Button when it detects that it is a submenu anchor. This is arguably intuitive because ActionMenu.Button represents the 'default' anchor for ActionMenu, so it makes sense that it would be the default for submenus too. But at the same time, it would be misleading for a component called Button to return a menu item.

Maybe we can update it for now, then in a future major release we could plan to rename ActionMenu.Button to something like ActionMenu.DefaultAnchor?

[broccolinisoup]

Hey @iansan5653 thanks for the reply!

Although I do prefer to provide the trailing visual by default because I'm concerned we might end up with inconsistent iconography otherwise. It's important to have a consistent visual here so we can intuitively indicate (to sighted users) that a submenu is available. If some apps use KebabHorizontalIcon and some use ArrowRightIcon and some use TriangleRightIcon and finally others use ChevronRightIcon, it's going to be a poor experience.

That is true, I underestimated the trailing visual issue.

One option might be for Anchor to detect if its child is an ActionList.Item and clone it to add the icon. I really want to avoid this because it's fragile and generally not a good idea to manipulate children (in React or IRL 😄).

😄 Yeah that is not a good enough reason to manipulate the children, I agree.

This could work - we could easily change ActionMenu.Button to return an ActionList.Item instead of a Button when it detects that it is a submenu anchor.

This could definitely work and make it intuitive enough when we update the name to make it more generic as you mentioned like ActionMenu.DefaultAnchor. Although, since this is going to be a major change, it will add another item to the major upgrade list and I am now thinking its value. Would it worth the effort? I guess my question would be do we want to add another sub component and increase the API surface area or do we want to keep the API lean and increase the code complexity under the hood? Especially with the major release headache, I am leaning more towards what you initially built ActionMenu.MenuItemAnchor and I am glad we had this conversation. Thanks for following up with the feedback. Obviously, this is totally my personal opinion and I'd love to make sure at least one of my other peers seen the API change and provide if they have any feedback 🙌🏻 (I nudged Sid on the DM 🫠)

[pksjce]

Hi @iansan5653 !
Thanks so much for working on this feature for us.
This helps me a lot with ActionBar where we have this requirement.
I have created a PR branching off of this branch here. #4421 . I would love if you had any feedback on this.
It seems to work pretty well except for two critiques

1. There is no way to provide MenuItemAnchor with a LeadingVisual. Using ActionList.LeadingVisual is a warning since its not under the right parent.
2. I agree that the naming of MenuItemAnchor could be better. Maybe something that includes SubMenu in it?

[iansan5653]

Using ActionList.LeadingVisual is a warning since its not under the right parent.

This will work in practice even if it's a lint error, because ActionMenu.MenuItemAnchor is rendering children directly into ActionList.Item without wrapping them with anything. I think it's just a matter of updating our linting configuration to indicate that it will work.

[pksjce]

🥳

[broccolinisoup]

This approach is consistent with operating system menus:

Right!! That is great to know. Yeah I thought that it is not very straightforward to solve this but wanted to mention anyway. I was mainly worry about the sighted keyboard users but seems like this is a common learnt pattern, I guess we are good here as well 👍🏻

turns out this doesn't actually work with wrapped components unless they forward all props to the underlying component

Yeah this is what I was thinking as a test case. Like https://github.com/github/github/blob/master/ui/packages/copilot-chat/components/ChatReference.tsx#L90. I tried with a pseudo story and it works 👍🏻

[broccolinisoup]

@iansan5653 I just realised that the tests are failing. Could you please make sure they are all green as well before going ahead with merge?