feat(MenuButton): match menu parent theme

[IgnacioBecerra]

Closes carbon-design-system/carbon-labs#536
Closes carbon-design-system/carbon-labs#537

This PR introduces a new prop for MenuButton and Menu called matchTheme.

In @carbon-labs, for the Default UI Shell story, upon opening the MenuButton labeled Select Category, the focus is directed to the new Menu itself rather than the first MenuItem.

This is due to the Default story using target={headerRef.current} for the MenuButton, referring to the HeaderContainer housing the component. Removing it means the menu renders as the last element in the document.body, fixing this focus issue, as seen in the Header story.

However there is the side effect of the menu not rendering with the correct parent theme, as it's being render outside of its parent's theme scope. I'm not entirely sure why this fixes the focus error, but for the moment, this change allows for the menu to match its parent's theme.

Changelog

New

• added matchTheme boolean prop to MenuButton and Menu

Testing / Reviewing

Go to the new With matched theme story under MenuButton, and the menu will display in it's parent's theme. Note that the button itself is still the default kind.

[netlify]

✅ Deploy Preview for v11-carbon-web-components ready!

Name	Link
🔨 Latest commit	719c299
🔍 Latest deploy log	https://app.netlify.com/sites/v11-carbon-web-components/deploys/67f5b2dcc39b9d00081563ae
😎 Deploy Preview	https://deploy-preview-19059--v11-carbon-web-components.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[netlify]

✅ Deploy Preview for v11-carbon-react ready!

Name	Link
🔨 Latest commit	719c299
🔍 Latest deploy log	https://app.netlify.com/sites/v11-carbon-react/deploys/67f5b2dc8afadc0008edf682
😎 Deploy Preview	https://deploy-preview-19059--v11-carbon-react.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[alisonjoseph]

@IgnacioBecerra I think you need to run yarn test -u to fix the failing test / update the snapshot

[alisonjoseph]

This appears to be working as intended, however I'm wondering if its needed if we are able to solve this bug around the focus and the menuTarget prop.

[IgnacioBecerra]

@alisonjoseph The way I understand it, the menuTarget prop is so the menu doesn't render behind a modal by default. If menuTarget={modalRef.current}, the focus would work as intended within the modal as it has focus wrap. I'm assuming the focus is weird in our use case due to this, but I am still not 100% sure as to why it happens. I haven't been able to find out the main reason behind it.

And if we're not specifying the menuTarget, it does work properly, but the Header theme doesn't get carried over to the Menu component as it's rendered outside the header DOM-wise.

So I believe the focus "bug" isn't a bug per se, but rather we're using it incorrectly. I think it's meant to render in document.body and not within another component, so this new prop would still deliver the correct parent theme to it.

I'd like to hear the other devs' thoughts on this though!

[adamalston]

I think there's a bug with the focus behavior.

Using the menuTarget prop causes the menu to render in a portal rather than inline. Because of this rendering method, focusItem runs before the menu items are fully mounted and their refs are attached. As a result, when the code attempts to focus the first menu item, it fails to find it. Without menuTarget, the menu items are rendered inline, and their refs are available in time for the focus logic to work correctly.

I wonder if the fix here is to delay focusItem (using setTimeout or requestAnimationFrame) which should allow React to finish rendering and attaching refs to all menu items.

CI for 42d2702 passed (see https://github.com/adamalston/carbon/actions/runs/14394192300/job/40366844549) and I can't reproduce #19076 in the test story in that commit. I suppose I could open a PR for that unless someone sees something inherently wrong with it or has other thoughts. I'll need to figure out a way to test it.

Side note: the JSDoc comment for target in MenuProps is incorrect.

[tay1orjones]

There's lots of issues related to this. #19076 that Alison mentioned, @2nikhiltom is working on #18997, and we've discussed the idea of removing portal usage from Menu in #16883

There's also been discussion in #18935 of a presence/visibility helper that maybe could be used to help with this situation as well?

In any case I think it would be best if we could find another way to fix this without having to introduce this new prop to conditionally wrap it in a Theme.

[2nikhiltom]

I feel removing the portal rendering and instead leveraging Floating UI for positioning in Menu would fix should fix majority of bugs like Focus Management, MenuTarget inconsistency and themes related issues.
z-index and overflow benefits portals gave us can be implemented using Floating UI middleware

[alisonjoseph]

@tay1orjones @IgnacioBecerra I'm going to go ahead and close this, this isn't blocking anything for Platforms right now, I think fixing #18997 might solve the issue anyway.