Closed Bug 1197451 Opened 9 years ago Closed 8 years ago

Support clipboard access in open extension API

Categories

(WebExtensions :: Untriaged, defect)

defect
Not set
normal

Tracking

(firefox51 verified)

VERIFIED FIXED
mozilla51
Tracking Status
firefox51 --- verified

People

(Reporter: 4mr.minj, Assigned: robwu)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete, Whiteboard: [permissions]triaged)

Attachments

(2 files, 2 obsolete files)

Attached file webext-clip-test.xpi
Chrome uses[1] |clipboardRead| and |clipboardWrite| permissions for this. The actual copying is done with |document.execCommand('copy')| in the background context.

I attached a sample add-on that works on chrome (visit http://www.example.org to test). Firefox seems to silently ignore |execCommand| though.

I think this core functionality would be useful for add-on developers.

[1] https://developer.chrome.com/extensions/declare_permissions
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: [permissions]
Flags: blocking-webextensions-
Just to update the description, this is the error I see now in the latest nightly:
> document.execCommand('cut'/'copy') was denied because it was not called from inside a short running user-generated event handler.
I think we need at a minimum support for the clipboardWrite permission as per Chrome, so we can run execCommand('copy') as privileged code.

The "context of user-initiated code" affordance is insufficient because it precludes the use of the following between the user-initiated event and the copy (since it puts the copy outside of the event handler call stack):

- setTimeout (eg, to rate-limit a "click to copy")
- an async method (eg, chrome.tabs.query() as the source of text to copy)

Additionally, browser chrome events do not qualify as "context of user-initiated code", making it impossible in the absence of a clipboardWrite permission to copy to the clipboard in response to a user click on browser_action, page_action, or context menu item.
Whiteboard: [permissions] → [permissions]triaged
Example of an extension port that's blocked by this bug:

TabCopy - Chrome Web Store
https://chrome.google.com/webstore/detail/tabcopy/micdllihgoppmejpecmkilggmaagfdmb

XPI available at: tabcopy.com/firefox.3.0.xpiunsigned
I believe this bug is fundamentally due to FF's current limitation of prohibiting document.execCommand('copy') outside of the call stack of a user-initiated event handler, as addressed in bug 1185052.

FWIW, I created a page for testing a browser's support for both sync and async calls to document.execCommand('copy'): 

Clipboard Copy Test
http://hansifer.com/clipboardCopyTest.html
Assignee: nobody → rob
Bobby, I have a question about how you think expanded principals should work. Right now when we have a content script, its principal is an expanded principal that contains [<content page principal>, <principal for the add-on>]. The add-on principal has the add-on ID set on its origin attributes.

The only place that cares about this is MayLoadInternal. The expanded principal implementation of this iterates over the component principals and checks if they can load the given URI.

Rob's clipboard patch needs to determine, in execCommand, whether the currently running add-on has clipboard permission. He's taking SubjectPrincipal() and trying to get the add-on ID out of it. For content scripts, that doesn't work because the expanded principal doesn't have an add-on ID set.

One solution is to add a method to nsPrincipal that called AddonHasPermission or something. The nsExpandedPrincipal implementation would check each component principal, and codebase principals would check their origin attributes and then call into the add-on policy service.

Or we could add a GetAddonId method to nsPrincipal that returns the first add-on ID it finds in expanded principals.

Or we could set the add-on ID on expanded principal's origin attributes and be done. What do you think?
Flags: needinfo?(bobbyholley)
Boris, we're having a problem where we can't get the selection of an HTML element in a windowless docshell [1]. It returns null. Do you know if this should work, or who would be the right person to ask? I saw you were in bug 827585, which might be related. However, I think that the windowless docshell does actually have a pres shell.

In general, I'm having trouble figuring out where to even look for the problem.

[1] http://dev.searchfox.org/mozilla-central/rev/bcd7fc0f642b97c9b0a2618750e1788547aa8322/xpfe/appshell/nsAppShellService.cpp#494
Need to needinfo Boris tomorrow.
Flags: needinfo?(wmccloskey)
(In reply to Bill McCloskey (:billm) from comment #9)
> Boris, we're having a problem where we can't get the selection of an HTML
> element in a windowless docshell [1]. It returns null. Do you know if this
> should work, or who would be the right person to ask? I saw you were in bug
> 827585, which might be related. However, I think that the windowless
> docshell does actually have a pres shell.
> 
> In general, I'm having trouble figuring out where to even look for the
> problem.

I was also running into this for bug 1274775. I had just as much trouble tracing things out, but the conclusion I came to was that for some reason we don't have a pres shell. I haven't looked at anything in a debugger yet, though, so I could be misreading something.
(In reply to Bill McCloskey (:billm) from comment #8)
> One solution is to add a method to nsPrincipal that called
> AddonHasPermission or something. The nsExpandedPrincipal implementation
> would check each component principal, and codebase principals would check
> their origin attributes and then call into the add-on policy service.

This approach definitely feels like the most correct to me - I'd prefer something that doesn't fall down if we happen to have an nsEP containing two principals with different addon IDs. The union approach to permissions makes sense to me, and we could even make it a virtual method on BasePrincipal if we wanted to.
Flags: needinfo?(bobbyholley)
> However, I think that the windowless docshell does actually have a pres shell.

In general it should, yes...  At least as I recall.

> In general, I'm having trouble figuring out where to even look for the problem.

Which API are you using to try to get the selection?  window.getSelection?  If so, are you taking any of the early returns in nsGlobalWindow::GetSelectionOuter, or is presShell->GetCurrentSelection() returning null?
(In reply to Boris Zbarsky [:bz] from comment #13)
> > However, I think that the windowless docshell does actually have a pres shell.
> 
> In general it should, yes...  At least as I recall.

I'm running into a similar problem for bug 1274775, and we're definitely
winding up without a pres shell here:

http://dev.searchfox.org/mozilla-central/source/dom/canvas/CanvasRenderingContext2D.cpp#3383

Looking at this in a debugger, we definitely do not have a pres shell in the
<browser> that we load the background page into. Its parent document, which
is in a windowless browser, though, does.
> The range we get in the last frame is null

OK.  So at that point you have a non-null aPresShell and aSelection, but there are no ranges in the selection?  I guess so, given you reached that line at all.

The caller passes in null for the selection, so the thing worth checking is what happens under nsCopySupport::GetSelectionForCopy and especially the PresShell::GetSelectionControllerForFocusedContent call it makes.  In particular, is the textarea coming back as the focusedContent in that last function?

> and we're definitely winding up without a pres shell here

Sounds like a different issue from Bill's.  In your case, it's worth checking whether your <browser> is display:none or for some other reason has no frame....
Flags: needinfo?(bzbarsky)
(In reply to Boris Zbarsky [:bz] from comment #16)
> > and we're definitely winding up without a pres shell here
> 
> Sounds like a different issue from Bill's.  In your case, it's worth
> checking whether your <browser> is display:none or for some other reason has
> no frame....

We're talking about the same frame, so if it somehow has a pres shell at that
point, I'd be interested to know how.

According to getComputedStyle, the <browser> has visibility: visible and
display: inline. But getComputedStyle() returns a 0x0 rectangle, and according
to the debugger, no element in the document is ever getting a layout frame,
and nsSubDocumentFrame::Init is never being called, so it doesn't wind up with
a parent widget or a pres shell.

(In reply to Bill McCloskey (:billm) from comment #14)
> The range we get in the last frame is null. I assume this means it thinks
> nothing is selected. This is the test code we used:
> 
>     var d = document.createElement('textarea')
>     d.value = 'testdeded'
>     document.body.appendChild(d);
>     d.select();
>     return document.execCommand('copy');

Someone reported an issue with this when the call was coming from a key event
handler in another document, and I came to the conclusion that .select() was
failing because it was failing to focus the select element. It might be a
different issue here, though.
> Someone reported an issue with this when the call was coming from a key event
> handler in another document, and I came to the conclusion that .select() was
> failing because it was failing to focus the select element. It might be a
> different issue here, though.

Our test case shows that the copy action succeeds when the script is executed in a real tab instead of the background page, so that is probably a different issue.
(In reply to Rob Wu from comment #18)
> Our test case shows that the copy action succeeds when the script is
> executed in a real tab instead of the background page, so that is probably a
> different issue.

Right, that was the issue. The select() call was failing in the background page because the element couldn't be focused. Or, at least, I confirmed that the select call was failing to select or focus the element. There may have been other problems once that succeeded.
The only solution I've found to the pres shell issue so far is to use a XUL document rather than about:blank in the top-level hidden window.

I'll have a patch for that later today, but once it's fixed, I don't know whether or not there will still be focus/selection issues.
Depends on: 1274775
The patch adds support for clipboardWrite in content scripts and visible extension pages. Background pages are not supported yet for the reasons mentioned before.
Attachment #8769782 - Flags: review?(wmccloskey)
Bug 1272869 already exists for the problems in background pages, so let's treat that as a separate bug.
See Also: → 1272869
Comment on attachment 8769782 [details] [diff] [review]
Bug 1197451 - Add clipboardWrite permission r=billm

Review of attachment 8769782 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks Rob! This looks really close. I've mostly asked for changes related to the method signature. Once you've made the changes, please post a new patch for review. It should go very quickly then.

::: caps/BasePrincipal.cpp
@@ +559,4 @@
>  }
>  
>  NS_IMETHODIMP
> +BasePrincipal::HasAddonPermission(const nsAString& aPerm, bool* _retval)

I would call this AddonHasPermission (similar to AddonAllowsLoad). Also, _retval should be aResult or aSomething. However, this doesn't need to be an XPCOM method since it isn't called from JS. So you can just use the signature:

bool
BasePrincipal::AddonHasPermission(const nsAString& aPerm)

@@ +562,5 @@
> +BasePrincipal::HasAddonPermission(const nsAString& aPerm, bool* _retval)
> +{
> +  *_retval = false;
> +  nsresult rv = NS_OK;
> +  if (!mOriginAttributes.mAddonId.IsEmpty()) {

It would be a little easier to write this as:
  if (mOriginAttributes.mAddonId.IsEmpty()) {
    return false;
  }

::: caps/nsIPrincipal.idl
@@ +355,5 @@
> +
> +    /**
> +     * Returns true if this principal is granted the |aPerm| permission.
> +     */
> +    boolean HasAddonPermission(in AString aPerm);

No need to have this here at all.

::: caps/nsPrincipal.cpp
@@ +786,5 @@
>    return NS_ERROR_NOT_AVAILABLE;
>  }
>  
> +NS_IMETHODIMP
> +nsExpandedPrincipal::HasAddonPermission(const nsAString& aPerm, bool* _retval)

You'll have to define this method on nsExpandedPrincipal with the changes I've suggested. Please mark it "override".

@@ +790,5 @@
> +nsExpandedPrincipal::HasAddonPermission(const nsAString& aPerm, bool* _retval)
> +{
> +  nsresult rv = NS_OK;
> +  for (size_t i = 0; i < mPrincipals.Length() && !*_retval; ++i) {
> +    rv = mPrincipals[i]->HasAddonPermission(aPerm, _retval);

You'll need to use BasePrincipal::Cast(mPrincipals[i]) here with the change I've suggested.

::: toolkit/components/extensions/test/mochitest/test_clipboard.html
@@ +17,5 @@
> +  let field = document.createElement("textarea");
> +  document.body.appendChild(field);
> +  field.value = txt;
> +  field.select();
> +  return document.execCommand("copy");

I'm a little worried that we're not actually testing that the data ends up in the clipboard. Search for waitForClipboard to see how you can be stricter here.
Attachment #8769782 - Flags: review?(wmccloskey)
Please ask :mrbkap (Blake Kaplan) for review on the new patch since I'm not a peer for the principal code.
Comment on attachment 8769782 [details] [diff] [review]
Bug 1197451 - Add clipboardWrite permission r=billm

Review of attachment 8769782 [details] [diff] [review]:
-----------------------------------------------------------------

I've addressed the signature change request in the new patch.

::: toolkit/components/extensions/test/mochitest/test_clipboard.html
@@ +17,5 @@
> +  let field = document.createElement("textarea");
> +  document.body.appendChild(field);
> +  field.value = txt;
> +  field.select();
> +  return document.execCommand("copy");

waitForClipboard is used, below. This "doCopy" method is used in a background page / content script, so it does not have access to test utilities such as waitForClipboard.
Blake, could you review the principal code?
Attachment #8769782 - Attachment is obsolete: true
Attachment #8770341 - Flags: review?(mrbkap)
I'll get to this review tomorrow.
Comment on attachment 8770341 [details] [diff] [review]
Bug 1197451 - Add clipboardWrite permission r=billm

Review of attachment 8770341 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.

::: toolkit/components/extensions/test/mochitest/test_clipboard.html
@@ +41,5 @@
> +  yield extension.unload();
> +  info("extension unloaded");
> +});
> +
> +/** Selecting text in a bg page is not possible, skip test until it's fixed.

As a note, I find commenting out this test a bit odd. Is there any reason you couldn't have this be a line comment and simply not add_task the function (but leave it defined to be enabled later)?
Attachment #8770341 - Flags: review?(mrbkap) → review+
(In reply to Blake Kaplan (:mrbkap) from comment #28)
> Comment on attachment 8770341 [details] [diff] [review]
> Bug 1197451 - Add clipboardWrite permission r=billm
> 
> Review of attachment 8770341 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good.
> 
> ::: toolkit/components/extensions/test/mochitest/test_clipboard.html
> @@ +41,5 @@
> > +  yield extension.unload();
> > +  info("extension unloaded");
> > +});
> > +
> > +/** Selecting text in a bg page is not possible, skip test until it's fixed.
> 
> As a note, I find commenting out this test a bit odd. Is there any reason
> you couldn't have this be a line comment and simply not add_task the
> function (but leave it defined to be enabled later)?

I just commented out the code so that syntax highlighters clearly mark the code as disabled.
I think that turning it into a free named function would cause eslint (the JavaScript linter) to complain about an unused variable.
Doesn't this need checkin-needed?
Comment on attachment 8770341 [details] [diff] [review]
Bug 1197451 - Add clipboardWrite permission r=billm

I rebased and published the updated patch on reviewboard.
Attachment #8770341 - Attachment is obsolete: true
Attachment #8777985 - Flags: review?(wmccloskey) → review+
It would be nice to have this documented. I hope that in WebExtensions, the copy-to-clipboard feature is available in a more clear and straightforward syntax than the odd redundant "create a textarea, assign the string to copy to its value, select its contents, call document.execCommand('copy')" approach available for web content. Something like `copyString('example')` like in XPCOM. Thanks.
Keywords: checkin-needed
Pushed by kwierso@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/2263d29df716
Add clipboardWrite permission r=billm
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/2263d29df716
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Notes for documentation:
The clipboardWrite permission is implemented, but clipboardRead ("paste") is not implemented yet.

Also due to the inability to focus an input field (and/or selecting text) in the background page, document.execCommand('copy') does not copy anything in the background page. So this feature is limited to extension tabs, popups and content scripts.
(In reply to Rob Wu [:robwu] from comment #37)
> Notes for documentation:
> The clipboardWrite permission is implemented, but clipboardRead ("paste") is
> not implemented yet.
> 
> Also due to the inability to focus an input field (and/or selecting text) in
> the background page, document.execCommand('copy') does not copy anything in
> the background page. So this feature is limited to extension tabs, popups
> and content scripts.

Rob, can you clarify the use cases enabled by this fix? Using document.execCommand('copy') is already possible in FF for simple cases without the need for a clipboardWrite perm. In Chrome, this perm is just a formality and does nothing for extensions.

Does the clipboardWrite perm address the current FF limitation that prevents calling document.execCommand('copy') from an extension's browser_action handler, page_action handler, context menu item handler, or keyboard shortcut command handler without raising the "denied because it was not called from inside a short running user-generated event handler" exception?
Flags: needinfo?(rob)
(In reply to Hans Meyer from comment #38)
> Does the clipboardWrite perm address the current FF limitation that prevents
> calling document.execCommand('copy') from an extension's browser_action
> handler, page_action handler, context menu item handler, or keyboard
> shortcut command handler without raising the "denied because it was not
> called from inside a short running user-generated event handler" exception?

Indeed, the clipboardWrite permission removes the need for being in a user gesture in order to modify the clipboard. However, it can still not copy text *in the background page* because of a bug (see comment 37).
Flags: needinfo?(rob)
(In reply to Rob Wu [:robwu] from comment #39)
> However, it can still not copy
> text *in the background page* because of a bug (see comment 37).

Apparently there are methods that work without needing a selection that work fine in background pages, like the one from http://stackoverflow.com/questions/3436102/copy-to-clipboard-in-chrome-extension/12693636#12693636
(In reply to Martin Giger [:freaktechnik] from comment #40)
> Apparently there are methods that work without needing a selection that work
> fine in background pages, like the one from
> http://stackoverflow.com/questions/3436102/copy-to-clipboard-in-chrome-
> extension/12693636#12693636

Neat. We should document that.
(In reply to Rob Wu [:robwu] from comment #39)
> Indeed, the clipboardWrite permission removes the need for being in a user
> gesture in order to modify the clipboard. However, it can still not copy
> text *in the background page* because of a bug (see comment 37).

That's awesome Rob. Thanks for the clarification.

I should correct the following statement I made, as I misunderstood Chrome's documentation:

> In Chrome, [clipboardWrite] perm is just a formality and does nothing for extensions.

More accurately: 

Chrome is more permissive than FF about what constitutes a user gesture context. Namely, it allows document.execCommand('copy') to be called *asynchronously*, as long as the call occurs within 1 second of a user gesture. 

However, to allow the call *entirely independently* of a user gesture, the clipboardWrite permission is indeed required for Chrome extensions.
(In reply to Hans Meyer from comment #42)
> (In reply to Rob Wu [:robwu] from comment #39)
> > Indeed, the clipboardWrite permission removes the need for being in a user
> > gesture in order to modify the clipboard. However, it can still not copy
> > text *in the background page* because of a bug (see comment 37).
> 
> That's awesome Rob. Thanks for the clarification.
> 
> I should correct the following statement I made, as I misunderstood Chrome's
> documentation:
> 
> > In Chrome, [clipboardWrite] perm is just a formality and does nothing for extensions.
> 
> More accurately: 
> 
> Chrome is more permissive than FF about what constitutes a user gesture
> context. Namely, it allows document.execCommand('copy') to be called
> *asynchronously*, as long as the call occurs within 1 second of a user
> gesture. 
> 
> However, to allow the call *entirely independently* of a user gesture, the
> clipboardWrite permission is indeed required for Chrome extensions.

Welp, never mind. Ignore my correction in Comment 42. It seems I was right the first time and that the clipboardWrite perm is not required at all for document.execCommand('copy') calls in Chrome extensions, even outside of user gestures.

Sorry for infecting this thread with my confusion.
(In reply to Kris Maglione [:kmag] from comment #41)
> (In reply to Martin Giger [:freaktechnik] from comment #40)
> > Apparently there are methods that work without needing a selection that work
> > fine in background pages, like the one from
> > http://stackoverflow.com/questions/3436102/copy-to-clipboard-in-chrome-
> > extension/12693636#12693636
> 
> Neat. We should document that.

This does not work for me in Firefox (the oncopy listener is not called). It does work in Chrome though.
(In reply to Will Bamberg [:wbamberg] from comment #44)
Looks like this works in Firefox, but requires an explicit user action like clicking a button. In other words, this does NOT work in `DOMContendLoaded` listener, but DOES work in `click` listener. Working JS code:

    var copy = function(str, mimetype) {
        document.oncopy = function(event) {
            event.clipboardData.setData(mimetype, str);
            event.preventDefault();
        };
    
        document.execCommand('copy', false, null);
    };
    
    document.addEventListener('DOMContentLoaded', function() {
        document.getElementById('test-button').addEventListener('click', function() {
            copy('Hello, world', 'text/plain');
        });
    }, false);
(In reply to Marat Tanalin | tanalin.com from comment #45)
> (In reply to Will Bamberg [:wbamberg] from comment #44)
> Looks like this works in Firefox, but requires an explicit user action like
> clicking a button. In other words, this does NOT work in `DOMContendLoaded`
> listener, but DOES work in `click` listener. Working JS code:
> 
>     var copy = function(str, mimetype) {
>         document.oncopy = function(event) {
>             event.clipboardData.setData(mimetype, str);
>             event.preventDefault();
>         };
>     
>         document.execCommand('copy', false, null);
>     };
>     
>     document.addEventListener('DOMContentLoaded', function() {
>         document.getElementById('test-button').addEventListener('click',
> function() {
>             copy('Hello, world', 'text/plain');
>         });
>     }, false);

I meant, it does not work in the background page, which I think was the context of comment 40. I'm not sure how a user action can happen in a background page.
(In reply to Will Bamberg [:wbamberg] from comment #46)
You don't have to quote the entire message you're replying to.

Substantively, the code I've provided does work in a regular nonprivileged webpage with a button (that confirms this can work in Firefox). If called in a WebExtensions-powered-addon's background page, copying a string this way, as expected, does not work (since it isn't caused by a user's action), and the following message is logged into console:

> document.execCommand(‘cut’/‘copy’) was denied because it was not called from inside a short running user-generated event handler.
:robwu, I've written something on this: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Interact_with_the_clipboard. Please let me know if I'm missing anything important.
Flags: needinfo?(rob)
lgtm
Flags: needinfo?(rob)
Is there a follow-up bug for clipboardRead?
Flags: needinfo?(rob)
See Also: → 1312260
Now there is: bug 1312260
Flags: needinfo?(rob)
I can reproduce this issue on Firefox 51.0a2 (20160811030201) under Windows 7 64-bit, here is a video: http://screencast.com/t/ijcvdninWyP 


This issue is verified as fixed on Firefox 51.0a1 (20160813030202) and Firefox 52.0a1 (20161106030203) under Windows 7 64-bit,there are no errors displayed in the console, here is a video: http://screencast.com/t/kzuwkQZ6v7
Status: RESOLVED → VERIFIED
For the record:

>    function copy(str, mimetype) {
>        document.oncopy = function(event) {
>            event.clipboardData.setData(mimetype, str);
>            event.preventDefault();
>        };
>        document.execCommand("Copy", false, null);
>    }

also isn't working due to bug 995961. But this would be a nice solution to get clipboard write working without having bug 1272869 fixed.
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.