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

Enable Firefox clipboard commands #2601

Merged
merged 10 commits into from Nov 25, 2017
Merged

Conversation

mrmr1993
Copy link
Contributor

@mrmr1993 mrmr1993 commented Aug 15, 2017

This replaces #2524.

  • Copying from the clipboard works in both Chrome and Firefox.
  • Pasting from the clipboard only works in Chrome.
    • I can't work out why it's not working in Firefox.
    • the method here for pasting is almost exactly what we're using, so I'm pretty low on ideas.
  • I haven't converted the LinkHints.activateModeToCopyLinkUrl command to use the new handler, since I'm focusing on getting Firefox to paste. Done

NB: this directs the copy/paste commands via the HUD. This feels (and is) weird, but it's the best we've got if the background page doesn't do this for us on FF.

/cc @smblott-github

@gdh1995
Copy link
Contributor

gdh1995 commented Aug 15, 2017

What if you add the "clipboardRead" permission into the permissions array in manifest.json?

Just an idea needing checks - I have no laptop at hand.

@mrmr1993
Copy link
Contributor Author

What if you add the "clipboardRead" permission into the permissions array in manifest.json?

We already have it, I'm afraid.

@mrmr1993
Copy link
Contributor Author

With 1548561 (implied necessary by https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Interact_with_the_clipboard), document.execCommand("Paste") returns true (rather than false before), but still nothing is pasted.

@mrmr1993
Copy link
Contributor Author

The 'working' example from the issue in their issue tracker doesn't work on FF54 or today's nightly. I'm giving up on reading the clipboard for now.

@mrmr1993
Copy link
Contributor Author

Pushed some more commits and reordered. This probably wants merging up to c50c172. (There's no point merging 6659501 and d5db97e since they don't actually fix pasting).

@mrmr1993 mrmr1993 changed the title WIP: Enable Firefox clipboard commands Enable Firefox clipboard commands Aug 15, 2017
@mrmr1993
Copy link
Contributor Author

I've done all I want to with this now; IMO it's ready to go.

For posterity, several issues combined to stop pasting from working for me earlier:

  • Firefox won't paste in frames that are display: none
  • Firefox only pastes in <textarea contentEditable=true> elements
  • Sometimes (specifically, when the active element is the document <body>), the HUD frame gets selected as document.activeElement, and we don't receive any more key events until we blur it from the document
  • Firefox (almost?) always pastes nothing/whitespace when the clipboard contains rich text
    • this includes text selected from webpages, which was pretty confusing while testing
    • text from textboxes, the URL bar, etc. are fine

The last of these is still an issue for us, but there's no way of working around it.

@gdh1995
Copy link
Contributor

gdh1995 commented Aug 16, 2017

According to https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Interact_with_the_clipboard, an web-extension is able to do pasting on its background page using any content-editable elment. And I've succeed in pasting on background page, using <body contenteditable="true"> or <textarea contenteditable="true">.

But there're still some problems if using <textarea c~e~=true>:

  • copied multiline text are converted into single-line text and all of \n are removed
  • only pure text data (seems to be copied as text/plain) can be pasted
  • document.execCommand('Paste') wouldn't work (although still return true) any more, if only executing textarea.value = "", even when I created and used new <textarea>s - I don't know why but it was

Considering <body ce> supports multiline rich text and works robustly, I suggest using html > body > div[contenteditable=true] on the background page to do pasting.

Updated: div[ce=true] is not safe enough, because Chrome will paste HTML content including images...

@mrmr1993
Copy link
Contributor Author

Chrome are implementing an async clipboard API. It's been suggested on (one of the many) Firefox issue(s). We should keep an eye to see if this comes to anything, since it would make all the pain and weirdness go away.

@smblott-github
Copy link
Collaborator

@mrmr1993 What is the status of this? Is it good-to-go, from your perspective?

@mrmr1993
Copy link
Contributor Author

mrmr1993 commented Sep 15, 2017 via email

@ahstro
Copy link
Contributor

ahstro commented Oct 6, 2017

Any chance of merging this soon? 😇

@smblott-github
Copy link
Collaborator

Any chance of merging this soon?

Maybe. Maybe not. My understanding is that it's not as reliable as we'd like. And something which works only sometimes is a bad UX.

@majutsushi
Copy link

Would it be possible to at least get the yanking/copying to the clipboard part merged in the not-too-distant future? Judging by the multiple duplicates here and also my own experience that is the much more common operation, and the one that apparently works already. Especially if pasting from the clipboard might have to wait for the Firefox bug mentioned above, which would surely take a few months to actually be available in release versions. Thanks!

@smblott-github
Copy link
Collaborator

@mrmr1993... @majutsushi makes a good point, no?

If the yanking is reliable then we should merge that and (perhaps) disable all of the paste commands on FF.

@mrmr1993
Copy link
Contributor Author

Sounds good. I'll rebase now.

@mrmr1993
Copy link
Contributor Author

Rebased. (Old version here)

@eyaleizenberg
Copy link

Any updates on this?

@ahstro
Copy link
Contributor

ahstro commented Nov 22, 2017

I had a dream this was merged. Now, I'm not saying I possess the gift of precognition, but I'm hoping for an early christmas miracle 🎄 ✨

@philc
Copy link
Owner

philc commented Nov 22, 2017

@smblott-github I think this has reached a good state; it LGTM. Let's merge if there are no further objections.

@smblott-github
Copy link
Collaborator

Yes, @philc. I'm permanently running behind at the moment.

@mrmr1993... Should we not be removing Clipboard and everything related to it from main.coffee too?

@mrmr1993
Copy link
Contributor Author

Can do, but I preferred not to. If there are any issues, I thought it would be better if we could just point the commands back to the old code on Chrome.

This adds support for pasting rich text from the clipboard
@mrmr1993
Copy link
Contributor Author

In latest Firefox, we can paste into contenteditable elements that aren't <textarea>. This gives us the ability to paste rich text, fixing the last of the issues I mentioned above. I've pushed a commit to take advantage of this.

This PR now gives Firefox the same full clipboard support as Chrome.

@gdh1995
Copy link
Contributor

gdh1995 commented Nov 25, 2017

@mrmr1993 As I mentioned before, div[ce=true] is not safe enough, because Chrome will paste HTML content including images (in my tests, <img>s). This means unexpected images might be loaded, if a user copy a line containing image icons from other web pages.

@gdh1995
Copy link
Contributor

gdh1995 commented Nov 25, 2017

I'm going to test the different behavior on pasting between Chrome and Firefox, and try using browser-specified APIs like onpaste to solve them safely.

Here're test results:

  • On Chrome, div[contenteditable=plaintext-only] is just enough. textarea is only OKay.
  • On Firefox, div[contenteditable=true] is able to contain pasted rich text like <img>s.
  • On both Chrome and Firefox, div[contenteditable=true] with a custom onpaste listener works well.
  • However, on Firefox a config item "dom.event.clipboardevents.enabled" can be set to false manually to disallow copy/paste events. If false, <div>.onpaste is still null and mutable, but no "paste" events can be received.

So I suggest that Vimium uses a try-catch to initial the element (if text[ce=plain-only] fails then div[ce=true] with onpaste), and the onpaste can be:

function (this: HTMLElement, event: ClipboardEvent): void {
  const d = event.clipboardData, text = d && typeof d.getData === "function" ? d.getData("text/plain") : "";
  event.preventDefault();
  event.stopImmediatePropagation();
  if (!text) { return; }
  (event.target as HTMLElement).ownerDocument.execCommand("insertText", false, text + "");
}

Copy link
Collaborator

@smblott-github smblott-github left a comment

Choose a reason for hiding this comment

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

@mrmr1993 ... I'm going to merge this then make the two small changes mentioned. Let me know if I've got those wrong.

Thanks. We should probably push this to FF soon. What's your recommendation, @mrmr1993?

url = url[0..25] + "...." if 28 < url.length
HUD.showForDuration("Yanked #{url}", 2000)

openCopiedUrlInNewTab: (count) ->
HUD.pasteFromClipboard (url) ->
for i in [0...count] by 1
Copy link
Collaborator

Choose a reason for hiding this comment

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

I might change this to use mkRepeatCommand in main.coffee (to keep the count logic in as few places as possible).

Clipboard.copy data
focusedElement?.focus()
window.parent.focus()
UIComponentServer.postMessage {name: "unfocusIfFocused", data}
Copy link
Collaborator

Choose a reason for hiding this comment

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

unfocusIfFocused, above, does not take data, does it? I'll remove data here; let me know if that's incorrect, @mrmr1993.

@smblott-github smblott-github merged commit 58f45ac into philc:master Nov 25, 2017
smblott-github added a commit to smblott-github/vimium that referenced this pull request Nov 25, 2017
smblott-github added a commit to smblott-github/vimium that referenced this pull request Nov 25, 2017
smblott-github added a commit to smblott-github/vimium that referenced this pull request Nov 25, 2017
These are no longer needed following philc#2601.

@mrmr1993... If these are ever needed again, then we can just revert
this commit (and make them background commands again).
@smblott-github
Copy link
Collaborator

@mrmr1993 I see that the new permission here causes Chrome to disable Vimium until the user accepts the new permission.

Might you be able to look into doing this dynamically: https://developer.chrome.com/apps/permissions?

@gdh1995
Copy link
Contributor

gdh1995 commented Nov 25, 2017

The clipboardWrite permission is safe to remove from manifest on Chrome.

The only effects I can see are:

  • Chrome will show an alert dialog when I add it.
  • The extension's detail will become "reading and changing your data...", while the old is only "reading"

But I think it's a good change to notice users that Vimium is allowed to modify the system clipboard.

@mrmr1993
Copy link
Contributor Author

Might you be able to look into doing this dynamically: https://developer.chrome.com/apps/permissions?

@smblott-github I had a look into it (branch here), but to no success. To quote Mozilla's docs:

The request can only be made inside the handler for a user action, such as

  • clicking the extension's browser action or page action
  • selecting its context menu item
  • activating a keyboard shortcut defined by the extension
  • clicking a button in a page bundled with the extension.

None of those things happens when a user tries to paste a URL using our commands. It looks like optional permissions are a no-go for us on FF.

(Sorry for the delay with this, I've had a few busy days.)

@smblott-github
Copy link
Collaborator

OK. Thank you.

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

Successfully merging this pull request may close these issues.

None yet

7 participants