Skip to content

[context menu] Create new ContextMenu component#1665

Merged
atomiks merged 50 commits intomui:masterfrom
atomiks:feat/ContextMenu
May 23, 2025
Merged

[context menu] Create new ContextMenu component#1665
atomiks merged 50 commits intomui:masterfrom
atomiks:feat/ContextMenu

Conversation

@atomiks
Copy link
Contributor

@atomiks atomiks commented Apr 2, 2025

@atomiks atomiks added component: menu Changes related to the menu component. component: context menu Changes related to the context menu component. labels Apr 2, 2025
@netlify
Copy link

netlify bot commented Apr 2, 2025

Deploy Preview for base-ui ready!

Name Link
🔨 Latest commit 06cf974
🔍 Latest deploy log https://app.netlify.com/projects/base-ui/deploys/682fda4e9970420008efa630
😎 Deploy Preview https://deploy-preview-1665--base-ui.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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

@atomiks atomiks force-pushed the feat/ContextMenu branch 3 times, most recently from 4d55392 to a9acad9 Compare April 3, 2025 00:31
@atomiks atomiks marked this pull request as ready for review April 3, 2025 01:06
@michaldudak
Copy link
Member

We haven't discussed the API of it. I'm wondering if an ordinary Menu with a different Trigger wouldn't be enough:

<Menu.Root>
  <Menu.ContextTrigger />
  ...

@atomiks
Copy link
Contributor Author

atomiks commented Apr 3, 2025

@michaldudak discoverability-wise, it might be better as a component for Radix parity? The Root needs to know if it should act as a context menu, where a component provider works nicely in the API, since otherwise it would need a prop in conjunction with using Menu.ContextTrigger (unless we set the state in an effect to know it's been rendered, which is a bit annoying).

It's also better if it's part of ContextMenu for tree-shaking reasons. (Edit: if ContextTrigger isn't used, where most of the logic is contained, then tree-shaking still works under your suggestion) - I think many extraneous features, like Tooltip's trackCursorAxis may be better as a component/provider than a prop if it has non-negligible logic.

@mj12albert
Copy link
Member

mj12albert commented Apr 7, 2025

Testing on an iPhone SE (iOS 17.7.2), a long press on the trigger area when the context menu is already open will make a large text selection:

RPReplay_Final1744034094.mp4

I screen-capped Safari only but it repros on Firefox and Chrome as well

According to react-aria it requires quite a heavy handed approach to handle this for all browsers: https://github.com/adobe/react-spectrum/blob/main/packages/%40react-aria/interactions/src/textSelection.ts

@atomiks atomiks force-pushed the feat/ContextMenu branch from 06e668f to 850dee6 Compare April 9, 2025 02:14
@atomiks
Copy link
Contributor Author

atomiks commented Apr 9, 2025

@mj12albert looks like WebkitUserSelect: none on the internal backdrop is enough to fix that

@mj12albert
Copy link
Member

looks like WebkitUserSelect: none on the internal backdrop is enough to fix that

Nice, glad we can avoid that extra Safari handling ~

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged. label Apr 15, 2025
@michaldudak
Copy link
Member

We could add an attribute indicating how the menu was open (touch or right-click). It seems that on mobile menus are often placed above the trigger (so they are not covered by user's hand). A callback variant of side/placement props could be useful here.

} = useMenuRootContext();
const { side, align, floatingContext } = useMenuPositionerContext();
const contextMenuContext = useContextMenuRootContext();
const hasContextMenuParent = Boolean(contextMenuContext) && !nested;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the Menubar PR I implemented a more universal solution to determine the menu's parent and its capabilities: https://github.com/mui/base-ui/pull/1684/files#diff-6da7b84ff007a487d62a9d48ba9fb32ad4b286ef666fd3d0419b14e9ebbe472a.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Context menus don't have any parents though, so I think this should suffice?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to avoid having all kinds of boolean values like nested, isInContextMenu, isInMenubar, (possibly) isInNavigationMenu. The approach I took in the Menubar PR lets a menu part know what kind of menu it's in in a standard way and provides a type-safe way to access the parent context (without having to check for its existence)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright 👍 I'll wait for Menubar to be merged and then use its logic for Menu

@atomiks
Copy link
Contributor Author

atomiks commented May 21, 2025

@michaldudak if you mean it doesn't lock scroll in all situations, that's from #1890

@atomiks
Copy link
Contributor Author

atomiks commented May 22, 2025

@ImSingee sorry for the super late reply but I see why this doesn't work (but should). Working to fix

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged. label May 22, 2025
Signed-off-by: atomiks <cc.glows@gmail.com>
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged. label May 22, 2025
@atomiks atomiks force-pushed the feat/ContextMenu branch from 5bef1dc to ad9a0c6 Compare May 22, 2025 05:00
@michaldudak
Copy link
Member

No, I mean you are able to click on elements outside of it. As if the internal backdrop doesn't exist.

@atomiks
Copy link
Contributor Author

atomiks commented May 22, 2025

@michaldudak ah right. I switched outside press dismissal from mousedown to pointerdown since there was a bug with the menu closing after long press if you held for 500ms but less than some X time, say 800ms, it would close immediately

@atomiks atomiks force-pushed the feat/ContextMenu branch from b2d2fad to a031f43 Compare May 22, 2025 06:04
@atomiks
Copy link
Contributor Author

atomiks commented May 22, 2025

Changed to a 500ms grace period to block outside press after long press, so we can still use mousedown

Fixes inner/outer nesting: https://codesandbox.io/p/sandbox/base-ui-context-menu-stack-forked-z2hclk

@michaldudak
Copy link
Member

michaldudak commented May 22, 2025

The click handler now fires twice sometimes. It's pretty hard to reproduce. In this example I was click-and-dragging, but I also noticed it a couple of times when just clicking

Screen.Recording.2025-05-22.at.11.20.38.mov

EDIT: Never mind, it's an issue with the CSB built-in console. The browser console doesn't show duplicates.

@atomiks
Copy link
Contributor Author

atomiks commented May 22, 2025

There's a bug with return focus causing a scroll to the previously focused element if it was out of the viewport, but will be updated in Floating UI before release

@atomiks atomiks merged commit 03472e5 into mui:master May 23, 2025
22 checks passed
@atomiks atomiks deleted the feat/ContextMenu branch May 23, 2025 02:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

component: context menu Changes related to the context menu component. type: new feature Expand the scope of the product to solve a new problem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[context menu] Create Context Menu component

7 participants