Repository navigation
terminal: copy/paste keys, a pane context menu, opt-in copy-on-select (#131) - #133
Conversation
…#131) Selecting text in the live pane was unreliable and copying was a side effect of it: every mouseup with a selection overwrote the clipboard, and a right-click — the obvious way to reach Copy/Paste — had nothing to open. This is the first slice of #131, in the order written there. - Ctrl+Shift+C / Ctrl+Shift+V on Linux and Windows, Cmd+C / Cmd+V on macOS, through attachCustomKeyEventHandler. The handler returns false for a chord it handled: without that xterm forwards it to the TUI, so Ctrl+Shift+C would both copy and send ^C to the running agent. Plain Ctrl+C is untouched, so SIGINT still reaches it. - The pane answers right-click itself: Copy / Paste / Select all / Copy on select. Copy is disabled while nothing is selected, and a right-button release no longer copies first, so the selection the user just made is what Copy acts on. Paste reuses the existing bracketed-paste path (pasteToTerminal), which never auto-submits; it gained an optional { silent } so a paste from the keyboard does not toast. - Copy-on-select is now a setting, off by default, toggled from that menu and remembered in localStorage. It used to be unconditional and the only way to copy. - macOptionClickForcesSelection: true, rightClickSelectsWord: false, and a title on the pane saying Shift+drag selects when the app reports mouse events. Tests: test/terminal-clipboard.test.js runs the helpers and _createTerminal itself in a vm with a fake xterm, element and clipboard — chords per platform, Ctrl+C passthrough, copy-on-select default and persistence, the menu model, and the mouseup/contextmenu wiring. 12 checks. test/composer-terminal.test.js keeps its assertions but two of them described the old mouseup shape (no event argument, unconditional copy), so they are re-pointed rather than dropped. Verified: every test module that reads public/index.html passes (20 modules, including i18n completeness at 139 checks with the 6 new keys in all 5 languages); the three that need better-sqlite3 pass under Node 24, which is what it was built for; all three inline script blocks of index.html parse. Not in this slice: the addons (web-links, unicode11, search, clipboard, webgl) and the mobile proposals in the second comment on the issue. They change rendering or need vendored files, so they get their own PR.
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
1672e2a to
d0c7e9d
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1672e2af17
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Dismissal, deferred by a task: the click that opened the menu is still being | ||
| // dispatched, and a listener registered now would see it and close immediately. | ||
| setTimeout(() => { | ||
| document.addEventListener('mousedown', _hideTermMenu, { once: true }); |
There was a problem hiding this comment.
When a user clicks any menu item, its mousedown bubbles to this document listener, which removes the entire menu before mouseup and click can reach the button. Since the actions are registered on click, Copy, Paste, Select all, and Copy on select cannot be activated normally with the mouse. Ignore presses inside the menu when dismissing it, and close it only after an item action or an outside press.
Useful? React with 👍 / 👎.
| _pasteFromClipboard(); | ||
| return false; |
There was a problem hiding this comment.
When pressing Cmd+V on macOS or Ctrl+Shift+V in Chromium, this handler reads the clipboard and sends a paste frame but does not call ev.preventDefault(). In the shipped public/vendor/xterm.js, returning false from the custom handler only exits _keyDown; it does not cancel the browser event. The browser can therefore also dispatch its native paste event, which xterm handles separately through onData, inserting the clipboard contents twice. Cancel the default keyboard action before performing the custom paste.
Useful? React with 👍 / 👎.
Closes part of #131. The first slice, in the order written in the issue.
Why
Selecting text in the live pane was unreliable and copying was a side effect of it. Every
mouseup that had a selection wrote it to the clipboard, so a drag you did not mean to copy
replaced whatever you had. And the one gesture that would normally give you Copy/Paste —
right-click — had nothing to open: the pane installs no context menu, and the native one is
not usable in the desktop build.
What changed
via
attachCustomKeyEventHandler. The handler returns false for a chord it handled —without that xterm forwards it to the TUI, so Ctrl+Shift+C would copy and send
^Ctothe running agent. Plain Ctrl+C is untouched, so SIGINT still reaches it.
disabled while nothing is selected. A right-button release is skipped before the copy
branch, so the selection the user just made is what Copy acts on instead of being
copied and dropped. Paste reuses the existing bracketed-paste path
(
pasteToTerminal), which never auto-submits; it gained an optional{ silent }so apaste from the keyboard does not toast.
localStorage(ccs:termCopyOnSelect). It used to be unconditional and the only way tocopy.
macOptionClickForcesSelection: true,rightClickSelectsWord: false, and a title on the pane saying that Shift+drag selectswhen the app reports mouse events.
New i18n keys (
term.menu.copy,term.menu.paste,term.menu.selectAll,term.menu.copyOnSelect,term.select.hint,term.paste.denied) are in all fivedictionaries.
How tested
test/terminal-clipboard.test.jsruns the real code — the helper block and_createTerminalitself are cut out ofpublic/index.htmland evaluated in a vm with afake xterm, a fake element and a fake clipboard. 12 checks: the chord map per platform,
Ctrl+C passthrough, copy-on-select default and persistence (including a browser that
throws on storage), the menu model, and the mouseup/contextmenu wiring.
test/composer-terminal.test.jskeeps all of its assertions; two of them described the oldmouseup shape (no event argument, unconditional copy), so they are re-pointed at the new
one rather than dropped.
Locally: every test module that reads
public/index.htmlpasses (20 modules), includingi18n completeness (139 checks) and the new file. The three modules that need
better-sqlite3 (
config-resolve,interrupt-idle-config,interrupted-recovery) passunder Node 24, which is the ABI the module in this checkout was built for. All three inline
script blocks of
index.htmlparse.Not in this slice: the addons (web-links, unicode11, search, clipboard, webgl) and the
mobile proposals in the second comment — they change rendering or need vendored files.