Merge pull request #455 from SixLabors/js/fix-451

JimBobSquarePants · web-flow · commit eadc8dbc041d · 2025-04-23T22:26:16.000+10:00
Do not over-apply sub-tables during contextual substitution list apply
diff --git a/src/SixLabors.Fonts/GlyphShapingData.cs b/src/SixLabors.Fonts/GlyphShapingData.cs
@@ -61,7 +61,10 @@ public GlyphShapingData(GlyphShapingData data, bool clearFeatures = false)
             this.Features.AddRange(data.Features);
         }
 
-        this.AppliedFeatures.AddRange(data.AppliedFeatures);
+        foreach (Tag feature in data.AppliedFeatures)
+        {
+            this.AppliedFeatures.Add(feature);
+        }
 
         this.Bounds = data.Bounds;
     }
@@ -124,7 +127,7 @@ public GlyphShapingData(GlyphShapingData data, bool clearFeatures = false)
     /// <summary>
     /// Gets or sets the collection of applied features.
     /// </summary>
-    public List<Tag> AppliedFeatures { get; set; } = new();
+    public HashSet<Tag> AppliedFeatures { get; set; } = new();
 
     /// <summary>
     /// Gets or sets the shaping bounds.
diff --git a/src/SixLabors.Fonts/Tables/AdvancedTypographic/AdvancedTypographicUtils.cs b/src/SixLabors.Fonts/Tables/AdvancedTypographic/AdvancedTypographicUtils.cs
@@ -51,7 +51,6 @@ public static bool ApplyLookupList(
         int index,
         int count)
     {
-        bool hasChanged = false;
         SkippingGlyphIterator iterator = new(fontMetrics, collection, index, lookupFlags);
         int currentCount = collection.Count;
 
@@ -62,7 +61,7 @@ public static bool ApplyLookupList(
             iterator.Index = index;
             iterator.Increment(sequenceIndex);
             GSub.LookupTable lookup = table.LookupList.LookupTables[lookupIndex];
-            hasChanged |= lookup.TrySubstitution(fontMetrics, table, collection, feature, iterator.Index, count - (iterator.Index - index));
+            _ = lookup.TrySubstitution(fontMetrics, table, collection, feature, iterator.Index, count - (iterator.Index - index));
 
             // Account for substitutions changing the length of the collection.
             if (collection.Count != currentCount)
@@ -72,7 +71,7 @@ public static bool ApplyLookupList(
             }
         }
 
-        return hasChanged;
+        return true;
     }
 
     public static bool ApplyLookupList(
@@ -85,7 +84,6 @@ public static bool ApplyLookupList(
         int index,
         int count)
     {
-        bool hasChanged = false;
         SkippingGlyphIterator iterator = new(fontMetrics, collection, index, lookupFlags);
         foreach (SequenceLookupRecord lookupRecord in records)
         {
@@ -94,10 +92,10 @@ public static bool ApplyLookupList(
             iterator.Index = index;
             iterator.Increment(sequenceIndex);
             LookupTable lookup = table.LookupList.LookupTables[lookupIndex];
-            hasChanged |= lookup.TryUpdatePosition(fontMetrics, table, collection, feature, iterator.Index, count - (iterator.Index - index));
+            _ = lookup.TryUpdatePosition(fontMetrics, table, collection, feature, iterator.Index, count - (iterator.Index - index));
         }
 
-        return hasChanged;
+        return true;
     }
 
     public static bool MatchInputSequence(SkippingGlyphIterator iterator, Tag feature, ushort increment, ushort[] sequence, Span<int> matches)
@@ -222,7 +220,7 @@ public static bool CheckAllCoverages(
         CoverageTable[] lookahead)
     {
         // Check that there are enough context glyphs.
-        if (index - backtrack.Length < 0 || input.Length + lookahead.Length > count)
+        if (index < backtrack.Length || input.Length + lookahead.Length > count)
         {
             return false;
         }
@@ -253,7 +251,8 @@ public static void ApplyAnchor(
         int index,
         AnchorTable baseAnchor,
         MarkRecord markRecord,
-        int baseGlyphIndex)
+        int baseGlyphIndex,
+        Tag feature)
     {
         GlyphShapingData baseData = collection[baseGlyphIndex];
         AnchorXY baseXY = baseAnchor.GetAnchor(fontMetrics, baseData, collection);
@@ -264,18 +263,21 @@ public static void ApplyAnchor(
         markData.Bounds.X = baseXY.XCoordinate - markXY.XCoordinate;
         markData.Bounds.Y = baseXY.YCoordinate - markXY.YCoordinate;
         markData.MarkAttachment = baseGlyphIndex;
+        markData.AppliedFeatures.Add(feature);
     }
 
     public static void ApplyPosition(
         GlyphPositioningCollection collection,
         int index,
-        ValueRecord record)
+        ValueRecord record,
+        Tag feature)
     {
         GlyphShapingData current = collection[index];
         current.Bounds.Width += record.XAdvance;
         current.Bounds.Height += record.YAdvance;
         current.Bounds.X += record.XPlacement;
         current.Bounds.Y += record.YPlacement;
+        current.AppliedFeatures.Add(feature);
     }
 
     public static bool IsMarkGlyph(FontMetrics fontMetrics, ushort glyphId, GlyphShapingData shapingData)
diff --git a/src/SixLabors.Fonts/Tables/AdvancedTypographic/GPos/LookupType1SubTable.cs b/src/SixLabors.Fonts/Tables/AdvancedTypographic/GPos/LookupType1SubTable.cs
@@ -81,7 +81,7 @@ public override bool TryUpdatePosition(
         if (coverage > -1)
         {
             ValueRecord record = this.valueRecord;
-            AdvancedTypographicUtils.ApplyPosition(collection, index, record);
+            AdvancedTypographicUtils.ApplyPosition(collection, index, record, feature);
 
             return true;
         }
@@ -152,7 +152,7 @@ public override bool TryUpdatePosition(
         if (coverage > -1)
         {
             ValueRecord record = this.valueRecords[coverage];
-            AdvancedTypographicUtils.ApplyPosition(collection, index, record);
+            AdvancedTypographicUtils.ApplyPosition(collection, index, record, feature);
 
             return true;
         }
diff --git a/src/SixLabors.Fonts/Tables/AdvancedTypographic/GPos/LookupType2SubTable.cs b/src/SixLabors.Fonts/Tables/AdvancedTypographic/GPos/LookupType2SubTable.cs
@@ -117,10 +117,10 @@ public override bool TryUpdatePosition(
                 if (pairSet.TryGetPairValueRecord(glyphId2, out PairValueRecord pairValueRecord))
                 {
                     ValueRecord record1 = pairValueRecord.ValueRecord1;
-                    AdvancedTypographicUtils.ApplyPosition(collection, index, record1);
+                    AdvancedTypographicUtils.ApplyPosition(collection, index, record1, feature);
 
                     ValueRecord record2 = pairValueRecord.ValueRecord2;
-                    AdvancedTypographicUtils.ApplyPosition(collection, index + 1, record2);
+                    AdvancedTypographicUtils.ApplyPosition(collection, index + 1, record2, feature);
 
                     return true;
                 }
@@ -285,10 +285,10 @@ public override bool TryUpdatePosition(
                 Class2Record class2Record = class1Record.Class2Records[classDef2];
 
                 ValueRecord record1 = class2Record.ValueRecord1;
-                AdvancedTypographicUtils.ApplyPosition(collection, index, record1);
+                AdvancedTypographicUtils.ApplyPosition(collection, index, record1, feature);
 
                 ValueRecord record2 = class2Record.ValueRecord2;
-                AdvancedTypographicUtils.ApplyPosition(collection, index + 1, record2);
+                AdvancedTypographicUtils.ApplyPosition(collection, index + 1, record2, feature);
 
                 return true;
             }
diff --git a/src/SixLabors.Fonts/Tables/AdvancedTypographic/GPos/LookupType4SubTable.cs b/src/SixLabors.Fonts/Tables/AdvancedTypographic/GPos/LookupType4SubTable.cs
@@ -126,7 +126,7 @@ public override bool TryUpdatePosition(
 
             MarkRecord markRecord = this.markArrayTable.MarkRecords[markIndex];
             AnchorTable baseAnchor = this.baseArrayTable.BaseRecords[baseIndex].BaseAnchorTables[markRecord.MarkClass];
-            AdvancedTypographicUtils.ApplyAnchor(fontMetrics, collection, index, baseAnchor, markRecord, baseGlyphIndex);
+            AdvancedTypographicUtils.ApplyAnchor(fontMetrics, collection, index, baseAnchor, markRecord, baseGlyphIndex, feature);
 
             return true;
         }
diff --git a/src/SixLabors.Fonts/Tables/AdvancedTypographic/GPos/LookupType5SubTable.cs b/src/SixLabors.Fonts/Tables/AdvancedTypographic/GPos/LookupType5SubTable.cs
@@ -140,7 +140,7 @@ public override bool TryUpdatePosition(
 
             MarkRecord markRecord = this.markArrayTable.MarkRecords[markIndex];
             AnchorTable baseAnchor = ligatureAttach.ComponentRecords[compIndex].LigatureAnchorTables[markRecord.MarkClass];
-            AdvancedTypographicUtils.ApplyAnchor(fontMetrics, collection, index, baseAnchor, markRecord, baseGlyphIndex);
+            AdvancedTypographicUtils.ApplyAnchor(fontMetrics, collection, index, baseAnchor, markRecord, baseGlyphIndex, feature);
 
             return true;
         }
diff --git a/src/SixLabors.Fonts/Tables/AdvancedTypographic/GPos/LookupType6SubTable.cs b/src/SixLabors.Fonts/Tables/AdvancedTypographic/GPos/LookupType6SubTable.cs
@@ -158,7 +158,7 @@ public override bool TryUpdatePosition(
 
             MarkRecord markRecord = this.mark1ArrayTable.MarkRecords[mark1Index];
             AnchorTable baseAnchor = this.mark2ArrayTable.Mark2Records[mark2Index].MarkAnchorTable[markRecord.MarkClass];
-            AdvancedTypographicUtils.ApplyAnchor(fontMetrics, collection, index, baseAnchor, markRecord, prevIdx);
+            AdvancedTypographicUtils.ApplyAnchor(fontMetrics, collection, index, baseAnchor, markRecord, prevIdx, feature);
 
             return true;
         }
diff --git a/tests/Images/ReferenceOutput/Issue_451_A-.png b/tests/Images/ReferenceOutput/Issue_451_A-.png
@@ -0,0 +1,3 @@
+version https://git-lfs.github.com/spec/v1
+oid sha256:17b4d0c60a62c2a7d0dae921906c803f67d6885ea2b53cb58c7c62c25a1654dc
+size 2213
diff --git a/tests/Images/ReferenceOutput/Issue_451_B-.png b/tests/Images/ReferenceOutput/Issue_451_B-.png
@@ -0,0 +1,3 @@
+version https://git-lfs.github.com/spec/v1
+oid sha256:8662ca4a1d837e5c0714e4deb36a679edccf3aeaab245d70494f5643c04a5475
+size 1843
diff --git a/tests/Images/ReferenceOutput/Issue_451_C-.png b/tests/Images/ReferenceOutput/Issue_451_C-.png
@@ -0,0 +1,3 @@
+version https://git-lfs.github.com/spec/v1
+oid sha256:cd9256bf5cd2dcd4a48a8ba6f6a9f4d10cfd5ee858dc99f3122cf340b14140ad
+size 23102
diff --git a/tests/SixLabors.Fonts.Tests/Fonts/VeryBerryProRegular.ttf b/tests/SixLabors.Fonts.Tests/Fonts/VeryBerryProRegular.ttf
diff --git a/tests/SixLabors.Fonts.Tests/Issues/Issues_451.cs b/tests/SixLabors.Fonts.Tests/Issues/Issues_451.cs
@@ -0,0 +1,47 @@
+// Copyright (c) Six Labors.
+// Licensed under the Six Labors Split License.
+
+using System.Numerics;
+
+namespace SixLabors.Fonts.Tests.Issues;
+
+public class Issues_451
+{
+    private readonly FontFamily berry = new FontCollection().Add(TestFonts.VeryBerryProRegular);
+
+    [Fact]
+    public void Issue_451_A()
+    {
+        Font font = this.berry.CreateFont(85);
+        TextLayoutTestUtilities.TestLayout(
+            "The",
+            new TextOptions(font)
+            {
+                Origin = new Vector2(0, 50),
+            });
+    }
+
+    [Fact]
+    public void Issue_451_B()
+    {
+        Font font = this.berry.CreateFont(85);
+        TextLayoutTestUtilities.TestLayout(
+            "Th",
+            new TextOptions(font)
+            {
+                Origin = new Vector2(0, 50),
+            });
+    }
+
+    [Fact]
+    public void Issue_451_C()
+    {
+        Font font = this.berry.CreateFont(85);
+        TextLayoutTestUtilities.TestLayout(
+            "The quick brown fox jumps over the lazy dog",
+            new TextOptions(font)
+            {
+                Origin = new Vector2(0, 50),
+            });
+    }
+}
diff --git a/tests/SixLabors.Fonts.Tests/TestFonts.cs b/tests/SixLabors.Fonts.Tests/TestFonts.cs
@@ -261,6 +261,8 @@ public static class TestFonts
 
     public static string CharisSILRegular => GetFullPath("CharisSIL-Regular.ttf");
 
+    public static string VeryBerryProRegular => GetFullPath("VeryBerryProRegular.ttf");
+
     public static Stream TwemojiMozillaData() => OpenStream(TwemojiMozillaFile);
 
     public static Stream SegoeuiEmojiData() => OpenStream(SegoeuiEmojiFile);

Original file line number	Diff line number	Diff line change
`@@ -51,7 +51,6 @@ public static bool ApplyLookupList(`
`51`	`51`	`int index,`
`52`	`52`	`int count)`
`53`	`53`	`{`
`54`		`- bool hasChanged = false;`
`55`	`54`	`SkippingGlyphIterator iterator = new(fontMetrics, collection, index, lookupFlags);`
`56`	`55`	`int currentCount = collection.Count;`
`57`	`56`
`@@ -62,7 +61,7 @@ public static bool ApplyLookupList(`
`62`	`61`	`iterator.Index = index;`
`63`	`62`	`iterator.Increment(sequenceIndex);`
`64`	`63`	`GSub.LookupTable lookup = table.LookupList.LookupTables[lookupIndex];`
`65`		`- hasChanged \|= lookup.TrySubstitution(fontMetrics, table, collection, feature, iterator.Index, count - (iterator.Index - index));`
	`64`	`+ _ = lookup.TrySubstitution(fontMetrics, table, collection, feature, iterator.Index, count - (iterator.Index - index));`
`66`	`65`
`67`	`66`	`// Account for substitutions changing the length of the collection.`
`68`	`67`	`if (collection.Count != currentCount)`
`@@ -72,7 +71,7 @@ public static bool ApplyLookupList(`
`72`	`71`	`}`
`73`	`72`	`}`
`74`	`73`
`75`		`- return hasChanged;`
	`74`	`+ return true;`
`76`	`75`	`}`
`77`	`76`
`78`	`77`	`public static bool ApplyLookupList(`
`@@ -85,7 +84,6 @@ public static bool ApplyLookupList(`
`85`	`84`	`int index,`
`86`	`85`	`int count)`
`87`	`86`	`{`
`88`		`- bool hasChanged = false;`
`89`	`87`	`SkippingGlyphIterator iterator = new(fontMetrics, collection, index, lookupFlags);`
`90`	`88`	`foreach (SequenceLookupRecord lookupRecord in records)`
`91`	`89`	`{`
`@@ -94,10 +92,10 @@ public static bool ApplyLookupList(`
`94`	`92`	`iterator.Index = index;`
`95`	`93`	`iterator.Increment(sequenceIndex);`
`96`	`94`	`LookupTable lookup = table.LookupList.LookupTables[lookupIndex];`
`97`		`- hasChanged \|= lookup.TryUpdatePosition(fontMetrics, table, collection, feature, iterator.Index, count - (iterator.Index - index));`
	`95`	`+ _ = lookup.TryUpdatePosition(fontMetrics, table, collection, feature, iterator.Index, count - (iterator.Index - index));`
`98`	`96`	`}`
`99`	`97`
`100`		`- return hasChanged;`
	`98`	`+ return true;`
`101`	`99`	`}`
`102`	`100`
`103`	`101`	`public static bool MatchInputSequence(SkippingGlyphIterator iterator, Tag feature, ushort increment, ushort[] sequence, Span<int> matches)`
`@@ -222,7 +220,7 @@ public static bool CheckAllCoverages(`
`222`	`220`	`CoverageTable[] lookahead)`
`223`	`221`	`{`
`224`	`222`	`// Check that there are enough context glyphs.`
`225`		`- if (index - backtrack.Length < 0 \|\| input.Length + lookahead.Length > count)`
	`223`	`+ if (index < backtrack.Length \|\| input.Length + lookahead.Length > count)`
`226`	`224`	`{`
`227`	`225`	`return false;`
`228`	`226`	`}`
`@@ -253,7 +251,8 @@ public static void ApplyAnchor(`
`253`	`251`	`int index,`
`254`	`252`	`AnchorTable baseAnchor,`
`255`	`253`	`MarkRecord markRecord,`
`256`		`- int baseGlyphIndex)`
	`254`	`+ int baseGlyphIndex,`
	`255`	`+ Tag feature)`
`257`	`256`	`{`
`258`	`257`	`GlyphShapingData baseData = collection[baseGlyphIndex];`
`259`	`258`	`AnchorXY baseXY = baseAnchor.GetAnchor(fontMetrics, baseData, collection);`
`@@ -264,18 +263,21 @@ public static void ApplyAnchor(`
`264`	`263`	`markData.Bounds.X = baseXY.XCoordinate - markXY.XCoordinate;`
`265`	`264`	`markData.Bounds.Y = baseXY.YCoordinate - markXY.YCoordinate;`
`266`	`265`	`markData.MarkAttachment = baseGlyphIndex;`
	`266`	`+ markData.AppliedFeatures.Add(feature);`
`267`	`267`	`}`
`268`	`268`
`269`	`269`	`public static void ApplyPosition(`
`270`	`270`	`GlyphPositioningCollection collection,`
`271`	`271`	`int index,`
`272`		`- ValueRecord record)`
	`272`	`+ ValueRecord record,`
	`273`	`+ Tag feature)`
`273`	`274`	`{`
`274`	`275`	`GlyphShapingData current = collection[index];`
`275`	`276`	`current.Bounds.Width += record.XAdvance;`
`276`	`277`	`current.Bounds.Height += record.YAdvance;`
`277`	`278`	`current.Bounds.X += record.XPlacement;`
`278`	`279`	`current.Bounds.Y += record.YPlacement;`
	`280`	`+ current.AppliedFeatures.Add(feature);`
`279`	`281`	`}`
`280`	`282`
`281`	`283`	`public static bool IsMarkGlyph(FontMetrics fontMetrics, ushort glyphId, GlyphShapingData shapingData)`
Original file line number	Diff line number	Diff line change
`@@ -81,7 +81,7 @@ public override bool TryUpdatePosition(`
`81`	`81`	`if (coverage > -1)`
`82`	`82`	`{`
`83`	`83`	`ValueRecord record = this.valueRecord;`
`84`		`- AdvancedTypographicUtils.ApplyPosition(collection, index, record);`
	`84`	`+ AdvancedTypographicUtils.ApplyPosition(collection, index, record, feature);`
`85`	`85`
`86`	`86`	`return true;`
`87`	`87`	`}`
`@@ -152,7 +152,7 @@ public override bool TryUpdatePosition(`
`152`	`152`	`if (coverage > -1)`
`153`	`153`	`{`
`154`	`154`	`ValueRecord record = this.valueRecords[coverage];`
`155`		`- AdvancedTypographicUtils.ApplyPosition(collection, index, record);`
	`155`	`+ AdvancedTypographicUtils.ApplyPosition(collection, index, record, feature);`
`156`	`156`
`157`	`157`	`return true;`
`158`	`158`	`}`
Original file line number	Diff line number	Diff line change
`@@ -126,7 +126,7 @@ public override bool TryUpdatePosition(`
`126`	`126`
`127`	`127`	`MarkRecord markRecord = this.markArrayTable.MarkRecords[markIndex];`
`128`	`128`	`AnchorTable baseAnchor = this.baseArrayTable.BaseRecords[baseIndex].BaseAnchorTables[markRecord.MarkClass];`
`129`		`- AdvancedTypographicUtils.ApplyAnchor(fontMetrics, collection, index, baseAnchor, markRecord, baseGlyphIndex);`
	`129`	`+ AdvancedTypographicUtils.ApplyAnchor(fontMetrics, collection, index, baseAnchor, markRecord, baseGlyphIndex, feature);`
`130`	`130`
`131`	`131`	`return true;`
`132`	`132`	`}`
Original file line number	Diff line number	Diff line change
`@@ -140,7 +140,7 @@ public override bool TryUpdatePosition(`
`140`	`140`
`141`	`141`	`MarkRecord markRecord = this.markArrayTable.MarkRecords[markIndex];`
`142`	`142`	`AnchorTable baseAnchor = ligatureAttach.ComponentRecords[compIndex].LigatureAnchorTables[markRecord.MarkClass];`
`143`		`- AdvancedTypographicUtils.ApplyAnchor(fontMetrics, collection, index, baseAnchor, markRecord, baseGlyphIndex);`
	`143`	`+ AdvancedTypographicUtils.ApplyAnchor(fontMetrics, collection, index, baseAnchor, markRecord, baseGlyphIndex, feature);`
`144`	`144`
`145`	`145`	`return true;`
`146`	`146`	`}`
Original file line number	Diff line number	Diff line change
`@@ -158,7 +158,7 @@ public override bool TryUpdatePosition(`
`158`	`158`
`159`	`159`	`MarkRecord markRecord = this.mark1ArrayTable.MarkRecords[mark1Index];`
`160`	`160`	`AnchorTable baseAnchor = this.mark2ArrayTable.Mark2Records[mark2Index].MarkAnchorTable[markRecord.MarkClass];`
`161`		`- AdvancedTypographicUtils.ApplyAnchor(fontMetrics, collection, index, baseAnchor, markRecord, prevIdx);`
	`161`	`+ AdvancedTypographicUtils.ApplyAnchor(fontMetrics, collection, index, baseAnchor, markRecord, prevIdx, feature);`
`162`	`162`
`163`	`163`	`return true;`
`164`	`164`	`}`