fix: Support the Clipboard API in modern browsers #20058

diegomedina248 · 2022-05-13T13:21:44Z

SUMMARY

The copy command, though largely still supported, is deprecated.

In Safari, latest versions, the copy to clipboard of a permalink no longer works because of it.

This PR adds support for the Clipboard API, with all the nuances and small differences between the different browser engines. Be sure to read the comments & associated discussions linked for more information.

The new API is not yet fully supported by all browsers, so the current copy code was left as a fallback.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before:

Screen.Recording.2022-05-13.at.10.19.43.mov

After:

new.mov

TESTING INSTRUCTIONS

Go to any chart/dashboard and ensure the copy to permalink works across browsers.

ADDITIONAL INFORMATION

Fixes #19994

Has associated issue:
Required feature flags:
Changes UI
Includes DB Migration (follow approval process in SIP-59)
- Migration is atomic, supports rollback & is backwards-compatible
- Confirm DB migration upgrade and downgrade tested
- Runtime estimates and downtime expectations provided
Introduces new feature or API
Removes existing feature or API

codecov · 2022-05-13T20:00:03Z

Codecov Report

Merging #20058 (f02bf0b) into master (a813528) will decrease coverage by 0.00%.
The diff coverage is 64.58%.

@@            Coverage Diff             @@
##           master   #20058      +/-   ##
==========================================
- Coverage   66.50%   66.50%   -0.01%     
==========================================
  Files        1726     1726              
  Lines       64789    64803      +14     
  Branches     6829     6830       +1     
==========================================
+ Hits        43090    43097       +7     
- Misses      19967    19974       +7     
  Partials     1732     1732

Flag	Coverage Δ
javascript	`51.34% <64.58%> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...mponents/useExploreAdditionalActionsMenu/index.jsx	`63.63% <0.00%> (+0.71%)`	⬆️
...UD/data/components/SyntaxHighlighterCopy/index.tsx	`46.66% <0.00%> (ø)`
.../src/views/CRUD/data/savedquery/SavedQueryList.tsx	`63.30% <0.00%> (-0.59%)`	⬇️
superset-frontend/src/views/CRUD/hooks.ts	`47.05% <0.00%> (-0.26%)`	⬇️
superset-frontend/src/utils/copy.ts	`68.57% <68.57%> (-14.77%)`	⬇️
...-frontend/src/components/CopyToClipboard/index.jsx	`100.00% <100.00%> (+4.00%)`	⬆️
...dashboard/components/menu/ShareMenuItems/index.tsx	`89.47% <100.00%> (-0.53%)`	⬇️
superset-frontend/src/utils/common.js	`90.00% <100.00%> (+0.52%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a813528...f02bf0b. Read the comment docs.

stephenLYZ · 2022-05-16T14:05:58Z

/testenv up

github-actions · 2022-05-16T14:07:54Z

@stephenLYZ Ephemeral environment spinning up at http://34.220.180.16:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

stephenLYZ

Thanks for the fix! Left a comment and let me know if it makes sense.

stephenLYZ · 2022-05-16T15:25:12Z

superset-frontend/src/utils/copy.ts

-        if (!document.execCommand('copy')) {
+// Use the new Clipboard API if the browser supports it
+const copyTextWithClipboardApi = async (getText: () => Promise<string>) => {
+  try {


Actually It is not recommended to use nested try catches from async/await, which cause errors to bubble up, meaning that all functions under the catch scope are called (You can see copy is called twice in the chrome). I think It is okay to guarantee top-level try catches, maybe more appropriate to use promise here?

Thanks for that! The change in the parent function (copyTextToClipboard) should do it.

stephenLYZ · 2022-05-17T01:41:22Z

@diegomedina248 Hi, It looks like the copy will still be called twice (send the request twice). can you check it again?

diegomedina248 · 2022-05-20T01:28:07Z

@diegomedina248 Hi, It looks like the copy will still be called twice (send the request twice). can you check it again?

Yes, that seems to be a limitation on the API, for anything other than Safari, the promise inside ClipboardItem will be executed, even if it fails down the road and needs to re-execute.

We could check for the browser engine and try to assign the proper clipboard api execution: That has the drawback of browser engine & version miss match, aka, not all webkit browsers behave the way Safari does (in fact, I think only Safari has this weird bug).
We cannot reverse the condition because we cannot await anything in-between the user interaction and the clipboard api execution.

stephenLYZ · 2022-05-31T15:46:11Z

@diegomedina248 So we just need to do some checks for safari?

villebro

LGTM with a minor cleanup suggestion. Tested to work as expected with both Safari and Chrome (also verified SQL Lab which uses the same utils).

villebro · 2022-06-02T15:08:52Z

superset-frontend/src/utils/copy.ts

-      selection.addRange(range);
+const copyTextToClipboard = (getText: () => Promise<string>) =>
+  copyTextWithClipboardApi(getText)
+    // If the Clipboard API is not supported, fallback to the older method.


nit: it could clean up the code by breaking the old logic into a separate function

villebro · 2022-06-02T15:13:47Z

@diegomedina248 Hi, It looks like the copy will still be called twice (send the request twice). can you check it again?

Yes, that seems to be a limitation on the API, for anything other than Safari, the promise inside ClipboardItem will be executed, even if it fails down the road and needs to re-execute.

We could check for the browser engine and try to assign the proper clipboard api execution: That has the drawback of browser engine & version miss match, aka, not all webkit browsers behave the way Safari does (in fact, I think only Safari has this weird bug). We cannot reverse the condition because we cannot await anything in-between the user interaction and the clipboard api execution.

We could dedupe the permalink request by saving the request payload and response of the previous permalink request, and if the old payload is identical to the new one, we just return the original response without calling the API again. This would have the benefit of also not creating new permalinks if the user clicks one the link button multiple times in a row.

villebro

It'd be nice to get that duplicated request fixed before merging

villebro

LGTM, verified that it works as expected on Firefox, Chrome and Safari, and that there are no longer duplicated requests to the permalink endpoint.

stephenLYZ

Nice work!

github-actions · 2022-06-03T11:34:23Z

Ephemeral environment shutdown and build artifacts deleted.

isabellalacerda · 2022-06-28T18:14:39Z

copy to clipboard doesn't work in any browser for me, how can I fix this? it also doesn't show a proper error message

* fix: Support the Clipboard API in modern browsers * fix tests * PR comment * Improvements

pull-request-size bot added the size/L label May 13, 2022

diegomedina248 force-pushed the fix/create-permalink-in-safari branch 2 times, most recently from a4fd00f to d321279 Compare May 13, 2022 15:34

rusackas requested a review from stephenLYZ May 13, 2022 20:50

stephenLYZ requested changes May 16, 2022

View reviewed changes

diegomedina248 force-pushed the fix/create-permalink-in-safari branch from 2bbb585 to 25672fc Compare May 20, 2022 01:28

villebro approved these changes Jun 2, 2022

View reviewed changes

villebro self-requested a review June 2, 2022 15:41

villebro requested changes Jun 2, 2022

View reviewed changes

diegomedina248 added 4 commits June 2, 2022 13:15

fix: Support the Clipboard API in modern browsers

6231f6b

fix tests

a545d7e

PR comment

c8f4de9

Improvements

f02bf0b

diegomedina248 force-pushed the fix/create-permalink-in-safari branch from 25672fc to f02bf0b Compare June 2, 2022 16:54

villebro approved these changes Jun 3, 2022

View reviewed changes

stephenLYZ approved these changes Jun 3, 2022

View reviewed changes

stephenLYZ merged commit 0e38c68 into apache:master Jun 3, 2022

nytai mentioned this pull request Jun 6, 2022

Copy to clipboard fails in safari #18154

Closed

3 tasks

michael-s-molina added v1.5 and removed v1.5 labels Jun 24, 2022

michael-s-molina added the v1.5 label Aug 29, 2022

michael-s-molina pushed a commit that referenced this pull request Aug 30, 2022

fix: Support the Clipboard API in modern browsers (#20058)

e5de090

* fix: Support the Clipboard API in modern browsers * fix tests * PR comment * Improvements

michael-s-molina removed the v1.5 label Jan 3, 2023

mistercrunch added 🍒 1.5.2 🍒 1.5.3 🚢 2.0.0 🚢 2.1.0 and removed 🍒 1.5.3 🍒 1.5.2 🚢 2.0.0 labels Feb 18, 2024

mistercrunch added 🍒 1.5.2 🍒 1.5.3 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.0.0 and removed 🚢 2.1.0 labels Mar 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Support the Clipboard API in modern browsers #20058

fix: Support the Clipboard API in modern browsers #20058

diegomedina248 commented May 13, 2022

codecov bot commented May 13, 2022 •

edited

Loading

stephenLYZ commented May 16, 2022

github-actions bot commented May 16, 2022

stephenLYZ left a comment

stephenLYZ May 16, 2022 •

edited

Loading

diegomedina248 May 16, 2022

stephenLYZ commented May 17, 2022 •

edited

Loading

diegomedina248 commented May 20, 2022

stephenLYZ commented May 31, 2022

villebro left a comment

villebro Jun 2, 2022

villebro commented Jun 2, 2022 •

edited

Loading

villebro left a comment

villebro left a comment

stephenLYZ left a comment

github-actions bot commented Jun 3, 2022

isabellalacerda commented Jun 28, 2022 •

edited

Loading

fix: Support the Clipboard API in modern browsers #20058

fix: Support the Clipboard API in modern browsers #20058

Conversation

diegomedina248 commented May 13, 2022

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

codecov bot commented May 13, 2022 • edited Loading

Codecov Report

stephenLYZ commented May 16, 2022

github-actions bot commented May 16, 2022

stephenLYZ left a comment

Choose a reason for hiding this comment

stephenLYZ May 16, 2022 • edited Loading

Choose a reason for hiding this comment

diegomedina248 May 16, 2022

Choose a reason for hiding this comment

stephenLYZ commented May 17, 2022 • edited Loading

diegomedina248 commented May 20, 2022

stephenLYZ commented May 31, 2022

villebro left a comment

Choose a reason for hiding this comment

villebro Jun 2, 2022

Choose a reason for hiding this comment

villebro commented Jun 2, 2022 • edited Loading

villebro left a comment

Choose a reason for hiding this comment

villebro left a comment

Choose a reason for hiding this comment

stephenLYZ left a comment

Choose a reason for hiding this comment

github-actions bot commented Jun 3, 2022

isabellalacerda commented Jun 28, 2022 • edited Loading

codecov bot commented May 13, 2022 •

edited

Loading

stephenLYZ May 16, 2022 •

edited

Loading

stephenLYZ commented May 17, 2022 •

edited

Loading

villebro commented Jun 2, 2022 •

edited

Loading

isabellalacerda commented Jun 28, 2022 •

edited

Loading