fix: trim surrounding whitespace in <title> same as <desc> - #2287
Open
si-kui-a wants to merge 1 commit into
Open
fix: trim surrounding whitespace in <title> same as <desc>#2287si-kui-a wants to merge 1 commit into
si-kui-a wants to merge 1 commit into
Conversation
textElems in plugins/_collections.js explicitly included 'title' alongside 'pre' and the genuinely-rendered text-content elements (text, tspan, etc.), which are the elements where surrounding whitespace is semantically meaningful and must be preserved verbatim. <title> is a non-rendered accessibility/metadata element -- like its sibling <desc> and <metadata> in elemsGroups.descriptive -- where whitespace has no such significance, but it was the only member of that group also present in textElems. lib/parser.js's sax.ontext handler branches on textElems membership: members keep raw (untrimmed) text, non-members get text.trim(). Since 'title' was in textElems, <title> My Title </title> kept its leading/trailing whitespace while the structurally identical <desc> My Desc </desc> got trimmed to "My Desc" -- the exact asymmetry reported in svg#2052. lib/stringifier.js also branches on the same textElems set to decide whether an element's children get pretty-mode indentation/newlines, so this fix also makes <title> format consistently with <desc> when js2svg pretty output is used (see the updated inlineStyles.16 fixture below). Fix: remove 'title' from the textElems Set literal (one line). No other file references textElems besides parser.js/stringifier.js and the definition itself (confirmed via repo-wide code search), so the change is fully self-contained. Added a regression test in lib/parser.test.js reproducing the exact before/after asymmetry from svg#2052 (title now trims identically to desc). Updated test/plugins/inlineStyles.16.svg.txt's expected output: with pretty formatting, <title>button</title> now gets the same indented multi-line layout <desc> already receives, which is the same underlying textElems membership change, not a separate behavior. Fixes svg#2052. Verified for real, not just hand-traced: installed deps via `pnpm install --frozen-lockfile` and ran `pnpm vitest run` (521 passed, 3 skipped -- confirmed the new test fails without the fix and passes with it by temporarily reverting and re-running), `pnpm lint` (eslint + prettier, clean). This is possible because Node/pnpm are available in the environment this PR was prepared in, unlike the PHP-based WordPress plugin fixes from the same session, which had to be hand-traced and disclosed as such. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HG5wiw3Z8QdjNWb5bFDLiA
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.
Fixes #2052.
Problem
textElemsinplugins/_collections.jsexplicitly included'title'alongside'pre'and the genuinely-rendered text-content elements (text,tspan, etc.) — the elements where surrounding whitespace is semantically meaningful and must be preserved verbatim.<title>is a non-rendered accessibility/metadata element — like its sibling<desc>and<metadata>inelemsGroups.descriptive— where whitespace has no such significance, but it was the only member of that group also present intextElems.lib/parser.js'ssax.ontexthandler branches ontextElemsmembership: members keep raw (untrimmed) text, non-members gettext.trim(). Sincetitlewas intextElems,<title> My Title </title>kept its leading/trailing whitespace while the structurally identical<desc> My Desc </desc>got trimmed to"My Desc"— the exact asymmetry reported in #2052.Fix
Remove
'title'from thetextElemsSet literal (one line). Confirmed via repo-wide code search that onlylib/parser.jsandlib/stringifier.jsreferencetextElemsbesides the definition itself, so the change is fully self-contained.lib/stringifier.jsalso branches on the sametextElemsset to decide whether an element's children get pretty-mode indentation/newlines, so this also makes<title>format consistently with<desc>whenjs2svgpretty output is used — same underlying membership change, not a separate behavior. This surfaced as one existing fixture test failure (inlineStyles.16), whose expected output I've updated to match:<title>button</title>now gets the same indented multi-line layout<desc>already receives in pretty mode.Testing
Added a regression test in
lib/parser.test.jsreproducing the exact before/after asymmetry from #2052 (title now trims identically to desc).Ran the actual suite locally (not just hand-traced):
pnpm install --frozen-lockfilepnpm vitest run— 521 passed, 3 skipped. Confirmed the new test genuinely catches the bug by temporarily reverting the fix and re-running (it fails as expected), then restored the fix and re-ran (passes).pnpm lint(eslint + prettier) — clean.pnpm typecheck(tsc) — clean.Did not run
test:bundles/test:regression(the fullqascript's remaining steps) — those build the rollup bundle and pixel-diff a large external SVG corpus, which felt disproportionate to verify for a one-lineSetmembership change already covered by the unit/fixture suite above; happy to run them too if maintainers want that before merge.🤖 Generated with Claude Code
https://claude.ai/code/session_01HG5wiw3Z8QdjNWb5bFDLiA