Repository: googlefonts/ufo2ft
Pull request #678 (2022-11-14 20:16:51)

Improve mark writer for character used in multiple scripts
If a character is used in multiple scripts, and on of these scripts uses e.g. USE shaper, the mark writer will write its anchors in abvm/blwm features exclusively. This breaks the mark attachment if the shaper of the indented script does not enable abvm/blwm features.

This is a partial fix, by looking into the language systems declared in the feature file and filtering out abvm scripts not in them.

Part of https://github.com/googlefonts/ufo2ft/issues/579

khaledhosny: > > a partial fix
> 
> what's left for a full fix?

If the character is used in multiple scripts, write features for all scripts, right now if one of the scripts needs abvm/blwm we don’t write mark feature.

> > by looking into the language systems declared in the feature file
> 
> does it mean that to enable generating abvm/blwm features we would now require that the indic scripts be explicitly defined as global `languagesystem` statements? Is it conceivable that they might not declared as such in real-world font projects? I am only concerned this might break existing workflows.

The assumption is that since all scripts requiring abvm/blwm are scripts that require other features to work correctly, it is unlikely that they will be missing such declaration. We also don’t write any script/language statements in the generated code, so if global languagesystem is not there the mark positioning is unlikely to work anyway.
simoncozens: I'll try to take a look at this today.
khaledhosny: > > > a partial fix
> > 
> > 
> > what's left for a full fix?
> 
> If the character is used in multiple scripts, write features for all scripts, right now if one of the scripts needs abvm/blwm we don’t write mark feature.

This is done now.
simoncozens: Looks good to me; thank you!

Patch:
diff --git a/Lib/ufo2ft/featureWriters/markFeatureWriter.py b/Lib/ufo2ft/featureWriters/markFeatureWriter.py
index 6e4154d..a7042d1 100644
--- a/Lib/ufo2ft/featureWriters/markFeatureWriter.py
+++ b/Lib/ufo2ft/featureWriters/markFeatureWriter.py
@@ -4,6 +4,7 @@ from collections import OrderedDict, defaultdict
 from functools import partial
 
 from fontTools.misc.fixedTools import otRound
+from fontTools.unicodedata import script_extension
 
 from ufo2ft.constants import INDIC_SCRIPTS, USE_SCRIPTS
 from ufo2ft.featureWriters import BaseFeatureWriter, ast
@@ -300,6 +301,7 @@ class MarkFeatureWriter(BaseFeatureWriter):
         ctx.gdefClasses = self.getGDEFGlyphClasses()
         ctx.anchorLists = self._getAnchorLists()
         ctx.anchorPairs = self._getAnchorPairs()
+        ctx.feaScripts = set(ast.getScriptLanguageSystems(feaFile).keys())
 
     def shouldContinue(self):
         if not self.context.anchorPairs:
@@ -822,13 +824,13 @@ class MarkFeatureWriter(BaseFeatureWriter):
         )
         ctx.markToMarkAttachments = self._makeMarkToMarkAttachments()
 
-        abvmGlyphs = self._getAbvmGlyphs()
+        abvmGlyphs, notAbvmGlyphs = self._getAbvmGlyphs()
 
         def isAbvm(glyphName):
             return glyphName in abvmGlyphs
 
         def isNotAbvm(glyphName):
-            return glyphName not in abvmGlyphs
+            return glyphName in notAbvmGlyphs
 
         features = {}
         todo = ctx.todo
@@ -852,19 +854,43 @@ class MarkFeatureWriter(BaseFeatureWriter):
         return features
 
     def _getAbvmGlyphs(self):
-        cmap = self.makeUnicodeToGlyphNameMapping()
-        unicodeIsAbvm = partial(unicodeInScripts, scripts=self.scriptsUsingAbvm)
-        if any(unicodeIsAbvm(uv) for uv in cmap):
-            # If there are any characters from Indic/USE/Khmer scripts in the cmap, we
-            # compile a temporary GSUB table to resolve substitutions and get
-            # the set of all the relevant glyphs, including alternate glyphs.
-            gsub = self.compileGSUB()
-            glyphGroups = classifyGlyphs(unicodeIsAbvm, cmap, gsub)
-            # the 'glyphGroups' dict is keyed by the return value of the
-            # classifying include, so here 'True' means all the Indic/USE/Khmer glyphs
-            return glyphGroups.get(True, set())
-        else:
-            return set()
+        glyphSet = set(self.getOrderedGlyphSet().keys())
+        scriptsUsingAbvm = self.scriptsUsingAbvm
+        if self.context.feaScripts:
+            # https://github.com/googlefonts/ufo2ft/issues/579 Some characters
+            # can be used in multiple scripts and some of these scripts might
+            # need an abvm feature and some might not, so we filter-out the
+            # abvm scripts that the font does not intend to support.
+            scriptsUsingAbvm = scriptsUsingAbvm & self.context.feaScripts
+        if scriptsUsingAbvm:
+            cmap = self.makeUnicodeToGlyphNameMapping()
+            unicodeIsAbvm = partial(unicodeInScripts, scripts=scriptsUsingAbvm)
+
+            def unicodeIsNotAbvm(uv):
+                return bool(script_extension(chr(uv)) - self.scriptsUsingAbvm)
+
+            if any(unicodeIsAbvm(uv) for uv in cmap):
+                # If there are any characters from Indic/USE/Khmer scripts in
+                # the cmap, we compile a temporary GSUB table to resolve
+                # substitutions and get the set of all the relevant glyphs,
+                # including alternate glyphs.
+                gsub = self.compileGSUB()
+                glyphGroups = classifyGlyphs(unicodeIsAbvm, cmap, gsub)
+                # the 'glyphGroups' dict is keyed by the return value of the
+                # classifying include, so here 'True' means all the
+                # Indic/USE/Khmer glyphs
+                abvmGlyphs = glyphGroups.get(True, set())
+
+                # If a character can be used in Indic/USE/Khmer scripts as well
+                # as other scripts, we want to return it in both 'abvmGlyphs'
+                # (done above) and 'notAbvmGlyphs' (done below) sets.
+                glyphGroups = classifyGlyphs(unicodeIsNotAbvm, cmap, gsub)
+                notAbvmGlyphs = glyphGroups.get(True, set())
+                # Since cmap might not cover all glyphs, we union with the
+                # glyph set.
+                notAbvmGlyphs |= glyphSet - abvmGlyphs
+                return abvmGlyphs, notAbvmGlyphs
+        return set(), glyphSet
 
     def _write(self):
         self._pruneUnusedAnchors()


Test patch:
diff --git a/tests/featureWriters/markFeatureWriter_test.py b/tests/featureWriters/markFeatureWriter_test.py
index 3ea705d..2445195 100644
--- a/tests/featureWriters/markFeatureWriter_test.py
+++ b/tests/featureWriters/markFeatureWriter_test.py
@@ -824,6 +824,95 @@ class MarkFeatureWriterTest(FeatureWriterTest):
             """  # noqa: B950
         )
 
+    def test_shared_script_char(self, FontClass):
+        ufo = FontClass()
+        ufo.info.unitsPerEm = 1000
+
+        dottedCircle = ufo.newGlyph("kashida-ar")
+        dottedCircle.unicode = 0x0640
+        dottedCircle.anchors = [
+            {"name": "top", "x": 100, "y": 100},
+            {"name": "bottom", "x": 100, "y": -100},
+        ]
+
+        nukta = ufo.newGlyph("fatha-ar")
+        nukta.unicode = 0x064E
+        nukta.appendAnchor({"name": "_top", "x": 0, "y": 0})
+
+        nukta = ufo.newGlyph("kasra-ar")
+        nukta.unicode = 0x0650
+        nukta.appendAnchor({"name": "_bottom", "x": 0, "y": 547})
+
+        ufo.features.text = dedent(
+            """\
+            languagesystem DFLT dflt;
+            languagesystem arab dflt;
+            """
+        )
+        generated = self.writeFeatures(ufo)
+
+        assert str(generated) == dedent(
+            """\
+            markClass kasra-ar <anchor 0 547> @MC_bottom;
+            markClass fatha-ar <anchor 0 0> @MC_top;
+
+            feature mark {
+                lookup mark2base {
+                    pos base kashida-ar
+                        <anchor 100 -100> mark @MC_bottom
+                        <anchor 100 100> mark @MC_top;
+                } mark2base;
+
+            } mark;
+            """  # noqa: B950
+        )
+
+        expected = dedent(
+            """\
+            markClass kasra-ar <anchor 0 547> @MC_bottom;
+            markClass fatha-ar <anchor 0 0> @MC_top;
+
+            feature abvm {
+                lookup abvm_mark2base {
+                    pos base kashida-ar
+                        <anchor 100 100> mark @MC_top;
+                } abvm_mark2base;
+
+            } abvm;
+
+            feature blwm {
+                lookup blwm_mark2base {
+                    pos base kashida-ar
+                        <anchor 100 -100> mark @MC_bottom;
+                } blwm_mark2base;
+
+            } blwm;
+
+            feature mark {
+                lookup mark2base {
+                    pos base kashida-ar
+                        <anchor 100 -100> mark @MC_bottom
+                        <anchor 100 100> mark @MC_top;
+                } mark2base;
+
+            } mark;
+            """  # noqa: B950
+        )
+
+        ufo.features.text = ""
+        generated = self.writeFeatures(ufo)
+        assert str(generated) == expected
+
+        ufo.features.text = dedent(
+            """\
+            languagesystem DFLT dflt;
+            languagesystem arab dflt;
+            languagesystem adlm dflt;
+            """
+        )
+        generated = self.writeFeatures(ufo)
+        assert str(generated) == expected
+
     def test_all_features(self, testufo):
         ufo = testufo
         ufo.info.unitsPerEm = 1000
@@ -889,6 +978,12 @@ class MarkFeatureWriterTest(FeatureWriterTest):
             "foo_bar_baz",
             "bar_foo",
         ]
+        ufo.features.text = dedent(
+            """\
+            languagesystem DFLT dflt;
+            languagesystem taml dflt;
+            """
+        )
         generated = self.writeFeatures(testufo)
 
         assert str(generated) == dedent(