Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GSUB lookup table type 7 subtable format 1 #486

Merged
merged 3 commits into from
Feb 26, 2023

Conversation

st0nerhat
Copy link
Contributor

@st0nerhat st0nerhat commented Aug 12, 2021

Description

In featureQuery.js:chainingSubstitutionFormat3

  • Modify var types of subtable, lookup, and substitutionType to be lets so they can be mutated in the next step.
  • Detect substitution type "71" which means lookup table type 7 and format 1.
  • When detected, mutate the substitution type and lookup method based on the linked extension subtable
  • Finally update the subtable variable to be the extension table
  • Throw an error if the chained substitution type is not handled. This will avoid fonts that use chaining substitutions from silently rendering incorrectly if they use an unhandled changed substitution.

In featureQuery.js:FeatureQuery.prototype.lookupFeature

Same changes as above.

Motivation and Context

Addresses issue #415

GSUB tables have a lookup table type 7 called the Extension Substitution Subtable. Only one format officially exists (format 1).
See the OpenType spec page.

Per the spec, Extension tables are used in fonts that use a large number of substitution subtables. Fonts for scripts where letters are connected are a situation where many substitutions are needed.

How Has This Been Tested?

So far I have tested this on 3 custom fonts in my library, one of which contains type 7 lookup tables and 2 that do not. Unfortunately I do not have the license to upload these fonts in the PR.

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I did npm run test and all tests passed green (including code styling checks).
  • I have added tests to cover my changes.
  • My change requires a change to the documentation.
  • I have updated the README accordingly.
  • I have read the CONTRIBUTING document.

@Connum
Copy link
Contributor

Connum commented Jan 14, 2022

Unfortunately I do not have the license to upload these fonts in the PR.

The tests don't use actual font files to run (see 3177c13 for example), so it would be great if you could add (a) test case(s) to your contribution!

Copy link
Contributor

@Connum Connum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

requires test(s)

@Connum Connum linked an issue Feb 5, 2023 that may be closed by this pull request
@Connum Connum added enhancement Spec Related to the implementation of the Opentype specification and removed enhancement labels Feb 5, 2023
@Connum Connum added this to the Release 2.0.0 milestone Feb 5, 2023
@Connum
Copy link
Contributor

Connum commented Feb 11, 2023

I just added a PR FilamentGames#2 to the feature branch that adds a test for this parsing feature. @st0nerhat, could you please merge that so we can get this PR merged?

@Connum
Copy link
Contributor

Connum commented Feb 13, 2023

As I provided the test, a different reviewer should make a review as well, so I tagged @ILOVEPIE

Copy link
Contributor

@ILOVEPIE ILOVEPIE left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, apart from the fact that we're still uncertain on the tests/no examples in the microsoft documentation, perhaps we should check how libfreetype handles it?

@Connum Connum mentioned this pull request Feb 14, 2023
@Connum
Copy link
Contributor

Connum commented Feb 15, 2023

Looks good to me, apart from the fact that we're still uncertain on the tests/no examples in the microsoft documentation, perhaps we should check how libfreetype handles it?

The functionality is relatively straight-forward. I tried looking at libfreetype, but I didn't know where to look exactly. In my opinion, the test that we have now should be sufficient.

@Connum
Copy link
Contributor

Connum commented Feb 26, 2023

I haven't come across a better stripped down example for a test, but I didn't come across any problems either so far, so I'm merging this now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Spec Related to the implementation of the Opentype specification
Projects
None yet
Development

Successfully merging this pull request may close these issues.

substFormat: 1 is not yet supported for Calibri Font
3 participants