Add WebGL addon to web termal by avatus · Pull Request #36763 · gravitational/teleport

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

    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

+      // 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


    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

  }

+  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

Why are we doing this twice?

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

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

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 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 ibeckermayer, kimlisa, rudream and zmb3 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 gzdunek and ravicious 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

Comment thread web/packages/teleport/src/lib/term/terminal.ts Outdated

Copy link
Copy Markdown

Collaborator

zmb3 Jan 18, 2024

Choose a reason for hiding this comment

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

Why are we doing this twice?

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

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

	// 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();
+    logger.info('WebGL context lost. Falling back to canvas');
+    this._webglAddon?.dispose();

Conversation

avatus commented Jan 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

orca-security-us Bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Orca Security Scan Summary

Uh oh!

kimlisa Jan 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zmb3 left a comment

Choose a reason for hiding this comment

Uh oh!

marcoandredinis commented Jan 17, 2024

Uh oh!

avatus commented Jan 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

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