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

Support checking height in checkBoldBroken #582

Merged
merged 5 commits into from
Mar 10, 2017
Merged

Support checking height in checkBoldBroken #582

merged 5 commits into from
Mar 10, 2017

Conversation

tmyt
Copy link
Contributor

@tmyt tmyt commented Mar 3, 2017

This PR resolve #581

After apply this patch, resolve issue and gets these results.

  • With no Bold chars

image

  • With Bold chars

image

And each row DOM elements has styles.
image

Please consider to fix.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.06%) to 67.727% when pulling 43cb4f4 on tmyt:propose/row-height into 8693d06 on sourcelair:master.

@Tyriar
Copy link
Member

Tyriar commented Mar 3, 2017

@tmyt what font are you seeing the issue on?

@tmyt
Copy link
Contributor Author

tmyt commented Mar 5, 2017

@Tyriar my browser choose 'courier' from demo app default stylesheet.

Tyriar
Tyriar requested changes Mar 7, 2017
View reviewed changes
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.

Instead of this approach I think we should roll this into checkBoldBroken in Renderer.ts. This function checks whether an element's width is the same with and without bold, if not it will disable bold in the terminal. Fundamentally this is an issue with the font and it shouldn't be used.

I also think we should add a console.error when bold is broken to make it clear in the console why bold doesn't work.

@Tyriar Tyriar self-assigned this Mar 7, 2017
@tmyt
Copy link
Contributor Author

tmyt commented Mar 8, 2017

Thanks to your review, I agree with that.
Then I tried to detect broken bold with checkBoldBroken, but the function does not detect correctly.
Because, generally body does not have monospace font. and Chrome does not calculate scrollWidth or scrollHeight for display: inline element. The reason similarly to #157.

First, needs to fix checkBoldBroken to works correctly.

@coveralls
Copy link

Coverage Status

Coverage decreased (-7.4%) to 60.383% when pulling 4b2ae6a on tmyt:propose/row-height into 8693d06 on sourcelair:master.

Tyriar
Tyriar reviewed Mar 8, 2017
View reviewed changes
src/Renderer.ts
function checkBoldBroken(document) {
const body = document.getElementsByTagName('body')[0];
function checkBoldBroken(terminal) {
const document = terminal.ownerDocument;
Copy link
Member

Choose a reason for hiding this comment

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

Based on this https://github.com/sourcelair/xterm.js/blob/ec91a6c7f4b8032e3483a8730e6ed66a672415d0/src/xterm.js#L589

Shouldn't terminal.document be the same as terminal.ownerDocument since terminal.parent should be owned by the same document?

Copy link
Contributor Author

@tmyt tmyt Mar 9, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

This is bad naming terminal. The terminal passed with terminal.element. So I should change naming terminal to terminalElement.
terminal.element is created by terminal.parent.ownerDocument.createElement.
As a result terminal.document same as terminalElement.ownerDocument.

Copy link
Member

Choose a reason for hiding this comment

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

If terminal.document is the same as terminalElement.ownerDocument why not leave it as it was? (<any>this._terminal).document

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I needed (this._terminal).element for apply monospace font.
I think, document could get from element, then I want to simplify function arguments.

Copy link
Member

Choose a reason for hiding this comment

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

Ah makes sense, you add it to terminal instead of body which is much nicer.

src/Renderer.ts
body.appendChild(el);
const w1 = el.scrollWidth;
terminal.appendChild(el);
const w1 = el.offsetWidth;
Copy link
Member

Choose a reason for hiding this comment

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

I believe we want scrollWidth/scrollHeight here as offset* may not give the correct results if the element is off screen? https://developer.mozilla.org/en-US/docs/Web/API/CSS_Object_Model/Determining_the_dimensions_of_elements

Did doing this fix the issue for you?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I seems to fix the issue on my environment (Chrome 59 & Firefox 54).
This changes for scroll* does not work for display: inline element (e.g. span) on Chrome. Then I change scroll* to offset*.
Should I change offset* to scroll* and use div instead of span?

Copy link
Member

Choose a reason for hiding this comment

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

Main just concerned about if this would break when the terminal is either detached from the DOM or is invisible (display:none, visibility:hidden).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Both properties could not get the correct size from detached or invisible element. Anyway, currently or my PR both needs on visible DOM tree. Do you have you any great ideas?

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for checking if there's no regression this is all good 👍

@Tyriar Tyriar changed the title Set charMeasure.height values to each row Support checking height in checkBoldBroken Mar 10, 2017
Tyriar
Tyriar approved these changes Mar 10, 2017
View reviewed changes
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 for the contribution and bearing with me @tmyt 😄

While reviewing this I thought we really should merge this function into CharMeasure so I created a follow up item that someone can get to separate from this #592

src/Renderer.ts
function checkBoldBroken(document) {
const body = document.getElementsByTagName('body')[0];
function checkBoldBroken(terminal) {
const document = terminal.ownerDocument;
Copy link
Member

Choose a reason for hiding this comment

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

Ah makes sense, you add it to terminal instead of body which is much nicer.

src/Renderer.ts
body.appendChild(el);
const w1 = el.scrollWidth;
terminal.appendChild(el);
const w1 = el.offsetWidth;
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for checking if there's no regression this is all good 👍

@Tyriar Tyriar merged commit 91994a3 into xtermjs:master Mar 10, 2017
@Tyriar Tyriar added this to the 2.5.0 milestone Mar 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Tyriar Tyriar Tyriar approved these changes

Assignees

@Tyriar Tyriar

Labels
None yet
Projects
None yet
Milestone
2.5.0
Development

Successfully merging this pull request may close these issues.

Consider to set height to each rows
3 participants
@tmyt @coveralls @Tyriar