New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 758466 link

Starred by 1 user

Issue metadata

Status: Available
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 3
Type: Bug



Sign in to add a comment

hterm: automatic copying inserts newlines in selection (when using middle mouse paste)

Project Member Reported by vapier@chromium.org, Aug 24 2017

Issue description

to reproduce:
- use Linux
- use default preferences
- run:
  set -- `stty size`
  printf '%*s http://example.com/\n' $(($2 - 5))
- select the URL with the mouse
- hterm should trigger an automatic copy
- middle mouse paste includes the newline (bad)
- ctrl+v paste does not include the newline (good)

looks to be related to the logic that restores the selection:
hterm.Terminal.prototype.copyStringToClipboard
  if (selection.extend) {
    selection.collapse(anchorNode, anchorOffset);
    selection.extend(focusNode, focusOffset);
  }

if we disable that, then middle mouse paste works (no newlines), but then the selected text is cleared on us every time.
 

Comment 1 by vapier@chromium.org, Aug 25 2017

Components: Blink>DataTransfer
i'm not sure this is fixable with the current blink/HTML behavior.  we generate a new object to populate with the unwrapped line content so we can restore the content as it should be (no newlines), then select it for the copy step, then re-select the original selection.  that last step triggers an update of X's primary selection (middle mouse paste).

from blink's perspective, i think this is WAI, even though the JS API provides no way to control it.

https://chromium.googlesource.com/apps/libapps/+/nassh-0.8.36.11/hterm/js/hterm_terminal.js#2828
 Issue 799141  has been merged into this issue.
Cc: pwnall@chromium.org
vapier@: Do you happen to know if xterm.js figured out this problem?

Aside: I think that Microsoft is working on xterm.js under VS Code, and they seem to be doing a lot of good work. Is there any chance we could try to coordinate with them and end up working on the same terminal implementation? 
xterm.js is a completely independent implementation, and going in a completely different direction.  their current approach is to handle all the rendering themselves in a <canvas> element.  i don't see a merge happening.
Cc: marcots@google.com
Project Member

Comment 7 by bugdroid1@chromium.org, Jan 3

The following revision refers to this bug:
  https://chromium.googlesource.com/apps/libapps/+/406c76c5df008abff1df80d4832dd77872601480

commit 406c76c5df008abff1df80d4832dd77872601480
Author: Mike Frysinger <vapier@chromium.org>
Date: Thu Jan 03 08:02:27 2019

hterm: terminal: stop passing down this.document_ when copying

The local copySelectionToClipboard func has never taken an argument.
The usage here is a hold over from when this code called the common
hterm.copySelectionToClipboard which does take a document object.

Bug: chromium:758466
Change-Id: I3484b7a6b791a1977ab109676d87977965d4fcb2
Reviewed-on: https://chromium-review.googlesource.com/c/1393863
Reviewed-by: Vitaliy Shipitsyn <vsh@google.com>
Tested-by: Mike Frysinger <vapier@chromium.org>

[modify] https://crrev.com/406c76c5df008abff1df80d4832dd77872601480/hterm/js/hterm_terminal.js

Project Member

Comment 8 by bugdroid1@chromium.org, Jan 5

The following revision refers to this bug:
  https://chromium.googlesource.com/apps/libapps/+/96eacae237c65eb77a33a303f18e5dc0e447d8d0

commit 96eacae237c65eb77a33a303f18e5dc0e447d8d0
Author: Mike Frysinger <vapier@chromium.org>
Date: Sat Jan 05 23:08:06 2019

hterm: push selection logic down into copySelectionToClipboard

In preparation for using newer permission & clipboard APIs (which are
all Promise based), rework this API to accept the content we want to
copy directly.  If we have to use the document.execCommand method to
copy the data, we'll create the required nodes on the fly.  This helps
break up the changes into separate commits: converting existing APIs
before supporting newer APIs.

This step is particularly important because the newer APIs are both
asynchronous and take the state to be copied directly.  The current
execCommand API operates on the implicit state of the document and
whatever selection it has active (which is why we had to generate the
random nodes in the first place).

This also allows for more directed unittest coverage.

Bug: chromium:758466
Change-Id: I9a384d790b2ef0d73d30819ab03a16bbedf2d656
Reviewed-on: https://chromium-review.googlesource.com/c/1393864
Reviewed-by: Vitaliy Shipitsyn <vsh@google.com>
Tested-by: Mike Frysinger <vapier@chromium.org>

[modify] https://crrev.com/96eacae237c65eb77a33a303f18e5dc0e447d8d0/hterm/js/hterm_terminal.js
[modify] https://crrev.com/96eacae237c65eb77a33a303f18e5dc0e447d8d0/hterm/js/hterm_vt_tests.js
[modify] https://crrev.com/96eacae237c65eb77a33a303f18e5dc0e447d8d0/hterm/js/hterm_tests.js
[modify] https://crrev.com/96eacae237c65eb77a33a303f18e5dc0e447d8d0/hterm/js/hterm.js

Project Member

Comment 9 by bugdroid1@chromium.org, Jan 5

The following revision refers to this bug:
  https://chromium.googlesource.com/apps/libapps/+/46690a868e3cab9d2245267998b9bc04a7d419ca

commit 46690a868e3cab9d2245267998b9bc04a7d419ca
Author: Mike Frysinger <vapier@chromium.org>
Date: Sat Jan 05 23:10:54 2019

hterm: add support for new Clipboard API for copying text

If the platform supports the new Clipboard API, use it to copy the
text directly.  This has the advantage of needing to muck with the
active selection which can be racy when multiple events fire that
want to modify the terminal state.

This also changes hterm.copySelectionToClipboard to using Promises
which means it is now asynchronous.  Which leads us to the race.

For example, when you double click, a series of input events are
queued up on Chrome: mouseup -> click -> dblclick.  When the mouseup
code queues a copy operation which runs asynchronously, it gets put
behind the dblclick event.  When we process the dblclick event, it
changes the active selection (by design) before queueing a copy event
of its own.  If these get completed partially, we the selections get
out of sync and code breaks.

Bug: chromium:758466
Change-Id: Ifa1e8b3eacdb64ffcd6cd2b41237c4322d762baf
Reviewed-on: https://chromium-review.googlesource.com/c/1393865
Reviewed-by: Vitaliy Shipitsyn <vsh@google.com>
Tested-by: Mike Frysinger <vapier@chromium.org>

[modify] https://crrev.com/46690a868e3cab9d2245267998b9bc04a7d419ca/hterm/js/hterm.js

Sign in to add a comment