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

Gasp write support broken, opentype.js cannot read the fonts it creates #738

Closed
Balearica opened this issue Jul 8, 2024 · 0 comments · Fixed by #739
Closed

Gasp write support broken, opentype.js cannot read the fonts it creates #738

Balearica opened this issue Jul 8, 2024 · 0 comments · Fixed by #739
Labels
Milestone

Comments

@Balearica
Copy link
Contributor

Support for the gasp table was added in #595, which ostensibly included both read and write support. I cannot speak to the quality of the read function, as I am not familiar with the gasp table, however the write support is definitely broken. It appears that any font file generated by opentype.js with a gasp table will be broken, and cannot be re-imported.

Code At Issue

The write function attempts to iterate over a constant (gasp.numRanges) as if it were an array. As a result, any code inside the loop will never run, and the gasp table will be broken.

for (let i in gasp.numRanges) {
result.fields.push({name: 'rangeMaxPPEM', type: 'USHORT', value: gasp.numRanges[i].rangeMaxPPEM});
result.fields.push({name: 'rangeGaspBehavior', type: 'USHORT', value: gasp.numRanges[i].rangeGaspBehavior});
}

While a broken gasp table is not a big deal per se, opentype.js currently throws an uncaught error if the gasp table fails to parse. Therefore, opentype.js cannot read the fonts it writes that include a gasp table.

Reproducible Example

This can be reproduced with any font with a gasp table, including many of the fonts in the test directory. For example, the Changa-Regular.ttf file works. Simply load the font, convert it to an array buffer with .toArrayBuffer(), and then attempt to parse that ArrayBuffer using opentype.parse.

Suggested Fixes

I think there are several different aspects to this issue. I will try and write a small patch that prevents the crashing tonight. However, I have no knowledge of or interest in the gasp table, so don't plan on working on gasp support past that point.

1. Patch the bug in the gasp write function

It looks like the author was trying to iterate over gaspRanges (an array) rather than numRanges (an integer). I will confirm this change stops the error from being thrown and submit as a PR.

2. Add automated tests for the gasp write functions

While the PR that included the gasp write function did include some tests for reading gasp tables, it included no unit tests for writing gasp tables. I think anybody adding a new feature of this nature should, at the very least, add a test to confirm that their read functions can parse the tables produced by their own write functions.

3. Prevent issues with optional tables from throwing an uncaught error

The issue with the gasp write function being broken would not be a big deal if the only impact was that the gasp table could not be re-imported. This only breaks opentype.js for as many fonts as it does because failing to parse gasp correctly throws an uncaught error, which prevents the font from being read. Wrapping optional tables in try blocks would prevent this going forward. I think this is worth pursuing even after the issue with gasp is resolved, as the chances that some parsing function has an issue in the future seem quite high, especially as opentype.js continues to add support for more and more tables (and therefore increase surface area).

Your Environment

  • Version used: e9c090e
  • Font used:
  • Browser Name and version:
  • Operating System and version (desktop or mobile):
  • Link to your project:
Balearica added a commit to scribeocr/opentype.js that referenced this issue Jul 8, 2024
Balearica added a commit to Balearica/opentype.js that referenced this issue Jul 8, 2024
@Connum Connum added this to the Release 2.0.0 milestone Jul 9, 2024
@Connum Connum added the bug label Jul 9, 2024
@Connum Connum linked a pull request Jul 9, 2024 that will close this issue
8 tasks
Connum pushed a commit to Balearica/opentype.js that referenced this issue Sep 9, 2024
Connum pushed a commit to Balearica/opentype.js that referenced this issue Sep 9, 2024
Connum pushed a commit that referenced this issue Sep 9, 2024
* Fixed gasp support bugs per #738

* Added test for gasp table writing

* Minor update to test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants