From 5b96fff9ce867ede8bd0098a0f0797de99e69e74 Mon Sep 17 00:00:00 2001
From: Behdad Esfahbod <behdad@behdad.org>
Date: Mon, 1 May 2023 11:36:39 -0600
Subject: [PATCH] [featureVars] Process lookups for features other than rvrn
 last

Fixes https://github.com/fonttools/fonttools/issues/3097
---
 Lib/fontTools/varLib/featureVars.py           | 51 ++++++++++++++-----
 .../data/test_results/FeatureVars_rclt.ttx    | 36 ++++++-------
 2 files changed, 56 insertions(+), 31 deletions(-)

diff --git a/Lib/fontTools/varLib/featureVars.py b/Lib/fontTools/varLib/featureVars.py
index 3292750ac..c1b01b176 100644
--- a/Lib/fontTools/varLib/featureVars.py
+++ b/Lib/fontTools/varLib/featureVars.py
@@ -14,7 +14,9 @@ from collections import OrderedDict
 from .errors import VarLibError, VarLibValidationError
 
 
-def addFeatureVariations(font, conditionalSubstitutions, featureTag="rvrn"):
+def addFeatureVariations(
+    font, conditionalSubstitutions, featureTag="rvrn", processLast=None
+):
     """Add conditional substitutions to a Variable Font.
 
     The `conditionalSubstitutions` argument is a list of (Region, Substitutions)
@@ -45,6 +47,9 @@ def addFeatureVariations(font, conditionalSubstitutions, featureTag="rvrn"):
     # >>> f.save(dstPath)
     """
 
+    if processLast is None:
+        processLast = featureTag != "rvrn"
+
     _checkSubstitutionGlyphsExist(
         glyphNames=set(font.getGlyphOrder()),
         substitutions=conditionalSubstitutions,
@@ -60,7 +65,9 @@ def addFeatureVariations(font, conditionalSubstitutions, featureTag="rvrn"):
         font["GSUB"] = buildGSUB()
 
     # setup lookups
-    lookupMap = buildSubstitutionLookups(font["GSUB"].table, allSubstitutions)
+    lookupMap = buildSubstitutionLookups(
+        font["GSUB"].table, allSubstitutions, processLast
+    )
 
     # addFeatureVariationsRaw takes a list of
     #  ( {condition}, [ lookup indices ] )
@@ -71,7 +78,9 @@ def addFeatureVariations(font, conditionalSubstitutions, featureTag="rvrn"):
             (conditionSet, [lookupMap[s] for s in substitutions])
         )
 
-    addFeatureVariationsRaw(font, font["GSUB"].table, conditionsAndLookups, featureTag)
+    addFeatureVariationsRaw(
+        font, font["GSUB"].table, conditionsAndLookups, featureTag, processLast
+    )
 
 
 def _checkSubstitutionGlyphsExist(glyphNames, substitutions):
@@ -316,10 +325,15 @@ def cleanupBox(box):
 #
 
 
-def addFeatureVariationsRaw(font, table, conditionalSubstitutions, featureTag="rvrn"):
+def addFeatureVariationsRaw(
+    font, table, conditionalSubstitutions, featureTag="rvrn", processLast=None
+):
     """Low level implementation of addFeatureVariations that directly
     models the possibilities of the FeatureVariations table."""
 
+    if processLast is None:
+        processLast = featureTag != "rvrn"
+
     #
     # if there is no <featureTag> feature:
     #     make empty <featureTag> feature
@@ -378,9 +392,15 @@ def addFeatureVariationsRaw(font, table, conditionalSubstitutions, featureTag="r
             existingLookupIndices = table.FeatureList.FeatureRecord[
                 varFeatureIndex
             ].Feature.LookupListIndex
+            combinedLookupIndices = (
+                existingLookupIndices + lookupIndices
+                if processLast
+                else lookupIndices + existingLookupIndices
+            )
+
             records.append(
                 buildFeatureTableSubstitutionRecord(
-                    varFeatureIndex, lookupIndices + existingLookupIndices
+                    varFeatureIndex, combinedLookupIndices
                 )
             )
         featureVariationRecords.append(
@@ -463,27 +483,32 @@ def visit(visitor, obj, attr, value):
     setattr(obj, attr, visitor.shift + value)
 
 
-def buildSubstitutionLookups(gsub, allSubstitutions):
+def buildSubstitutionLookups(gsub, allSubstitutions, processLast=False):
     """Build the lookups for the glyph substitutions, return a dict mapping
     the substitution to lookup indices."""
 
     # Insert lookups at the beginning of the lookup vector
     # https://github.com/googlefonts/fontmake/issues/950
 
+    firstIndex = len(gsub.LookupList.Lookup) if processLast else 0
     lookupMap = {}
     for i, substitutionMap in enumerate(allSubstitutions):
-        lookupMap[substitutionMap] = i
+        lookupMap[substitutionMap] = firstIndex + i
 
-    # Shift all lookup indices in gsub by len(allSubstitutions)
-    shift = len(allSubstitutions)
-    visitor = ShifterVisitor(shift)
-    visitor.visit(gsub.FeatureList.FeatureRecord)
-    visitor.visit(gsub.LookupList.Lookup)
+    if not processLast:
+        # Shift all lookup indices in gsub by len(allSubstitutions)
+        shift = len(allSubstitutions)
+        visitor = ShifterVisitor(shift)
+        visitor.visit(gsub.FeatureList.FeatureRecord)
+        visitor.visit(gsub.LookupList.Lookup)
 
     for i, subst in enumerate(allSubstitutions):
         substMap = dict(subst)
         lookup = buildLookup([buildSingleSubstSubtable(substMap)])
-        gsub.LookupList.Lookup.insert(i, lookup)
+        if processLast:
+            gsub.LookupList.Lookup.append(lookup)
+        else:
+            gsub.LookupList.Lookup.insert(i, lookup)
         assert gsub.LookupList.Lookup[lookupMap[subst]] is lookup
     gsub.LookupList.LookupCount = len(gsub.LookupList.Lookup)
     return lookupMap
diff --git a/Tests/varLib/data/test_results/FeatureVars_rclt.ttx b/Tests/varLib/data/test_results/FeatureVars_rclt.ttx
index e930ce72f..436913647 100644
--- a/Tests/varLib/data/test_results/FeatureVars_rclt.ttx
+++ b/Tests/varLib/data/test_results/FeatureVars_rclt.ttx
@@ -1,5 +1,5 @@
 <?xml version="1.0" encoding="UTF-8"?>
-<ttFont sfntVersion="\x00\x01\x00\x00" ttLibVersion="4.38">
+<ttFont sfntVersion="\x00\x01\x00\x00" ttLibVersion="4.39">
 
   <fvar>
 
@@ -54,14 +54,14 @@
         <FeatureTag value="rclt"/>
         <Feature>
           <!-- LookupCount=1 -->
-          <LookupListIndex index="0" value="3"/>
+          <LookupListIndex index="0" value="0"/>
         </Feature>
       </FeatureRecord>
       <FeatureRecord index="1">
         <FeatureTag value="rclt"/>
         <Feature>
           <!-- LookupCount=1 -->
-          <LookupListIndex index="0" value="4"/>
+          <LookupListIndex index="0" value="1"/>
         </Feature>
       </FeatureRecord>
     </FeatureList>
@@ -72,7 +72,7 @@
         <LookupFlag value="0"/>
         <!-- SubTableCount=1 -->
         <SingleSubst index="0">
-          <Substitution in="uni0024" out="uni0024.nostroke"/>
+          <Substitution in="uni0041" out="uni0061"/>
         </SingleSubst>
       </Lookup>
       <Lookup index="1">
@@ -88,7 +88,7 @@
         <LookupFlag value="0"/>
         <!-- SubTableCount=1 -->
         <SingleSubst index="0">
-          <Substitution in="uni0061" out="uni0041"/>
+          <Substitution in="uni0024" out="uni0024.nostroke"/>
         </SingleSubst>
       </Lookup>
       <Lookup index="3">
@@ -104,7 +104,7 @@
         <LookupFlag value="0"/>
         <!-- SubTableCount=1 -->
         <SingleSubst index="0">
-          <Substitution in="uni0041" out="uni0061"/>
+          <Substitution in="uni0061" out="uni0041"/>
         </SingleSubst>
       </Lookup>
     </LookupList>
@@ -133,7 +133,7 @@
             <Feature>
               <!-- LookupCount=3 -->
               <LookupListIndex index="0" value="0"/>
-              <LookupListIndex index="1" value="1"/>
+              <LookupListIndex index="1" value="2"/>
               <LookupListIndex index="2" value="3"/>
             </Feature>
           </SubstitutionRecord>
@@ -141,9 +141,9 @@
             <FeatureIndex value="1"/>
             <Feature>
               <!-- LookupCount=3 -->
-              <LookupListIndex index="0" value="0"/>
-              <LookupListIndex index="1" value="1"/>
-              <LookupListIndex index="2" value="4"/>
+              <LookupListIndex index="0" value="1"/>
+              <LookupListIndex index="1" value="2"/>
+              <LookupListIndex index="2" value="3"/>
             </Feature>
           </SubstitutionRecord>
         </FeatureTableSubstitution>
@@ -169,15 +169,15 @@
             <FeatureIndex value="0"/>
             <Feature>
               <!-- LookupCount=2 -->
-              <LookupListIndex index="0" value="2"/>
-              <LookupListIndex index="1" value="3"/>
+              <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="2"/>
+              <LookupListIndex index="0" value="1"/>
               <LookupListIndex index="1" value="4"/>
             </Feature>
           </SubstitutionRecord>
@@ -199,7 +199,7 @@
             <FeatureIndex value="0"/>
             <Feature>
               <!-- LookupCount=2 -->
-              <LookupListIndex index="0" value="1"/>
+              <LookupListIndex index="0" value="0"/>
               <LookupListIndex index="1" value="3"/>
             </Feature>
           </SubstitutionRecord>
@@ -208,7 +208,7 @@
             <Feature>
               <!-- LookupCount=2 -->
               <LookupListIndex index="0" value="1"/>
-              <LookupListIndex index="1" value="4"/>
+              <LookupListIndex index="1" value="3"/>
             </Feature>
           </SubstitutionRecord>
         </FeatureTableSubstitution>
@@ -230,15 +230,15 @@
             <Feature>
               <!-- LookupCount=2 -->
               <LookupListIndex index="0" value="0"/>
-              <LookupListIndex index="1" value="3"/>
+              <LookupListIndex index="1" value="2"/>
             </Feature>
           </SubstitutionRecord>
           <SubstitutionRecord index="1">
             <FeatureIndex value="1"/>
             <Feature>
               <!-- LookupCount=2 -->
-              <LookupListIndex index="0" value="0"/>
-              <LookupListIndex index="1" value="4"/>
+              <LookupListIndex index="0" value="1"/>
+              <LookupListIndex index="1" value="2"/>
             </Feature>
           </SubstitutionRecord>
         </FeatureTableSubstitution>