Skip to content

fix: stop applying PX_TO_PT to font-size twice (#502) - #503

Merged
deeplook merged 1 commit into
deeplook:mainfrom
Zunhammer:fix/font-size-double-px-to-pt
Aug 10, 2026
Merged

fix: stop applying PX_TO_PT to font-size twice (#502)#503
deeplook merged 1 commit into
deeplook:mainfrom
Zunhammer:fix/font-size-double-px-to-pt

Conversation

@Zunhammer

Copy link
Copy Markdown
Contributor

The viewport group already carries the px-to-pt conversion, but font-size was converted again below it: applyStyleOnShape mapped it through convertLengthToPt, and convertText measured advance widths in points before adding them to x coordinates in user units. Text therefore rendered at 0.75x the size of the geometry around it, and multi-tspan runs were laid out in the wrong unit. Both now stay in user units, leaving the single conversion to the viewport transform.

Three tests in tests/test_units.py asserted the stored String.fontSize in points and so encoded the bug; they now assert the rendered size instead. The relational tests there and in test_basic.py are untouched and still pass. Adds regression coverage for the text-to-geometry ratio across seven root variants, for multi-tspan positioning and for per-character positioning; all six fail without the source change.

The viewport group already carries the px-to-pt conversion, but font-size was converted again below it: applyStyleOnShape mapped it through convertLengthToPt, and convertText measured advance widths in points before adding them to x coordinates in user units. Text therefore rendered at 0.75x the size of the geometry around it, and multi-tspan runs were laid out in the wrong unit. Both now stay in user units, leaving the single conversion to the viewport transform.

Three tests in tests/test_units.py asserted the stored String.fontSize in points and so encoded the bug; they now assert the rendered size instead. The relational tests there and in test_basic.py are untouched and still pass. Adds regression coverage for the text-to-geometry ratio across seven root variants, for multi-tspan positioning and for per-character positioning; all six fail without the source change.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Thank you for making your first pull request! Your contribution to svglib is highly appreciated!

@deeplook

Copy link
Copy Markdown
Owner

Thanks for the detailed writeup and repro — this is a genuine regression from the 2.0b1 unit rework, not expected behaviour like #471.

Verified independently: your repro script now emits Tf 16 inside the .75 viewport matrix (matching 1.5.1 output, vs. the buggy Tf 12), full test suite passes locally (188 passed, 5 skipped), and all 16 CI checks are green.

Fix is well-scoped, the rewritten tests correctly assert rendered size instead of raw String.fontSize, and the changelog entry is clear about the unit-semantics change. Merging — thanks for catching this.

@deeplook
deeplook merged commit 71b9e32 into deeplook:main Aug 10, 2026
16 checks passed
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