fix(menu): render through portal to avoid z-index and overflow issues

[janhassel]

Renders the Menu component through a portal to avoid it being misplaced, cut off or invisible in certain setups (such as the DataTable toolbar).

Changelog

Changed

• render Menu through ReactDOM.createPortal()

Testing / Reviewing

• Ensure keyboard navigation still works as expected
• Ensure the position of the menu is corretl attached to the trigger point
• Ensure that when the menu is closed through blur, the focus is returned to the trigger element (if any exists)
  • I added a test story "unstable_menu > OverflowMenu > In Toolbar" to test this.
  • The story should be removed prior to merging and only serves as an example setup to illustrate the purpose of this PR

[netlify]

✔️ Deploy Preview for carbon-elements ready!

🔨 Explore the source changes: 5072497

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-elements/deploys/60edde47de1a9700082703de

😎 Browse the preview: https://deploy-preview-8829--carbon-elements.netlify.app

[netlify]

✔️ Deploy Preview for carbon-components-react ready!

🔨 Explore the source changes: 5072497

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-components-react/deploys/60edde472a3ffc000748e6cd

😎 Browse the preview: https://deploy-preview-8829--carbon-components-react.netlify.app

[joshblack]

@janhassel in the past we got tripped up with portals (specifically the AC violation that content should be rendered in a landmark) and I was curious if this approach handles the use-case where screen reader uses can exit the menu with their virtual cursor and end up in a part of the document that they don't expect?

For an example, I wrote a bit about this over in: #7565 (comment) and just was curious if this approach was using an aria-owns trick or something to get around that 👀

[janhassel]

@joshblack Good point! I only thought of the focus management so far but not about the virtual screen reader cursor. Just pushed an update with aria-owns and it seems to work great (at least with VoiceOver on my end). Only caveat is that aria-owns creates a parent/children relationship and not an adjacent one, so I had to wrap the trigger button in a container for it to work:

<div aria-owns="menu-id">
  <button>...</button>
</div>

I don't think this should cause any issues though as I set display: inline-block on the container div to keep the flow predictable.

Could you check if the update works for you?

[sstrubberg]

Keyboard commands are firing like expected, but when I click on the menu, then click it again to close it, it doesn't close.

[janhassel]

@sstrubberg Good catch! Should be fixed now.

[tay1orjones]

Should focus return to the trigger element after an item is selected in the menu? Here I select one and then hit tab, which places focus on the first tab stop.

doesnt.return.to.trigger.element.mov

EDIT - I'm also I'm seeing the same with the voiceover virtual cursor. Here I start on the default OverflowMenu - I select an option from the list, hit tab, focus is returned to the trigger element. On the Menu "In Toolbar" story it goes back to the first tab stop instead of the trigger element

same.with.virtual.voiceover.cursor.mov

[netlify]

✔️ Deploy Preview for carbon-react-next ready!

🔨 Explore the source changes: 5072497

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-react-next/deploys/60eeefd7399d09446a0b6491

😎 Browse the preview: https://deploy-preview-8829--carbon-react-next.netlify.app

[janhassel]

@tay1orjones Apologies for the long delay! I just pushed an update which returns the focus to the trigger (if any) whenever the menu is closed. It should work with both mouse and keyboard operation (including VoiceOver controls).

[joshblack]

Just a quick note, before merging we should give @janhassel a heads up so he can toggle off the demo story that he wrote 👀

Stale