Upgrade xterm v3.12.0 to v4.10.0 #8260

sahil143 · 2021-03-01T10:46:41Z

Fixes: https://issues.redhat.com/browse/ODC-5547

Analysis / Root cause: To send commands programmatically to the terminal we need feature(addons) from the xterm.js v4.10.0

Solution Description: upgrade xterm version

Screen shots / Gifs for design review: No UI changes

Unit test coverage report:

Test setup:

Browser conformance:

Chrome
Firefox
Safari
Edge

frontend/packages/console-shared/src/utils/xterm-addon-fullscreen.ts

invincibleJai · 2021-03-08T11:39:54Z

frontend/packages/console-shared/src/utils/xterm-addon-fullscreen.ts

+  }
+
+  public dispose(): void {
+    if (this.terminal && this.terminal.element.classList.contains('fullscreen')) {


instead of checking for this.terminal here and below, will it make sense to just have

Suggested change

if (this.terminal && this.terminal.element.classList.contains('fullscreen')) {

if (his.terminal?.element?.classList?.contains('fullscreen')) {

rohitkrai03 · 2021-03-08T16:01:17Z

/assign

rohitkrai03 · 2021-03-09T17:46:52Z

frontend/public/components/terminal.jsx

-
-XTerminal.applyAddon(fit);
-XTerminal.applyAddon(full);
+import { XtermAddonFullscreen } from '@console/shared';


Why do we need custom implementation of fullscreen addon? Did xterm remove support for their fullscreen addon?

iirc @sahil143 said they removed this addon. I'm not sure if they replaced or why it was removed though.

Yeah, seems like they removed fullscreen addon - xtermjs/xterm.js#2065 (comment)

Yes, fullscreen addon was removed when addon api changed

christianvogt · 2021-03-09T18:59:23Z

/approve

christoph-jerolimov · 2021-03-10T08:36:30Z

@sahil143 I found two issues:

a) The small one is the "black gap" on the bottom right corner below the scrollbar:

update: This issue already exist on the master branch. Would be cool to fix this anyway. Up to you.

b) It looks like the page resize depends on the full screen height instead of the window height. I have two scrollbars. The one for the terminal is fine but the other one shows a white space below the terminal:

Peek.2021-03-10.09-35.mp4

update: Could reproduce this also on the latest Chrome and Firefox version. This does not happen on master. See comments below.

christoph-jerolimov · 2021-03-10T08:37:23Z

/retest

sahil143 · 2021-03-10T10:05:38Z

@jerolimov I'm not able to reproduce this on chrome version 89. Which version are you on?

invincibleJai · 2021-03-10T10:08:38Z

I was able to reproduce in chrome 88 but then I upgraded to 89 and can't see dual scroll anymore however on firefox I can on 78.8.0esr

christoph-jerolimov · 2021-03-10T10:25:19Z

I tested this also on Chrome 88 where this happen. I updated now to Chrome 89 and it works here.

Don't know if we want fix this anyway? As mentioned by Jai it happens also on the latest Firefox ESR version. Hmm?

sahil143 · 2021-03-10T10:34:25Z

I tried on Firefox 87 as well not able to reproduce it.

christoph-jerolimov · 2021-03-10T11:57:50Z

@sahil143 I can reproduce this also on (latest stable) 86.0. But it does not happen always! It only happens here if I first added some content (many ls -l outputs) to the output and then reduced the window size:

Peek.2021-03-10.12-57.mp4

Here the same issue with (latest) Chrome 89:

Peek.2021-03-10.12-59.mp4

I tested this also again on the master branch and could not reproduce this here.

My problem A with the black border below the scrollable area exists already on the master branch.

Tested with master:

Peek.2021-03-10.13-05.mp4

sahil143 · 2021-03-11T16:17:47Z

Took some time but finally figured out the issue. The main issue here is with the xterm-helper-textarea it has a position absolute and top, left calculated. When there is content in the terminal the text area is at the bottom but when the browser is resized, we update the height and width of the container/outerRef div then xterm updates the dimensions using the fit addon to fit into the container/outerRef but the xterm-helper-textarea stays at the bottom and its position doesn't update that's why the scrollbar appears. The issue fixes itself when the user starts to type again in the terminal.

cc @jerolimov @invincibleJai

sahil143 · 2021-03-11T16:21:07Z

There is already an open issue on xterm.js repo with similar behaviour xtermjs/xterm.js#3065

christoph-jerolimov

There is already an open issue on xterm.js repo with similar behaviour xtermjs/xterm.js#3065

Would be cool to fix it there and keep this bug in mind. I will create a new issue for this so that we can upgrade the dependency now for your depending work.

/lgtm

openshift-ci-robot · 2021-03-11T19:06:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: christianvogt, jerolimov, sahil143

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~frontend/OWNERS~~ [christianvogt]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci-robot added the component/core Related to console core functionality label Mar 1, 2021

openshift-ci-robot requested review from irosenzw and kyoto March 1, 2021 10:46

openshift-ci-robot added component/kubevirt Related to kubevirt-plugin component/shared Related to console-shared labels Mar 1, 2021

invincibleJai reviewed Mar 1, 2021

View reviewed changes

frontend/packages/console-shared/src/utils/xterm-addon-fullscreen.ts Outdated Show resolved Hide resolved

sahil143 force-pushed the xterm-4.10 branch from 1cc1ea8 to 52843ed Compare March 1, 2021 11:00

sahil143 mentioned this pull request Mar 4, 2021

create infra to execute commands in terminal #8302

Merged

4 tasks

sahil143 force-pushed the xterm-4.10 branch 2 times, most recently from 19b0e79 to b9361e8 Compare March 4, 2021 08:33

invincibleJai reviewed Mar 8, 2021

View reviewed changes

openshift-ci-robot assigned rohitkrai03 Mar 8, 2021

updgrade xterm to version 4.10.0

fa8e81b

sahil143 force-pushed the xterm-4.10 branch from b9361e8 to fa8e81b Compare March 8, 2021 16:28

rohitkrai03 reviewed Mar 9, 2021

View reviewed changes

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 9, 2021

christoph-jerolimov approved these changes Mar 11, 2021

View reviewed changes

openshift-ci-robot assigned christoph-jerolimov Mar 11, 2021

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 11, 2021

openshift-merge-robot merged commit a5ce40a into openshift:master Mar 12, 2021

spadgett added this to the v4.8 milestone Mar 18, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade xterm v3.12.0 to v4.10.0 #8260

Upgrade xterm v3.12.0 to v4.10.0 #8260

sahil143 commented Mar 1, 2021 •

edited

Loading

invincibleJai Mar 8, 2021

rohitkrai03 commented Mar 8, 2021

rohitkrai03 Mar 9, 2021

christianvogt Mar 9, 2021

rohitkrai03 Mar 10, 2021

sahil143 Mar 10, 2021

christianvogt commented Mar 9, 2021

christoph-jerolimov commented Mar 10, 2021 •

edited

Loading

christoph-jerolimov commented Mar 10, 2021

sahil143 commented Mar 10, 2021

invincibleJai commented Mar 10, 2021 •

edited

Loading

christoph-jerolimov commented Mar 10, 2021 •

edited

Loading

sahil143 commented Mar 10, 2021

christoph-jerolimov commented Mar 10, 2021 •

edited

Loading

sahil143 commented Mar 11, 2021

sahil143 commented Mar 11, 2021

christoph-jerolimov left a comment

openshift-ci-robot commented Mar 11, 2021

	if (this.terminal && this.terminal.element.classList.contains('fullscreen')) {
	if (his.terminal?.element?.classList?.contains('fullscreen')) {

Upgrade xterm v3.12.0 to v4.10.0 #8260

Upgrade xterm v3.12.0 to v4.10.0 #8260

Conversation

sahil143 commented Mar 1, 2021 • edited Loading

invincibleJai Mar 8, 2021

Choose a reason for hiding this comment

rohitkrai03 commented Mar 8, 2021

rohitkrai03 Mar 9, 2021

Choose a reason for hiding this comment

christianvogt Mar 9, 2021

Choose a reason for hiding this comment

rohitkrai03 Mar 10, 2021

Choose a reason for hiding this comment

sahil143 Mar 10, 2021

Choose a reason for hiding this comment

christianvogt commented Mar 9, 2021

christoph-jerolimov commented Mar 10, 2021 • edited Loading

christoph-jerolimov commented Mar 10, 2021

sahil143 commented Mar 10, 2021

invincibleJai commented Mar 10, 2021 • edited Loading

christoph-jerolimov commented Mar 10, 2021 • edited Loading

sahil143 commented Mar 10, 2021

christoph-jerolimov commented Mar 10, 2021 • edited Loading

sahil143 commented Mar 11, 2021

sahil143 commented Mar 11, 2021

christoph-jerolimov left a comment

Choose a reason for hiding this comment

openshift-ci-robot commented Mar 11, 2021

sahil143 commented Mar 1, 2021 •

edited

Loading

christoph-jerolimov commented Mar 10, 2021 •

edited

Loading

invincibleJai commented Mar 10, 2021 •

edited

Loading

christoph-jerolimov commented Mar 10, 2021 •

edited

Loading

christoph-jerolimov commented Mar 10, 2021 •

edited

Loading