Fixed metadata that enables fonts to be recognised as a family

[Lmpessoa]

I've found out that a few fields that might be passed to opentype.js were not being used while producing the font. I have been able to pinpoint those fields and where they should be applied to produce fonts that are recognised as a family. Tested only with a couple of fonts but changes were regonised by Windows and other apps.

An issue regarding the fields hhea.caretSlopeRise and hhea.caretSlopeRun for italic and oblique fonts is still pending review but does not prevent font usage or recognition as a family (caret slope had a weird inclination while testing on Word but not on Affinity Designer).

These same changes were applied to the copy of the library used by Glyphr Studio (v2) where the tests were conducted.

Description

Motivation and Context

How Has This Been Tested?

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.

[mattlag]

Having a set of font files be recognized as a family is a perennial ask we get at Glyphr Studio. Before @Lmpessoa we had no answer. Within Glyphr Studio we have implemented this fix... but it would probably be very appreciated by anyone using opentype.js who is trying to make multiple styles of a type family.

[yne]

seems like the tests needs an update.
see:

npm run test

[Connum]

seems like the tests needs an update.

I haven't found the time yet to look into this more thoroughly, but if the tests break, it's probably a breaking change. Or the new code can be adapted to prevent the error breaking the code. Looks like something that could be fixed using optional chaining.

[Connum]

Hi @Lmpessoa, could you please have a look at the failing tests?

[mattlag]

Hi @Lmpessoa and @Connum

(I am not very skilled at Git workflows, so bear with me...)

I looked at @Lmpessoa 's changes and ran the tests. It looked like to me we needed to provide a default for options.panose in case it didn't exist. I added that, and it fixed the failing tests... but then there was another test that failed, where I added the options.fsSelection check.

Now all tests are passing. But, I'm not sure where to check it in. I think I forked @Lmpessoa 's checkin and pushed it, but I don't know where it 'went' (my inexperience showing).

Anyway, my change is just to src\font.js and inserts the following code at line 105 (after the new selection checks, but before the this.tables assignment)

        if (options.fsSelection) {
            selection = options.fsSelection;
        }
        if (!options.panose || !Array.isArray(options.panose)) {
            options.panose = [0, 0, 0, 0, 0, 0, 0, 0, 0, 0];
        }

Apologies again for my inexperience, let me know if there is a way I can help out.

Update I tried to add this change as a commit to this fork... but it wasn't letting me, saying I didn't have the right permissions.

[Lmpessoa]

@mattlag, here might be the issue between my fix and the tests being run.

Pushing selection = options.fsSelection invalidates what is being done before it, which is ensuring the value of fsSelection on the table is in check with all other arguments being passed, that's why I ignore the fsSelection argument and I imagine tests are checking against this value, that's why they fail. Here I believe are the tests that have to change to reflect the fix, not the code, otherwise the fix will not work.

The part about options.panose could indeed be missing but it was supposed to be solved by other checks below (options.panose[0] || 0). Guess there might be something being asserted on the tests that is not in check with the code or maybe it's my own inexperience with JavaScript, I'm not sure, but the following would probably work best:

panose = Array.isArray(options.panose) ? option.panose : [];
while (panose.length < 9) {
  panose.push(0);
}

Perhaps would be interesting if someone else, more experienced with JavaScript and the lib itself, could take a look at it too.

[mattlag]

The tests being run come down to this object which is passed to the test, and it effects both sub-issues:

test\font.js line 22

beforeEach(function() {
    font = new Font({
        familyName: 'MyFont',
        styleName: 'Medium',
        unitsPerEm: 1000,
        ascender: 800,
        descender: 0,
        fsSelection: 42,
        tables: {os2: {achVendID: 'TEST'}},
        glyphs: glyphs
    });
});

The easier issue is the options.panose one - since there is no panose property on this test object, the test was failing. So, the check I wrote is a little overboard :-) but essentially it makes sure the panose property exists, and if it doesn't, it initializes it. Your or checks || will come into play if an array is passed, but maybe has less than 10 items.

The issue with options.fsSelection is that there is a test, based on the passed Font object above, that checks to see if fsSelection is 42:

test\font.js line 45

it('tables definition can override defaults values', function() {
    assert.equal(font.tables.os2.fsSelection, 42);
});

My idea was that we cover two cases: one, if the user does not specify fsSelection, then the code would figure out the right selection value based on italicAngle and weightClass. But, if the user did specify fsSelection directly, we would just use that. If options.fsSelection does not exist, then your new code still applies. The other option would be to just update the test to look for this.fsSelectionValues.REGULAR since thats what it would default to if the Font object was not passed a value for fsSelection.

[Connum]

then the code would figure out the right selection value based on italicAngle and weightClass

If this can be determined reliably, I think that's the way to go!

[mattlag]

@Connum or @Lmpessoa - is there an easy way to give me access to fork / update this commit? I'd be happy to make the changes mentioned above.

[Connum]

@Lmpessoa could add you as a contributor to their fork, or if they're no longer interested in being involved, you could fork it yourself, which would result in a new PR and this one being closed.

[mattlag]

I created a new pull request that include the changes that allow npm run test to pass.

[Connum]

superseded by #630