xterm.js icon indicating copy to clipboard operation
xterm.js copied to clipboard

The terminal instance cannot be rendered correctly after calling `.open()` for the second time.

Open 1zilc opened this issue 1 year ago • 9 comments

I am new to using xtermjs. I am not sure whether the terminal instance can call the open() method multiple times. According to the official documentation and https://github.com/xtermjs/xterm.js/issues/1323, I tried it. When I removed the dom and mounted again, and used terminal.open(), xterm did not render after the second open.

Details

  • Browser and browser version: chrome 122.0.6261.94
  • OS version: macOS 14.3
  • xterm.js version: 5.4.0

Steps to reproduce

Reproduce

  1. Click the toggle button, xterm renders correctly.
  2. Click the toggle button, remove dom
  3. Click the toggle button, mount dom, xterm did not render after the second open

I'm sorry if there's something wrong with my usage

1zilc avatar Mar 03 '24 03:03 1zilc

I am not very deep into the browser side of the code, @Tyriar has more insights here. Still I think it is not possible to move an already DOM-attached terminal to another DOM node by calling open again.

@Tyriar Do we need an attach/detach cycling for the DOM hooking? It never came to me before, that someone would want to move an existing terminal to another DOM node, so this might be just a wild idea of limited use. Loosely linked or more generalized to this would be support of a copy ctor like const term2 = new Terminal(term1), where the new terminal could start over in DOM-detached mode, but has inherited all VT-related settings/data. This has some intersections with the serialize addon, so might not be worth either the trouble.

jerch avatar Mar 03 '24 12:03 jerch

+1 to this. I have the same use case in our work application which has multiple kinds of terminals in various drawers that can be opened/closed at will.

jonmcgill avatar Oct 15 '24 00:10 jonmcgill

Calling open multiple times was never really supported that well and I think can be a little finicky, but it definitely does work as that's what VS Code does in order to move the terminal around the workbench.

If we can get a repro case without React it will be a lot more clear how to action this.

Tyriar avatar Oct 15 '24 13:10 Tyriar

Here's a little codesandbox (no React). The Open button mounts the terminal instance to the DOM and writes a line. The Replace button removes the DOM and replaces it with a new div. Running Open again results in no line being written.

https://codesandbox.io/p/sandbox/fz8kq2?file=%2Fsrc%2Findex.ts

https://github.com/user-attachments/assets/324bf07f-eec8-40c2-99ed-23e6782db565

jonmcgill avatar Oct 16 '24 17:10 jonmcgill

FWIW, my workaround for now has been to register when the terminal mount DOM has been changed and/or removed. If so, I dispose the terminal instance and any addons. The next time that terminal's container mounts, I re-initiate the instances and run open from a fresh terminal instance.

jonmcgill avatar Oct 16 '24 17:10 jonmcgill

Just wanted to mention that .open() works fine for [email protected]

akphi avatar Nov 02 '24 06:11 akphi

Can't seem to debug in the code sandbox, but it's likely it's hitting this:

https://github.com/xtermjs/xterm.js/blob/41e8ae395937011d6bf6c7cb618b851791aed395/src/browser/CoreBrowserTerminal.ts#L402-L409

Maybe this.element?.isConnected needs to be checked too?

Tyriar avatar Dec 12 '24 16:12 Tyriar

Can't seem to debug in the code sandbox, but it's likely it's hitting this:

https://github.com/xtermjs/xterm.js/blob/41e8ae395937011d6bf6c7cb618b851791aed395/src/browser/CoreBrowserTerminal.ts#L402-L409

Maybe this.element?.isConnected needs to be checked too?

@Tyriar I have tested locally for my use case, and like you suggested, adding this.element?.isConnected makes things work! Is there any other deep concerns? I have created a naive PR for the fix https://github.com/xtermjs/xterm.js/pull/5253

akphi avatar Dec 18 '24 06:12 akphi

This change actually caused a pretty bad regression in VS Code https://github.com/microsoft/vscode/issues/239917, I don't think I can fix it on that end so I'm going to revert this change.

Tyriar avatar Feb 14 '25 14:02 Tyriar