fix(): fix rendering of text when line height is set to 0 #10785
Conversation
Review or Edit in CodeSandboxOpen the branch in Web Editor • VS Code • Insiders |
|
Build Stats
|
1fa5453 to
13965e4
Compare
| height = 0; | ||
| for (let i = 0, len = this._textLines.length; i < len; i++) { | ||
| lineHeight = this.getHeightOfLine(i); | ||
| height += i === len - 1 ? lineHeight / this.lineHeight : lineHeight; |
There was a problem hiding this comment.
these calculations with 0 where giving NaN so I had to do fallbacks to fontSize
it passes still all unit tests and all visual tests
and you can see the new visual test output I added
There was a problem hiding this comment.
It seems there's a small issue — the visual appearance of a line-height set to 0.01 should be almost the same as when it's set to 0, but judging from the image, the latter appears to be shifted a few pixels upward overall.
There was a problem hiding this comment.
yeah that is what I was not sure about, the first rendering of 0.1 is the current behavior
the rendering of 0 is the new behavior (previously would not even be shown) because I am now falling back to the font size if line height is 0, to avoid having NaN.
I agree with you, they should be almost the same
I have another idea, will give it a try
There was a problem hiding this comment.
@zhe-he could you recheck the visual screenshot again?
There was a problem hiding this comment.
There’s still an issue — it’s probably caused by something that hasn’t been taken into account somewhere.
There was a problem hiding this comment.
mmmm So, this solution doesn't take in consideration style for fontsize. everything is so complicated with text.
does it make sense to go into every tiny detail
I mean this is already better than it was
maybe we can improve it incrementaly as we add more and more tests
There was a problem hiding this comment.
The issue is that for lineHeight = 0 we are assuming that fontSize is the size of the line, while the size of the line is the biggest fontSize in the line
There was a problem hiding this comment.
does it make sense to go into every tiny detail
I mean this is already better than it was
maybe we can improve it incrementaly as we add more and more tests
No it doesn't make sense to exagerate.
LineHeight 0 would work without style, but the change in calcTextHeight would have caused a bug for the normal text that had style in the last line and that could be fairly common.
In general if reading it you get an idea and you think you can make it better quickly, you do it, otherwise no.
For example the clipPath i have no idea what to do in the other PR seems a mistery to unravel, and the partial fix is fine, i rather spend time in more useful issues than perfecting that.
There was a problem hiding this comment.
At the end we fixed the issue, we added a test, and we removed 15 bytes of code instead of adding 200.
So was win / win.
04ccb4b to
e3e5e9b
Compare
e3e5e9b to
b8c6eec
Compare
closes #10775