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

Add clipboard support, continued #1562

Closed
wants to merge 5 commits into from

Conversation

snickell
Copy link

@snickell snickell commented Jul 13, 2021

This PR is a continuation of @juanjoDiaz 's hard work here: #1347

At the moment, its identical to #1347 , but with the merge conflict relative to noVNC:master resolved. I'll continue to push to this PR as required to resolve issues and get this ready for @samhed 's approval.

Please close this PR if its not helpful, but I don't think the other PR can be continued without @juanjoDiaz being involved, so it seemed like a new PR was necessary to fix #1511

@juanjoDiaz explained it better than I can:

This PR adds support for automatic copy-pasting. So when you are focused on the canvas and paste text it's pasted in the server and when you copy something in the server it's automatically copied to your local keyboard.

I'll try to keep a task list updated here for known remaining tasks:

  • npm test passes on Chrome
  • npm test passes on Safari
  • npm test passes on Firefox
  • Investigate and document best-possible approach to a good-ux fallback on Firefox (assuming that's necessary)
  • Validate current implementation on Chrome
  • Validate current implementation on Safari
  • Document behavior of current implementation on Firefox
  • Document behavior of current implementation on Edge
  • Document behavior of current implementation on iOS
  • Document behavior of current implementation on Android

@snickell
Copy link
Author

snickell commented Jul 13, 2021

After the merge conflict was resolved + added an expect(Clipboard.isSupported) to the tests, to assert we want the clipboard to be a working thing on all our target/test platforms.

all npm test are passing on my system with Chrome:

SUMMARY:
✔ 504 tests completed
ℹ 6 tests skipped

@snickell
Copy link
Author

@samhed / @CendioOssman : anything else that should be on the current task list for moving this toward mergeable?

Clipboard needs to be supported on all target browsers.
@snickell
Copy link
Author

snickell commented Jul 13, 2021

npm test results, including @juanjoDiaz 's test.clipboard.js tests:

Chrome: all tests pass

Firefox: all tests pass (!)

Safari 14.0.3 (=old version, will retest on current Safari later): automatic clipboard tests fail with exceptions on:
image

The offending line appears to be the new DataTransfer() constructor not being implemented, which

  1. is only referenced in the tests
  2. appears to have be implemented in the current version of Safari.

If this works on the current version we should be fine, because this needs-current-safari feature is only required to run the clipboard-test.js tests (e.g. to synthetically inject paste events), NOT to use the actual clipboard.js implementation as a user/client

SUMMARY:
✔ 502 tests completed
ℹ 6 tests skipped
✖ 3 tests failed

FAILED TESTS:
  Automatic Clipboard Sync
    ✖ incoming clipboard data from the server is copied to the local clipboard
      Safari 14.0.3 (Mac OS 10.15.6)
    Illegal constructor
    [native code]
    tests/test.clipboard.js:36:51

    ✖ should copy local pasted data to the server clipboard
      Safari 14.0.3 (Mac OS 10.15.6)
    Illegal constructor
    [native code]
    tests/test.clipboard.js:55:51

    Protocol Message Processing After Completing Initialization
      Normal Clipboard Handling Receive
        ✖ should fire the clipboard callback with the retrieved text on ServerCutText
          Safari 14.0.3 (Mac OS 10.15.6)
        Illegal constructor
        [native code]
        _handleServerCutText@core/rfb.js:1875:55
        _handleMessage@core/rfb.js:869:41
        _handleMessage@[native code]
        _recvMessage@core/websock.js:342:40
        _recvMessage@[native code]
        _receiveData@tests/fake.websocket.js:62:27
        tests/test.rfb.js:2169:53

@snickell
Copy link
Author

snickell commented Jul 13, 2021

Bingo, even the test code now passes on current version of Safari (14.1.1):

SUMMARY:
✔ 505 tests completed
ℹ 6 tests skipped
12 07 2021 18:02:04.380:WARN [Safari 14.1.1 (Mac OS 10.15.7)]: Disconnected (0 times) Client disconnected from CONNECTED state (server shutting down)

So in conclusion, I believe the auto-clipboard /does/ work on even old or current versions of Safari (by testing), but the auto sync clipboard tests fail unless you use a recent Safari version, which seems like no big deal to me?

Safari: tests pass
Firefox: tests pass
Chrome: tests pass

Next I'll try interactive usage to validate the feature, we'll see if Firefox actually now magically works or if the tests need updating to properly break!!!

@juanjoDiaz
Copy link
Contributor

I'd be happy to fix the conflicts and complete any needed work on my original PR as long as the maintainers are willing to merge it.

A big portion of my efforts to improve noVNC have been either rejected or introduced by the maintainers bypassing my PRs. So I am not confortable putting more effort here unless it's clear that the maintainers are willing to merge the PR.
If that's the case, @samhed please let me know and I can rebase my PR and amend anything that needs to be amended.

@snickell
Copy link
Author

@juanjoDiaz that would be amazing! I totally understand the feeling, I've been on both sides of this fence before..... please let me know if there's anything I can do to help out, sometimes a small tag team at the end can help, if only because it helps maintainers like @samhed know that somebody else has validated a PR (e.g. cross-platform) if they don't have the time/energy/other to check it themselves

@jk4235
Copy link

jk4235 commented Jul 24, 2021

@juanjoDiaz @snickell that is really nice work thanks! hope this PR would be merged soon

@samhed
Copy link
Member

samhed commented Aug 2, 2021

Thanks for moving this forward @snickell, I'm back from my vacation now. Since you're on top of this at the moment, let's continue here in this PR. Have you had a chance to look at Firefox?

@juanjoDiaz I believe we have given proper attribution when commiting your changes in the past.

@samhed samhed added the feature label Aug 2, 2021
@samhed samhed added this to the v1.3.0 milestone Aug 2, 2021
@samhed samhed modified the milestones: v1.3.0, Future Features Aug 26, 2021
@samhed samhed marked this pull request as draft August 26, 2021 14:08
@jabbera
Copy link

jabbera commented Oct 16, 2021

Am I missing something here? I merged these changes into 1.2 and while copying out to the clipboard works, pasting something copied from outside the browser pastes what was originally copied from the session, not what was copied outside the browser.

@samhed
Copy link
Member

samhed commented Oct 17, 2021

You still around @snickell?

@jabbera
Copy link

jabbera commented Oct 18, 2021

It turns out tightvnc doesn't support copy from virtual session into system clipboard. I'm looking at replacing it.

@crispy-peppers
Copy link

Any update on this? Also does this issue looks similar? fcwu/docker-ubuntu-vnc-desktop#272

@MaximMonin
Copy link

MaximMonin commented Aug 6, 2022

ceresimaging#1

my way to add paste latin clipboard text to TTY (or graphical field) by pressing ctrl-v or shift-Ins

@pavelfeldman
Copy link

@snickell

This did not quite work for me when I applied it locally. Even when I fixed it to actually dispatch the handleCopy/handlePaste.

Here is the logic I would suggest following:

  1. When focus is switched into the viewer, send the clipboard content to the target. That way any app can access it and paste upon corresponding keyboard or mouse event. Note that since this exposes clip content across the machines, it needs explicit setting / opt-in. readText is now universally available.
window.addEventListener('focus', () => {
  navigator.clipboard.readText().then(text => this.clipboardPasteFrom(text));
});
  1. When Copy is invoked, copy the contents of the Pastebin into the clipboard. It already contains the good value. This requires no permissions.
document.addEventListener('copy', () => {
  navigator.clipboard.writeText(document.getElementById('noVNC_clipboard_text').value);
});

That's pretty much it. It also requires to not preventDefault / stopEvent(e) on the Control, otherwise Ctrl+C won't dispatch the copy event and we don't want to simulate it for accessibility.

Would you be interested in getting a patch with those changes?

@cebas
Copy link

cebas commented Sep 17, 2022

@pavelfeldman

Would you be interested in getting a patch with those changes?
Yes, I am. Thanks

@samhed
Copy link
Member

samhed commented Nov 20, 2023

It just gets confusing to have multiple PR's open on the same topic, #1347 has slightly more recent activity, closing this one.

To confirm — we are still interested in merging this type of feature. The problem with this pull request is that we haven't had any activity from the author since July 2021.

@samhed samhed closed this Nov 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Automatic clipboard copying
9 participants