Repository: googlefonts/ufo2ft
Pull request #739 (2023-04-19 18:14:51)

Raise error for overloaded codepoints, and drop codepoints in colour-layer glyphs
Hello!

`util.makeUnicodeToGlyphNameMapping()` [intends to raise an error](https://github.com/googlefonts/ufo2ft/blob/0c0a57/Lib/ufo2ft/util.py#L224-L225) 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. [x] Fix [failing COLR test](https://github.com/googlefonts/ufo2ft/blob/0c0a57/tests/outlineCompiler_test.py#L846-L861):
   - 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. [x] 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).


Patch:
diff --git a/Lib/ufo2ft/filters/explodeColorLayerGlyphs.py b/Lib/ufo2ft/filters/explodeColorLayerGlyphs.py
index 259104c..72dd543 100644
--- a/Lib/ufo2ft/filters/explodeColorLayerGlyphs.py
+++ b/Lib/ufo2ft/filters/explodeColorLayerGlyphs.py
@@ -48,6 +48,10 @@ class ExplodeColorLayerGlyphsFilter(BaseFilter):
                 layerGlyphSet, glyphSet, component.baseGlyph, layerName
             )
             component.baseGlyph = baseLayerGlyphName
+        # The new alternates must have their codepoints stripped lest they
+        # become encoded, which they will be if placed in the default layer.
+        # See: https://github.com/googlefonts/ufo2ft/pull/739#issuecomment-1516075892
+        layerGlyph.unicodes = []
         glyphSet[layerGlyphName] = layerGlyph
         self.context.colorLayerGlyphNames.add(layerGlyphName)
         return layerGlyphName
diff --git a/Lib/ufo2ft/util.py b/Lib/ufo2ft/util.py
index 23f518d..cf515d6 100644
--- a/Lib/ufo2ft/util.py
+++ b/Lib/ufo2ft/util.py
@@ -236,7 +236,7 @@ def makeUnicodeToGlyphNameMapping(font, glyphOrder=None):
             else:
                 from ufo2ft.errors import InvalidFontData
 
-                InvalidFontData(
+                raise InvalidFontData(
                     "cannot map '%s' to U+%04X; already mapped to '%s'"
                     % (glyphName, uni, mapping[uni])
                 )


Test patch:
diff --git a/tests/data/ColorTest.ufo/glyphs.color1/a.glif b/tests/data/ColorTest.ufo/glyphs.color1/a.glif
index ab6576d..0b91678 100644
--- a/tests/data/ColorTest.ufo/glyphs.color1/a.glif
+++ b/tests/data/ColorTest.ufo/glyphs.color1/a.glif
@@ -1,7 +1,6 @@
 <?xml version='1.0' encoding='UTF-8'?>
 <glyph name="a" format="2">
   <advance width="500"/>
-  <unicode hex="0061"/>
   <outline>
     <contour>
       <point x="260" y="82" type="curve" smooth="yes"/>
diff --git a/tests/data/ColorTest.ufo/glyphs.color2/a.glif b/tests/data/ColorTest.ufo/glyphs.color2/a.glif
index 4d93657..ce391f6 100644
--- a/tests/data/ColorTest.ufo/glyphs.color2/a.glif
+++ b/tests/data/ColorTest.ufo/glyphs.color2/a.glif
@@ -1,7 +1,6 @@
 <?xml version='1.0' encoding='UTF-8'?>
 <glyph name="a" format="2">
   <advance width="500"/>
-  <unicode hex="0061"/>
   <outline>
     <contour>
       <point x="195" y="230" type="line"/>
diff --git a/tests/filters/explodeColorLayerGlyphs_test.py b/tests/filters/explodeColorLayerGlyphs_test.py
new file mode 100644
index 0000000..d674b4f
--- /dev/null
+++ b/tests/filters/explodeColorLayerGlyphs_test.py
@@ -0,0 +1,36 @@
+"""Comprises tests for the ExplodeColorLayerGlyphsFilter filter."""
+
+from pathlib import Path
+
+import pytest
+
+from ufo2ft.filters.explodeColorLayerGlyphs import ExplodeColorLayerGlyphsFilter
+from ufo2ft.util import _GlyphSet
+
+
+@pytest.fixture
+def data_dir():
+    return Path(__file__).parent.parent / "data"
+
+
+def test_strip_color_codepoints(FontClass, data_dir):
+    """Test that the filter strips codepoints from glyphs when copying them from
+    color layers into default layer alternates.
+
+    See: https://github.com/googlefonts/ufo2ft/pull/739#issuecomment-1516075892"""
+
+    # Load a test UFO with color layers, and give a codepoint to one of the
+    # glyphs in those layers.
+    ufo = FontClass(data_dir / "ColorTest.ufo")
+
+    color_glyph = ufo.layers["color1"]["a"]
+    color_glyph.unicode = 0x3020
+
+    # Apply the filter to the UFO.
+    filter = ExplodeColorLayerGlyphsFilter()
+    glyphset = _GlyphSet.from_layer(ufo)
+    _ = filter(ufo, glyphset)
+
+    # Test that the newly-copied alternate has had its codepoint removed.
+    new_default_alt = glyphset["a.color1"]
+    assert new_default_alt.unicode is None
diff --git a/tests/integration_test.py b/tests/integration_test.py
index bf47602..7a4a755 100644
--- a/tests/integration_test.py
+++ b/tests/integration_test.py
@@ -1,6 +1,7 @@
 import difflib
 import io
 import os
+import re
 import sys
 from pathlib import Path
 
@@ -17,6 +18,7 @@ from ufo2ft import (
     compileVariableTTFs,
 )
 from ufo2ft.constants import KEEP_GLYPH_NAMES
+from ufo2ft.errors import InvalidFontData
 from ufo2ft.filters import TransformationsFilter
 
 
@@ -415,6 +417,32 @@ class IntegrationTest:
             "DSv5/MutatorSerifVariable_Width-CFF2.ttx",
         )
 
+    @pytest.mark.parametrize(
+        "compileFunc",
+        [
+            compileOTF,
+            compileTTF,
+        ],
+    )
+    def test_compile_overloaded_codepoints(self, FontClass, compileFunc):
+        """Confirm that ufo2ft produces an error when compiling a UFO with
+        multiple glyphs using the same codepoint. Currently only covers
+        individual UFOs."""
+
+        # Create a UFO in-memory with two glyphs using the same codepoint.
+        ufo = FontClass()
+        glyph_a = ufo.newGlyph("A")
+        glyph_b = ufo.newGlyph("B")
+        glyph_a.unicode = glyph_b.unicode = 0x0041
+
+        # Confirm that ufo2ft raises an appropriate exception with an
+        # appropriate description when compiling.
+        with pytest.raises(
+            InvalidFontData,
+            match=re.escape("cannot map 'B' to U+0041; already mapped to 'A'"),
+        ):
+            _ = compileFunc(ufo)
+
 
 if __name__ == "__main__":
     sys.exit(pytest.main(sys.argv))
diff --git a/tests/outlineCompiler_test.py b/tests/outlineCompiler_test.py
index bb357c3..11f9ba3 100644
--- a/tests/outlineCompiler_test.py
+++ b/tests/outlineCompiler_test.py
@@ -985,6 +985,28 @@ class ColrCpalTest:
             # no clipboxes, neither manual nor automatic
             assert colr.ClipList is None
 
+    @pytest.mark.parametrize("compileFunc", [compileTTF, compileOTF])
+    def test_strip_color_codepoints(self, FontClass, compileFunc):
+        """Test that glyphs in the color layer do not become accessible by
+        codepoint in the final font, given that they are copied into the default
+        layer as alternates.
+
+        See: https://github.com/googlefonts/ufo2ft/pull/739#issuecomment-1516075892"""
+
+        # Load a test UFO with color layers, and give a codepoint to one of the
+        # glyphs in those layers.
+        ufo = FontClass(getpath("ColorTest.ufo"))
+
+        color_glyph = ufo.layers["color1"]["a"]
+        color_glyph.unicode = 0x3020
+
+        # Build the UFO into a TTF or OTF.
+        built = compileFunc(ufo)
+
+        # Confirm that it has no entry for the codepoint above.
+        cmap = built.getBestCmap()
+        assert 0x3020 not in cmap
+
 
 class CmapTest:
     def test_cmap_BMP(self, testufo):
diff --git a/tests/util_test.py b/tests/util_test.py
new file mode 100644
index 0000000..6eec98a
--- /dev/null
+++ b/tests/util_test.py
@@ -0,0 +1,36 @@
+"""Tests for utility functions that ufo2ft provides."""
+
+import re
+
+import pytest
+
+from ufo2ft import util
+from ufo2ft.errors import InvalidFontData
+
+
+def test_overloaded_mapping_raises_error(FontClass):
+    """Test that util.makeUnicodeToGlyphNameMapping() raises an error when
+    multiple glyphs are mapped to the same codepoint."""
+
+    # Make an empty font in memory with glyphs 'A' and 'B'.
+    test_ufo = FontClass()
+    glyph_a = test_ufo.newGlyph("A")
+    glyph_b = test_ufo.newGlyph("B")
+
+    # Test that the util function DOES NOT raise an error when the glyphs are
+    # mapped to distinct codepoints, and that the function returns the correct
+    # mapping.
+    glyph_a.unicodes = [0x0041]
+    glyph_b.unicodes = [0x0042]
+    assert util.makeUnicodeToGlyphNameMapping(test_ufo) == {0x0041: "A", 0x0042: "B"}
+
+    # Test that the util function DOES raise an error when multiple glyphs are
+    # mapped to the same codepoint, and that this error is generally
+    # descriptive.
+    glyph_a.unicodes = [0x0041]
+    glyph_b.unicodes = [0x0041]
+    with pytest.raises(
+        InvalidFontData,
+        match=re.escape("cannot map 'B' to U+0041; already mapped to 'A'"),
+    ):
+        util.makeUnicodeToGlyphNameMapping(test_ufo)