Add support for chaining and context substitution tables#816
Conversation
fdb
left a comment
There was a problem hiding this comment.
PR Review — assisted by Claude Code
Thanks for working on this! The GSUB format 1/2 support is genuinely needed — I confirmed that Roboto Variable Font fails at render time on master with substitutionType : 62 lookupType: 6 - substFormat: 2 is not yet supported. The font parses fine (1326 glyphs), but getPaths() and getAdvanceWidth() throw, so "doesn't work" is accurate even though parsing succeeds.
That said, I have some concerns I'd like to discuss before merging. I've split them by severity.
Bug verification
Tested on master with Roboto-VariableFont_wdth,wght.ttf:
parse()— passes (1326 glyphs)getPaths('affinity', ...)— throws on lookup type 6 format 2getAdvanceWidth('Hello', ...)— throws same
So the new GSUB code paths are definitely exercised by this font.
🔴 Blocking — getAdvanceWidth() now mutates glyph objects
In src/font.mjs, the third commit adds:
if (options._applyVariationMetrics && this.variation && this.tables.hvar && glyph.advanceWidth !== undefined) {
this.variation.getTransform(glyph, options.variation);
}getTransform writes to glyph.advanceWidth and glyph.leftSideBearing as a side effect. This means calling getAdvanceWidth() permanently mutates glyph state, which makes it order-dependent and breaks the expectation that it's a read-only query. For example, calling getAdvanceWidth() before getPaths() would produce different rendering results than not calling it.
Could we compute the variation-adjusted advance width without writing back to the glyph? One approach: read the delta and apply it locally within forEachGlyph instead of mutating the shared glyph object.
🔴 Blocking — applyLookupRecords silently skips unsupported nested lookup types
The shared applyLookupRecords helper defaults allowedTypes to ['11', '12'] (single substitution only). If a chaining rule's nested lookup targets a ligature substitution (41), multiple substitution (21), or anything else, it's silently skipped with no warning.
The OpenType spec allows any lookup type inside chaining substitution lookup records. Could we at minimum emit a console.warn (or the project's equivalent) when we hit an unsupported type, rather than silently dropping it? That would save people debugging time when they hit the next font that uses a less common combination.
🔴 Blocking — componentGlyph.getPath() called purely for side effects
In src/variationprocessor.mjs:
const componentGlyph = this.font.glyphs.get(component.glyphIndex);
componentGlyph.getPath();This appears to be called just to force lazy-loading of .points. It's worth a comment explaining why, but more importantly: calling getPath() on every component glyph on every getTransform() call is potentially expensive. Is there a lighter way to ensure points are loaded?
🟡 Important — Composite glyph shift heuristic has limited applicability
The cumulative advance-width delta shifting is a reasonable approach for horizontal ligatures like "ffi", and the PR description honestly notes it "does not correct all issues." But I want to flag that for composite glyphs positioned via explicit offsets or GPOS anchors (not sequential advance-based layout), this heuristic could produce incorrect results. Would it make sense to guard this behind a check for whether the components are actually advance-based? Or at minimum add a code comment documenting the assumption?
🟡 Important — Suggestion to split into two PRs
The GSUB format support (commits 1+2) and the variation/advance-width changes (commit 3 + the composite glyph shift) are fairly independent. The GSUB work looks close to mergeable on its own. The variation changes need more design discussion around the mutation issue. Splitting would let us land the GSUB fix sooner — it unblocks Roboto and similar fonts for most use cases.
🟢 Nits
- The
getGlyphClasshelper is duplicated intest/featureQuery.spec.mjsrather than imported fromsrc/layout.mjs. Minor, but worth considering. - Test coverage for
contextSubstitutionFormat2(type52) only verifies the routing (getSubstitutionTypereturns'52'), not the actual substitution logic with mock data. The61and62tests are much more thorough — would be great to add a similar end-to-end test for52.
Summary
The core GSUB work is solid and addresses a real gap. The refactoring into shared helpers in commit 2 is appreciated. Main concerns are around the advance-width mutation side effect and the silent skipping of unsupported nested lookups. Happy to discuss any of these points!
|
Thanks for the review! I appreciate the time and effort. I removed the problematic "advance width" change from the PR, it definitely needs some more work and I need to figure out how to integrate it at the right layer. I will create a separate PR for that. 🔴 Blocking — applyLookupRecords silently skips unsupported nested lookup types The previous code throws an error when it gets an unexpected subtable type. I've now changed the code here to also always throw to keep it consistent and simple. If we can find some test fonts that use more complex arrangements here, we can enhance the logic further as needed. 🔴 Blocking — componentGlyph.getPath() called purely for side effects I removed the call to .getPath() since the .points reference below will trigger a getter that will fill in needed values. It is definitely simpler and cheaper than the full .getPath(), but there is still work hidden there. I also noticed though that the glyf.mjs buildPath() function also calls .getPath() to seemingly just to get the points filled in. In my testing it works with that call removed, but I'll leave it as is since I don't completely understand what it is trying to accomplish. As noted, I have created a separate change for the advance width work which I have also expanded to include a bunch of changes for positioning combining marks which I think will be really useful for fonts that rely on that, like Google Noto seems to. I would like to add more tests for these, but I am unclear on how to get some sample fonts that exercise different parts of the code that could be checked in along with the tests, any suggestions? 🟢 Nits The getGlyphClass function is prototype method on Layout and it felt a little awkward to try and drill down to it in the test mock. I'll add a comment in the test. I increased the coverage for the 52 substitution case and also fixed a bug in the 53 which caused it to miss the code it was trying to test. |
|
any update on this? :) |
|
Hoping for further review! I have another good PR that is intended as a follow-on to this (currently includes these changes): #822 |
|
@wiltse439 can you take a look at the conflict please? |
|
Rebased/merged with master branch |
|
Being able to author GSUB features directly in the browser would remove the need for FontTools based post-processing or server-side font compilation, which is a significant win for privacy-focused and offline-first tools. Thanks for pushing this forward. I'd be happy to test real-world contextual alternate use cases if additional feedback is useful. |
This PR makes the Google Roboto font work with opentype.js, and presumably many others.
Note that AI assistance was used in creating this PR.
Description
Motivation and Context
The chaining formats are used by newer Google variable fonts, like Roboto, and these changes allow those fonts to work correctly with opentype.js. Also, words like "affinity" were not getting rendered correctly in variable width fonts.
How Has This Been Tested?
Primary testing has been with Google fonts such as Roboto and Noto Sans using the opentype.js font test pages.
Screenshots (if appropriate):
Types of changes
Checklist:
npm run testand all tests passed green (including code styling checks).