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 for #2488. Use element offsetHeight rather than getBoundingRect #4366

Merged
merged 3 commits into from
Aug 1, 2023

Conversation

arekouzounian
Copy link
Contributor

This was a simple fix for #2488, as using the offsetHeight/Width resolved the issue.

I tried to create a quick test for the patch by having a button that would apply the transform to the body, but applying the transform after the page had loaded didn't cause any issues with character size, even without the patch applied.

I decided just to leave out the button, because it didn't seem to help as a test.

This is my first open-source contribution ever, so any and all feedback is much appreciated. Thank you!

@Tyriar Tyriar added this to the 5.3.0 milestone Aug 1, 2023
@Tyriar Tyriar self-assigned this Aug 1, 2023
Copy link
Member

@Tyriar Tyriar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @arekouzounian! Sorry about taking so long for your first ever contributor

@Tyriar Tyriar enabled auto-merge August 1, 2023 11:55
@Tyriar Tyriar merged commit ce3bb35 into xtermjs:master Aug 1, 2023
@jerch jerch mentioned this pull request Aug 1, 2023
21 tasks
@jerch
Copy link
Member

jerch commented Aug 1, 2023

Eww, looking at this PR again - plz note that offsetWidth & offsetHeight are defined as integer, where getBoundingClientRect gives fractions. Ofc ideally we want the cells to be spaced in integers, but I am not sure if the dimension calculations in the renderers still will yield the same numbers here.
To get more accurate fractional numbers we'd have to measure multiple chars at once (already applied for width in the DOM PR).

Edit:
Still gonna change it in the width cache to match this (as I did not find any glitches from it), but we prolly should address this with a follow-up.

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.

3 participants