Skip to content

Include wordSpacing in boundsOfString with lineBreak: false - #1807

Open
youdie006 wants to merge 3 commits into
foliojs:masterfrom
youdie006:bounds-linebreak-false-wordspacing
Open

youdie006 wants to merge 3 commits into
foliojs:masterfrom
youdie006:bounds-linebreak-false-wordspacing

Conversation

@youdie006

Copy link
Copy Markdown
Contributor

What kind of change does this PR introduce?

Bug fix, follow-up to #1806.

With lineBreak: false, boundsOfString() measures each line with widthOfString(line, options) (lib/mixins/text.js:185-190), which leaves out wordSpacing. _fragment adds it to each word and to the underline width, so the bounds are narrower than what is drawn:

doc.fontSize(12);
doc.boundsOfString('Hello big world', 50, 50, { lineBreak: false, wordSpacing: 5 }).width;
// master: 78.74, rendered text and underline: 88.74

lineBreak: false is the only way to reach this branch, since _initOptions sets a width otherwise. This adds wordSpacing for each gap between words, with the word count computed as in _fragment, like the wrapped branch after #1806. Output without spacing options is unchanged (I compared boundsOfString over 256 combinations of text, alignment, lineBreak, width, characterSpacing, horizontalScaling and rotation before and after).

The new test follows the #1806 tests in tests/unit/text.spec.js and fails on master. prettier --check, lint and npm test (527 tests) pass on Node 22.

Checklist:

  • Unit Tests
  • Documentation N/A
  • Update CHANGELOG.md
  • Ready to be merged

Written with AI assistance (Claude); I have reviewed the change.

The single-line branch of boundsOfString used widthOfString, which does
not include word spacing, while _fragment adds it to each word and to
the underline width. Add wordSpacing for each word gap, as the wrapped
branch does since foliojs#1806.
Comment thread lib/mixins/text.js Outdated
Comment on lines +187 to +190
const wordCount = line.trim() ? line.trim().split(/\s+/).length : 0;
const lineWidth =
this.widthOfString(line, options) +
(options.wordSpacing ?? 0) * Math.max(0, wordCount - 1);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Perf can be improved: double trim call, temporary array creation.

Address review: avoid the second trim call and the split array.
Whitespace runs are collapsed when wordSpacing is set, so each gap is
one character and a global regex test loop counts them.
@youdie006

Copy link
Copy Markdown
Contributor Author

Changed: it trims once, and the gaps are counted with a /\s/g test loop instead of match(), so no array is built. When wordSpacing is set, whitespace runs are already collapsed to one character, so each match is one gap.

This branch has not been deployed

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants