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

Fixed characterHeight calculation, which could cause overflowing #598

Merged
merged 2 commits into from
Mar 15, 2017

Conversation

mikesir87
Copy link
Contributor

The previous calculation was simply using the height of the letter
W, but wasn't taking into account that each row has a lineHeight,
which might be greater. If the lineHeight is .4px off, after many
rows, it will cause the proposedGeometry to have an extra row,
potentially causing hidden rows (based on layout).

The previous calculation was simply using the height of the letter
W, but wasn't taking into account that each row has a lineHeight,
which might be greater. If the lineHeight is .4px off, after many
rows, it will cause the proposedGeometry to have an extra row,
potentially causing hidden rows (based on layout).
@Tyriar
Copy link
Member

Tyriar commented Mar 14, 2017

Deferring to @parisk since I don't use the fit addon.

@coveralls
Copy link

Coverage Status

Coverage increased (+7.3%) to 67.75% when pulling 3c3a646 on mikesir87:fix-fit-calculation into ff29fd6 on sourcelair:master.

Copy link
Contributor

@parisk parisk left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution @mikesir87!

It seems that the offsetHeight attribute takes into account the actual height, padding and border of an element, so it does not ignore the parent element's lineHeight.

My objection with this implementation is that it uses the style attribute, while it would be better if we could somehow extract the calculated height of the line.

The best would be to either utilize CharMeasure.ts (which is kind of difficult) or at least simulate this behavior here.

mikesir87 added a commit to mikesir87/xterm.js that referenced this pull request Mar 15, 2017
@mikesir87
Copy link
Contributor Author

It seems that the offsetHeight attribute takes into account the actual height, padding and border of an element, so it does not ignore the parent element's lineHeight.

You're absolutely correct here. The problem is that offsetHeight and clientHeight return the value as an integer. But, what if the value was non-integer? Chrome seems to truncate the decimal completely, while Safari and FF behave differently by using the decimal value. The Codepen below demonstrates this. You'll see in FF/Safari that if you adjust the line-height to another decimal value, the overall height of the container changes, while in Chrome, it stays constant.

http://codepen.io/anon/pen/LWzEYj

My objection with this implementation is that it uses the style attribute, while it would be better if we could somehow extract the calculated height of the line.

Haha... no objections about that here! Does seem like a "hack" of a solution. Looking at how the CharMeasure works and is being used, I think a better approach is to use get the boundingClientRect's height. I tested in FF/Safari/Chrome and they all return the expected value (supporting non-integer values). New commit reflects this change.

@coveralls
Copy link

Coverage Status

Coverage increased (+7.3%) to 67.75% when pulling 6cbf0b1 on mikesir87:fix-fit-calculation into ff29fd6 on sourcelair:master.

@@ -56,7 +56,7 @@
subjectRow.innerHTML = 'W'; // Common character for measuring width, although on monospace
characterWidth = subjectRow.getBoundingClientRect().width;
subjectRow.style.display = ''; // Revert style before calculating height, since they differ.
characterHeight = parseFloat(term.rowContainer.style.lineHeight);
characterHeight = term.getBoundingClientRect().height;
Copy link
Contributor

Choose a reason for hiding this comment

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

term is the terminal instance, not the subject row that you will want to measure the height on. I'm afraid this line will crash... 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch and sorry about that... yes, it would crash. Just pushed the fix.

@coveralls
Copy link

Coverage Status

Coverage increased (+7.3%) to 67.75% when pulling 0cb8ecc on mikesir87:fix-fit-calculation into ff29fd6 on sourcelair:master.

@parisk
Copy link
Contributor

parisk commented Mar 15, 2017

Looks great now. Thanks a lot @mikesir87!

@parisk parisk merged commit eb25e88 into xtermjs:master Mar 15, 2017
@Tyriar Tyriar modified the milestone: 2.5.0 Mar 31, 2017
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.

None yet

5 participants