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

Make terminal rendering work in popout windows #4069

Merged
merged 2 commits into from
Sep 9, 2022
Merged

Conversation

mihaip
Copy link
Contributor

@mihaip mihaip commented Aug 24, 2022

Make terminal rendering work in popout windows

Add support for the parent element (what is passed to Terminal.open)
being in a different (same origin) window. Accesses to DOM APIs such
as requestAnimationFrame and devicePixelRatio thus need to be scoped
to the corrent window, instead of assuming that it's the same as the
window/global scope where the code is running.

This is done by inferring a parent window at the creation time, and
then storing it in CoreBrowserService, which is already passed to most
places that need it.

To catch future regressions, an ESLint rule that checks for global
accesses is added (it uses AST selectors via the no-restricted-syntax
rule).

This should also be applicable when the parent element is an iframe.

Fixes #3758

Before After
xterm-poput-before xterm-poput-after

@mihaip
Copy link
Contributor Author

mihaip commented Aug 31, 2022

@Tyriar was wondering if you have any thoughts on this PR. I realize it's a bit invasive, I'm happy to brainstorm other approaches. The main use case that I'm looking for is to allow an application to open terminal sessions in a child/popout window (without requiring that the popout have a full copy of the app -- it should just be a rendering surface).

Copy link
Member

@Tyriar Tyriar left a comment

Choose a reason for hiding this comment

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

@mihaip it is indeed invasive. It's just the xterm.js lib you need to load into the other side and then some tweaks to allow your app to communicate with it isn't it? That doesn't seem like that much overhead.

@jerch
Copy link
Member

jerch commented Sep 2, 2022

Beside what @Tyriar already said I also want to question that idea from a security perspective. Cross window/document handling is very hard to secure properly and might not even be possible due to pending or upcoming bugs in browser engines. For something like a terminal this is really bad to rely on, as we might open attack vectors that we cannot address/mitigate at all.

Furthermore we cannot easily introduce a potential security risk in xterm.js and tell webdevs to deal with it on their own, as many webdevs simply dont care (mind you - already had some heated discussions about risky CDN usage, where ppl will tell you "I always do it that way" - very problematic mindset with a terminal component, if you ask me).

@mihaip
Copy link
Contributor Author

mihaip commented Sep 4, 2022

@Tyriar there is some overhead in that the application now has to have a different chunk that is loaded independently (to avoid loading the entire bundle in the popout window) and a communication protocol/mechanism must be established with it. Each application that wishes to do this will need to replicate this setup. FWIWW other commonly used libraries (including React itself) support rendering in poput windows.

@jerch Note that this PR is only intended for use in same-origin/process popout windows. This does not relax any security constraints, and should not open any attack vectors.

Copy link
Member

@Tyriar Tyriar left a comment

Choose a reason for hiding this comment

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

The only issue I have with this is how much of a pain it will be to respect this. What about instead of passing around the parent window we create a new IWindowService like this:

interface IWindowService {
  window: Window;
  dpr: number;
}

dpr is here for convenience as it's used a lot. If we go this route we could maybe create an eslint rule to prevent future occurrences of window, as I'm sure they'll slip in unless we have some lint/compile rule to prevent it.

mihaip added a commit to tailscale/tailscale that referenced this pull request Sep 8, 2022
… windows

Allows other work to be unblocked while xtermjs/xterm.js#4069 is worked
through.

To enable testing the popup window handling, the standalone app allows
opening of SSH sessions in new windows by holding down the alt key
while pressing the SSH button.

Signed-off-by: Mihai Parparita <mihai@tailscale.com>
mihaip added a commit to tailscale/tailscale that referenced this pull request Sep 8, 2022
… windows

Allows other work to be unblocked while xtermjs/xterm.js#4069 is worked
through.

To enable testing the popup window handling, the standalone app allows
opening of SSH sessions in new windows by holding down the alt key
while pressing the SSH button.

Signed-off-by: Mihai Parparita <mihai@tailscale.com>
@mihaip
Copy link
Contributor Author

mihaip commented Sep 8, 2022

The IWindowService approach sounds promising, will look into reworking this PR.

The poput window is just a rendering surface, the termminal code still
executes in the original/parent window.
Add support for the parent element (what is passed to Terminal.open)
being in a different (same origin) window. Accesses to DOM APIs such
as requestAnimationFrame and devicePixelRatio thus need to be scoped
to the corrent window, instead of assuming that it's the same as the
window/global scope where the code is running.

This is done by inferring a parent window at the creation time, and
then storing it in CoreBrowserService, which is already passed to most
places that need it.

To catch future regressions, an ESLint rule that checks for global
accesses is added (it uses AST selectors via the no-restricted-syntax
rule).

This should also be applicable when the parent element is an iframe.

Fixes xtermjs#3758
@mihaip
Copy link
Contributor Author

mihaip commented Sep 9, 2022

@Tyriar I've reworked this to use a service. Instead of a new IWindowService, I ended up adding this to ICoreBrowserService, since it already seemed like the right place for such global-ish information (the existing isFocused() method is also per-window).

I also added an ESLint check for window.requestAnimationFrame(), window.devicePixelRatio and similar access patterns.

@mihaip mihaip requested a review from Tyriar September 9, 2022 00:00
@Tyriar Tyriar added this to the 5.0.0 milestone Sep 9, 2022
@Tyriar Tyriar self-assigned this Sep 9, 2022
Copy link
Member

@Tyriar Tyriar left a comment

Choose a reason for hiding this comment

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

Very nice, thanks for going the extra mile with the test button and lint rules 👍

@Tyriar Tyriar merged commit f3622df into xtermjs:master Sep 9, 2022
mihaip added a commit to tailscale/tailscale that referenced this pull request Sep 9, 2022
xtermjs/xterm.js#4069 was merged and published (in 5.0.0-beta.58),
no need for the fork added by 01e6565.

Signed-off-by: Mihai Parparita <mihai@tailscale.com>
mihaip added a commit to tailscale/tailscale that referenced this pull request Sep 9, 2022
xtermjs/xterm.js#4069 was merged and published (in 5.0.0-beta.58),
no need for the fork added by 01e6565.

Signed-off-by: Mihai Parparita <mihai@tailscale.com>
mihaip added a commit to tailscale/tailscale that referenced this pull request Oct 4, 2022
xterm 5.0 was released a few weeks ago, and it picks up
xtermjs/xterm.js#4069, which was the main reason why we were on a 5.0
beta.

Signed-off-by: Mihai Parparita <mihai@tailscale.com>
mihaip added a commit to tailscale/tailscale that referenced this pull request Oct 4, 2022
xterm 5.0 was released a few weeks ago, and it picks up
xtermjs/xterm.js#4069, which was the main reason why we were on a 5.0
beta.

Signed-off-by: Mihai Parparita <mihai@tailscale.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Terminal cannot display text when created in a react portal window.
3 participants