Repository navigation
Conversation
1f6c1f4 to
1c66211
Compare
Co-authored-by: Zhai Can <bczhc0@126.com>
This is not rendered correctly before. Just ignore and make the test happy for now.
1c66211 to
3e7475e
Compare
|
The CI outputs: On my machine it passes. Falling back to Twitter Color Emoji: Indeterministic font fallback causes the test failure. Honestly I have no idea why is that. Solution thoughts? |
|
Why does this PR change Thank you. |
|
True, #583 does track that RTL is mishandled. The golde test image changing makes sense to me. The PR changes which font supplies which run, so |
|
@nicoburns and @DJMcNab , I think this PR might be hitting a CI/harness issue. It sometimes passes and sometimes not because of the iteration order of I am going to open a PR to replace |
|
@luisbg please can you stop tagging me in these. Tagging me does not give me any more time to actually review these PRs. Actual review comments that explain what review steps you have performed (reviewing the implementation, comparing it with various specs, testing the code against SVGs, etc), and/or reasons for why you think a given PR is good/bad are helpful. |
|
Harness fix at #1136 What I checked to review this PR:
Overall I think this is worth landing: it fixes #301 (open since 2020) and #916 , from what I've read the approach matches how CSS specifies font-family resolution, and the conflict is confined to |
|
Hi @bczhc, thanks for this PR. We hit this bug while rendering charts with mixed Latin/CJK labels, and I tested your branch against current ReproductionTo make the result independent of directory order, I skipped system fonts and loaded the fonts in a fixed order: resvg --skip-system-fonts \
--use-font-file NotoSans-Regular.ttf \
--use-font-file NotoSerifCJK-Regular.ttc \
--use-font-file NotoSansCJK-Regular.ttc \
--sans-serif-family "Noto Sans" \
in.svg out.png
On Review notes
OfferWould you like to keep driving this? If you're short on time, I'm happy to rebase it onto Disclosure: I used an AI assistant to help investigate and build the comparison above. I checked the results by hand. |
|
This PR is only trying to make font get resolved at a character level (instead of old behavior - applying to the whole text) from a deterministic given font-family priority list. I guess it goes to have other issues with things like generic family etc; and this pr may only serve as a workaround now because they claimed there's a wholly new font resolving engine under development anyway. |
|
Thanks for getting back to me, @bczhc, and for the context! That framing makes sense to me. This PR doesn't try to be a full font cascade, and I don't think it needs to. It fixes the part users run into most: falling back through the On generic families: in my tests, If you're OK with it, I'll open a follow-up PR based on your branch that keeps your commits and authorship. On top of them, I'd:
I'll link it here once it's up. Of course, if you'd rather keep going with this one yourself, just let me know and I'll send the changes your way instead. Thanks again for starting this! |


The old font resolving approach is to scan the whole tspan text and try to find a font to match all the text. According to CSS
font-familyfont resolving schema, it should resolve and fallback using the family name one by one, making font uses more ergonomic.This implements a simple resolving algorithm doing what it states above.
Fix #301, #916.
Tests
Before:

After:
