Skip to content

Disable unsupported context menu options for Firefox #17668

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

Conversation

bvaughn
Copy link
Contributor

@bvaughn bvaughn commented Dec 19, 2019

Unfortunately the "copy to clipboard" and "go to definition" context menu options do not work for the Firefox add-on. I'm not sure why "go to definition" doesn't work. (Seems like it should but it just fails silently.) The clipboard option seems more obviously broken though.

We currently use document.execCommand("copy") which works for Chrome but fails for Firefox with the following error:
image

Unfortunately, using the newer Clipboard API fails for both browsers:
image

So for now it seems the best we can do is stick with the old execCommand API and disable the menu option for Firefox.

Chrome demo

Context menu in Chrome

Firefox demo

Context menu in Firefox

@codesandbox-ci
Copy link

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit f8f2440:

Sandbox Source
confident-cohen-j6g9n Configuration

@bvaughn
Copy link
Contributor Author

bvaughn commented Dec 19, 2019

Tagging a few random reviewers who (I think) aren't yet on PTO 😄

@sizebot
Copy link

sizebot commented Dec 19, 2019

No significant bundle size changes to report.

Size changes (experimental)

Generated by 🚫 dangerJS against f8f2440

@sizebot
Copy link

sizebot commented Dec 19, 2019

No significant bundle size changes to report.

Size changes (stable)

Generated by 🚫 dangerJS against f8f2440

@digitarald
Copy link

(Seems like it should but it just fails silently.) The clipboard option seems more obviously broken though.

inspect should work (per @jasonLaster). Maybe we can have a follow up to investigate.

@bvaughn
Copy link
Contributor Author

bvaughn commented Dec 20, 2019

@digitarald inspect works when run manually in the console, but it does not seem to work when run via an extension.

@jasonLaster
Copy link

@bvaughn that's disappointing, but maybe it shouldn't be too surprising as I did not test it. My hope was that console commands, would just work in the extension.

CC @nchevobbe @rpl

@bvaughn
Copy link
Contributor Author

bvaughn commented Dec 20, 2019

@jasonLaster No worries! Here's a pointer to the code:

const viewAttributeSourceFunction = (id, path) => {
const rendererID = store.getRendererIDForElement(id);
if (rendererID != null) {
// Ask the renderer interface to find the specified attribute,
// and store it as a global variable on the window.
bridge.send('viewAttributeSource', {id, path, rendererID});
setTimeout(() => {
// Ask Chrome to display the location of the attribute,
// assuming the renderer found a match.
chrome.devtools.inspectedWindow.eval(`
if (window.$attribute != null) {
inspect(window.$attribute);
}
`);
}, 100);
}
};

The way this works currently is the frontend sends a postMessage command saying to inspect an attribute at a specific path (e.g. some-fiber-id -> props -> foo -> bar). The "renderer" (which is injected to run in the page in the same context as React itself) stores the value at the specified path in a global, window.$attribute. Then the extension itself asks the browser to inspect() it.

The extension is involved because JavaScript running in the document itself can't call inspect() since it's part of the DevTools API and not a standard JavaScript feature. In Firefox though, it seems to just silently fail when the extension calls inspect().

@rpl
Copy link

rpl commented Dec 20, 2019

In firefox the interaction with the clipboard from an extension requires additional permissions ("clipboardRead" and/or "clipboardWrite"), some details about that are available in the following MDN doc page:

In particular this section may be helpful: https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/Interact_with_the_clipboard#Browser-specific_considerations

(and in case it may be also helpful, here are the related test cases from mozilla-central: https://searchfox.org/mozilla-central/source/toolkit/components/extensions/test/mochitest/test_ext_clipboard.html)

I just gave a quick look and (at least if I'm reading it correctly) currently document.execCommand("copy") seems to be executed in the webpage, but to leverage the extension permissions it should be executed from the extension content script attached to that page.

About the issue with inspect, the current behavior on Firefox is the following:

  • If the parameter passed to inspect is a DOM node, the developer toolbox should be switched to the "Inspector" panel and the given element should be selected
  • If the parameter is a javascript object, the given object should be shown in the webconsole sidebar panel (which should also be opened automatically)

devtools.inspectedWindow.eval("inspect(...)") seems to also be used by the extension to select the DOM element related to the currently selected React component instance, and that seems to be working as expected (I just tried to use that by building the react devtools extension from this repo)

And so I'm wondering what is set on the $attribute property? is it a plain js object? which is the behavior expected by the React DevTools extension?

@jasonLaster @bvaughn would you mind to give me some steps to be able to reproduce this issue locally?
I gave a quick look and I was immediately able to trigger the issue with the clipboard but I didn't found the context menu item to trigger the issue with inspect (and I'm wondering if it has been already disabled on Firefox or I was just not looking to the right context menu).

@bvaughn
Copy link
Contributor Author

bvaughn commented Dec 20, 2019

@rpl

In firefox the interaction with the clipboard from an extension requires additional permissions ("clipboardRead" and/or "clipboardWrite"), some details about that are available in the following MDN doc page:

I mentioned this in the Firefox Slack but I guess I forgot to add it to this PR- I already tried adding the clipboardWrite permission to the Firefox manifest. It did not change the reported behavior. Using the newer Clipboard API to copy a value in the document/page context just prints an error in the console: uncaught exception: undefined.

I pushed this test to a branch (DevTools-update-clipboard-API) if you'd be curious to run it.

I just gave a quick look and (at least if I'm reading it correctly) currently document.execCommand("copy") seems to be executed in the webpage, but to leverage the extension permissions it should be executed from the extension content script attached to that page.

The reason execCommand is run within the context of the document/page is that the data being copied exists within that context. Transferring it between contexts (via postMessage) could be extremely slow in cases where the data is large.

In general, DevTools avoids sending data across the postMessage bridge unless necessary- because it's a choke point. For props and things that are inspected, it only sends a shallow copy. As the user explores the props in the UI, it sends a little more info as they drill down- gradually building up the representation in the other context. This prevents significant performance problems that were reported back when we sent the full data across.

devtools.inspectedWindow.eval("inspect(...)") seems to also be used by the extension to select the DOM element related to the currently selected React component instance, and that seems to be working as expected (I just tried to use that by building the react devtools extension from this repo)

Neither inspect use case works fo rme, at least not will Firefox 71.0 (64-bit) / Mac OS 10.14.6 (18G2022). I'm surprised to hear it works for you. Can you share some more information about which browser version and OS you're trying this on?

@bvaughn
Copy link
Contributor Author

bvaughn commented Dec 20, 2019

I'm going to merge this for now so the feature doesn't block an extension release to the browser stores. I'll be happy to follow up and re-add support for Firefox in a future PR if we figure out a path forward that satisfies the above constraints.

@bvaughn bvaughn merged commit 7973477 into facebook:master Dec 20, 2019
@bvaughn bvaughn deleted the devtools-disable-context-menu-copy-for-firefox branch December 20, 2019 16:31
@jasonLaster
Copy link

I filed #17681 as a follow up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants