opentype.js icon indicating copy to clipboard operation
opentype.js copied to clipboard

Handle rlig for latin script

Open Connum opened this issue 2 years ago • 5 comments

Description

This handles the rlig feature for latin scripts.

Motivation and Context

Despite being enabled for latin scripts even by default, rlig ligatures were only handled for Arabic script.

I did some minor refactoring of the ligature application to diminish code duplication betweeen arabic and latin script ligatures and the new rlig implementation. They now use the same method in a new src/features/commonFeatureUtils.js file, without breaking the existing file/module structure, with parameters for the script, feature and range. This might come in handy as we progress to implement new font features. Speaking of which, we should probably overhaul feature handling as we do so. This PR only enables rlig for the latin script, which fixes #554 (and fixes #479), while other scripts might need it as well and there are other features that might not be script-specific.

How Has This Been Tested?

Tested with the Pecita font provided in #479 and added corresponding tests for it (the font is under SIL Open Font License so I was able to use it directly for the tests), not breaking any other tests.

Screenshots (if appropriate):

before: image after (note the "qu" and "ck" shapes): image The rest of the differences to how it should look according to #479 can be attributed to not supporting the calt feature.

Types of changes

  • [X] 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:

  • [X] I did npm run test and 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.

Connum avatar Feb 14 '23 17:02 Connum

This might conflict with #535 and #557. Which should we merge first, one of those two PRs reworks the feature system (can't remember which one).

ILOVEPIE avatar Feb 14 '23 21:02 ILOVEPIE

@Connum do we want to merge this first or rework it once #535 and #557 are merged?

ILOVEPIE avatar Feb 17 '23 00:02 ILOVEPIE

Good question... Both of them made changes to bidi.js which I also altered, and both of them could probably benefit from the common helper function that I introduced. I think it's easier to merge those two first and have me adapt the rlig code to those changes instead of heaving to incorporate mine into both of the PRs.

Connum avatar Feb 17 '23 09:02 Connum

up-to-date with current master, blocked by #535 and #557

Connum avatar Feb 27 '23 08:02 Connum

I somehow managed to mess up this branch... Going to convert to draft until I find time to fix this.

Connum avatar Nov 30 '23 23:11 Connum