Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Raise error for overloaded codepoints, and drop codepoints in colour-layer glyphs #739

Merged
merged 5 commits into from
Jun 5, 2023

Conversation

Hoolean
Copy link
Collaborator

@Hoolean Hoolean commented Apr 19, 2023

Hello!

util.makeUnicodeToGlyphNameMapping() intends to raise an error when multiple glyphs claim the same code point, but unfortunately a typo leads this error to be swallowed. Instead, with the current behaviour, we silently give the first glyph that claims an overloaded code point precedence.

This PR adds a raise to fix the typo that caused the error to be swallowed, and adds a test for the intended behaviour.

Next Steps

  1. Fix failing COLR test:
    • This test UFO specifies a code point for the 'a' glyph in its colour layers; when these are exploded into the default layer, they clash with the default layer 'a', which specifies the same code point.
    • Should colour glyphs be allowed to specify code points? If not, should we warn and strip such code points or raise an InvalidFontData error?
  2. Check whether this issue was reachable through the top-level compilation functions:
    • The COLR test failing suggests that this issue may have been reachable through our top-level compileTTF() functions, as opposed to solely through calling the util function directly.
    • If we do not have one already, we could add a general test for compileTTF() to check that ufo2ft raises an appropriate error when supplied with a UFO that overloads code points.
    • To be safe, we could try compiling a small corpus of test fonts to see if any fonts are accidentally relying on this implicit behaviour (cc @madig).

Sorry, something went wrong.

Hoolean added 2 commits April 19, 2023 17:19
This is the documented behaviour of util.makeUnicodeToGlyphNameMapping()
but a typo means that the exception is not raised. This commit adds a
failing test to check for the documented behaviour, to confirm a future
fix and avoid regressions.

This commit also creates a file to contain tests for ufo2ft's util
functions to give this test a happy home.
@anthrotype
Copy link
Member

good catch, thanks!

Should colour glyphs be allowed to specify code points?

no they should not. the filter that creates them should probably unset any codepointds they may have. No need to warn IMO.

add a general test for compileTTF() to check that ufo2ft raises an appropriate error when supplied with a UFO that overloads code points

you can add one in integration_test.py, where compileTTF is tested I believe; I suggest you make the test UFO from code using ufoLib2/defcon APIs right before you compile and assert; better than checking in a lot of boilerplate test files.

we could try compiling a small corpus of test fonts to see if any fonts are accidentally relying on this implicit behaviour]

I don't think these double encoded glyphs happen frequently; and it is a font source bug that is worth raising an error for, as compiler can't really encode the ambiguous mappings. Not too worried about sudden breakages.

Hoolean added 2 commits May 16, 2023 19:01
Glyphs in color layers should not be encoded in the final font, even if
they have 'unicode' properties. Unfortunately, this currently happens as
a side-effect of copying color glyphs into default layer alternates, and
so this commit introduces explicit tests to track a fix.

In addition, this commit removes the unicode properties from color
layers in the test font 'ColorTest'. These properties revealed the bug
implicitly as their values were overloaded with those in the default
layer, but are unwanted now we have explicit tests.

See: googlefonts#739 (comment)
Glyphs that are copied into the default layer from color layers should
not become encoded; this commit introduces a fix to prevent this, fixing
the tests added to track this issue in the previous commit.

See: googlefonts#739 (comment)
@Hoolean
Copy link
Collaborator Author

Hoolean commented May 19, 2023

The commits I have just pushed modify the filter to unset codepoints when exploding colour glyphs, and add some dedicated tests for this behaviour. When I have added integration tests for the handling of overloaded codepoints, I will mark the PR as Ready for Review :) Thanks for the feedback Cosimo!

In addition, this commit makes the unit test for the util function
slightly more specific and corrects spelling for consistency.
@Hoolean Hoolean marked this pull request as ready for review June 5, 2023 17:39
@Hoolean
Copy link
Collaborator Author

Hoolean commented Jun 5, 2023

I have now completed the PR with an integration test; as discussed, the UFO is constructed in-memory to avoid unnecessary boilerplate 🚀

Running the integration test on main shows that previously no error was raised at the top-level when compiling sources with duplicate codepoints. Because of this, the next release of ufo2ft may reveal this source issue to users for the first time, where it will need to be corrected for compilation to succeed.

@Hoolean Hoolean changed the title Ensure error is raised when multiple glyphs claim the same code point Raise error for overloaded codepoints, and drop codepoints in colour-layer glyphs Jun 5, 2023
@anthrotype
Copy link
Member

the next release of ufo2ft may reveal this source issue to users for the first time, where it will need to be corrected for compilation to succeed.

it'd say that's as an improvement 👍

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.

None yet

2 participants