Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix[devtools]: use native clipboard api instead of clipboard-js #26539

Closed
wants to merge 1 commit into from

Conversation

hoxyq
Copy link
Contributor

@hoxyq hoxyq commented Apr 3, 2023

Fixes #26500

Summary

  • Bumping min version for Firefox to 63, the version in which Clipboard API is fully supported
  • Using Clipboard API instead of clipboard-js lib

How did you test this change?

  • Tested electron app
  • Tested react-devtools-inline app

@hoxyq hoxyq requested a review from mondaychen April 3, 2023 13:47
@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Apr 3, 2023
@react-sizebot
Copy link

react-sizebot commented Apr 3, 2023

Comparing: 790ebc9...4fd7d9a

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js = 165.19 kB 165.19 kB = 51.87 kB 51.87 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 166.77 kB 166.77 kB = 52.39 kB 52.39 kB
facebook-www/ReactDOM-prod.classic.js = 555.36 kB 555.36 kB = 98.44 kB 98.44 kB
facebook-www/ReactDOM-prod.modern.js = 539.21 kB 539.21 kB = 95.77 kB 95.77 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against 4fd7d9a

@@ -49,12 +48,11 @@ export function copyToClipboard(value: any): void {
// On Firefox navigator.clipboard.writeText has to be called from
// the content script js code (because it requires the clipboardWrite
// permission to be allowed out of a "user handling" callback),
// clipboardCopyText is an helper injected into the page from.
// injectGlobalHook.
// clipboardCopyText is a helper injected into the page from injectGlobalHook.
if (typeof clipboardCopyText === 'function') {
clipboardCopyText(text).catch(err => {});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you use this opportunity this verify if this is still needed? it has been a few years since this workaround was introduced #17740 #17681

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you use this opportunity this verify if this is still needed? it has been a few years since this workaround was introduced #17740 #17681

Nothing changed for Firefox, as I understand. If we want to use clipboard.writeText, we need one of the two things:

  • Asking for "clipboardWrite" permission. Not sure if this should be each time before calling clipboard api
  • This api to be called only in user-initiated event callbacks, which will be won't work for us, since we are operating mostly as an extension

I can later test it on Firefox with this injection removed and in extension environment

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mondaychen

We still need this injection, because of the one use case when we are pressing "Copy value to clipboard" on some attributes of the inspected elements, like prop or field in state, or hook.

Everything from this explanation comment is actual.

Seems like this is the only copy operation that we do via bridge and we call clipboard api on the backend side. Since there is no user-initiated event and Firefox thinks that this is not coming from browser extension, it rejects the call.

@hoxyq hoxyq force-pushed the fix/use-native-clipboard-api branch from 414ec1b to 7b3c172 Compare April 4, 2023 14:55
@hoxyq hoxyq force-pushed the fix/use-native-clipboard-api branch from 7b3c172 to 3d1c3ae Compare April 5, 2023 10:43
@hoxyq hoxyq force-pushed the fix/use-native-clipboard-api branch from 3d1c3ae to 4fd7d9a Compare April 5, 2023 12:01
@hoxyq hoxyq marked this pull request as ready for review April 5, 2023 12:09
@mondaychen
Copy link
Contributor

Did you try it on extensions? I got this error:
image

Seems to be relevant: https://stackoverflow.com/questions/56306153/domexception-on-calling-navigator-clipboard-readtext

@hoxyq
Copy link
Contributor Author

hoxyq commented Apr 5, 2023

Did you try it on extensions? I got this error: image

Seems to be relevant: https://stackoverflow.com/questions/56306153/domexception-on-calling-navigator-clipboard-readtext

Good catch, I've tested it on Firefox, looks like this error occurs only when we try to call clipboard.writeText from backend. Looking into it

UPD: probably need to reiterate once again on this, we should not use browsers api like clipboard from backend process

@hoxyq
Copy link
Contributor Author

hoxyq commented Apr 12, 2023

Closing this in favour of #26604.

Had some issues with making navigator.clipboard api to work in Chrome:

  1. Looks like all DevTools panels are running in an iframe, Chrome requires document to be focused before using clipboard api.
  2. Even if I will focus window manually via window.focus(), Chrome will restrict from accessing clipboard, because container (parent) iframe doesn't have clipboard-write permission in allow attribute. I don't think we can somehow change this.

However, there were no problems with this approach in Firefox.

@hoxyq hoxyq closed this Apr 12, 2023
hoxyq added a commit that referenced this pull request Apr 12, 2023
Fixes #26500

## Summary
- No more using `clipboard-js` from the backend side, now emitting
custom `saveToClipboard` event, also adding corresponding listener in
`store.js`
- Not migrating to `navigator.clipboard` api yet, there were some issues
with using it on Chrome, will add more details to
#26539

## How did you test this change?
- Tested on Chrome, Firefox, Edge
- Tested on standalone electron app: seems like context menu is not
expected to work there (cannot right-click on value, the menu is not
appearing), other logic (pressing on copy icon) was not changed
kassens pushed a commit to kassens/react that referenced this pull request Apr 17, 2023
…#26604)

Fixes facebook#26500

## Summary
- No more using `clipboard-js` from the backend side, now emitting
custom `saveToClipboard` event, also adding corresponding listener in
`store.js`
- Not migrating to `navigator.clipboard` api yet, there were some issues
with using it on Chrome, will add more details to
facebook#26539

## How did you test this change?
- Tested on Chrome, Firefox, Edge
- Tested on standalone electron app: seems like context menu is not
expected to work there (cannot right-click on value, the menu is not
appearing), other logic (pressing on copy icon) was not changed
@hoxyq hoxyq deleted the fix/use-native-clipboard-api branch May 2, 2023 08:58
EdisonVan pushed a commit to EdisonVan/react that referenced this pull request Apr 15, 2024
…#26604)

Fixes facebook#26500

## Summary
- No more using `clipboard-js` from the backend side, now emitting
custom `saveToClipboard` event, also adding corresponding listener in
`store.js`
- Not migrating to `navigator.clipboard` api yet, there were some issues
with using it on Chrome, will add more details to
facebook#26539

## How did you test this change?
- Tested on Chrome, Firefox, Edge
- Tested on standalone electron app: seems like context menu is not
expected to work there (cannot right-click on value, the menu is not
appearing), other logic (pressing on copy icon) was not changed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[DevTools Bug]: copy operations don't work in Chrome
4 participants