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

fix terminal width calculation #1288

Merged
merged 1 commit into from
Feb 24, 2018
Merged

fix terminal width calculation #1288

merged 1 commit into from
Feb 24, 2018

Conversation

ZoeyR
Copy link
Contributor

@ZoeyR ZoeyR commented Feb 16, 2018

Fixes #1284

The problem breaks down as follows:

  • The regression was introduced in Support setting padding on the .xterm element #1208 to support padding in the .xterm css class
  • Padding .xterm is calculated from the right hand side of the scrollbar
  • The PR attempted to make the effective area of the terminal match the padding exactly
  • The addition of scrollBarWidth in the code segment below causes the effective interactive area of the terminal to be (column_width * num_columns) + scrollbar_width

Renderer.ts

this._terminal.screenElement.style.width = `${this.dimensions.canvasWidth + this._terminal.viewport.scrollBarWidth}px`;
  • This means that there is an area past where the terminal will actually have text that has all the interactions of a terminal, when the padding is less than the width of the scroll bar this will cause interactivity issues with the scroll bar element.

I believe the correct fix is to remove the addition of scrollBarWidth when setting the screenElement width. This does mean that padding on the right side of the terminal will appear to be too large but the terminal already had the implicit padding of avoiding the scroll bar anyways.

Edit: After looking at the results more closely this will have the same visual layout as before, it just makes the clickable area match the actual terminal area.

A workaround for anyone affected by this would be to set the right padding of .xterm to be equal to the width of the scroll bar.

@Tyriar Tyriar added this to the 3.2.0 milestone Feb 17, 2018
@Tyriar
Copy link
Member

Tyriar commented Feb 24, 2018

This broke the scrollbar in VS Code as well when I reduced the horizontal margin between the xterm element and the scrollbar. The fix works 👍

@Tyriar Tyriar merged commit c801734 into xtermjs:master Feb 24, 2018
facebook-github-bot pushed a commit to facebookarchive/nuclide that referenced this pull request Jul 16, 2018
Summary:
The original reason for playing with the background and z-indices was to fix scroll bar bug. However that was fixed more robustly upstream:
xtermjs/xterm.js#1288

Now that we have pulled in that fix, we don't need the workaround here anymore.

Reviewed By: hansonw

Differential Revision: D8848848

fbshipit-source-id: a9725b522f59df6a7969f5f2299bb0b8ce9d8bb3
hansonw pushed a commit to facebookarchive/atom-ide-ui that referenced this pull request Jul 18, 2018
Summary:
The original reason for playing with the background and z-indices was to fix scroll bar bug. However that was fixed more robustly upstream:
xtermjs/xterm.js#1288

Now that we have pulled in that fix, we don't need the workaround here anymore.

Reviewed By: hansonw

Differential Revision: D8848848

fbshipit-source-id: a9725b522f59df6a7969f5f2299bb0b8ce9d8bb3
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.

Fit addon covers scrollbar
3 participants