fix(svg): keep the stripped width/height when detecting a percentage - #1936
Merged
Merged
Conversation
extract_shape_info() calls width_str.strip() and height_str.strip() as bare
statements. Strings are immutable, so the results are discarded and the
whitespace survives. A trailing space then defeats the percentage check:
<svg width="100% "> -> ValueError: 100% uses unsupported relative length %
endswith("%") is False, so the value falls through to resolve_length(), which
parses the unit as % and rejects it as a relative length.
Absolute lengths are unaffected -- unit_splitter tolerates surrounding
whitespace -- so only percentages are affected. viewbox, thirteen lines below
in the same method, already assigns its strip() result.
andersonhc
approved these changes
Sep 2, 2026
andersonhc
left a comment
Collaborator
There was a problem hiding this comment.
Ready to merge. Thank you @Anai-Guo
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.
Problem
SVGObject.extract_shape_info()calls.strip()on thewidth/heightattributes as bare statements:Python strings are immutable, so
.strip()returns a new string and the original is unchanged — the whitespace is still there on the next line.A trailing space then defeats the percentage check:
"100% ".endswith("%")isFalse, so the value falls through toresolve_length(), which parses the unit as%and rejects it as a relative length:Thirteen lines below, in the same method,
viewboxalready does it correctly:Scope
Only percentages are affected.
unit_splitter(\s*(?P<value>[-+]?[\d\.]+)\s*(?P<unit>%|[a-zA-Z]*)) tolerates leading whitespace and.match()ignores anything trailing, so absolute lengths already survive the missing strip. Measured on the current code:width"100%"Percent(100.0)Percent(100.0)"100% "ValueErrorPercent(100.0)" 100% "ValueErrorPercent(100.0)"100"/"100 "/"100px "Leading-only whitespace (
" 100%") happens to work already, becauseendswith("%")still holds andfloat(" 100")accepts the space — so 3 of the 4 whitespace cases in the new test fail without the fix.Fix
Keep the stripped value, which is what the code was already trying to do:
Test
Added
test_document_shape_info_percent_surrounded_by_whitespacetoTestSVGObject, parametrized over"100% "," 100%"," 100% ","100%\n"(a newline is easy to hit when the attribute is written across lines in hand-authored SVG).Verified both directions rather than just the happy one:
black(26.3.1, the revision pinned in.pre-commit-config.yaml) reports both files unchanged.Note on the base commit and the CHANGELOG
My fork cannot be synced to
master— the GitHub API refusesmerge-upstreamfor this token because the sync would touch.github/workflows/codeql.yml, which needs theworkflowOAuth scope — so this branch is based on my fork's own tip. I checked the drift explicitly:fpdf/svg.pyandtest/svg/test_svg.pyare byte-identical between that commit and currentmaster, so the diff is exactly what it looks like.CHANGELOG.mdis the one file that has moved since (the[2.8.9]section gained its### Fixedheading and two entries). Writing my entry against the older copy either conflicts or auto-merges into a duplicated### Fixedheading, so I have deliberately leftCHANGELOG.mdout rather than commit something that merges wrong. The entry I would add is:Happy to add it (or rebase the whole branch) on request.
🤖 Generated with Claude Code