Skip to content

fix: accept unquoted non-ASCII font-family values - #384

Closed
amandeavor wants to merge 3 commits into
yWorks:masterfrom
amandeavor:fix/font-family-unquoted-utf8
Closed

amandeavor wants to merge 3 commits into
yWorks:masterfrom
amandeavor:fix/font-family-unquoted-utf8

Conversation

@amandeavor

Copy link
Copy Markdown
Contributor

Summary

  • font-family-papandreou throws on unquoted non-ASCII family names such as font-family="微软雅黑".
  • Catch the parse error and fall back to treating the attribute value as a single family name, matching browser behavior.
  • Adds a focused vitest covering unquoted CJK names and a mixed fallback list.

Fixes #330.

Test plan

font-family-papandreou throws on unquoted names with non-ASCII characters
(e.g. font-family="微软雅黑"). Browsers still honor those attribute values,
so fall back to treating the whole string as a single family name (yWorks#330).
Copilot AI lite review requested due to automatic review settings September 25, 2026 07:41

Copilot AI 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.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@yGuy

yGuy commented Sep 28, 2026

Copy link
Copy Markdown
Member

The test should verify that the PDF actually shows the correct glyphs from that font, i.e. that when adding that specific font to jspdf, the resulting pdf will actually use the embedded matching font. It's not enough to show that a library does not throw an exception. You need to check the output and verify that it's actually working....

@amandeavor

Copy link
Copy Markdown
Contributor Author

@yGuy Added! The test now defines a fallback list containing an unquoted non-ASCII family name alongside the registered custom font (font-family="微软雅黑, Batang"), and asserts that the resulting PDF matches the embedded glyph snapshot in test/custom-fonts/custom-fonts.pdf.

Also enhanced the fallback parsing in src/applyparseattributes.ts to split unquoted comma-separated fallback lists into individual family names, ensuring subsequent registered fonts in the list are matched and rendered properly.

@yGuy

yGuy commented Sep 28, 2026

Copy link
Copy Markdown
Member

So just to get this right, your code now calls the font parsing library and if the library fails to parse properly, it does the parsing itself in a very rudimentary form?

I guess you should rather fix the font parsing library, then.
The better fix is not to implement things multiple times but to fix the root problem, IMHO.

Also please disclose it, if you this is actually just all AI slop (which seems likely, looking at your github history).
What model are you and is there actually a human reviewing your contributions or are they just claiming authorship?

@amandeavor

Copy link
Copy Markdown
Contributor Author

Thanks for the candid feedback, @yGuy. You make a completely fair point on the architecture — resolving this in the font parsing library directly is the cleaner root fix and avoids maintaining duplicate fallback parsing here.

To answer your question directly: I do use AI-assisted tooling in my prototyping workflow, alongside local test reproduction and manual review before opening PRs. That said, I hear your concern loud and clear about avoiding piecemeal workarounds when an upstream parser fix is the right path.

I'll close this PR so we don't carry an ad-hoc fallback here, and look into addressing the font family handling in the parser library instead. Thanks for the review and perspective.

@amandeavor amandeavor closed this Oct 10, 2026
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.

font-family attribute parse error.

3 participants