Support setting a padding on the .xterm element #1123

mofux · 2017-11-09T23:42:06Z

This PR moves the canvas layers into their own div element, .xterm-screen. This allows us to apply a css padding to the .xterm element and to have the scrollbar still fill the whole height of the .xterm element.

The fit() addon has also been updated to support this new feature.

Tyriar

I don't see a scroll bar anymore on macOS Sierra, even during scrolling?

Tyriar · 2017-11-18T20:17:50Z

src/Viewport.ts


-      if (this.lastRecordedViewportHeight !== this.terminal.renderer.dimensions.canvasHeight) {
-        this.lastRecordedViewportHeight = this.terminal.renderer.dimensions.canvasHeight;
-        this.viewportElement.style.height = this.lastRecordedViewportHeight + 'px';


Why do we not need to set this anymore?

Well the new .xterm-screen element is positioned relative to the .xterm parent and has the same height as the canvas elements inside. Its width equals the width of the canvas + the width of the scrollbar on the viewport. If you add a padding to the .xterm element, that means that the .xterm element will grow by that padding. The viewport is positioned absolute to always match the size of the .xterm element. For that reason, we don't have to manually set the height of the viewport element any longer.

mofux · 2017-11-19T01:33:59Z

The scrollbar is still there, but OSX draws it black, so it is really hard to see:

OSX draws a light or dark scrollbar, depending on the background color of the scrolling container. Unfortunately we cannot set the background color on the scrolling container because that would hide the content (canvas) underneath it 😟 Setting a opacity on the scroll container also applies that opacity to the scrollbar, so that doesn't work. The only thing that is working is to set the terminal background color on the scroll container with a very low alpha value e.g. background-color: rgba(0,0,0,0.01) if the terminal background was rgb(0,0,0), but that would mean that we would have to calculate that very opaque color from the currently used background color of the theme, and it would also add a little performance impact to the rendering pipeline to draw the (likely invisible) semi-transparent background. Not sure how to go on with this really...

mofux · 2017-11-19T11:47:43Z

I have added a workaround for the overlay scrollbar being barely visible on OSX. It sets the background color of the viewport element by taking the background color from the theme (which by the current design must be a 7 digit hex value, e.g. #000000) and adding a very low alpha (01) to it (#00000001).

This background color on the viewport is not perceptible by the human eye, but it fools OSX into calculating the scrollbar thumb color based on that background color, ignoring the alpha. This solution adds another alpha pass to the browser's rendering pipeline which will slow it down a little bit.

Unfortunately I couldn't find any other workaround, so I made sure this workaround is only applied on OSX, and if an overlay scrollbar is used (which we can detect if the scrollbar width is 0).

mofux · 2017-11-19T13:33:51Z

The approach above doesn't work on all browsers (tested with the demo):

Chrome: Draws a white scrollbar thumb 👍
Firefox: Draws a black scrollbar thumb, works correctly if no background color is applied to the viewport element 👍/👎
Safari: Draws a black scrollbar thumb, only works when setting a background color alpha > 0.9 👎

Conclusion:
We can't get the overlay scrollbar to work reliably on all browsers.

Solutions:

Account some extra space for the scrollbar if the scrollbar width is 0 (OSX overlay scrollbar). This will waste some space and make the terminal look weird if not currently being scrolled, but I think that is what we did before.
Overwrite the scrollbar style with custom css. This will force OSX to always draw the scrollbar, but unfortunately we cannot re-use the system scrollbar style 😡
Introduce the workarounds for Chrome and Firefox and fallback to 1. for the other browsers.
Have somebody smarter than me figure out a css trick that works across all these browsers.

Workarounds I've tried:

Set opacity on the viewport element: Also applies that opacity to the scrollbar
Use the css clip property to clip away the background: Also clips away the scrollbar
Use background color with alpha: See above
Use transform: scaleX to scale down the width of the viewport element: Also applies that scale to the scrollbar
Make the viewport container only 1px wide but set overflow-x: visible on it: Only shows 1px of the scrollbar

mofux · 2017-11-20T21:49:46Z

I've fixed the code to support Firefox. At this stage, Chrome and Firefox both show the scrollbar thumb correctly - based on the terminal background color. I still can't get Safari to work correctly, but I'd argue that that's not a huge problem. The scrollbar is still visible on Safari, it's only dark no matter what background the terminal has...

Tyriar · 2017-11-22T21:47:32Z

I don't think the 01 alpha trick is ideal as that's extra a bunch of extra processing that needs to happen when compositing. I looked into a possible solution and got something that kind of works, the basic outline is:

Move the .xterm-screen element after .xterm-viewport so that it is drawn on top
If the OS is macOS and scroll bar width is 0, assume it's x pixels large
Make room for the scroll bar in .xterm-viewport, not .xterm-screen

The dark scroll bar still happens on Safari but that was there before.

Here's a very messy diff as a proof of concept:

diff --git a/src/Terminal.ts b/src/Terminal.ts
index c5a7575..a101473 100644
--- a/src/Terminal.ts
+++ b/src/Terminal.ts
@@ -589,10 +589,6 @@ export class Terminal extends EventEmitter implements ITerminal, IInputHandlingT
 
     this.element.setAttribute('tabindex', '0');
 
-    this.screenElement = document.createElement('div');
-    this.screenElement.classList.add('xterm-screen');
-    this.element.appendChild(this.screenElement);
-
     this.viewportElement = document.createElement('div');
     this.viewportElement.classList.add('xterm-viewport');
     this.element.appendChild(this.viewportElement);
@@ -600,6 +596,10 @@ export class Terminal extends EventEmitter implements ITerminal, IInputHandlingT
     this.viewportScrollArea.classList.add('xterm-scroll-area');
     this.viewportElement.appendChild(this.viewportScrollArea);
 
+    this.screenElement = document.createElement('div');
+    this.screenElement.classList.add('xterm-screen');
+    this.element.appendChild(this.screenElement);
+
     // preload audio
     this.syncBellSound();
 
diff --git a/src/Viewport.ts b/src/Viewport.ts
index 0581445..07fcbcb 100644
--- a/src/Viewport.ts
+++ b/src/Viewport.ts
@@ -33,7 +33,7 @@ export class Viewport implements IViewport {
     private scrollArea: HTMLElement,
     private charMeasure: CharMeasure
   ) {
-    this.scrollBarWidth = this.viewportElement.offsetWidth - this.scrollArea.offsetWidth;
+    this.scrollBarWidth = (this.viewportElement.offsetWidth - this.scrollArea.offsetWidth) || 10;
     this.viewportElement.addEventListener('scroll', this.onScroll.bind(this));
 
     // Perform this async to ensure the CharMeasure is ready.
@@ -47,9 +47,10 @@ export class Viewport implements IViewport {
     // calculates the scrollbar thumb color from the background color of the viewport element.
     // Since the viewport element has to be transparent in order to see the underlying canvas,
     // we use the terminal background color from the theme and add a very low alpha value to it.
-    if (isMac && isChrome && this.scrollBarWidth === 0 && colors.background.length === 7) {
-      this.viewportElement.style.backgroundColor = `${colors.background}01`;
-    }
+    // if (isMac && isChrome && this.scrollBarWidth === 0 && colors.background.length === 7) {
+
+    this.viewportElement.style.backgroundColor = colors.background;
+    // }
   }
 
   /**
@@ -57,6 +58,8 @@ export class Viewport implements IViewport {
    * necessary.
    */
   private refresh(): void {
+    console.log('scroll', this.scrollBarWidth);
+    this.viewportElement.style.width = `${this.terminal.renderer.dimensions.canvasWidth + this.scrollBarWidth}px`;
     if (this.charMeasure.height > 0) {
       this.currentRowHeight = this.terminal.renderer.dimensions.scaledCellHeight / window.devicePixelRatio;
       this.lastRecordedViewportHeight = this.viewportElement.offsetHeight;
diff --git a/src/renderer/Renderer.ts b/src/renderer/Renderer.ts
index b6a5c68..a7f6c0b 100644
--- a/src/renderer/Renderer.ts
+++ b/src/renderer/Renderer.ts
@@ -136,7 +136,7 @@ export class Renderer extends EventEmitter implements IRenderer {
     this._renderLayers.forEach(l => l.resize(this._terminal, this.dimensions, didCharSizeChange));
 
 		// Resize the screen
-    this._terminal.screenElement.style.width = `${this.dimensions.canvasWidth + this._terminal.viewport.scrollBarWidth}px`;
+    this._terminal.screenElement.style.width = `${this.dimensions.canvasWidth}px`;
     this._terminal.screenElement.style.height = `${this.dimensions.canvasHeight}px`;
 
     // Force a refresh

mofux · 2017-11-28T12:10:02Z

@Tyriar I'm tempted to not use a workaround for this at all. I agree that the hex value hack is bad and shouldn't go into production.

I think that having empty space beside the screen area is even worse than having a barely visible overlay scrollbar. Ultimately, this bug should be solved by Chrome and Safari - Firefox seems to do the best job here. Last but not least, implementors can always provide their own scrollbar style via css (like vscode does), which will work just fine. @parisk @Tyriar Any thoughts?

parisk

For me it works great on Chrome and Firefox on High Sierra, but I get the barely visible scrollbar on Safari.

I think it's quite important to fix this, as it can lead Safari users to believe that they cannot scroll.

What we could do here is just detect safari and apply a custom scrollbar color on it. It won't look stellar, but it would do the work.

In general I am all in for this update after we fix this somehow. 👍

Tyriar · 2017-12-11T16:49:24Z

Let's defer this as we need to get v3 out, @mofux could you pull out the fix for #1081 into it's own PR? We should be able to merge something like that easily.

mofux · 2017-12-11T16:51:43Z

@Tyriar no problem. I'll get to it tomorrow, hope that's soon enough.

Tyriar · 2017-12-11T17:03:47Z

@mofux sounds good 👌

mofux · 2017-12-13T15:12:02Z

@Tyriar @parisk I have updated the PR to account additional 15px as the scrollbar width if the actual scrollbar width is 0 (as suggested by @Tyriar above). I think this is a good compromise for now (and it mirrors the current behaviour). Please review again, hope it's good to merge now. 🧐

Tyriar · 2017-12-14T16:56:27Z

Terminal.scrollToBottom doesn't seem to work properly when you have padding. On macOS, I scroll up to top then press a key (which triggers scrollToBottom) and the terminal scrolls to the second last line.

There is also a test failure on Linux (xterm output comparison t0500-bash_long_line.in https://travis-ci.org/xtermjs/xterm.js/jobs/315932024) and a compile problem :src\Viewport.ts(9,17): error TS2305: Module '"C:/Users/Daniel/Documents/dev/mofux/xterm.js/src/utils/Browser"' has no exported member 'isChrome'.

Tyriar · 2017-12-18T17:42:49Z

Pulled the fit sizing fix into #1154

The main reason for updating xterm.js is to get the fixes for the wrong height and width computation. Now, the output should not be too long or to short for the available space, both vertically and horizontally. Because of how things are computed now, it is not possible to apply a padding on the terminal container as we currently do. The padding will work, but the canvas width won't be adjusted, so the output will go too far on the right and at the bottom. Once this PR xtermjs/xterm.js#1123 gets merged in xterm.js, we should be able to add back the padding. The addon system has been reworked. Before, just importing the addons (fit and attach) would modify the Terminal prototype and add some methods. Now, we have to call the static method Terminal.applyAddon. Also, because of the changes to the addon system, we don't need the ignore-loader snippets in the webpack configuration anymore. Signed-off-by: Simon Marchi <simon.marchi@ericsson.com>

The main reason for updating xterm.js is to get the fixes for the wrong height and width computation. Now, the output should not be too long or to short for the available space, both vertically and horizontally. Because of how things are computed now, it is not possible to apply a padding on the terminal container as we currently do. The padding will work, but the canvas width won't be adjusted, so the output will go too far on the right and at the bottom. Once this PR xtermjs/xterm.js#1123 gets merged in xterm.js, we should be able to add back the padding. The addon system has been reworked. Before, just importing the addons (fit and attach) would modify the Terminal prototype and add some methods. Now, we have to call the static method Terminal.applyAddon. Also, because of the changes to the addon system, we don't need the ignore-loader snippets in the webpack configuration anymore. Signed-off-by: Simon Marchi <simon.marchi@ericsson.com> Signed-off-by: Anton Kosiakov <anton.kosyakov@typefox.io>

The main reason for updating xterm.js is to get the fixes for the wrong height and width computation. Now, the output should not be too long or to short for the available space, both vertically and horizontally. Because of how things are computed now, it is not possible to apply a padding on the terminal container as we currently do. The padding will work, but the canvas width won't be adjusted, so the output will go too far on the right and at the bottom. Once this PR xtermjs/xterm.js#1123 gets merged in xterm.js, we should be able to add back the padding. The addon system has been reworked. Before, just importing the addons (fit and attach) would modify the Terminal prototype and add some methods. Now, we have to call the static method Terminal.applyAddon. Also, because of the changes to the addon system, we don't need the ignore-loader snippets in the webpack configuration anymore. Signed-off-by: Simon Marchi <simon.marchi@ericsson.com>

The main reason for updating xterm.js is to get the fixes for the wrong height and width computation. Now, the output should not be too long or to short for the available space, both vertically and horizontally. Because of how things are computed now, it is not possible to apply a padding on the terminal container as we currently do. The padding will work, but the canvas width won't be adjusted, so the output will go too far on the right and at the bottom. Once this PR xtermjs/xterm.js#1123 gets merged in xterm.js, we should be able to add back the padding. The addon system has been reworked. Before, just importing the addons (fit and attach) would modify the Terminal prototype and add some methods. Now, we have to call the static method Terminal.applyAddon. Also, because of the changes to the addon system, we don't need the ignore-loader snippets in the webpack configuration anymore. Finally, I think we don't need @types/xterm anymore, since the xterm package provides its own typings. Signed-off-by: Simon Marchi <simon.marchi@ericsson.com>

cancan101 · 2018-01-12T01:24:54Z

Any updates on this PR? Also it seems like the black scroll bars are already an issue on master.

mofux · 2018-01-15T15:12:24Z

Closing this PR in favour of #1208

mofux added 2 commits November 10, 2017 00:18

Allow to add padding

22ddfaf

Merge branch 'v3' of https://github.com/sourcelair/xterm.js into fit

6172f58

mofux requested a review from Tyriar November 9, 2017 23:42

Tyriar reviewed Nov 18, 2017

View reviewed changes

Make scrollbar thumb visible on OSX

8a2af9e

Add overlay scrollbar support for Firefox

101c985

parisk suggested changes Dec 8, 2017

View reviewed changes

mofux added 2 commits December 13, 2017 15:17

Merge branch 'v3' of https://github.com/sourcelair/xterm.js into fit

65677e7

Account additional space for OSX overlay scrollbars.

f891164

Tyriar mentioned this pull request Dec 18, 2017

Fix fit() sizing and also support letterSpacing #1154

Closed

Tyriar added this to the 3.1.0 milestone Dec 18, 2017

simark mentioned this pull request Dec 21, 2017

Update xterm.js eclipse-theia/theia#1022

Merged

Tyriar changed the base branch from v3 to master January 5, 2018 16:27

cancan101 mentioned this pull request Jan 10, 2018

Upgrade xterm.js to 3.1.0 jupyter/notebook#3189

Merged

mofux closed this Jan 15, 2018

svenefftinge mentioned this pull request Jan 16, 2018

[terminal] add padding to the left eclipse-theia/theia#1064

Closed

Tyriar removed this from the 3.1.0 milestone Feb 6, 2018

mofux deleted the fit branch September 15, 2023 08:27

Support setting a padding on the .xterm element #1123

Support setting a padding on the .xterm element #1123

Uh oh!

Conversation

mofux commented Nov 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Tyriar left a comment

Choose a reason for hiding this comment

Uh oh!

Tyriar Nov 18, 2017

Choose a reason for hiding this comment

Uh oh!

mofux Nov 19, 2017 • 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!

mofux commented Nov 19, 2017

Uh oh!

mofux commented Nov 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mofux commented Nov 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mofux commented Nov 20, 2017

Uh oh!

Tyriar commented Nov 22, 2017

Uh oh!

mofux commented Nov 28, 2017

Uh oh!

parisk left a comment

Choose a reason for hiding this comment

Uh oh!

Tyriar commented Dec 11, 2017

Uh oh!

mofux commented Dec 11, 2017

Uh oh!

Tyriar commented Dec 11, 2017

Uh oh!

mofux commented Dec 13, 2017

Uh oh!

Tyriar commented Dec 14, 2017

Uh oh!

Tyriar commented Dec 18, 2017

Uh oh!

cancan101 commented Jan 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mofux commented Jan 15, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mofux commented Nov 9, 2017 •

edited

Loading

mofux Nov 19, 2017 •

edited

Loading

mofux commented Nov 19, 2017 •

edited

Loading

mofux commented Nov 19, 2017 •

edited

Loading

cancan101 commented Jan 12, 2018 •

edited

Loading