Replace clipboard.js with native Clipboard API
#37713
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ciampo
added
[Type] Code Quality
ciampo
commented
Jan 4, 2022
Comment on lines
63
to
68
| // Clear selection and ensure focus stays on the trigger element. | ||
| if ( trigger ) { | ||
| currentWindow?.getSelection()?.removeAllRanges(); | ||
|
|
||
| trigger.focus(); | ||
| } |
Contributor
Author
There was a problem hiding this comment.
The previous clipboard.js implementation used a hack to copy text to the clipboard which involved creating a temporary element, moving focus to it, selecting its text, and executing a "copy" command.
By switching to the new Clipboard APIs, clearing the selection and restoring focus may not be necessary — what do folks think?
Member
There was a problem hiding this comment.
I agree. If that's the case, we shouldn't need the focus handling. It's probably worth double checking that focus stays on the button after clicking it still :)
ciampo
commented
Jan 4, 2022
Comment on lines
+40
to
+50
| // `triggers` is always an array, regardless of the value of the `ref` param. | ||
| if ( typeof ref.current === 'string' ) { | ||
| // expect `ref` to be a DOM selector | ||
| triggers = Array.from( document.querySelectorAll( ref.current ) ); | ||
| } else if ( 'length' in ref.current ) { | ||
| // Expect `ref` to be a `NodeList` | ||
| triggers = Array.from( ref.current ); | ||
| } else { | ||
| // Expect `ref` to be a single `Element` | ||
| triggers = [ ref.current ]; | ||
| } |
Contributor
Author
There was a problem hiding this comment.
Previously, the different values for ref.current were handles by Clipboard's constructor
ciampo
commented
Jan 4, 2022
Comment on lines
52
to
54
| // Clear selection and ensure focus stays on the trigger element. | ||
| currentWindow?.getSelection()?.removeAllRanges(); | ||
| trigger.focus(); |
|
Size Change: -1.26 kB (0%) Total Size: 1.13 MB
ℹ️ View Unchanged
|
Contributor
Author
|
Not sure how to fix the Static Analysis CI errors — running |
Member
Taking a wild guess -- is the npm version the same? |
noahtallen
reviewed
Jan 4, 2022
Comment on lines
63
to
68
| // Clear selection and ensure focus stays on the trigger element. | ||
| if ( trigger ) { | ||
| currentWindow?.getSelection()?.removeAllRanges(); | ||
|
|
||
| trigger.focus(); | ||
| } |
Member
There was a problem hiding this comment.
I agree. If that's the case, we shouldn't need the focus handling. It's probably worth double checking that focus stays on the button after clicking it still :)
| /** | ||
| * External dependencies | ||
| */ | ||
| import Clipboard from 'clipboard'; |
Member
There was a problem hiding this comment.
While the changes in this file are pretty convoluted, it is a deprecated hook after all. The changes in the other hook are a lot more palatable. :P I still think it's worth it to remove the dependency.
Contributor
Author
There was a problem hiding this comment.
I had the same train of thoughts, and came to the same conclusion — refactoring this deprecated hook allows us to remove the dependency from the repo :)
Contributor
Author
I noticed that the Anyway, I've also addressed the feedback and added a changelog entry. |
Member
|
Hi @ciampo it seems currently on the trunk the "Copy all content" button does not work at all (nothing happens when clicked, the button is the top options menu). The button to copy a block on the block elipsis menu also is not working. If I revert this PR it starts working again. |
Contributor
Author
Hey @jorgefilipecosta , thank you for reporting this! Feel free to revert this PR. Could also open an issue that shows how to reproduce the bug you just described? I'll look into it next week. Thank you! |
jorgefilipecosta
added a commit
that referenced
this pull request
Jan 7, 2022
This reverts commit b3be067.
6 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Closes #37700
Changes in this PR:
useCopyToClipboardand the (deprecated)useCopyOnClickhooks in@wordpress/composeso that they use the native Clipboard APIclipboarddependency from the project's list of dependencies (saving2.55 kB (23%)for the@wordpress/composepackage)How has this been tested?
useCopyToClipboardhook work as expected (same as production) — see screenshots below for a few examplesScreenshots
"Copy" menu item when editing any block in the editor:
copy-clipboard-refactor.mp4
"Copy URL to clipboard" button when viewing an attachment details in the media library:
"Copy URL" button when editing an attachment in the File block:
"Copy" button when reviewing a post's info after publishing it:
Types of changes
Checklist:
*.native.jsfiles for terms that need renaming or removal).