Merge pull request #1747 from justvanrossum/rclt_issue1625

[designspaceLib] [varLib] Allow FeatureVariations to be processed *after* other features

Lib/fontTools/designspaceLib/__init__.py

@@ -358,7 +358,7 @@ class BaseDocWriter(object):
     def __init__(self, documentPath, documentObject):
         self.path = documentPath
         self.documentObject = documentObject
-        self.documentVersion = "4.0"
+        self.documentVersion = "4.1"
         self.root = ET.Element("designspace")
         self.root.attrib['format'] = self.documentVersion
         self._axes = []     # for use by the writer only
@@ -371,7 +371,11 @@ class BaseDocWriter(object):
             self._addAxis(axisObject)
 
         if self.documentObject.rules:
-            self.root.append(ET.Element("rules"))
+            if getattr(self.documentObject, "rulesProcessingLast", False):
+                attributes = {"processing": "last"}
+            else:
+                attributes = {}
+            self.root.append(ET.Element("rules", attributes))
         for ruleObject in self.documentObject.rules:
             self._addRule(ruleObject)
 
@@ -675,6 +679,14 @@ class BaseDocReader(LogMixin):
     def readRules(self):
         # we also need to read any conditions that are outside of a condition set.
         rules = []
+        rulesElement = self.root.find(".rules")
+        if rulesElement is not None:
+            processingValue = rulesElement.attrib.get("processing", "first")
+            if processingValue not in {"first", "last"}:
+                raise DesignSpaceDocumentError(
+                    "<rules> processing attribute value is not valid: %r, "
+                    "expected 'first' or 'last'" % processingValue)
+            self.documentObject.rulesProcessingLast = processingValue == "last"
         for ruleElement in self.root.findall(".rules/rule"):
             ruleObject = self.ruleDescriptorClass()
             ruleName = ruleObject.name = ruleElement.attrib.get("name")
@@ -996,6 +1008,7 @@ class DesignSpaceDocument(LogMixin, AsDictMixin):
         self.instances = []
         self.axes = []
         self.rules = []
+        self.rulesProcessingLast = False
         self.default = None         # name of the default master
 
         self.lib = {}

Lib/fontTools/varLib/__init__.py

@@ -613,7 +613,7 @@ def _merge_OTL(font, model, master_fonts, axisTags):
 		font['GPOS'].table.remap_device_varidxes(varidx_map)
 
 
-def _add_GSUB_feature_variations(font, axes, internal_axis_supports, rules):
+def _add_GSUB_feature_variations(font, axes, internal_axis_supports, rules, rulesProcessingLast):
 
 	def normalize(name, value):
 		return models.normalizeLocation(
@@ -648,7 +648,11 @@ def _add_GSUB_feature_variations(font, axes, internal_axis_supports, rules):
 
 		conditional_subs.append((region, subs))
 
-	addFeatureVariations(font, conditional_subs)
+	if rulesProcessingLast:
+		featureTag = 'rclt'
+	else:
+		featureTag = 'rvrn'
+	addFeatureVariations(font, conditional_subs, featureTag)
 
 
 _DesignSpaceData = namedtuple(
@@ -661,6 +665,7 @@ _DesignSpaceData = namedtuple(
 		"masters",
 		"instances",
 		"rules",
+		"rulesProcessingLast",
 	],
 )
 
@@ -761,6 +766,7 @@ def load_designspace(designspace):
 		masters,
 		instances,
 		ds.rules,
+		ds.rulesProcessingLast,
 	)
 
 
@@ -865,7 +871,7 @@ def build(designspace, master_finder=lambda s:s, exclude=[], optimize=True):
 	if 'cvar' not in exclude and 'glyf' in vf:
 		_merge_TTHinting(vf, model, master_fonts)
 	if 'GSUB' not in exclude and ds.rules:
-		_add_GSUB_feature_variations(vf, ds.axes, ds.internal_axis_supports, ds.rules)
+		_add_GSUB_feature_variations(vf, ds.axes, ds.internal_axis_supports, ds.rules, ds.rulesProcessingLast)
 	if 'CFF2' not in exclude and 'CFF ' in vf:
 		_add_CFF2(vf, model, master_fonts)
 		if "post" in vf:

Lib/fontTools/varLib/featureVars.py

@@ -11,7 +11,7 @@ from fontTools.otlLib.builder import buildLookup, buildSingleSubstSubtable
 from collections import OrderedDict
 
 
-def addFeatureVariations(font, conditionalSubstitutions):
+def addFeatureVariations(font, conditionalSubstitutions, featureTag='rvrn'):
     """Add conditional substitutions to a Variable Font.
 
     The `conditionalSubstitutions` argument is a list of (Region, Substitutions)
@@ -43,7 +43,8 @@ def addFeatureVariations(font, conditionalSubstitutions):
     """
 
     addFeatureVariationsRaw(font,
-                            overlayFeatureVariations(conditionalSubstitutions))
+                            overlayFeatureVariations(conditionalSubstitutions),
+                            featureTag)
 
 def overlayFeatureVariations(conditionalSubstitutions):
     """Compute overlaps between all conditional substitutions.
@@ -255,15 +256,15 @@ def cleanupBox(box):
 # Low level implementation
 #
 
-def addFeatureVariationsRaw(font, conditionalSubstitutions):
+def addFeatureVariationsRaw(font, conditionalSubstitutions, featureTag='rvrn'):
     """Low level implementation of addFeatureVariations that directly
     models the possibilities of the FeatureVariations table."""
 
     #
-    # assert there is no 'rvrn' feature
+    # if there is no <featureTag> feature:
-    # make dummy 'rvrn' feature with no lookups
+    #     make empty <featureTag> feature
-    # sort features, get 'rvrn' feature index
+    #     sort features, get <featureTag> feature index
-    # add 'rvrn' feature to all scripts
+    #     add <featureTag> feature to all scripts
     # make lookups
     # add feature variations
     #
@@ -278,20 +279,25 @@ def addFeatureVariationsRaw(font, conditionalSubstitutions):
 
     gsub.FeatureVariations = None  # delete any existing FeatureVariations
 
-    for feature in gsub.FeatureList.FeatureRecord:
+    varFeatureIndices = []
-        assert feature.FeatureTag != 'rvrn'
+    for index, feature in enumerate(gsub.FeatureList.FeatureRecord):
+        if feature.FeatureTag == featureTag:
+            varFeatureIndices.append(index)
 
-    rvrnFeature = buildFeatureRecord('rvrn', [])
+    if not varFeatureIndices:
-    gsub.FeatureList.FeatureRecord.append(rvrnFeature)
+        varFeature = buildFeatureRecord(featureTag, [])
-    gsub.FeatureList.FeatureCount = len(gsub.FeatureList.FeatureRecord)
+        gsub.FeatureList.FeatureRecord.append(varFeature)
+        gsub.FeatureList.FeatureCount = len(gsub.FeatureList.FeatureRecord)
 
-    sortFeatureList(gsub)
+        sortFeatureList(gsub)
-    rvrnFeatureIndex = gsub.FeatureList.FeatureRecord.index(rvrnFeature)
+        varFeatureIndex = gsub.FeatureList.FeatureRecord.index(varFeature)
 
-    for scriptRecord in gsub.ScriptList.ScriptRecord:
+        for scriptRecord in gsub.ScriptList.ScriptRecord:
-        langSystems = [lsr.LangSys for lsr in scriptRecord.Script.LangSysRecord]
+            langSystems = [lsr.LangSys for lsr in scriptRecord.Script.LangSysRecord]
-        for langSys in [scriptRecord.Script.DefaultLangSys] + langSystems:
+            for langSys in [scriptRecord.Script.DefaultLangSys] + langSystems:
-            langSys.FeatureIndex.append(rvrnFeatureIndex)
+                langSys.FeatureIndex.append(varFeatureIndex)
+
+        varFeatureIndices = [varFeatureIndex]
 
     # setup lookups
 
@@ -311,8 +317,11 @@ def addFeatureVariationsRaw(font, conditionalSubstitutions):
             conditionTable.append(ct)
 
         lookupIndices = [lookupMap[subst] for subst in substitutions]
-        record = buildFeatureTableSubstitutionRecord(rvrnFeatureIndex, lookupIndices)
+        records = []
-        featureVariationRecords.append(buildFeatureVariationRecord(conditionTable, [record]))
+        for varFeatureIndex in varFeatureIndices:
+            existingLookupIndices = gsub.FeatureList.FeatureRecord[varFeatureIndex].Feature.LookupListIndex
+            records.append(buildFeatureTableSubstitutionRecord(varFeatureIndex, existingLookupIndices + lookupIndices))
+        featureVariationRecords.append(buildFeatureVariationRecord(conditionTable, records))
 
     gsub.FeatureVariations = buildFeatureVariations(featureVariationRecords)
 

Tests/designspaceLib/data/test.designspace

@@ -1,5 +1,5 @@
 <?xml version='1.0' encoding='UTF-8'?>
-<designspace format="4.0">
+<designspace format="4.1">
   <axes>
     <axis tag="wght" name="weight" minimum="0" maximum="1000" default="0">
       <labelname xml:lang="en">Wéíght</labelname>
@@ -13,7 +13,7 @@
       <map input="1000" output="990"/>
     </axis>
   </axes>
-  <rules>
+  <rules processing="last">
     <rule name="named.rule.1">
       <conditionset>
         <condition name="axisName_a" minimum="0" maximum="1"/>

Tests/designspaceLib/designspace_test.py

@@ -49,6 +49,7 @@ def test_fill_document(tmpdir):
     instancePath1 = os.path.join(tmpdir, "instances", "instanceTest1.ufo")
     instancePath2 = os.path.join(tmpdir, "instances", "instanceTest2.ufo")
     doc = DesignSpaceDocument()
+    doc.rulesProcessingLast = True
 
     # write some axes
     a1 = AxisDescriptor()
@@ -698,6 +699,7 @@ def test_rulesDocument(tmpdir):
     testDocPath = os.path.join(tmpdir, "testRules.designspace")
     testDocPath2 = os.path.join(tmpdir, "testRules_roundtrip.designspace")
     doc = DesignSpaceDocument()
+    doc.rulesProcessingLast = True
     a1 = AxisDescriptor()
     a1.minimum = 0
     a1.maximum = 1000
@@ -741,6 +743,7 @@ def test_rulesDocument(tmpdir):
     _addUnwrappedCondition(testDocPath)
     doc2 = DesignSpaceDocument()
     doc2.read(testDocPath)
+    assert doc2.rulesProcessingLast
     assert len(doc2.axes) == 2
     assert len(doc2.rules) == 1
     assert len(doc2.rules[0].conditionSets) == 2

Tests/varLib/data/FeatureVars.designspace

@@ -6,7 +6,7 @@
             <labelname xml:lang="en">Contrast</labelname>
         </axis>
     </axes>
-    <rules>
+    <rules processing="last">
         <rule name="dollar-stroke">
             <conditionset>
                 <condition name="weight" minimum="500" /> <!-- intentionally omitted maximum -->

Tests/varLib/data/test_results/FeatureVars.ttx

@@ -43,7 +43,7 @@
     <FeatureList>
       <!-- FeatureCount=1 -->
       <FeatureRecord index="0">
-        <FeatureTag value="rvrn"/>
+        <FeatureTag value="rclt"/>
         <Feature>
           <!-- LookupCount=0 -->
         </Feature>

Tests/varLib/data/test_results/FeatureVars_rclt.ttx

@@ -0,0 +1,249 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<ttFont sfntVersion="\x00\x01\x00\x00" ttLibVersion="3.29">
+
+  <fvar>
+
+    <!-- Weight -->
+    <Axis>
+      <AxisTag>wght</AxisTag>
+      <Flags>0x0</Flags>
+      <MinValue>0.0</MinValue>
+      <DefaultValue>368.0</DefaultValue>
+      <MaxValue>1000.0</MaxValue>
+      <AxisNameID>256</AxisNameID>
+    </Axis>
+
+    <!-- Contrast -->
+    <Axis>
+      <AxisTag>cntr</AxisTag>
+      <Flags>0x0</Flags>
+      <MinValue>0.0</MinValue>
+      <DefaultValue>0.0</DefaultValue>
+      <MaxValue>100.0</MaxValue>
+      <AxisNameID>257</AxisNameID>
+    </Axis>
+  </fvar>
+
+  <GSUB>
+    <Version value="0x00010001"/>
+    <ScriptList>
+      <!-- ScriptCount=1 -->
+      <ScriptRecord index="0">
+        <ScriptTag value="latn"/>
+        <Script>
+          <DefaultLangSys>
+            <ReqFeatureIndex value="65535"/>
+            <!-- FeatureCount=1 -->
+            <FeatureIndex index="0" value="1"/>
+          </DefaultLangSys>
+          <!-- LangSysCount=1 -->
+          <LangSysRecord index="0">
+            <LangSysTag value="NLD "/>
+            <LangSys>
+              <ReqFeatureIndex value="65535"/>
+              <!-- FeatureCount=1 -->
+              <FeatureIndex index="0" value="0"/>
+            </LangSys>
+          </LangSysRecord>
+        </Script>
+      </ScriptRecord>
+    </ScriptList>
+    <FeatureList>
+      <!-- FeatureCount=2 -->
+      <FeatureRecord index="0">
+        <FeatureTag value="rclt"/>
+        <Feature>
+          <!-- LookupCount=1 -->
+          <LookupListIndex index="0" value="0"/>
+        </Feature>
+      </FeatureRecord>
+      <FeatureRecord index="1">
+        <FeatureTag value="rclt"/>
+        <Feature>
+          <!-- LookupCount=1 -->
+          <LookupListIndex index="0" value="1"/>
+        </Feature>
+      </FeatureRecord>
+    </FeatureList>
+    <LookupList>
+      <!-- LookupCount=5 -->
+      <Lookup index="0">
+        <LookupType value="1"/>
+        <LookupFlag value="0"/>
+        <!-- SubTableCount=1 -->
+        <SingleSubst index="0" Format="1">
+          <Substitution in="uni0041" out="uni0061"/>
+        </SingleSubst>
+      </Lookup>
+      <Lookup index="1">
+        <LookupType value="1"/>
+        <LookupFlag value="0"/>
+        <!-- SubTableCount=1 -->
+        <SingleSubst index="0" Format="1">
+          <Substitution in="uni0041" out="uni0061"/>
+        </SingleSubst>
+      </Lookup>
+      <Lookup index="2">
+        <LookupType value="1"/>
+        <LookupFlag value="0"/>
+        <!-- SubTableCount=1 -->
+        <SingleSubst index="0" Format="1">
+          <Substitution in="uni0024" out="uni0024.nostroke"/>
+        </SingleSubst>
+      </Lookup>
+      <Lookup index="3">
+        <LookupType value="1"/>
+        <LookupFlag value="0"/>
+        <!-- SubTableCount=1 -->
+        <SingleSubst index="0" Format="1">
+          <Substitution in="uni0041" out="uni0061"/>
+        </SingleSubst>
+      </Lookup>
+      <Lookup index="4">
+        <LookupType value="1"/>
+        <LookupFlag value="0"/>
+        <!-- SubTableCount=1 -->
+        <SingleSubst index="0" Format="1">
+          <Substitution in="uni0061" out="uni0041"/>
+        </SingleSubst>
+      </Lookup>
+    </LookupList>
+    <FeatureVariations>
+      <Version value="0x00010000"/>
+      <!-- FeatureVariationCount=4 -->
+      <FeatureVariationRecord index="0">
+        <ConditionSet>
+          <!-- ConditionCount=2 -->
+          <ConditionTable index="0" Format="1">
+            <AxisIndex value="1"/>
+            <FilterRangeMinValue value="0.75"/>
+            <FilterRangeMaxValue value="1.0"/>
+          </ConditionTable>
+          <ConditionTable index="1" Format="1">
+            <AxisIndex value="0"/>
+            <FilterRangeMinValue value="0.20886"/>
+            <FilterRangeMaxValue value="1.0"/>
+          </ConditionTable>
+        </ConditionSet>
+        <FeatureTableSubstitution>
+          <Version value="0x00010000"/>
+          <!-- SubstitutionCount=2 -->
+          <SubstitutionRecord index="0">
+            <FeatureIndex value="0"/>
+            <Feature>
+              <!-- LookupCount=3 -->
+              <LookupListIndex index="0" value="0"/>
+              <LookupListIndex index="1" value="2"/>
+              <LookupListIndex index="2" value="3"/>
+            </Feature>
+          </SubstitutionRecord>
+          <SubstitutionRecord index="1">
+            <FeatureIndex value="1"/>
+            <Feature>
+              <!-- LookupCount=3 -->
+              <LookupListIndex index="0" value="1"/>
+              <LookupListIndex index="1" value="2"/>
+              <LookupListIndex index="2" value="3"/>
+            </Feature>
+          </SubstitutionRecord>
+        </FeatureTableSubstitution>
+      </FeatureVariationRecord>
+      <FeatureVariationRecord index="1">
+        <ConditionSet>
+          <!-- ConditionCount=2 -->
+          <ConditionTable index="0" Format="1">
+            <AxisIndex value="1"/>
+            <FilterRangeMinValue value="0.0"/>
+            <FilterRangeMaxValue value="0.25"/>
+          </ConditionTable>
+          <ConditionTable index="1" Format="1">
+            <AxisIndex value="0"/>
+            <FilterRangeMinValue value="-1.0"/>
+            <FilterRangeMaxValue value="-0.45654"/>
+          </ConditionTable>
+        </ConditionSet>
+        <FeatureTableSubstitution>
+          <Version value="0x00010000"/>
+          <!-- SubstitutionCount=2 -->
+          <SubstitutionRecord index="0">
+            <FeatureIndex value="0"/>
+            <Feature>
+              <!-- LookupCount=2 -->
+              <LookupListIndex index="0" value="0"/>
+              <LookupListIndex index="1" value="4"/>
+            </Feature>
+          </SubstitutionRecord>
+          <SubstitutionRecord index="1">
+            <FeatureIndex value="1"/>
+            <Feature>
+              <!-- LookupCount=2 -->
+              <LookupListIndex index="0" value="1"/>
+              <LookupListIndex index="1" value="4"/>
+            </Feature>
+          </SubstitutionRecord>
+        </FeatureTableSubstitution>
+      </FeatureVariationRecord>
+      <FeatureVariationRecord index="2">
+        <ConditionSet>
+          <!-- ConditionCount=1 -->
+          <ConditionTable index="0" Format="1">
+            <AxisIndex value="1"/>
+            <FilterRangeMinValue value="0.75"/>
+            <FilterRangeMaxValue value="1.0"/>
+          </ConditionTable>
+        </ConditionSet>
+        <FeatureTableSubstitution>
+          <Version value="0x00010000"/>
+          <!-- SubstitutionCount=2 -->
+          <SubstitutionRecord index="0">
+            <FeatureIndex value="0"/>
+            <Feature>
+              <!-- LookupCount=2 -->
+              <LookupListIndex index="0" value="0"/>
+              <LookupListIndex index="1" value="3"/>
+            </Feature>
+          </SubstitutionRecord>
+          <SubstitutionRecord index="1">
+            <FeatureIndex value="1"/>
+            <Feature>
+              <!-- LookupCount=2 -->
+              <LookupListIndex index="0" value="1"/>
+              <LookupListIndex index="1" value="3"/>
+            </Feature>
+          </SubstitutionRecord>
+        </FeatureTableSubstitution>
+      </FeatureVariationRecord>
+      <FeatureVariationRecord index="3">
+        <ConditionSet>
+          <!-- ConditionCount=1 -->
+          <ConditionTable index="0" Format="1">
+            <AxisIndex value="0"/>
+            <FilterRangeMinValue value="0.20886"/>
+            <FilterRangeMaxValue value="1.0"/>
+          </ConditionTable>
+        </ConditionSet>
+        <FeatureTableSubstitution>
+          <Version value="0x00010000"/>
+          <!-- SubstitutionCount=2 -->
+          <SubstitutionRecord index="0">
+            <FeatureIndex value="0"/>
+            <Feature>
+              <!-- LookupCount=2 -->
+              <LookupListIndex index="0" value="0"/>
+              <LookupListIndex index="1" value="2"/>
+            </Feature>
+          </SubstitutionRecord>
+          <SubstitutionRecord index="1">
+            <FeatureIndex value="1"/>
+            <Feature>
+              <!-- LookupCount=2 -->
+              <LookupListIndex index="0" value="1"/>
+              <LookupListIndex index="1" value="2"/>
+            </Feature>
+          </SubstitutionRecord>
+        </FeatureTableSubstitution>
+      </FeatureVariationRecord>
+    </FeatureVariations>
+  </GSUB>
+
+</ttFont>

Tests/varLib/varLib_test.py

@@ -7,6 +7,7 @@ from fontTools.varLib import set_default_weight_width_slant
 from fontTools.designspaceLib import (
     DesignSpaceDocumentError, DesignSpaceDocument, SourceDescriptor,
 )
+from fontTools.feaLib.builder import addOpenTypeFeaturesFromString
 import difflib
 import os
 import shutil
@@ -107,7 +108,8 @@ class BuildTest(unittest.TestCase):
         return font, savepath
 
     def _run_varlib_build_test(self, designspace_name, font_name, tables,
-                               expected_ttx_name, save_before_dump=False):
+                               expected_ttx_name, save_before_dump=False,
+                               post_process_master=None):
         suffix = '.ttf'
         ds_path = self.get_test_input(designspace_name + '.designspace')
         ufo_dir = self.get_test_input('master_ufo')
@@ -116,7 +118,9 @@ class BuildTest(unittest.TestCase):
         self.temp_dir()
         ttx_paths = self.get_file_list(ttx_dir, '.ttx', font_name + '-')
         for path in ttx_paths:
-            self.compile_font(path, suffix, self.tempdir)
+            font, savepath = self.compile_font(path, suffix, self.tempdir)
+            if post_process_master is not None:
+                post_process_master(font, savepath)
 
         finder = lambda s: s.replace(ufo_dir, self.tempdir).replace('.ufo', suffix)
         varfont, model, _ = build(ds_path, finder)
@@ -213,6 +217,47 @@ class BuildTest(unittest.TestCase):
             save_before_dump=True,
         )
 
+    def test_varlib_build_feature_variations_with_existing_rclt(self):
+        """Designspace file contains <rules> element, used to build GSUB
+        FeatureVariations table. <rules> is specified to do its OT processing
+        "last", so a 'rclt' feature will be used or created. This test covers
+        the case when a 'rclt' already exists in the masters.
+
+        We dynamically add a 'rclt' feature to an existing set of test
+        masters, to avoid adding more test data.
+
+        The multiple languages are done to verify whether multiple existing
+        'rclt' features are updated correctly.
+        """
+        def add_rclt(font, savepath):
+            features = """
+            languagesystem DFLT dflt;
+            languagesystem latn dflt;
+            languagesystem latn NLD;
+
+            feature rclt {
+                script latn;
+                language NLD;
+                lookup A {
+                    sub uni0041 by uni0061;
+                } A;
+                language dflt;
+                lookup B {
+                    sub uni0041 by uni0061;
+                } B;
+            } rclt;
+            """
+            addOpenTypeFeaturesFromString(font, features)
+            font.save(savepath)
+        self._run_varlib_build_test(
+            designspace_name="FeatureVars",
+            font_name="TestFamily",
+            tables=["fvar", "GSUB"],
+            expected_ttx_name="FeatureVars_rclt",
+            save_before_dump=True,
+            post_process_master=add_rclt,
+        )
+
     def test_varlib_gvar_explicit_delta(self):
         """The variable font contains a composite glyph odieresis which does not
         need a gvar entry, because all its deltas are 0, but it must be added