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

Clipboard API for Plugins #5994

Merged
merged 1 commit into from
Sep 17, 2019
Merged

Clipboard API for Plugins #5994

merged 1 commit into from
Sep 17, 2019

Conversation

azatsarynnyy
Copy link
Member

@azatsarynnyy azatsarynnyy commented Aug 20, 2019

What it does

Adds Clipboard API for Plugins.
Closes #5519
This PR is based on the code extracted from #5527

Some notes about the implementation

Different browsers and it's native extensions work a bit differently with the APIs around the clipboard: Async Clipboard API, Permissions API, document.execCommand API.
So the PR adds a minimal level of support for Chrome and FireFox browsers. Support for other cases may be added later as needed.

How to test

Run Theia with the test VS Code extension https://github.com/azatsarynnyy/test-clipboard-api/blob/master/clipboard-api-test-0.0.1.vsix

To test vscode.env.clipboard.readText API:

  • copy an arbitrary text to the clipboard
  • call Clipboard: read from clipboard command
  • the text from the clipboard should be shown with a notification

To test vscode.env.clipboard.writeText API:

  • select an arbitrary text in Theia editor
  • call Clipboard: write selection to clipboard command
  • paste the clipboard content to any place to check that it was really copied

Browser-specific behavior:

  • Chrome
    The user is able to block the access to the clipboard by setting a preference
    perm

In that case, Theia shows the message
image

  • FireFox
    By default, Async Clipboard API is disabled but it can be enabled by setting dom.events.testing.asyncClipboard preference. User will be informed about that with a notification
    image

Besides that, Async Clipboard API isn't gated with the permissions in FireFox, so it just bypassed.

Review checklist

Reminder for reviewers

@akosyakov
Copy link
Member

@azatsarynnyy is it ready for a review?

@akosyakov akosyakov added the vscode issues related to VSCode compatibility label Aug 21, 2019
@azatsarynnyy
Copy link
Member Author

@akosyakov it's not ready yet. I want to create another test plugin which will test Clipboard API only. I'll ping you once it's ready.

@azatsarynnyy azatsarynnyy added clipboard issues related to the clipboard plug-in system issues related to the plug-in system labels Aug 21, 2019
@azatsarynnyy azatsarynnyy marked this pull request as ready for review August 21, 2019 14:21
@azatsarynnyy azatsarynnyy requested a review from a team as a code owner August 21, 2019 14:21
@azatsarynnyy
Copy link
Member Author

It's ready for review and test

@akosyakov
Copy link
Member

I'm offline tomorrow. If someone gives a try proposed test cases in the description on different browsers it would be helpful. Also if something does not work think about suggestion to fix them, i.e. use DOM api as we do in some places in the code for paste, for copy don't have an idea.

Copy link

@fangnx fangnx left a comment

Choose a reason for hiding this comment

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

Works well on Chrome! However, it did not seem to work on Firefox. Running the commands did not have any effect, and there was no browser pop-up requesting permission.

@akosyakov
Copy link
Member

it did not seem to work on Firefox

It is strange. Latest Firefox supports should support these APIs. @fangnx which version of Firefox do you use?

@azatsarynnyy could you debug please what is going wrong on FireFox. Theia claims to support latest version of Firefox and Chrome.

@akosyakov
Copy link
Member

We need to verify as well that @benoitf use case is working: #5527 (review)

@akosyakov akosyakov requested a review from benoitf August 23, 2019 08:40
@fangnx
Copy link

fangnx commented Aug 23, 2019

it did not seem to work on Firefox

It is strange. Latest Firefox supports should support these APIs. @fangnx which version of Firefox do you use?

@azatsarynnyy could you debug please what is going wrong on FireFox. Theia claims to support latest version of Firefox and Chrome.

Firefox Quantum 68.0.1 (64-bit), Ubuntu 18.04.1

@akosyakov
Copy link
Member

From https://developer.mozilla.org/en-US/docs/Web/API/Clipboard#Browser_compatibility:

Firefox only supports reading the clipboard in browser extensions, using the "clipboardRead" extension permission.

Writing to the clipboard is available without permission in secure contexts and browser extensions, but only from user-initiated event callbacks. Browser extensions with the "clipboardWrite" permission can write to the clipboard at any time.

So basically FireFox does not support these APIs yet :( We need a fallback strategy.

@akosyakov
Copy link
Member

Also found in https://developer.mozilla.org/en-US/docs/Web/API/Clipboard/read:

At this time, while Firefox does implement read(), it does not recognize the "clipboard-read" permission, so attempting to use the Permissions API to manage access to the API will not work.

So maybe we should no query permissions, but just always call writeText or readText. As far as I understood they will request permissions automatically and if not possible throw. If they throw then we should fallback to document based strategy.

@azatsarynnyy
Copy link
Member Author

Thank you for testing it, guys! 🙇‍♂️
I'll be able to continue to work on your feedback on Tuesday.

@azatsarynnyy
Copy link
Member Author

It doesn't work in FF for me also. I'm going to investigate the problem.

@azatsarynnyy
Copy link
Member Author

Also found in https://developer.mozilla.org/en-US/docs/Web/API/Clipboard/read:

At this time, while Firefox does implement read(), it does not recognize the "clipboard-read" permission, so attempting to use the Permissions API to manage access to the API will not work.

So maybe we should no query permissions, but just always call writeText or readText. As far as I understood they will request permissions automatically and if not possible throw. If they throw then we should fallback to document based strategy.

Async Clipboard API works well in Chrome without asking the permissions.
FF still requires enabling the dom.events.testing.asyncClipboard preference in about:config to turn on that API.

@akosyakov
Copy link
Member

akosyakov commented Aug 30, 2019

Async Clipboard API works well in Chrome without asking the permissions.
FF still requires enabling the dom.events.testing.asyncClipboard preference in about:config to turn on that API.

@azatsarynnyy Can we somehow detect it and notify a user to enable it? Is there any security concerns with enabling it?

@azatsarynnyy
Copy link
Member Author

I think relying on dom.events.testing.asyncClipboard preference would be a hack since it's actually intended to be used by FF automated tests only. It allows bypassing the security checks when interacting with the clipboard. See https://github.com/mozilla/gecko-dev/blob/e04021f29e6d8a37753ba2b510432315ce05a8d7/dom/events/Clipboard.h#L61

Although FF declares support of Async Clipboard API, it's exposed to secure contexts and browser extension only. So, looks like we cannot use it for this task.

execCommand API can be used only in a short running user-generated event handlers, thus it also doesn't suit us.

As I understand, the only way we can follow is by relying on some 3rd party lib.
WDYT?

@akosyakov
Copy link
Member

As I understand, the only way we can follow is by relying on some 3rd party lib.

I don't think that 3rd party lib will do anything special, just calling execCommand.

execCommand API can be used only in a short running user-generated event handlers, thus it also doesn't suit us.

Could we try it anyway, like suggested in #5994 (comment)? We are already using such approach in some places it was good so far. It should be only a fallback.

@azatsarynnyy
Copy link
Member Author

azatsarynnyy commented Sep 6, 2019

@akosyakov the reason why we cannot use document.execCommand API as a fallback is it works without any restrictions in Chrome but not in FireFox. And copy-text-to-clipboard doesn't work in FF for the same reason.

The main issue here is that FF requires calling document.execCommand('cut'/'copy') APIs inside the short-living event handlers of user actions like mouse click, keypress etc.

I examined all the places in Theia where document.execCommand API is used. All of them are actually called as Theia commands handlers, which means they're called inside the short-living event handlers of user actions, e.g. click, keypress.

I tried to call ClipboardService.writeText as Theia command handler, with the following code:

async writeText(value: string): Promise<void> {
       const onCopy = (e: ClipboardEvent) => {
            document.removeEventListener('copy', onCopy);
            if (e.clipboardData) {
                e.clipboardData.setData('text/plain', value);
                e.preventDefault();
            }
        };
        document.addEventListener('copy', onCopy);
        document.execCommand('copy');
}

It worked well in both, Chrome and FF.

But when I call it through the Plugin API, it works in Chrome only. And FireFox hints in the console with the following message:
document.execCommand(‘cut’/‘copy’) was denied because it was not called from inside a short running user-generated event handler.

I believe it's because of document.execCommand API is called by plugin via JSON-RPC but not inside a short-living event handler of user action.

@azatsarynnyy
Copy link
Member Author

To allow the user using Clipboard Plugin API in FireFox, I don't see any other options except recommending to enable dom.events.testing.asyncClipboard preference.

@akosyakov
Copy link
Member

akosyakov commented Sep 7, 2019

To allow the user using Clipboard Plugin API in FireFox, I don't see any other options except recommending to enable dom.events.testing.asyncClipboard preference.

@azatsarynnyy Let's notify a user with an error that he head to enable it and how in FF, i.e. point to a some FF page which explains it (with security concerns). Do I understand correctly that then readTest and writeText should work as well in Firefox and we don't need to work around them?

@akosyakov
Copy link
Member

Thought a bit about it again, we still want to have a fallback for native extensions, since they can call ClipboardService from the event handlers and then will work better. But it can be done by a separate PR, for this one notifying a user that action failed and explain how to fix it would be enough.

@azatsarynnyy
Copy link
Member Author

Do I understand correctly that then readTest and writeText should work as well in Firefox and we don't need to work around them?

correct

@azatsarynnyy
Copy link
Member Author

I've updated the PR with FF support. Also, the PR description has been updated.

Signed-off-by: Artem Zatsarynnyi <azatsary@redhat.com>
@akosyakov
Copy link
Member

@fangnx @benoitf could you check how you like behaviour now please?

Copy link
Member

@akosyakov akosyakov left a comment

Choose a reason for hiding this comment

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

It works very well for me.

@akosyakov
Copy link
Member

@azatsarynnyy please file a follow-up issue to use ClipboardService in native extensions as well and enhance to try to run in the event handler.

@akosyakov
Copy link
Member

@azatsarynnyy please merge it

@azatsarynnyy
Copy link
Member Author

The follow-up issue #6209

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clipboard issues related to the clipboard plug-in system issues related to the plug-in system vscode issues related to VSCode compatibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[vscode] complete clipboard api
3 participants