opentype.js
opentype.js copied to clipboard
opentypejs
Add UI names for Stylistic Set / Character Variants features
Description
When parsing the features list, the featureParams offset value is being retrieved but the FeatureParams table itself is not. With these changes, the FeatureParams table will be extracted and the uiNameId from it will be matched against the name table, and the resulting name record will be inserted into the feature definition for features ss01–ss20.
Motivation and Context
This is intended to close #399.
How Has This Been Tested?
I tested the code with several fonts that had stylistic set names assigned as well as with ones that didn't.
I ran the automated tests – I had to adjust the parse.js test to include the tableOffset parameter needed to go back to the feature table and add missing data. This implementation is not ideal but I had no better idea due to the highly functional/declarative style of this project (also, my noob skills). Open to suggestions on a better implementation here.
Screenshots (if appropriate):
Types of changes
- [ ] Bug fix (non-breaking change which fixes an issue)
- [x] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to change)
Checklist:
- [x] I did
npm run testand all tests passed green (including code styling checks). - [x] I have added tests to cover my changes.
- [ ] My change requires a change to the documentation.
- [ ] I have updated the README accordingly.
- [x] I have read the CONTRIBUTING document.
Nice!
I think it could use an example & test with a font that has Stylistic Set,
How did you test this?
One note: Not only Stylistic Sets (ss01–ss20) can have an optional UI Name ID but also the Character Variant feature (cv01–cv99). Maybe it would be good to implement this as well, once you are working on it?
The font Source Code Variabe is using both.
@Jolg42 I tested this in development on the included example page (the one served with the start script) with a few simple console.logs and by inspecting the GSUB table print on the Font Inspector page – I dragged various fonts onto the file input. Some fonts may have the feature names defined for stylistic sets while others may not. Google fonts seem to have that data stripped while the same fonts elsewhere may retain this data (e.g. Poppins on FontSquirrel – the version from Google still has the nameIds in place but the name table is brutally trimmed; this causes no issue as far as we are concerned, the uiName field is just left undefined).
I've now added automated tests for the added features. As for an example, do you have anything specific in mind? An addition to the example page or the README?
@vinuel Good point, thanks. The FeatureParams table is now being parsed for ccv01–ccv99 as well (with a separate function – the table's format is dependent on the specific feature). Only the featUiLabelNameId field is being resolved against the name table though since featUiLabelName seems to be more commonly implemented. (Or do you think all fields should be pre-extracted? If someone needs them, they can simply address the name table with the nameId that is already in place.)
@ILOVEPIE I only resolved merge conflicts in .js.map files that will be re-generated anyway, but therefore my review does not count towards allowing the merge because I'm technically the last pusher - do you want to review it as well, or shall I proceed with the merge?
@ILOVEPIE I only resolved merge conflicts in .js.map files that will be re-generated anyway, but therefore my review does not count towards allowing the merge because I'm technically the last pusher - do you want to review it as well, or shall I proceed with the merge?
We always want to have someone else review first.
@Connum quick question, when we merge this, will it affect #542 at all? Just curious.
@Connum quick question, when we merge this, will it affect #542 at all? Just curious.
No, these labels are stored with the features and independent from the platform-specific font names.
@Connum could you look over the review I did. I'll probably do a separate PR to fix this. Any other issues, or is it just the forEach loops?
Yeah, let's do the optimizations in a separate PR! Other than that, it looks good to go!
@Connum seeing as this needs a code update anyways to merge, can we change those forEach calls to for-in loops? I'll do the Parser optimization in a separate PR after this gets merged.
Ok, so the thing is that the "Preserve platform specific names in fonts" feature has broken this one. Now, all the feature labels for character variants and stylistic types are added to each of the platform names as a numeric index.
We could just take the first platform with a value for that key that we encounter, but I'm honestly not content with the memory overhead all these duplications are causing. I'm not really sure on the best way to handle this. Put the labels wit ha numeric key into a separate place? But where?
So I just read the spec and feature names can indeed be platform-specific as well, so we can't get rid of the overhead that the platform preservation has caused: https://learn.microsoft.com/en-us/typography/opentype/spec/name#naming-table-header Sooo... feature.uiName and feature.featUiLabelName should not return an object like
{el: 'απλό a', en: 'simple a', ru: 'простой а'}
but rather
{
unicode: {el: 'απλό a', en: 'simple a', ru: 'простой а'},
windows: {el: 'απλό a', en: 'simple a', ru: 'простой а'},
macintosh: {el: 'απλό a', en: 'simple a', ru: 'простой а'}
}
I guess?
that sounds good but I don't think those numeric indexes are supposed to be showing up in the names object.
that sounds good but I don't think those numeric indexes are supposed to be showing up in the names object.
Where should we store them instead?
Numeric names indexes are by the way already sotored that way, so I don't think this should be fixed in this PR.