Skip to content

Conversation

@lynette-li
Copy link
Contributor

When the <pre> element is hidden by default, lineNumberSizer.getBoundingClientRect().height will be 0.
When the user sets hidden to false,line numbers will be displayed as follows.This is not the expected display.

image

lineNumberSizer.textContent = line || '\n';
var lineSize = lineNumberSizer.getBoundingClientRect().height;
lineNumbersWrapper.children[lineNumber].style.height = lineSize + 'px';
var lineSize = getComputedStyle(lineNumberSizer).lineHeight;
Copy link
Member

Choose a reason for hiding this comment

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

Can we assume that getComputedStyle(lineNumberSizer).lineHeight is equivalent to lineNumberSizer.getBoundingClientRect().height + 'px'? Especially cross browser?

(Line Numbers is one of our most-used plugins, so I don't want to break websites just because this change seems sound.)

Copy link
Contributor Author

@lynette-li lynette-li May 7, 2020

Choose a reason for hiding this comment

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

How about we set lineSize to be lineNumberSizer.getBoundingClientRect().height + 'px' by default?Only when lineNumberSizer.getBoundingClientRect().height equals 0 will we set it to getComputedStyle(lineNumberSizer).lineHeight.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@RunDevelopment Request for your comments. :)

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 the reminder)

I tested your solution (without fallback) and it doesn't work with soft wrap.

Details

Screenshot from 2020-05-11 15-04-12

Should be:

Screenshot from 2020-05-11 15-13-23

I don't think there's a lot we can do to fix this. The browser needs to break and wrap the text for us.

@mAAdhaTTah
Copy link
Member

Yeah, I don't think we should fix this internally. If the pre is display: none initially, my suggestion would be to rehighlight it when you reveal it. I'd be really hesitant to make this change cuz I'd definitely be concerned about breaking consumers.

@RunDevelopment I'd be ok with closing (we still appreciate the contribution!), but I'd like to hear your opinion given you've played with this.

@RunDevelopment
Copy link
Member

@mAAdhaTTah I agree. Rehighlighting will trivially solve this issue (though it's a bit of a waste performance-wise).

I think we should rather focus on #2413. After this PR, we can let the browser do the positioning of line numbers (and line highlights), so we don't have to worry about this (and a few other issues) at all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants