Support clipboardRead permission to read from the clipboard

RESOLVED FIXED in Firefox 54

Status

()

Toolkit
WebExtensions: General
P2
normal
RESOLVED FIXED
7 months ago
a month ago

People

(Reporter: robwu, Assigned: zombie)

Tracking

({dev-doc-complete})

unspecified
mozilla54
dev-doc-complete
Points:
---

Firefox Tracking Flags

(firefox54 fixed)

Details

(Whiteboard: triaged)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(5 attachments, 1 obsolete attachment)

(Reporter)

Description

7 months ago
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.

Updated

7 months ago
Priority: -- → P2
Whiteboard: triaged
Keywords: dev-doc-needed

Updated

5 months ago
webextensions: --- → +
(Assignee)

Updated

4 months ago
Assignee: nobody → tomica
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

4 months ago
Attachment #8834705 - Flags: review?(bzbarsky)
Attachment #8834706 - Flags: review?(aswan)
(Assignee)

Comment 3

4 months ago
mozreview-review
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 5

4 months ago
mozreview-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?
(Assignee)

Comment 6

4 months ago
> 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)

Comment 7

4 months ago
How about one message to accommodate both permissions? Like so:

Read/write to clipboard
Flags: needinfo?(sdevaney)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

4 months ago
Attachment #8835556 - Flags: review?(bzbarsky)
(Assignee)

Comment 11

4 months ago
(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 12

4 months ago
mozreview-review
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+
(Assignee)

Comment 13

4 months ago
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+

Comment 15

4 months ago
Hm, do these two versions work for the permissions? 


Copy data from the clipboard

Insert data into the clipboard
Flags: needinfo?(sdevaney)

Comment 16

4 months ago
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.

Comment 17

4 months ago
Yeah that is confusing. aswan if you prefer it, I'm okay with:

Paste data from the clipboard

Copy data to the clipboard
(Assignee)

Comment 18

4 months ago
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.
(Assignee)

Comment 19

4 months ago
> 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

Comment 21

4 months ago
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.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 26

4 months ago
Nobody objected, requesting checkin.
Keywords: checkin-needed

Comment 27

3 months ago
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
https://hg.mozilla.org/mozilla-central/rev/b712b9d40d00
https://hg.mozilla.org/mozilla-central/rev/ef9ee708bede
https://hg.mozilla.org/mozilla-central/rev/b151cb9f4cb0
Status: ASSIGNED → RESOLVED
Last Resolved: 3 months ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Doesn't work for me on Nightly 2017-02-15. I'll add a test case using an existing DOM element on GitHub.com.
Created attachment 8838142 [details]
bug-clipboardread-1.0.0.zip

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.
(Assignee)

Comment 37

3 months ago
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)
Created attachment 8838165 [details]
bug-clipboardread-1.1.0.zip

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
(Assignee)

Comment 39

3 months ago
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.
Created attachment 8838169 [details]
bug-clipboardread-2.0.0.zip

Working code (with a textarea)
(Assignee)

Comment 43

3 months ago
(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)

Comment 45

3 months ago
(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)
(Assignee)

Comment 48

2 months ago
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
You need to log in before you can comment on or make changes to this bug.