Bug 1983220: Fix pod terminal second scrollbar when user reduce the window size #9534
Conversation
openshift-ci
bot
added
component/core
|
/cc @sahil143 |
There was a problem hiding this comment.
@jerolimov I opened a bugzilla bug against OCP Management Console to make merging this easier, but see my comments about narrowing down the changes in this PR.
| @@ -116,8 +115,11 @@ class Terminal_ extends React.Component { | |||
| this.fitAddon.fit(); | |||
| // update the pty | |||
| this.props.onResize(terminal.rows, terminal.cols); | |||
| // The internal xterm textarea was not repositioned when the window was resized. | |||
| // This workaround triggers an position update. | |||
| this.terminal._core?._syncTextArea?.(); | |||
There was a problem hiding this comment.
Correct me if I'm wrong, but this seems to be the only line needed to fix the bug. I would prefer that the rest of the changes (besides your comments here) be removed from the PR as they don't seem to be relevant to this bug fix.
There was a problem hiding this comment.
@TheRealJon your right, and it also make sense to keep this PR small. I dropped all other changes and retested the fix, still works fine. PTAL.
TheRealJon
changed the title
openshift-ci
bot
added
the
bugzilla/severity-low
|
@jerolimov: This pull request references Bugzilla bug 1983220, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
Requesting review from QA contact: In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
openshift-ci
bot
added
the
bugzilla/valid-bug
As workaround to fix invalid textarea position when the user resize the browser window
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jerolimov, TheRealJon 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:
Approvers can indicate their approval by writing |
|
@jerolimov: All pull requests linked via external trackers have merged: Bugzilla bug 1983220 has been moved to the MODIFIED state. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
@jerolimov: All pull requests linked via external trackers have merged: Bugzilla bug 1983220 has been moved to the MODIFIED state. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Fixes:
https://issues.redhat.com/browse/ODC-5630
https://bugzilla.redhat.com/show_bug.cgi?id=1983220
Analysis / Root cause:
Xterm uses an invisible textarea to detect keyboard events. This textarea has an absolute position and was permanently moved when the cursor position is changed. But this does not happen when the user resizes the window.
Bad positioned textarea:
Solution Description:
This was NOT solved by updating xterm to 4.13.0. Hoped that because I found this issue: xtermjs/xterm.js#3169
Open a new xterm issue: xtermjs/xterm.js#3390
As workaround call the internal
_syncTextAreawhen the window was resized. This method was currently only called when the cursor moved: https://github.com/xtermjs/xterm.js/blob/4.10.0/src/browser/Terminal.ts#L474-L477Screen shots / Gifs for design review:
Broken before in Chrome:
Broken before in Firefox:
Fixed in Chrome:
Fixed in Firefox:
Unit test coverage report:
Unchanged.
Test setup:
find .orfind /Browser conformance: