From 8acabcf81ba906c9be113b11ecbd38fae0d03a46 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ju=CC=88rg=20Lehni?= Date: Fri, 8 May 2026 18:51:31 +0200 Subject: [PATCH] Honour user alternateNumber in GSUB Type 3 substitutions - Type 3 hardcoded USER_INDEX = 0; `nalt: 2` silently rendered as alt #1 - Thread user features through ShapingPlan so GSUB reads the 1-based index - Don't mutate caller's features in OTLayoutEngine.setup; overlay instead --- src/opentype/GPOSProcessor.js | 4 ++-- src/opentype/GSUBProcessor.js | 13 +++++++++++-- src/opentype/OTLayoutEngine.js | 14 ++++++++++---- src/opentype/OTProcessor.js | 5 +++-- src/opentype/ShapingPlan.js | 8 +++++++- test/shaping.js | 19 +++++++++++++++++++ 6 files changed, 52 insertions(+), 11 deletions(-) diff --git a/src/opentype/GPOSProcessor.js b/src/opentype/GPOSProcessor.js index 6a2c1f9f..0bf10d24 100644 --- a/src/opentype/GPOSProcessor.js +++ b/src/opentype/GPOSProcessor.js @@ -305,8 +305,8 @@ export default class GPOSProcessor extends OTProcessor { return { x, y }; } - applyFeatures(userFeatures, glyphs, advances) { - super.applyFeatures(userFeatures, glyphs, advances); + applyFeatures(featureTags, glyphs, advances, userFeatures) { + super.applyFeatures(featureTags, glyphs, advances, userFeatures); for (var i = 0; i < this.glyphs.length; i++) { this.fixCursiveAttachment(i); diff --git a/src/opentype/GSUBProcessor.js b/src/opentype/GSUBProcessor.js index f5672d5d..8ae80dc5 100644 --- a/src/opentype/GSUBProcessor.js +++ b/src/opentype/GSUBProcessor.js @@ -61,8 +61,17 @@ export default class GSUBProcessor extends OTProcessor { case 3: { // Alternate Substitution let index = this.coverageIndex(table.coverage); if (index !== -1) { - let USER_INDEX = 0; // TODO - this.glyphIterator.cur.id = table.alternateSet.get(index)[USER_INDEX]; + // Honour the user's 1-based alternate index for the current feature + // (e.g. `nalt: 2` selects the second alternate). Fall back to the + // first alternate when no value was given or the value isn't a + // positive integer. + let alt = this.userFeatures && this.userFeatures[this.currentFeature]; + let userIndex = Number.isInteger(alt) && alt > 0 ? alt - 1 : 0; + let alternates = table.alternateSet.get(index); + if (userIndex >= alternates.length) { + userIndex = 0; + } + this.glyphIterator.cur.id = alternates[userIndex]; return true; } diff --git a/src/opentype/OTLayoutEngine.js b/src/opentype/OTLayoutEngine.js index 15cf3627..b9e1a9ca 100644 --- a/src/opentype/OTLayoutEngine.js +++ b/src/opentype/OTLayoutEngine.js @@ -43,10 +43,16 @@ export default class OTLayoutEngine { this.plan = new ShapingPlan(this.font, script, glyphRun.direction); this.shaper.plan(this.plan, this.glyphInfos, glyphRun.features); - // Assign chosen features to output glyph run - for (let key in this.plan.allFeatures) { - glyphRun.features[key] = true; - } + // Build the output features object: every shaper-chosen feature marked + // `true`, then overlay the user's original values so alternateNumber + // selections (e.g. `nalt: 2` for Type 3 lookups) survive instead of being + // clobbered to `true`. The caller's input object is left untouched. + glyphRun.features = { + ...Object.fromEntries( + Object.keys(this.plan.allFeatures).map(k => [k, true]) + ), + ...glyphRun.features + }; } substitute(glyphRun) { diff --git a/src/opentype/OTProcessor.js b/src/opentype/OTProcessor.js index 80d1ce69..5314d05e 100644 --- a/src/opentype/OTProcessor.js +++ b/src/opentype/OTProcessor.js @@ -179,8 +179,9 @@ export default class OTProcessor { }); } - applyFeatures(userFeatures, glyphs, advances) { - let lookups = this.lookupsForFeatures(userFeatures); + applyFeatures(featureTags, glyphs, advances, userFeatures) { + this.userFeatures = userFeatures; + let lookups = this.lookupsForFeatures(featureTags); this.applyLookups(lookups, glyphs, advances); } diff --git a/src/opentype/ShapingPlan.js b/src/opentype/ShapingPlan.js index 121a7934..c3ebab2f 100644 --- a/src/opentype/ShapingPlan.js +++ b/src/opentype/ShapingPlan.js @@ -17,6 +17,11 @@ export default class ShapingPlan { this.stages = []; this.globalFeatures = {}; this.allFeatures = {}; + // Snapshot of the user's original features object — preserved for + // Type 3 (Alternate Substitution) lookups that need the per-feature + // alternate index (1-based; e.g. `nalt: 2` selects the second + // alternate). Set in setFeatureOverrides. + this.userFeatures = null; } /** @@ -76,6 +81,7 @@ export default class ShapingPlan { if (Array.isArray(features)) { this.add(features); } else if (typeof features === 'object') { + this.userFeatures = features; for (let tag in features) { if (features[tag]) { this.add(tag); @@ -111,7 +117,7 @@ export default class ShapingPlan { } } else if (stage.length > 0) { - processor.applyFeatures(stage, glyphs, positions); + processor.applyFeatures(stage, glyphs, positions, this.userFeatures); } } } diff --git a/test/shaping.js b/test/shaping.js index dc005ea0..5964e14a 100644 --- a/test/shaping.js +++ b/test/shaping.js @@ -582,4 +582,23 @@ describe('shaping', function () { test('SHBALI-2/12', 'NotoSans/NotoSansBalinese-Regular.ttf', "ᬓ᭄ᭅᬸ", '23+2275|162+0|60@0,-1000+0'); }); }); + + describe('alternate substitution (GSUB Type 3) honours user-supplied alternateNumber', function () { + // FiraSans's aalt lookup carries multiple alternates per coverage entry. + // For 'A' (glyph 3) the alternate set is [1078 'ordfeminine', 764 'a.sc']. + // Without honouring the user-supplied alternateNumber, every aalt request + // resolves to the first entry — matching HarfBuzz's `aalt=2` requires + // selecting the second. + let font = fontkit.openSync(new URL('data/FiraSans/FiraSans-Regular.ttf', import.meta.url)); + + it('selects the first alternate when aalt is `true` (or 1)', function () { + let { glyphs } = font.layout('A', { aalt: true }); + assert.deepEqual(glyphs.map(g => g.id), [1078]); + }); + + it('selects the second alternate when aalt is 2', function () { + let { glyphs } = font.layout('A', { aalt: 2 }); + assert.deepEqual(glyphs.map(g => g.id), [764]); + }); + }); });