Closed Bug 1312260 Opened 8 years ago Closed 7 years ago

Support clipboardRead permission to read from the clipboard

Categories

(WebExtensions :: General, defect, P2)

Product:
WebExtensions
Component:
General
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
mozilla54
Blocking Flags:
webextensions +

People

(Reporter: robwu, Assigned: zombie)

References

Details

(Keywords: dev-doc-complete, Whiteboard: triaged)

Attachments

(5 files, 1 obsolete file)

Bug 1312260 - Part 1: Allow access to execCommand("paste") with permission
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details
Bug 1312260 - Part 2: Test access to "paste" command with clipboardRead permission
59 bytes, text/x-review-board-request
aswan
: review+
Details
Bug 1312260 - Part 3: Fix trailing whitespace in nsHTMLDocument.cpp
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details
bug-clipboardread-1.1.0.zip
970 bytes, application/zip
Details
bug-clipboardread-2.0.0.zip
1.08 KB, application/zip
Details 
Bug 1197451 added the ability to write to the clipboard without user interaction, but reading without user action by document.execCommand('paste') is still not implemented.
Priority: -- → P2
Whiteboard: triaged
webextensions: --- → +
Assignee: nobody → tomica
Status: NEW → ASSIGNED
Attachment #8834705 - Flags: review?(bzbarsky)
Attachment #8834706 - Flags: review?(aswan)
Comment on attachment 8834705 [details]
Bug 1312260 - Part 1: Allow access to execCommand("paste") with permission

https://reviewboard.mozilla.org/r/110540/#review112084

::: dom/html/nsHTMLDocument.cpp:659
(Diff revision 1)
>    // from that for the document if the channel is a wyciwyg channel.
>    int32_t parserCharsetSource;
>    nsAutoCString parserCharset;
>  
>    nsCOMPtr<nsIWyciwygChannel> wyciwygChannel;
> -  
> +

Sorry for the whitespace noise, didn't even notice my editor does that on save.  Though unless you object, it's still correct to do.
Comment on attachment 8834705 [details]
Bug 1312260 - Part 1: Allow access to execCommand("paste") with permission

https://reviewboard.mozilla.org/r/110540/#review112086

r=me with the whitespace changes pulled out into a separate changeset.

::: dom/html/nsHTMLDocument.cpp:659
(Diff revision 1)
>    // from that for the document if the channel is a wyciwyg channel.
>    int32_t parserCharsetSource;
>    nsAutoCString parserCharset;
>  
>    nsCOMPtr<nsIWyciwygChannel> wyciwygChannel;
> -  
> +

Please pull the whitespace munging out into a separate changeset.  I agree it's the right thing to do, but not as part of substantive changes.  No need for a separate _bug_; just a separate changeset.
Attachment #8834705 - Flags: review?(bzbarsky) → review+
Comment on attachment 8834706 [details]
Bug 1312260 - Part 2: Test access to "paste" command with clipboardRead permission

https://reviewboard.mozilla.org/r/110542/#review112124

Looking good, but please add a permission string to browser.properties.  It looks like we don't have a string for the existing clipboardWrite permission, can you just add that here as well?  (you can file a separate bug if you like but I don't think it is necessary).  Scott Devaney can provide the contents of the permission string (though you can suggest something to him)

::: toolkit/components/extensions/test/mochitest/test_clipboard.html
(Diff revision 1)
>  <head>
> -  <title>clipboard permission test</title>
> +  <title>Clipboard permissions tests</title>
>    <script src="/tests/SimpleTest/SimpleTest.js"></script>
>    <script src="/tests/SimpleTest/SpawnTask.js"></script>
>    <script src="/tests/SimpleTest/ExtensionTestUtils.js"></script>
> -  <script src="head.js"></script>

why did you remove this?
> why did you remove this?
I started this test in a separate file, but botched the move into this own.


> Looking good, but please add a permission string to browser.properties.  It
> looks like we don't have a string for the existing clipboardWrite
> permission, can you just add that here as well?  (you can file a separate
> bug if you like but I don't think it is necessary).  Scott Devaney can
> provide the contents of the permission string (though you can suggest
> something to him)

Hey Scott, can you please provide the strings for clipboardRead and clipboardWrite permissions?  Here is the MDN description of what they allow: 

https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/permissions#Clipboard_access
Flags: needinfo?(sdevaney)
How about one message to accommodate both permissions? Like so:

Read/write to clipboard
Flags: needinfo?(sdevaney)
Attachment #8835556 - Flags: review?(bzbarsky)
(In reply to sdevaney from comment #7)
> How about one message to accommodate both permissions? Like so:
> 
> Read/write to clipboard

I don't think that would work without code changes to avoid duplicating the text when an extension asks for both (which is outside this bug), so I went with separate text strings.
Comment on attachment 8834706 [details]
Bug 1312260 - Part 2: Test access to "paste" command with clipboardRead permission

https://reviewboard.mozilla.org/r/110542/#review112466

r=me assuming we get consensus on the prompt language

::: browser/locales/en-US/chrome/browser/browser.properties:85
(Diff revision 2)
> +webextPerms.description.clipboardRead=Read from clipboard
> +webextPerms.description.clipboardWrite=Write to clipboard

I don't want to turn this into a bikeshed but two things:
- lets fix the grammar, ie "Read from the clipboard"
- I'm more conflicted about this but I think unsophisticated users might not easily grasp read and write here, maybe this would make more sense as something like "paste data from the clipboard" and "copy data into the clipboard"?  The downside is that that is less accurate which will agitate all the more technical users...
Attachment #8834706 - Flags: review?(aswan) → review+
Hey Scott, can you please chime in here again?
Flags: needinfo?(sdevaney)
Comment on attachment 8835556 [details]
Bug 1312260 - Part 3: Fix trailing whitespace in nsHTMLDocument.cpp

https://reviewboard.mozilla.org/r/111248/#review112572

r=me
Attachment #8835556 - Flags: review?(bzbarsky) → review+
Hm, do these two versions work for the permissions? 


Copy data from the clipboard

Insert data into the clipboard
Flags: needinfo?(sdevaney)
Those versions are accurate, but I just want to point out that the action described as "Copy data from the clipboard" is familiar to users as the common "Paste" operation and the action described as "Insert data into the clipbaord" is familiar as "Cut" or "Copy" so there's some potential for confusion.
Yeah that is confusing. aswan if you prefer it, I'm okay with:

Paste data from the clipboard

Copy data to the clipboard
I'm hesitant to express any opinion on this (to prolong it even more), especially as a non-native speaker, but I propose:

> Read data from the clipboard
> Copy data to the clipboard

As a user, "paste" is an operation that makes a thing from the clipboard appear in the document in front of me -- an action with a visible effect.  Giving an extension a permission to "Paste" doesn't sound like it should be able to silently/invisibly "spy" on my clipboard.  I think "Read" makes that a bit more explicit.
> As a user [...]

Note that I'm not "trying to think like a user" here, I honestly think what I wrote above.  Calling that operation "Paste" is confusing and unexpected to me.  

In fact, most APIs name this operation as either "Read" or "Get" from the clipboard [1].  The fact that .execCommand() API calls it "paste" stems from the origins of it being part of a WYSIWYG editing capability, it actually, literary "puts a things from the clipboard appear inside a text field" (even if a hidden one), and you still need to read it out via the value property.

1) http://searchfox.org/mozilla-central/source/testing/mochitest/tests/SimpleTest/SimpleTest.js#959
(In reply to Tomislav Jovanovic :zombie from comment #18)
> As a user, "paste" is an operation that makes a thing from the clipboard
> appear in the document in front of me -- an action with a visible effect. 
> Giving an extension a permission to "Paste" doesn't sound like it should be
> able to silently/invisibly "spy" on my clipboard.  I think "Read" makes that
> a bit more explicit.

+1
Thanks, @zombie. That's a good analysis. 

Perhaps we should avoid "copy" and "paste" altogether? They're such familiar terms to users with very distinct functions. I believe that's the heart of the confusion here. 

"Read" feels too passive to me for a permission, though I understand it's technically correct terminology. So let's go with "get" and the following:

Get data from the clipboard

Input data to the clipboard

... if everyone agrees the above is an accurate description of what the permissions do, then let's go with it.
Nobody objected, requesting checkin.
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b712b9d40d00
Part 1: Allow access to execCommand("paste") with permission r=bz
https://hg.mozilla.org/integration/autoland/rev/ef9ee708bede
Part 2: Test access to "paste" command with clipboardRead permission r=aswan
https://hg.mozilla.org/integration/autoland/rev/b151cb9f4cb0
Part 3: Fix trailing whitespace in nsHTMLDocument.cpp r=bz
Keywords: checkin-needed
Doesn't work for me on Nightly 2017-02-15. I'll add a test case using an existing DOM element on GitHub.com.
Attached file bug-clipboardread-1.0.0.zip (obsolete) — 
Test case
To use the test case, go to https://github.com, make sure the page is focused and press INSERT on your keyboard. It should paste the text from the clipboard into the search box on GitHub, but in Nightly 2017-02-15 this doesn't work.
Flags: needinfo?(tomica)
Those steps to reproduce require being signed in to github, right?
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #32)
> Those steps to reproduce require being signed in to github, right?

Not at all.
Ah, I see.  It depends on the actual insert key, which my keyboard doesn't have, because it's testing keycodes...
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #34)
> Ah, I see.  It depends on the actual insert key, which my keyboard doesn't
> have, because it's testing keycodes...

PageDown has keyCode 34, PageUp has keycode 33. Maybe you could use that to test it.
OK, so we're taking this early return from execCommand:

  if (!isCutCopy && !IsEditingOnAfterFlush()) {
    return false;
  }

I belieev this is a fundamental difference in how execCommand works in Firefox and Chrome in general.  In Firefox document.execCommand only applies to designmode iframes and contenteditable areas (except "cut" and "copy").  In Chrome it applies to form controls.
Yes, that is exactly the issue.  You need a designmode element to work with paste.  Check out the example from the test:

http://searchfox.org/mozilla-central/source/toolkit/components/extensions/test/mochitest/test_clipboard.html#20
Flags: needinfo?(tomica)
Thanks for the feedback. It doesn't work yet (see attachment).

Output is now:
INSERT EVENT  bug-clipboardread.js:8:3
document.activeElement has name q  bug-clipboardread.js:17:3
NS_ERROR_FAILURE:   bug-clipboardread.js:19
Attachment #8838142 - Attachment is obsolete: true
ok, I'll investigate in a bit.
Flags: needinfo?(tomica)
It does work with a textarea, but not with a normal "input" element which has contenteditable set to true.
(In reply to Tomislav Jovanovic :zombie from comment #37)
> Yes, that is exactly the issue.  You need a designmode element to work with
> paste.

We should file a separate bug to address that use case, then.
Working code (with a textarea)
(In reply to Kris Maglione [:kmag] from comment #41)
> We should file a separate bug to address that use case, then.

I wanted to first investigate the reasons behind this discrepancy, as it might impact web compat.

Since this is how execCommand has always worked in Firefox, I'm not sure we need to address this, but I filed bug 1340578 for the decision (as well as bug 1340556 for the missing background page test).
Flags: needinfo?(tomica)
According to https://docs.google.com/document/d/1-aLncxcKpinCNpvksmknwWZhrM2bEChDmQ3C422C6Vc/edit clipboardRead permission should have the following description: “Access text clipboard” but in Comment 21 is specified “Get data from the clipboard” and “Input data to the clipboard”. 

Which is the final decision?
Flags: needinfo?(sdevaney)
(In reply to Vasilica Mihasca, QA [:vasilica_mihasca] from comment #44)
> According to
> https://docs.google.com/document/d/1-
> aLncxcKpinCNpvksmknwWZhrM2bEChDmQ3C422C6Vc/edit clipboardRead permission
> should have the following description: “Access text clipboard” but in
> Comment 21 is specified “Get data from the clipboard” and “Input data to the
> clipboard”. 
> 
> Which is the final decision?

Final decision is: 

“Get data from the clipboard” and “Input data to the clipboard”. 

Unfortunately the copy doc you reference has not been maintained to reflect the various changes to messaging that occurred here in Bugzilla, Github, and in the since-resolved comments within the doc itself.
Flags: needinfo?(sdevaney)
(In reply to sdevaney from comment #45)

> Final decision is: 
> 
> “Get data from the clipboard” and “Input data to the clipboard”. 
> 
> Unfortunately the copy doc you reference has not been maintained to reflect
> the various changes to messaging that occurred here in Bugzilla, Github, and
> in the since-resolved comments within the doc itself.

Thanks for clarifying this. Seems that the expected descriptions are successfully displayed: https://www.screencast.com/t/hKQNeDGwf
Geoffrey already updated the docs for this, I just copy-edited it: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Interact_with_the_clipboard#Browser-specific_considerations_2.

Let me know if we need anything else here though, please.
Flags: needinfo?(tomica)
I believe the limitation is that the element must be in content editable mode, which for content scripts only works with <textarea>s, so I edited the page to reflect that.  I tried to be clear, but can you please double-check my english Will?
Flags: needinfo?(tomica)
Looks good to me!
Keywords: dev-doc-needed → dev-doc-complete
I can't get the example from #42 working on FF54 or nightly, nor any of the pasting code from [1]. Have the conditions for paste succeeding changed since this landed?

document.execCommand("Paste") isn't erroring, and returns true if the target is a <textarea contentEditable=true>, but the clipboard contents never materialise.

See also [2], where I'm trying (and failing) to get Vimium's clipboard commands working in our Firefox version.

[1]: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Interact_with_the_clipboard
[2]: https://github.com/philc/vimium/pull/2601
Flags: needinfo?(tomica)
see bug 1364708 comment #2
Flags: needinfo?(tomica)
That works, thank you!

I hadn't noticed it working because document.execCommand("Paste") doesn't paste anything (or occasionally whitespace) if the clipboard content is rich content (eg. text copied from a webpage).
See Also: → 1397690
Product: Toolkit → WebExtensions

Has the "clipboard-read" permission been implemented because I find contradictory information.

Here it says "Firefox supports the clipboardRead permission from version 54, but only supports pasting into elements in content editable mode" but this page writes "At this time, while Firefox does implement read(), it does not recognize the clipboard-read permission".

When trying to request the permission myself in Firefox 64, I got a TypeError saying "TypeError: 'name' member of PermissionDescriptor 'clipboardRead' is not a valid value for enumeration PermissionName".

status-firefox54: fixed → ---

Firefox only allows extensions with the clipboardRead extension permission to use the navigator.clipboard.read (with dom.events.asyncClipboard.dataTransfer pref) / readText (without pref) APIs. Ordinary web content cannot use the API, as announced at the time of the intent-to-implement:
https://groups.google.com/d/msg/mozilla.dev.platform/skHoHwfapEY/VQZZXvNxAQAJ

The navigator.clipboard.readText API was briefly exposed to ordinary web content in Firefox 63, but this has been fixed (i.e. removed) in 64: see bug 1479935 and bug 1483552 .

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: