Make ClipboardButton inside a block work correctly in Safari

[mirka]

Description

This PR addresses some quirks in Safari that can cause a ClipboardButton inside a block to not work correctly.

I encountered this problem while implementing the Copy URL functionality in the File block (#6805).

Test directions

1. Add a ClipboardButton component to some block

import { ClipboardButton } from '@wordpress/components';

// Put this somewhere inside render()
<ClipboardButton text="foo">{ 'Copy' }</ClipboardButton>

2. In Safari, start a new post (/wp-admin/post-new.php), and add the block

3. Click on the Copy button

   Expected behavior: “foo” is copied to clipboard

   Actual behavior (before this fix): A serialized string of the block is copied to clipboard

Other things to test

• Make sure that multiblock copy is working correctly (Select multiple blocks and ⌘C)
• Make sure that a ClipboardButton outside of a block still works correctly (like the Copy Link button that appears after publishing a post)

Technical notes

This is a strange combination of multiple Safari quirks and how they interact with CopyHandler.

1. In Safari, the hidden textarea created by clipboard.js is not the document.activeElement at the moment when the copy event fires, causing CopyHandler to misinterpret the event as documentHasSelection() === false. This causes a serialized block string to be copied instead of the intended text.

   The workaround is to listen to copy events on ClipboardButton, and explicitly e.target.focus() on the textarea.

2. There is an inconsistency in event order between the following two cases, presumably because of mount order.

   • Pre-saved block instances (i.e. at least one instance of that block type was already saved in the post when the page loaded): Copy handler on ClipboardButton fires first, then copy handler on document
   • New blocks: Copy handler on document fires first, then copy handler on ClipboardButton

   While this behavior is the same across browsers, in Safari it causes a problem in the latter case because the e.target.focus() workaround in 1 will not work unless the copy handler on ClipboardButton fires first.

   The workaround is to attach the event listeners directly on CopyHandler in React, instead of doing document.addEventListener(). This ensures a consistent event order.

3. While the two workarounds above are enough to fix the copy bug, there is another quirk in Safari where multiblock copy events can only be caught at the document level. Therefore, we need to also keep the document-level event listeners to have multiblock copy work in Safari.

Is this workaround worth the trouble?

I’m not entirely sure. Maybe we wouldn’t need this if we rewrote clipboard.js. But that seems like a lot of trouble, too.

[mcsf]

@mirka: thanks for a great report and patch! It's unfortunate that this flew under the radar.

This does need a rebase, though. Among other things, I see that a part of this patch has already been merged via #7622 (commit).

From quick testing it seems the onCopy trick in copy-handler is still needed. Actually, it's needed even for the correct focusing of textarea of document.activeElement.

Do you have some time to circle back to this? 🙏

[mirka]

@mcsf Done! Tested in Chrome, Firefox, and Safari.

[aduth]

Because the copy handler does not stop propagation, the changes here introduce an issue in that copying within the editor area will be handled once by the div wrapper, and a second time by the document.

One option may be a matter of just adding event.stopPropagation to avoid letting the document handle the event. (Though, it may in-fact require stopImmediatePropagation since, as far as I recall, React events are actually bound to the document).

Which speaks to another point: One of the root causes here, as you noted, is the awkward conflict between binding to the DOM directly, vs. letting it be handled by React. I think as you've reimplemented this toward a wrapping component is how it ought to have been implemented in the first place. It's unfortunate then to hear that this doesn't work correctly for copying multi-selections in Safari. Have you been able to track down any resources on why this may occur? Is it because of the type of element that the copy is being performed on?

[mirka]

@aduth I tried removing the multi-selection workaround, and luckily it looks like it isn't needed any more. (Seven month old PR 😅) Thanks for the prompt.

[mcsf]

@mirka: thanks for circling back so quickly! This looks a lot better.

Seems in very good shape, nothing awkward coming up in testing, event flow seems clear.

[aduth]

We should consider how to describe this for the CHANGELOGs of each package.

https://github.com/WordPress/gutenberg/blob/master/packages/README.md#maintaining-changelogs

I worry this would be considered a breaking change, if one had come to rely on the fact that the component would bind to the document.

[mirka]

@aduth Good point! I updated the changelogs for the affected packages.

[aduth]

An update in passing, just to break the silence: I raised the breaking change as a concern since, well... we can't just be doing breaking changes now that this component is available as part of a stable WordPress release. It's admittedly not so likely that someone would be using this component, but it doesn't immediately exempt it from the rule. I'm not sure what to do here, then, other than add some compatibility layer for the prior usage. The fact that it was previously buggy behavior could add some credence to the idea that it allowed to go through as-is.

Related: https://make.wordpress.org/core/2018/11/14/technical-organisation-post-5-0/

Deprecations / breaking changes have also been a topic of discussion for recent core JavaScript meetings.

https://make.wordpress.org/core/2019/01/08/javascript-chat-summary-january-7th/

[kwight]

The fact that it was previously buggy behavior could add some credence to the idea that it allowed to go through as-is.

This sounds like a solid reason to get it in. The longer it waits, the more risk of harm. @aduth what's needed at this point for a decision to move forward? And thanks again @mirka for the patch!

[aduth]

I agree, it won't help matters to let this one linger.

As one metric, I searched and found no occurrences of CopyHandler in any plugins in the WordPress.org plugin repository, aside from Gutenberg itself:

https://wpdirectory.net/search/01D6911R475FRT87RVDAQ79ZYQ

I expected it would probably not find much usage outside our own, so in combination with the NPM major version bump, I think it would be reasonable to merge this one as-is.

The alternative would be to handle the backwards-compatibility by keeping the current logic (with document handlers), but only run when there are no children passed to the component (with deprecated logging). The downside is that this would effectively live on as dead code. With that in mind and of the previous note of unuse, I don't consider it blocking.

[aduth]

This does need a rebase, however.

#7106 (review)

[mcsf]

Let's rebase and merge as-is 👍

[mirka]

@aduth @mcsf Rebased ✅

[aduth]

Thanks for the fix, and for your patience!