base-ui icon indicating copy to clipboard operation
base-ui copied to clipboard

[context menu] Create new `ContextMenu` component

Open atomiks opened this issue 7 months ago • 11 comments
trafficstars

Closes #51

Preview: https://deploy-preview-1665--base-ui.netlify.app/react/components/context-menu

atomiks avatar Apr 02 '25 06:04 atomiks

Deploy Preview for base-ui ready!

Name Link
Latest commit 06cf9747800dd2f724aeaa7840347e333e248318
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.

netlify[bot] avatar Apr 02 '25 06:04 netlify[bot]

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

michaldudak avatar Apr 03 '25 08:04 michaldudak

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

atomiks avatar Apr 03 '25 09:04 atomiks

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:

https://github.com/user-attachments/assets/d2ad1be0-4ea7-44d3-b9c2-4b9c3b8dfff4

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

mj12albert avatar Apr 07 '25 14:04 mj12albert

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

atomiks avatar Apr 09 '25 04:04 atomiks

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

Nice, glad we can avoid that extra Safari handling ~

mj12albert avatar Apr 09 '25 10:04 mj12albert

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.

michaldudak avatar Apr 23 '25 12:04 michaldudak

A callback variant of side/placement props could be useful here.

This could also be used instead of useMediaQuery to determine what side to use in narrower viewports. Would be good if we found a robust solution to https://github.com/mui/base-ui/pull/817 though.

atomiks avatar Apr 23 '25 13:04 atomiks

From https://x.com/rsms/status/1522583516265029633, the click-drag-release flow doesn't work properly:

https://github.com/user-attachments/assets/2668a1af-fb22-428f-a4b4-9c99062d3d48

https://www.radix-ui.com/primitives/docs/components/context-menu, https://ariakit.org/examples/menu-context-menu don't support this correctly either. So maybe OK to ignore fore a v0


Great to see this, we will need this for 👍 https://github.com/mui/mui-x/issues/12021

oliviertassinari avatar Apr 30 '25 23:04 oliviertassinari

Open in StackBlitz

npm i https://pkg.pr.new/@base-ui-components/react@1665

commit: 06cf974

pkg-pr-new[bot] avatar May 06 '25 12:05 pkg-pr-new[bot]

Is it possible to stack two context menus? For example, can one context menu appear when clicking on a div and another when clicking outside of it?

I try to do it directly, but sadly it doesn't work:

https://codesandbox.io/p/sandbox/sweet-wave-h4lygt

image

I have two boxes here. Right-clicking the outer one displays the context menu as expected, but right-clicking the inner one does not show its context menu.

ImSingee avatar May 08 '25 06:05 ImSingee

The menu should return focus to the previously focused element when an item is clicked or Esc is pressed. This is pretty annoying when keyboard is used to open the menu.

michaldudak avatar May 21 '25 09:05 michaldudak

Testing in Chrome on iOS, when I open it and scroll before I release my longpress, it gets all shaky.

colmtuite avatar May 21 '25 10:05 colmtuite

The menu is not modal on mobile devices (tested on iPhone and iPad). Is this expected?

michaldudak avatar May 21 '25 13:05 michaldudak

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

atomiks avatar May 21 '25 21:05 atomiks

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

atomiks avatar May 22 '25 03:05 atomiks

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

michaldudak avatar May 22 '25 05:05 michaldudak

@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 avatar May 22 '25 05:05 atomiks

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

atomiks avatar May 22 '25 06:05 atomiks

~~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~~

https://github.com/user-attachments/assets/1720d921-9957-4bb7-9b71-77286b6da254

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

michaldudak avatar May 22 '25 09:05 michaldudak

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 avatar May 22 '25 09:05 atomiks