Fix(fabric.Text): Avoid reiterating measurements when width is 0 and measure also empty lines for consistency. #7497
Conversation
avra-m3
force-pushed
the
bugfix/line-size-0
branch
from
71b368e
to
dcc6e40
Compare
|
Looking into this further, fabric is actually dependent on the line === '' check failing as it doesn't create the charBounds value otherwise |
…e undefined in some edge cases
| lineInfo = this.measureLine(lineIndex); | ||
| width = lineInfo.width; | ||
| } | ||
| var lineInfo = this.measureLine(lineIndex); |
There was a problem hiding this comment.
does measureLine have the same check inside?
There was a problem hiding this comment.
Not as far as I have seen.
There was a problem hiding this comment.
When I stepped through line by line, I got as far as this._measureChar with an empty string.
There was a problem hiding this comment.
So why we want to skip the shortcut and go far down with the empty line?
There was a problem hiding this comment.
I m happy about the fix, i want to understand why we also remove the shortcut. Is not of immediate understanding to me.
There was a problem hiding this comment.
Originally I fixed the shortcut, it caused an error when a line consisted solely of a blank space because _charBounds[x] is undefined.
My understanding is that this._measureLine sets _charBounds for each line so we shouldn't skip it otherwise other parts of the code will fail :(.
We could restore the check but currently it's the equivalent of if(false) because [''] will never equal '',
Fixing it to act as a shortcut is probably possible, I'd need to investigate further and may have consequences when it comes to how different platforms measure blank newlines?.
There was a problem hiding this comment.
no is ok as it is.
The fact that skipping measurements was creating error elsewhere is good to me.
|
|
that failure is just the coverage, i have no idea how to make it work on forks. |
asturur
changed the title
…measure also empty lines for consistency. (fabricjs#7497)
Very small PR, I noticed we were hitting measure text more than expected and found this to be the cause.
Happy for this to be declined If I've misunderstood these checks :)