feat(components): add right click copy/cut/paste context menu COMPASS-9919 #7420
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds right-click context menu functionality for cut/copy/paste operations in text elements. The implementation provides contextual menu items based on text selection state and element editability, using the modern Clipboard API with fallback to deprecated execCommand.
- Implements a new hook
useCopyPasteContextMenuthat detects text selection and element editability - Adds keyboard interaction preservation when context menu items are clicked
- Integrates copy/paste context menu into the main component provider system
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/compass-context-menu/src/context-menu-provider.tsx | Extracts inline styles to a constant for consistency |
| packages/compass-components/src/hooks/use-copy-paste-context-menu.tsx | New hook implementing copy/paste context menu logic with clipboard operations |
| packages/compass-components/src/hooks/use-copy-paste-context-menu.spec.tsx | Comprehensive test suite for the copy/paste context menu functionality |
| packages/compass-components/src/components/context-menu.tsx | Adds mouse event handling to preserve focus when menu items are clicked |
| packages/compass-components/src/components/content-with-fallback.spec.tsx | Updates test expectations to account for new copy/paste context menu container |
| packages/compass-components/src/components/compass-components-provider.tsx | Integrates CopyPasteContextMenu component into the provider hierarchy |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
Assigned |
| ): element is HTMLElement => { | ||
| if (!element) return false; | ||
|
|
||
| const tagName = element.tagName.toLowerCase(); |
There was a problem hiding this comment.
I'm curious if there's a reason you use toLowerCase here?
| if (tagName === 'input') { | ||
| const inputType = (element as HTMLInputElement).type.toLowerCase(); | ||
| return ( | ||
| !NON_TEXT_INPUT_TYPES.includes(inputType) && | ||
| !(element as HTMLInputElement).readOnly | ||
| ); | ||
| } |
There was a problem hiding this comment.
This seems like a good place to use instanceof to avoid two unsafe type asserts.
| if (tagName === 'input') { | |
| const inputType = (element as HTMLInputElement).type.toLowerCase(); | |
| return ( | |
| !NON_TEXT_INPUT_TYPES.includes(inputType) && | |
| !(element as HTMLInputElement).readOnly | |
| ); | |
| } | |
| if (element instanceof HTMLInputElement) { | |
| return ( | |
| !NON_TEXT_INPUT_TYPES.includes(element.type.toLowerCase()) && | |
| !element.readOnly | |
| ); | |
| } |
WDYT @addaleax?
| element: Element | null | ||
| ): element is HTMLInputElement | HTMLTextAreaElement { | ||
| return ( | ||
| !!element && (element.tagName === 'INPUT' || element.tagName === 'TEXTAREA') |
There was a problem hiding this comment.
Is there any particular reason you need to rely on tagName branding checks instead of a simple instanceof? I'm might be totally off here, but I personally don't consider these non-idiomatic JS.
| SELECTION_EVENTS.forEach((event) => | ||
| document.addEventListener(event, captureSelectionState) | ||
| ); |
There was a problem hiding this comment.
Nit: Any reason you prefer this over a for...of loop? I know the language specification guarantees the callback is invoked synchronously, the for...of loop however makes that more apparent IMO and it handles edge-cases with promises a bit more gracefully (i.e. if addEventListener returned a promise, that would be "shollowed" and not awaited here). Not feeling strongly here, but wanted to mention and I'm curious if there's a reason for the preference to use forEach here.
| editCapabilities.canCut | ||
| ? { | ||
| label: 'Cut', | ||
| onAction: onCut, | ||
| } | ||
| : undefined, | ||
| editCapabilities.canCopy | ||
| ? { | ||
| label: 'Copy', | ||
| onAction: onCopy, | ||
| } | ||
| : undefined, | ||
| editCapabilities.canPaste | ||
| ? { | ||
| label: 'Paste', | ||
| onAction: onPaste, | ||
| } | ||
| : undefined, |
There was a problem hiding this comment.
I wonder if we'd want these actions to be disabled instead of missing entirely, when not supported?
| // The `execCommand` API is deprecated but still widely supported. | ||
| // We try to use the Clipboard API when available. | ||
| try { | ||
| await navigator.clipboard.writeText(selectedText); |
There was a problem hiding this comment.
Could we lower complexity and solve for the vast majority of users by only support this feature when navigator.clipboard.writeText is available and enabled? I mean, what's the expected number of users which will fallback on document.execCommand and are we sure this justify the added complexity in implementation and tests?
2 participants
COMPASS-9919
Adds a context menu for cut/copy/paste when right clicking in a scenario that allows the action. This does not work in modals.
cut.copy.paste.mp4