Add WebGL addon to web termal #36763

avatus · 2024-01-16T21:05:34Z

This will add the xterm-webgl-addon to the web ui (not Connect) to help properly display underscores (and perhaps other characters that we haven't discovered).

Tested on Ubuntu 24

Without addon (there should be an underscore between "testing" and "thing")

With addon

changelog: The web terminal now properly displays underscores on Linux

orca-security-us

Orca Security Scan Summary

Status	Check	Issues by priority
Passed	Infrastructure as Code	0 0 0 0	View in Orca
Failed	Vulnerabilities	3 1 0 0	View in Orca
Passed	Secrets	0 0 0 0	View in Orca

kimlisa · 2024-01-17T01:39:27Z

web/packages/teleport/src/lib/term/terminal.ts

@@ -78,6 +80,12 @@ export default class TtyTerminal {

    this.term.loadAddon(this._fitAddon);
    this.term.loadAddon(this._webLinksAddon);
+    // handle context loss and load webgl addon
+    this._webglAddon.onContextLoss(() => {
+      this._webglAddon.dispose();


i think we need to also call this in the destory() func.

i'm not sure i understand what the onContextLoss does, but shouldn't we try to re-instantiate it after disposing?

Yeah we need it in destroy() for sure. thank you!

onContextLoss is essentially when the browser loses access to the GPU when rendering (if you do some weird driver updates or some kind of switch between GPUs). it's rare. However, the addon itself runs a timeout internally to automatically reinstantiate after a couple seconds and keeps trying.

The docs say:

An easy, but suboptimal way, to handle this is to..

Is there a more optimal way to handle this scenario?

zmb3

Do we need a fallback to canvas renderer? Do we need any sort of feature detection in case webgl is not available?

marcoandredinis · 2024-01-17T08:44:16Z

I can confirm that this works for me 👍

avatus · 2024-01-17T09:20:25Z

Do we need a fallback to canvas renderer? Do we need any sort of feature detection in case webgl is not available?

We dont need to, it falls back to the dom renderer by default. Also, the "suboptimal" way I believe is them talking about just disposing and falling back to dom rendering. I checked out vscode and they basically do webgl -> canvas -> dom. I can do the same here

Do we need any sort of feature detection in case webgl is not available?

the WebglAddon constructor will throw if webgl is not supported, so catching that and falling back to canvas/dom is good enough. we don't really need any independent feature detection/state imo

zmb3 · 2024-01-17T13:30:31Z

web/packages/teleport/src/lib/term/terminal.ts

+      // wont start looking for the context again until the OS has given the browser the context again.
+      // When the initial context lost event is fired, the webgl addon consumes the event
+      // and waits for a bit to see if it can get the context back. If it fails repeatedly, it
+      // will propegate the context loss event itself in which case we fall back to context.


Suggested change

// will propegate the context loss event itself in which case we fall back to context.

// will propagate the context loss event itself in which case we fall back to canvas.

zmb3 · 2024-01-17T13:31:44Z

web/packages/teleport/src/lib/term/terminal.ts

@@ -77,7 +81,28 @@ export default class TtyTerminal {
    });

    this.term.loadAddon(this._fitAddon);
-    this.term.loadAddon(this._webLinksAddon);
+    // this.term.loadAddon(this._webLinksAddon);


No more link support? 😕

Oh my goodness thank you for catching this

zmb3 · 2024-01-17T13:32:05Z

web/packages/teleport/src/lib/term/terminal.ts

@@ -98,6 +123,21 @@ export default class TtyTerminal {
    window.addEventListener('resize', this._debouncedResize);
  }

+  fallbackToCanvas() {
+    console.log('WebGL context lost. Falling back to canvas');


Should we use our logger instead of console here?

zmb3 · 2024-01-18T17:50:54Z

web/packages/teleport/src/lib/term/terminal.ts

+    } catch (err) {
+      this.fallbackToCanvas();
+    }
+    this.term.loadAddon(this._webglAddon);


Why are we doing this twice?

kimlisa · 2024-01-18T20:15:13Z

web/packages/teleport/src/lib/term/terminal.ts

+    try {
+      // try to create a new WebglAddon. If webgl is not supported, this
+      // constructor will throw an error and fallback to canvas. We also fallback
+      // to canvas is the webgl context is lost after a timeout.


Suggested change

// to canvas is the webgl context is lost after a timeout.

// to canvas if the webgl context is lost after a timeout.

kimlisa · 2024-01-18T20:21:52Z

web/packages/teleport/src/lib/term/terminal.ts

+    logger.info('WebGL context lost. Falling back to canvas');
+    this._webglAddon.dispose();


seems safer since it's initially undefined

Suggested change

logger.info('WebGL context lost. Falling back to canvas');

this._webglAddon.dispose();

logger.info('WebGL context lost. Falling back to canvas');

this._webglAddon?.dispose();

absolutely right. ty

public-teleport-github-review-bot · 2024-01-18T20:51:40Z

@avatus See the table below for backport results.

Branch	Result
branch/v13	Failed
branch/v14	Failed
branch/v15	Create PR

avatus added backport-required backport/branch/v13 backport/branch/v14 backport/branch/v15 labels Jan 16, 2024

orca-security-us bot reviewed Jan 16, 2024

View reviewed changes

github-actions bot requested review from gzdunek and ravicious January 16, 2024 21:06

github-actions bot added size/lg ui labels Jan 16, 2024

avatus requested review from zmb3, kimlisa, rudream and ibeckermayer January 16, 2024 21:10

avatus enabled auto-merge January 16, 2024 21:27

zmb3 requested a review from marcoandredinis January 16, 2024 21:34

avatus requested review from ryanclark and removed request for ravicious and gzdunek January 17, 2024 00:03

kimlisa reviewed Jan 17, 2024

View reviewed changes

zmb3 reviewed Jan 17, 2024

View reviewed changes

marcoandredinis removed their request for review January 18, 2024 12:04

zmb3 reviewed Jan 18, 2024

View reviewed changes

zmb3 approved these changes Jan 18, 2024

View reviewed changes

kimlisa approved these changes Jan 18, 2024

View reviewed changes

avatus added this pull request to the merge queue Jan 18, 2024

kimlisa removed this pull request from the merge queue due to a manual request Jan 18, 2024

public-teleport-github-review-bot bot removed the request for review from rudream January 18, 2024 20:24

public-teleport-github-review-bot bot removed request for ibeckermayer and ryanclark January 18, 2024 20:24

avatus force-pushed the avatus/webgl branch from b8ed488 to bfafb8f Compare January 18, 2024 20:26

avatus enabled auto-merge January 18, 2024 20:27

avatus disabled auto-merge January 18, 2024 20:28

Add WebGL addon to web terminal

b5a8df4

avatus force-pushed the avatus/webgl branch from bfafb8f to b5a8df4 Compare January 18, 2024 20:29

avatus enabled auto-merge January 18, 2024 20:29

avatus added this pull request to the merge queue Jan 18, 2024

Merged via the queue into master with commit 269150e Jan 18, 2024
38 checks passed

avatus deleted the avatus/webgl branch January 18, 2024 20:49

This was referenced Jan 18, 2024

[v15] Add WebGL addon to web termal #36889

Merged

[v14] Add WebGL addon to terminal #36890

Merged

[v13] Add WebGL addon to web terminal #36891

Merged

marcoandredinis mentioned this pull request Jan 29, 2024

Terminal in Web UI: _ show as spaces #29126

Closed

zmb3 mentioned this pull request Jul 7, 2024

It's not possible to select text in vim in Web UI #42032

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add WebGL addon to web termal #36763

Add WebGL addon to web termal #36763

avatus commented Jan 16, 2024 •

edited

Loading

orca-security-us bot left a comment •

edited

Loading

kimlisa Jan 17, 2024 •

edited

Loading

avatus Jan 17, 2024

zmb3 Jan 17, 2024

zmb3 left a comment

marcoandredinis commented Jan 17, 2024

avatus commented Jan 17, 2024 •

edited

Loading

zmb3 Jan 17, 2024

zmb3 Jan 17, 2024

avatus Jan 17, 2024

zmb3 Jan 17, 2024

zmb3 Jan 18, 2024

kimlisa Jan 18, 2024

kimlisa Jan 18, 2024

avatus Jan 18, 2024

public-teleport-github-review-bot bot commented Jan 18, 2024

	// will propegate the context loss event itself in which case we fall back to context.
	// will propagate the context loss event itself in which case we fall back to canvas.

	// to canvas is the webgl context is lost after a timeout.
	// to canvas if the webgl context is lost after a timeout.

		logger.info('WebGL context lost. Falling back to canvas');
		this._webglAddon.dispose();

Add WebGL addon to web termal #36763

Add WebGL addon to web termal #36763

Conversation

avatus commented Jan 16, 2024 • edited Loading

orca-security-us bot left a comment • edited Loading

Choose a reason for hiding this comment

Orca Security Scan Summary

kimlisa Jan 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zmb3 left a comment

Choose a reason for hiding this comment

marcoandredinis commented Jan 17, 2024

avatus commented Jan 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

public-teleport-github-review-bot bot commented Jan 18, 2024

avatus commented Jan 16, 2024 •

edited

Loading

orca-security-us bot left a comment •

edited

Loading

kimlisa Jan 17, 2024 •

edited

Loading

avatus commented Jan 17, 2024 •

edited

Loading