Conversation
dfrg
approved these changes
Sep 28, 2026
| } | ||
|
|
||
| impl GlyphMetrics<'_> { | ||
| pub(crate) fn has_glyf_contours(&self, glyph_id: GlyphId) -> Option<bool> { |
Contributor
There was a problem hiding this comment.
I think this should probably be fn glyf_y_min(...) -> Option<i16> gated by number of contours being non-zero? That gives us the answer we actually want and avoids a second load of the glyph.
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.
Ok so, I've been going into a bit of a rabbit hole prompted by some issues we encountered with the placement of sbix glyphs in Vello. As part of this, i had AI generate a test case that is checks different combination of metrics in a sbix glyph to see how it behaves.
Consider the following two SVGs, which contain the fonts embedded (once with CFF and once with glyf outlines)
If I open these SVGs in Safari, all letters are centered in each cell correctly:

If I open these SVGs in Chrome on Windows, the glyf one looks like this:


the CFF one like this:
(on Chrome on MacOS, the CFF one is actually completetly broken, interestingly enough).
Anyway, current Vello fails the glyf test case in the same way. For the CFF one it's even more broken as there also is a vertical shift:

I believe the differences stem from two issues in skrifa, both of which this PR fixes.
Vertical placement of CFF glyphs
The spec says
I guess the spec must literally mean only for
glyftables, i.e. for CFF tables we should not applyyMin, even if there is a CFF outline. Because if I do this (applied in the first commit), Vello now exactly matches Chrome rendering, and compared to CoreText there are only horizontal shifts now. I guess Chrome must be getting those metrics differently somehow, hence why it's not affected by this specific bug.LSB placement
As quoted above, the spec also says
The lsb value for the current glyph ID from the 'hmtx' table has no effect.. So for CFF fonts or for TTF fonts where the glyph has no outline, the lsb value should not be used at all. If I apply this patch (in the second commit), Vello now renders all glyphs (except for the two that don't work because skrifa doesn't supportdupeglyphs yet) correctly!I hope I didn't miss anything and that these changes are correct.