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.

Sign up for GitHub

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

Jump to bottom

Character size computation is affected by CSS transforms #2488

Closed
cebamps opened this issue Oct 20, 2019 · 4 comments
Closed

Character size computation is affected by CSS transforms #2488

cebamps opened this issue Oct 20, 2019 · 4 comments
Assignees
@Tyriar
Labels
area/addon/canvas area/addon/webgl area/renderer-dom good first issue help wanted type/bug Something is misbehaving
Milestone
5.3.0

Comments

@cebamps
Copy link

cebamps commented Oct 20, 2019 •
edited

When a Terminal is opened in an element subject to CSS transformations such as scale or rotate, the computation of the character size is affected by that transformation, which is wrong because the rendering should be transform-agnostic.

As a result, this makes the integration of Xterm.js into reveal.js slides render incorrectly when the slides are scaled down to fit the browser viewport.

Changing the measurement method in CharSizeService to use getComputedStyle instead of getBoundingClientRect makes the computation independent of CSS transforms.

Details

  • Browser and browser version: Firefox 69.0 and Chromium 77.0
  • OS version: Arch Linux
  • xterm.js version: current master (6311076)

Steps to reproduce

  1. Add style="transform: rotate(90deg);" to the <body> tag in demo/index.html
  2. Notice that character boxes in the demo have the wrong dimensions

Workaround

With the rotate(90deg) CSS transform Xterm.js renders character cells with the width and height swapped around:
getBoundingClientRect

The cause is in the measurement methods in CharSizeService: it uses getBoundingClientRect, which returns the size of the bounding box after CSS transforms.

xterm.js/src/browser/services/CharSizeService.ts

Lines 71 to 86 in 6311076

public measure(): IReadonlyMeasureResult {
this._measureElement.style.fontFamily = this._optionsService.options.fontFamily;
this._measureElement.style.fontSize = `${this._optionsService.options.fontSize}px`;
// Note that this triggers a synchronous layout
const geometry = this._measureElement.getBoundingClientRect();
// If values are 0 then the element is likely currently display:none, in which case we should
// retain the previous value.
if (geometry.width !== 0 && geometry.height !== 0) {
this._result.width = geometry.width;
this._result.height = Math.ceil(geometry.height);
}
return this._result;
}

My understanding is that the rendering of the terminal should not be affected by said transforms. And indeed, replacing the geometry definition in the code above with (POC)

    const computedStyle = getComputedStyle(this._measureElement);
    const geometry = {
      height: Number(computedStyle.getPropertyValue('height').replace('px', '')),
      width: Number(computedStyle.getPropertyValue('width').replace('px', '')),
    }

yields a correct rendering:
getComputedStyle

Note that my proof-of-concept fix above may be insufficient in cases where the element or its parent are display: none.
@Tyriar Tyriar added area/renderer help wanted type/bug Something is misbehaving labels Oct 21, 2019
@Tyriar
Copy link
Member

Tyriar commented Oct 21, 2019

@cebamps the workaround seems reasonable.

Note that my proof-of-concept fix above may be insufficient in cases where the element or its parent are display: none.

I don't think this should matter.

@leomoty
Copy link
Contributor

leomoty commented Oct 22, 2019

I am not really able to reproduce. The selection with cursor is broken when you rotate the terminal.

@Tyriar Tyriar mentioned this issue Oct 19, 2021
Closed
@Tyriar
Copy link
Member

Tyriar commented Dec 15, 2022

Still reproduces:

image

@Tyriar Tyriar mentioned this issue Dec 15, 2022
Closed
arekouzounian added a commit to arekouzounian/xterm.js that referenced this issue Jan 6, 2023
@arekouzounian
Fix for xtermjs#2488. Use element offsetHeight rather than
7701557 
getBoundingRect
@arekouzounian arekouzounian mentioned this issue Jan 6, 2023
Merged
@Tyriar Tyriar self-assigned this Aug 1, 2023
@Tyriar Tyriar added this to the 5.3.0 milestone Aug 1, 2023
Tyriar added a commit that referenced this issue Aug 1, 2023
@Tyriar
Merge pull request #4366 from arekouzounian/master
ce3bb35 
Fix for #2488. Use element offsetHeight rather than getBoundingRect
@jerch
Copy link
Member

jerch commented Aug 1, 2023 •
edited

Oh well, wasnt aware of this - also needs to be changed in #4605.

@Tyriar Tyriar closed this as completed Aug 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@Tyriar Tyriar

Labels
area/addon/canvas area/addon/webgl area/renderer-dom good first issue help wanted type/bug Something is misbehaving
Projects
None yet
Milestone
5.3.0
Development

No branches or pull requests
4 participants
@leomoty @Tyriar @jerch @cebamps