Support for lineHeight in DOM Renderer

[leomoty]

Related to #1700. I've attempted to port the calculations from Canvas renderer, it seems to be a few pixels off.

Also, does xterm.js support zooming the browser officially? That is not working properly.

@Tyriar, can you please take a look.

[Tyriar]

Also, does xterm.js support zooming the browser officially? That is not working properly.

@leomoty yes using the browser zoom should work.

[Tyriar]

@leomoty it looks like browser zoom is working? I do notice that selection is not taking line height into account though (click and drag in the terminal with line height = 2).

[leomoty]

@Tyriar, maybe what I am seeing is font sizing/spacing related? Yeah, I didn't test selection. Will do after work.

[Tyriar]

Hm, seems alright to me. Note that you're not aiming for exact parity with the canvas renderer, it's fine if they're a little different.

[leomoty]

@Tyriar, updated _createSelectionElement, the selection height is working properly now, but the selection doesn't seem to match as you go down the lines. Any pointers?

[Tyriar]

@leomoty here's the function that grabs the coordinates from the MouseEvent:

xterm.js/src/SelectionManager.ts

Line 485 in b650db5

this._model.selectionStart = this._getMouseBufferCoords(event);

[leomoty]

@Tyriar I have been a bit busy with work, just noticed the status change, I also took another look, I might have found the issue. Unsure why it happens tho.

Following the code you gave me it goes here:

https://github.com/xtermjs/xterm.js/blob/master/src/utils/MouseHelper.ts#L51-L72

When I tried to inspect this._renderer.dimensions there, it doesnt seem to account for the changes I did in:

https://github.com/xtermjs/xterm.js/pull/1733/files#diff-0a9d734ca7737653ca5c980f3c663f7aR96

I.e. actualCellHeight is 17px instead of the expected 34px. I can confirm that inside _updateDimensions it is fine.

[leomoty]

As a follow-up: getCoords gets lineHeight passed in, if I apply that to actualCellHeight it works as expected.

I wonder if this might break the canvas renderer tho. I'll wait for your feedback.

[Tyriar]

I don't think you'll need to change anything in the selection or coord code that would impact the canvas renderer as it's working for the canvas renderer. You could use the canvas renderer and see how exactly the line height is being taken into account for the selection stuff. That way you can apply whatever the canvas renderer does to the DOM renderer.

I have been a bit busy with work, just noticed the status change, I also took another look

No rush of course, that was just me organizing a little.

[leomoty]

@Tyriar Yeah, I didn't mean to change canvas renderer nor selection at all, when I noticed the MouseHelper@getCoords function, I checked the this._renderer object there.

What I noticed is:

• Canvas renderer works fine
• The changes I did to Dom Renderer are not applied

Inside DomRenderer@_updateDimensions with lineHeight = 2.0, I get 34px for actualCellHeight.
However MouseHelper@getCoords still accounts for 17px.

I have not been able to track down why it happens.

[leomoty]

@Tyriar when lineHeight is updated, it falls through to rendererType, and thus destroying the renderer, in which mouseHelper is never updated.

I am not sure we want to destroy the current renderer when those props are updated.

Either way, selection works properly with this change.

[Tyriar]

Thanks for figuring that out 😄

Looks good, I made a couple of tweaks