Merge pull request #815 from googlefonts/no-marks-anchor-propagation

Don't propagate anchors to "mark" composite glyphs with anchors

Lib/ufo2ft/featureWriters/baseFeatureWriter.py

@@ -1,15 +1,15 @@
 import logging
-from collections import OrderedDict, namedtuple
+from collections import OrderedDict
 from types import SimpleNamespace
 
 from fontTools.designspaceLib import DesignSpaceDocument
 from fontTools.feaLib.variableScalar import VariableScalar
 from fontTools.misc.fixedTools import otRound
 
-from ufo2ft.constants import OPENTYPE_CATEGORIES_KEY
 from ufo2ft.errors import InvalidFeaturesData
 from ufo2ft.featureWriters import ast
 from ufo2ft.util import (
+    OpenTypeCategories,
     collapse_varscalar,
     get_userspace_location,
     quantize,
@@ -351,47 +351,7 @@ def extraSubstitutions(self):
     def getOpenTypeCategories(self):
         """Return 'public.openTypeCategories' values as a tuple of sets of
         unassigned, bases, ligatures, marks, components."""
-        font = self.context.font
-        unassigned, bases, ligatures, marks, components = (
-            set(),
-            set(),
-            set(),
-            set(),
-            set(),
-        )
-        openTypeCategories = font.lib.get(OPENTYPE_CATEGORIES_KEY, {})
-        # Handle case where we are a variable feature writer
-        if not openTypeCategories and isinstance(font, DesignSpaceDocument):
-            font = font.sources[0].font
-            openTypeCategories = font.lib.get(OPENTYPE_CATEGORIES_KEY, {})
-
-        for glyphName, category in openTypeCategories.items():
-            if category == "unassigned":
-                unassigned.add(glyphName)
-            elif category == "base":
-                bases.add(glyphName)
-            elif category == "ligature":
-                ligatures.add(glyphName)
-            elif category == "mark":
-                marks.add(glyphName)
-            elif category == "component":
-                components.add(glyphName)
-            else:
-                self.log.warning(
-                    f"The '{OPENTYPE_CATEGORIES_KEY}' value of {glyphName} in "
-                    f"{font.info.familyName} {font.info.styleName} is '{category}' "
-                    "when it should be 'unassigned', 'base', 'ligature', 'mark' "
-                    "or 'component'."
-                )
-        return namedtuple(
-            "OpenTypeCategories", "unassigned base ligature mark component"
-        )(
-            frozenset(unassigned),
-            frozenset(bases),
-            frozenset(ligatures),
-            frozenset(marks),
-            frozenset(components),
-        )
+        return OpenTypeCategories.load(self.context.font)
 
     def getGDEFGlyphClasses(self):
         """Return a tuple of GDEF GlyphClassDef base, ligature, mark, component

Lib/ufo2ft/filters/propagateAnchors.py

@@ -18,6 +18,7 @@
 from fontTools.misc.transform import Transform
 
 from ufo2ft.filters import BaseFilter
+from ufo2ft.util import OpenTypeCategories
 
 logger = logging.getLogger(__name__)
 
@@ -26,14 +27,14 @@ class PropagateAnchorsFilter(BaseFilter):
     def set_context(self, font, glyphSet):
         ctx = super().set_context(font, glyphSet)
         ctx.processed = set()
+        ctx.categories = OpenTypeCategories.load(font)
         return ctx
 
     def __call__(self, font, glyphSet=None):
-        if super().__call__(font, glyphSet):
-            modified = self.context.modified
-            if modified:
-                logger.info("Glyphs with propagated anchors: %i" % len(modified))
-            return modified
+        modified = super().__call__(font, glyphSet)
+        if modified:
+            logger.info("Glyphs with propagated anchors: %i" % len(modified))
+        return modified
 
     def filter(self, glyph):
         if not glyph.components:
@@ -44,11 +45,12 @@ def filter(self, glyph):
             glyph,
             self.context.processed,
             self.context.modified,
+            self.context.categories,
         )
         return len(glyph.anchors) > before
 
 
-def _propagate_glyph_anchors(glyphSet, composite, processed, modified):
+def _propagate_glyph_anchors(glyphSet, composite, processed, modified, categories):
     """
     Propagate anchors from base glyphs to a given composite
     glyph, and to all composite glyphs used in between.
@@ -58,7 +60,12 @@ def _propagate_glyph_anchors(glyphSet, composite, processed, modified):
         return
     processed.add(composite.name)
 
-    if not composite.components:
+    if not composite.components or (
+        # "If it is a 'mark' and there are anchors, it will not look into components"
+        # Georg said: https://github.com/googlefonts/ufo2ft/issues/802#issuecomment-1904109457
+        composite.name in categories.mark
+        and composite.anchors
+    ):
         return
 
     base_components = []
@@ -74,7 +81,7 @@ def _propagate_glyph_anchors(glyphSet, composite, processed, modified):
                 "in glyph {}".format(component.baseGlyph, composite.name)
             )
         else:
-            _propagate_glyph_anchors(glyphSet, glyph, processed, modified)
+            _propagate_glyph_anchors(glyphSet, glyph, processed, modified, categories)
             if any(a.name.startswith("_") for a in glyph.anchors):
                 mark_components.append(component)
             else:

Lib/ufo2ft/util.py

@@ -5,7 +5,7 @@
 import re
 from copy import deepcopy
 from inspect import currentframe, getfullargspec
-from typing import Any, Mapping, Set
+from typing import Any, Mapping, NamedTuple, Set
 
 from fontTools import subset, ttLib, unicodedata
 from fontTools.designspaceLib import DesignSpaceDocument
@@ -15,7 +15,7 @@
 from fontTools.pens.reverseContourPen import ReverseContourPen
 from fontTools.pens.transformPen import TransformPen
 
-from ufo2ft.constants import UNICODE_SCRIPT_ALIASES
+from ufo2ft.constants import OPENTYPE_CATEGORIES_KEY, UNICODE_SCRIPT_ALIASES
 from ufo2ft.errors import InvalidDesignSpaceData, InvalidFontData
 from ufo2ft.fontInfoData import getAttrWithFallback
 
@@ -713,3 +713,54 @@ def collapse_varscalar(varscalar, threshold=0):
     if not any(abs(v - values[0]) > threshold for v in values[1:]):
         return list(varscalar.values.values())[0]
     return varscalar
+
+
+class OpenTypeCategories(NamedTuple):
+    unassigned: frozenset[str]
+    base: frozenset[str]
+    ligature: frozenset[str]
+    mark: frozenset[str]
+    component: frozenset[str]
+
+    @classmethod
+    def load(cls, font):
+        """Return 'public.openTypeCategories' values as a tuple of sets of
+        unassigned, bases, ligatures, marks, components."""
+        unassigned, bases, ligatures, marks, components = (
+            set(),
+            set(),
+            set(),
+            set(),
+            set(),
+        )
+        openTypeCategories = font.lib.get(OPENTYPE_CATEGORIES_KEY, {})
+        # Handle case where we are a variable feature writer
+        if not openTypeCategories and isinstance(font, DesignSpaceDocument):
+            font = font.sources[0].font
+            openTypeCategories = font.lib.get(OPENTYPE_CATEGORIES_KEY, {})
+
+        for glyphName, category in openTypeCategories.items():
+            if category == "unassigned":
+                unassigned.add(glyphName)
+            elif category == "base":
+                bases.add(glyphName)
+            elif category == "ligature":
+                ligatures.add(glyphName)
+            elif category == "mark":
+                marks.add(glyphName)
+            elif category == "component":
+                components.add(glyphName)
+            else:
+                logging.getLogger("ufo2ft").warning(
+                    f"The '{OPENTYPE_CATEGORIES_KEY}' value of {glyphName} in "
+                    f"{font.info.familyName} {font.info.styleName} is '{category}' "
+                    "when it should be 'unassigned', 'base', 'ligature', 'mark' "
+                    "or 'component'."
+                )
+        return cls(
+            frozenset(unassigned),
+            frozenset(bases),
+            frozenset(ligatures),
+            frozenset(marks),
+            frozenset(components),
+        )

tests/filters/propagateAnchors_test.py

@@ -106,6 +106,25 @@
                         ("addComponent", ("macroncomb", (1, 0, 0, 1, 175, 0))),
                     ],
                 },
+                {
+                    "name": "r",
+                    "width": 350,
+                    "outline": [
+                        ("moveTo", ((0, 0),)),
+                        ("lineTo", ((0, 300),)),
+                        ("lineTo", ((175, 300),)),
+                        ("closePath", ()),
+                    ],
+                    "anchors": [(175, 300, "top"), (175, 0, "bottom")],
+                },
+                {
+                    "name": "rcombbelow",
+                    "width": 0,
+                    "outline": [
+                        ("addComponent", ("r", (0.5, 0, 0, 0.5, -100, -100))),
+                    ],
+                    "anchors": [(0, 0, "_bottom")],
+                },
             ]
         }
     ]
@@ -120,6 +139,12 @@ def font(request, FontClass):
             getattr(pen, operator)(*operands)
         for x, y, name in param.get("anchors", []):
             glyph.appendAnchor(dict(x=x, y=y, name=name))
+    # classify as 'mark' all glyphs with zero width and 'comb' in their name
+    font.lib["public.openTypeCategories"] = {
+        g["name"]: "mark"
+        for g in request.param["glyphs"]
+        if g.get("width", 0) == 0 and "comb" in g["name"]
+    }
     return font
 
 
@@ -149,6 +174,10 @@ def font(request, FontClass):
         ],
         {"a_a"},
     ),
+    # the composite glyph is a mark with anchors, hence propagation is not performed,
+    # i.e. 'top' and 'bottom' are *not* copied to 'rcombbelow':
+    # https://github.com/googlefonts/ufo2ft/issues/802
+    "rcombbelow": ([("_bottom", 0, 0)], set()),
 }
 
 
@@ -173,7 +202,7 @@ def test_include_one_glyph_at_a_time(self, font, name):
     def test_whole_font(self, font):
         philter = PropagateAnchorsFilter()
         modified = philter(font)
-        assert modified == set(EXPECTED)
+        assert modified == {k for k in EXPECTED if k in EXPECTED[k][1]}
         for name, (expected_anchors, _) in EXPECTED.items():
             assert [(a.name, a.x, a.y) for a in font[name].anchors] == expected_anchors