base-ui
base-ui copied to clipboard
[context menu] Create new `ContextMenu` component
Closes #51
Preview: https://deploy-preview-1665--base-ui.netlify.app/react/components/context-menu
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...Use your smartphone camera to open QR code link. |
To edit notification comments on pull requests, go to your Netlify project configuration.
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 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.
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 looks like WebkitUserSelect: none on the internal backdrop is enough to fix that
looks like
WebkitUserSelect: noneon the internal backdrop is enough to fix that
Nice, glad we can avoid that extra Safari handling ~
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.
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.
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
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
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.
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.
Testing in Chrome on iOS, when I open it and scroll before I release my longpress, it gets all shaky.
The menu is not modal on mobile devices (tested on iPhone and iPad). Is this expected?
@michaldudak if you mean it doesn't lock scroll in all situations, that's from #1890
@ImSingee sorry for the super late reply but I see why this doesn't work (but should). Working to fix
No, I mean you are able to click on elements outside of it. As if the internal backdrop doesn't exist.
@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
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
~~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.
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