Skip to content

fix: support letter and word spacing in text - #252

Open
SulimanAbdulrazzaq wants to merge 6 commits into
antvis:mainfrom
SulimanAbdulrazzaq:agent/fix-text-spacing
Open

SulimanAbdulrazzaq wants to merge 6 commits into
antvis:mainfrom
SulimanAbdulrazzaq:agent/fix-text-spacing

Conversation

@SulimanAbdulrazzaq

@SulimanAbdulrazzaq SulimanAbdulrazzaq commented Aug 10, 2026

Copy link
Copy Markdown

Summary

Fixes #184.

  • Include letterSpacing and wordSpacing in browser and SSR text measurements. Spacing is converted to numbers once (resolveSpacing, px and em) and shared by the canvas, span, and SSR paths.
  • The SSR fallback passes those numbers to measury, which applies them itself, and adds nothing afterwards. Only the canvas path adds spacing, because canvas measureText ignores it.
  • Preserve word-spacing through the text renderer and apply both spacing values as CSS lengths. null spacing values are skipped, as on main.
  • Add regression coverage: an unmocked SSR test against the real measury, spacing normalization, measured text boxes, and null spacing.

This keeps the change focused on the core Text pipeline; business-component-specific style props and optional measurement caching are intentionally outside this PR.

Validation

Run at ae10e84 in a Linux Codespace with Node 20.20.2 after npm install:

  • npx tsc --noEmit -p tsconfig.json: exit 0
  • npx vitest --run __tests__/unit/utils/measure-text.ssr.test.ts __tests__/unit/utils/measure-text.test.ts __tests__/unit/utils/text.test.ts: 30 passed
  • npm test: 91 test files, 771 tests passed
  • npm run build: exit 0
  • npm run lint:root: exit 0
  • Re-adding the removed SSR spacing addition makes both SSR tests fail; restoring the file makes them pass.
  • The null-spacing test fails on 680885e with TypeError: Cannot read properties of null (reading 'trim') and passes at ae10e84.

Not run: site install, build:site, and lint:site; this PR does not touch site/.

@Aarebecca

Copy link
Copy Markdown
Contributor

@SulimanAbdulrazzaq Thanks for working on this. The direction looks good, but the CI failure is a real type error. Also, measury@0.1.5 already applies numeric letterSpacing and wordSpacing, so passing them to measure() and then adding spacingWidth again would double-count spacing in SSR. The current mocked test hides this. Could you normalize the supported CSS lengths once, avoid the extra SSR addition, and add an unmocked regression test?

@codecov-commenter

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 82.25806% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 51.56%. Comparing base (69eae2e) to head (3ef8e01).
⚠️ Report is 5 commits behind head on main.

Files with missing lines Patch % Lines
src/utils/text.ts 52.17% 11 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #252      +/-   ##
==========================================
+ Coverage   50.90%   51.56%   +0.66%     
==========================================
  Files         343      343              
  Lines       28710    28769      +59     
  Branches     2665     2711      +46     
==========================================
+ Hits        14614    14836     +222     
+ Misses      14085    13922     -163     
  Partials       11       11              
Flag Coverage Δ
infographic 51.56% <82.25%> (+0.66%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
src/renderer/composites/text.ts 87.91% <100.00%> (+0.13%) ⬆️
src/types/attrs.ts 100.00% <ø> (ø)
src/utils/measure-text.ts 89.50% <100.00%> (+2.60%) ⬆️
src/utils/text.ts 85.77% <52.17%> (-3.33%) ⬇️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

SulimanAbdulrazzaq and others added 2 commits September 16, 2026 12:53
Codecov flagged 11 uncovered lines on this patch, all in src/utils/text.ts:
the measurement branch of updateTextElement, the CSS-length branch of
formatSpacing, and the spacing read-back in getTextElementProps.

Adds cases for each: bare numbers and numeric strings normalized to px,
explicit units left untouched, an explicit zero preserved, absent spacing
omitted, the measured box widening with letter spacing, explicitly sized
elements left unmeasured, and the spacing round-trip through
getTextElementProps. Patch coverage for src/utils/text.ts is now complete.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Bvrwu6iCpKP3C2rh8tBsdJ
getTextStyle now checks letter and word spacing with `!= null` instead of
`!== undefined`. UpdateElementCommand records `null` for attributes the
element did not have, so undoing a letter-spacing edit on unspaced text
passed `null` to formatSpacing, which threw. Main skipped such values.
@SulimanAbdulrazzaq

Copy link
Copy Markdown
Author

@Aarebecca thanks, all of your points are addressed, plus one fix I found while re-checking:

  1. Type error. measureText converts letterSpacing and wordSpacing to numbers once (resolveSpacing, px and em; src/utils/measure-text.ts:92-107 and :189-197), so measury receives numbers. CI's lint-and-build passed on 3ef8e01.
  2. SSR double counting. The SSR fallback passes those numbers to measury and adds nothing afterwards (:198-202). Only the canvas branch adds spacing, because canvas measureText ignores it (:144-149); the span branch sets CSS spacing (:160-161).
  3. Unmocked test. The mocked SSR assertion is gone. __tests__/unit/utils/measure-text.ssr.test.ts compares against the real measury, including em values. Re-adding the old SSR addition makes both of its tests fail.
  4. New in ae10e84. getTextStyle skipped only undefined, so a null spacing value threw in formatSpacing. UpdateElementCommand stores null when undoing a spacing edit on text that had none. It now skips null as main did, with a regression test.

Validation at ae10e84 (Node 20, npm install): tsc --noEmit clean; the three spacing test files 30 passed; npm test 771 passed; npm run build and npm run lint:root succeed.

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.

文本间距属性 (letter/wordSpacing) 在测量层缺失且在业务组件层未透传,导致排版功能不可用

3 participants