Repository navigation
Don't draw the measured text in heightOfString after a continued text call - #1832
Open
mkantautas wants to merge 1 commit into
Open
mkantautas wants to merge 1 commit into
mkantautas wants to merge 1 commit into
Conversation
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What kind of change does this PR introduce?
Bug fix. Fixes #1284.
doc.text('abc', 0, 0, { continued: true })followed bydoc.heightOfString('X')drawsXinto the page.heightOfStringmeasures through_text, which reuses the line wrapper a continuedtext()call leaves inthis._wrapper. That wrapper'slinelistener isthis._line, which draws, and the measuring callback thatheightOfStringpasses is never attached.heightOfStringnow clearsthis._wrapperbefore a wrapping measurement, unless the measurement itself passescontinued: true._textthen creates a new wrapper with the measuring callback. The returned height is measured from the start of a line, as when no text is pending, instead of from the pending text's offset. The default measurement still ends a pending run as before. Non-wrapping measurements preserve the pending wrapper, and measurements without a pending wrapper are unchanged.Limitation: when
continued: trueis passed toheightOfStringitself, the previous behavior is kept, including drawing the measured text. Clearing the wrapper in that case too would leave the measurement's own wrapper behind, and the nexttext()call would then not be drawn at all.Tests, in the new
tests/unit/heightOfString-1284.spec.js:Xis drawn), passes with itcontinued:Xis not drawnheightOfString('X', { continued: true })after a continuedtext(), thentext('def'):defis drawn (this test fails if the wrapper is cleared unconditionally)text()call: the nexttext()keeps its horizontal position (this test fails if the wrapper is cleared for a non-wrapping measurement)yarn lint,yarn prettier,yarn buildand all 583 tests pass on Node 20, 22 and 24. These local runs useyarn test --maxWorkers=2in containers limited to two CPUs, with the normal test timeout unchanged. Earlier Node 20 runs at default concurrency in the same kind of container timed out in 16 visual tests on the unchanged base and in 17 with an earlier revision of this change, while Node reported 32 available CPUs.Checklist: