Xterm.js: Selection should not change text color

[Tyriar]

https://github.com/sourcelair/xterm.js/pull/719 made it so that the selection is on top in order to be able to show the selection on characters with their background set. Ideally we shouldn't use a transparent selection though and instead draw the characters on top of the selection. Perhaps we should pull out the rendering of the background colors so that rendering occurs like this:

1. background colors
2. selection
3. text

[mofux]

I think we may be able to do that without splitting the dom into three layers - by using a css trick 🤓. I've created a simple example here:
https://jsfiddle.net/p6abybku/3/

The idea is to wrap the text inside an inner span which has z-index: 1 and position: relative. The outer span will get the background color, and the inner span the foreground color.

I believe this will also only have a minimal impact on the performance, compared to rendering and keeping separate background / foreground layers.

[Tyriar]

@mofux I didn't think about this trick :smile: we would need to wait for https://github.com/sourcelair/xterm.js/issues/654 if we wanted to try this out.

On the performance side, I'm thinking making them separate layers might actually end up being faster. The spans wrapping the text would need to be recreated every time the line changes, whereas for the new layer approach we could have x background elements that move/hide depending on the state. They would need to be present and have the layer trick even when there is no selection as when the renderer refreshes the viewport it does so without any knowledge of the selection.

Another perk of separation is that we could delay background layer positioning/rendering like how the evaluating links is delayed (say 50ms, ~3 frames?). This means that for a command that spits out a bunch of output (with/without bg colors) we would only need to recalc and render the background when there is a small break in rendering, meaning frames are rendered faster and therefore output is processed faster.

[Tyriar]

I may be overthinking things though :wink:

[mofux]

I think the biggest advantage with the css approach is that we would not have to manually set the size of the background layer spans - which we otherwise would have to set manually by measuring the text layer span's. Mmmh and maybe the outer bg span would only have to be drawn if there is actually a background set 🤔.

If I read the code correctly, we probably would have to optimise the spanElementObjectPool to have the wrapped fg/bg spans cached, otherwise we may end up with a similar unoptimised code path that is used for wide characters?
https://github.com/sourcelair/xterm.js/blob/master/src/Renderer.ts#L270

Anyway, awesome work on the virtual selection! Just wanted to point out this alternate approach 😊

[Tyriar]

Mmmh and maybe the outer bg span would only have to be drawn if there is actually a background set

The background being created/drawn is less of a concern here, the bigger concern is creating these additional text layers for each span 100% of the time.

we probably would have to optimise the spanElementObjectPool to have the wrapped fg/bg spans cached

Yeah currently the object pool only goes 1 level deep, this needs some work and benchmarking to see if there are gains from going deeper 🙂

[mofux]

I'm slowly starting to understand how the screen buffer works, and why your approach might be smarter 😅

[Tyriar]

@mofux yeah it became a little complicated to get perf so good 😉. You can probably get some good insights by reading through some of the links here https://github.com/Microsoft/vscode/issues/17875

[Tyriar]

Regresses in https://github.com/sourcelair/xterm.js/pull/991

[Luxcium]

I was in zeit/hyper (issues/2934) wanting to change the color of selected text (like in the editor in VS Code) but have been looking into it all the afternoon and now that I see your struggle (since almost 2 years) it kind of stopped my enthusiasm I was about to fix it naively and send a pull request but now I feel like a child with a solution to adults problems ... I wanted just to send it and wait for your feedback but now that I know it could add 0.3 milliseconds [sic] to the rendering I feel like I am not qualified enough to be the one to find the solution ...

[Tyriar]

@luxcium this is actually one of the issues that should be fixed by https://github.com/xtermjs/xterm.js/pull/1790, it's been a while since I looked but I think once that's ready we should be able to change the DOM renderer to also show the selection under the text as the only reason we regressed here was because the "default" renderer became the canvas (2d) renderer.

[Tyriar]

This issue was closed off as it's fixed in the experimental WebGL renderer addon. It's not ready yet but I'm closing this off so I don't forget about it. See this query for current webgl addon issues https://github.com/xtermjs/xterm.js/issues?q=is%3Aissue+is%3Aopen+label%3Aarea%2Faddon%2Fwebgl