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

Ligatures not working for specific font (LobsterTwo-Bold.otf) #590

Open
lwollnikowski opened this issue Mar 21, 2023 · 11 comments
Open

Ligatures not working for specific font (LobsterTwo-Bold.otf) #590

lwollnikowski opened this issue Mar 21, 2023 · 11 comments
Labels
Spec Related to the implementation of the Opentype specification

Comments

@lwollnikowski
Copy link

lwollnikowski commented Mar 21, 2023

Expected Behavior

Opentype.js should output text with ligatures for getPath() conversion.

Current Behavior

Result of font.getPath() call results in text which doesn't have ligatures, even if font supports them.

I explicitly request 'liga' feature in getPath() call:

font.getPath(
  htmlBlock.text, 
  0, 
  0, 
  htmlBlock.fontSize, 
  {
    letterSpacing: htmlBlock.letterSpacing,
    features: {
      'liga': true,
    }
  }
);

Release 1.3.4 converts to path silently changing ligatures to standard letters.

I tried @Connum fork of opentype.js, branch which fixes #554 (PR: #564) - this throw error "Substitution type 11 is not supported in chaining substitution". Seems logical, this branch adds check:

if (substitutionType === '12') {
  <lookup code>
} else {
  throw new Error(`Substitution type ${substitutionType} is not supported in chaining substitution`);
}

around substitution parsing.

FontForge reports this font contains mutiple definitions of 'liga' lookups:
image

To see the difference look at provided example, more informations about it in 'Steps to Reproduce' section.

Possible Solution

No idea, I'm newbie in font world :)

Steps to Reproduce (for bugs)

I provide minimal example - upper text is how browser renders this text (with ligatures), lower text is upper text converted by opentype.js conversion to path commands. Notice different letters 'd', 'u' and 'fft' ligature...

... or just look at this screenshot:
image

Context

I try to use opentype.js as a tool to convert HTML text to SVG path elements. I treat opentype.js as 'magic box' which returns appropriate drawing commands.

Your Environment

  • Version used: 1.3.4 / branch: (Handle rlig for latin script #564)
  • Font used: LobsterTwo-Bold.otf (provided in ZIP file)
  • Browser Name and version: Chromium 78 / Chrome 103
  • Operating System and version (desktop or mobile): Ubuntu 22.04 LTS
  • Link to your project: minimalexample.zip
@Connum Connum added the Spec Related to the implementation of the Opentype specification label Mar 21, 2023
@Connum
Copy link
Contributor

Connum commented Mar 21, 2023

Thanks for the report! So the issue at heart is that the only supported substitution type inside chained substitutions is '12' right now. I tried to understand the lookup-related code or at least locate the corresponding part of the OpenType spec, but I have to admit I'm at a bit of aloss here, unfortunately.

@Connum
Copy link
Contributor

Connum commented Mar 21, 2023

Here are two people who touched those parts of the code in recent years and might be able to shed more light on this and at least lead us currently active developers in the right direction: @solomancode @st0nerhat

@Connum
Copy link
Contributor

Connum commented Mar 21, 2023

If you remove the if/else statement checking for type 12, the dz and uz ligatures look like they are supposed to according to your image, but d and u then look like that no matter in which context.

if (substitutionType === '12') {
  // leave the code inside of this but remove the if/else statements
  // [...]
} else {
    throw new Error(`Substitution type ${substitutionType} is not supported in chaining substitution`);
}

@Connum
Copy link
Contributor

Connum commented Mar 21, 2023

What confuses me even more is the fact that we're not using the backtrack and lookahead at all, apart from checking their length for the contextRulesMatch constant, or am I missing sonething?

@st0nerhat
Copy link
Contributor

The exception being thrown here was added in PR #486. I believe it's having the desired effect which is crashing on an unsupported substitution instead of silently substituting incorrectly, which is what happens if the exception is commented out. Let me take a look at what substitution type '11' is.

@st0nerhat
Copy link
Contributor

Btw, totally open to changing this to a warning instead.

@st0nerhat
Copy link
Contributor

@st0nerhat
Copy link
Contributor

So yeah, this is just another gap in opentype.js's chained sequence context format 3 implementation. I don't think it would be particularly hard to add, given that it's the absolute simplest form of substitution, but nevertheless it is missing.

@st0nerhat
Copy link
Contributor

st0nerhat commented Mar 21, 2023

getLookupMethod supports 11 lookup tables amongst others, should the if statement on line 165 be removed? The logic inside there should work as is for any of formats. getLookupMethod already throws if it's passed a format that it doesn't support. @Connum I guess there is a difference between what was there before #486 where it had the if but no else, and just removing the if entirely which is what you tested. Not sure if the display you were getting is correct though.

@Connum
Copy link
Contributor

Connum commented Mar 22, 2023

Yeah, that's what I thought. However, it then always uses the replacement forms, because we're not actually checking the backtrack and lookahead context. Going on the commit message, I guess chaining substitution was never implemented correctly and it was more of a hotfix for some ligatures used in Arabic scripts.

@lwollnikowski
Copy link
Author

lwollnikowski commented Mar 24, 2023

@st0nerhat This exception is totally fine for me, even more correct when I explicitly tell opentype.js to use 'liga' feature. Font contains those ligatures but opentype.js can't use them properly in current state.

I tested more solutions to my task (text -> path conversion). One more comparison:

HTML <p> tag (correct output) :
image

Opentype.js (incorrect output, no ligatures):
image

Opentype.js without substitutionType check (incorrect output, ligatures everywhere instead of specific combinations of letters)
image

Typr.js (incorrect output, no ligatures):
image

Typr.js + HarfBuzz.js [explanation] (correct output):
image

I would call this piece of code "partially implemented" rather than "incorrect". Before contextRulesMatch there are some lookahead / backtrack calculations for Tashkeel Arabic characters, whatever they are.

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

No branches or pull requests

3 participants