opentypejs / opentype.js

Read and write OpenType fonts using JavaScript.
https://opentype.js.org/
MIT License
4.37k stars 467 forks source link

ligature seems to not work correctly #292

Closed Pomax closed 6 years ago

Pomax commented 7 years ago

Using Zilla-Regular (otf version, found here: https://github.com/mozilla/zilla-slab/tree/master/OTF_release%20files) on a page with opentypejs shows the font "doing the right thing" with the double slash in Moz://a as a text element, but when obtained through opentype.js, shows incorrect ligature of the // sequence (there should be a ligature, but there isn't).

Screenshot of a a page with a <p>Moz://a</p> and a canvas with the same string rendered using OpenType.js:

image

Code used:

      opentype.load(ZILLA_FONT_REGULAR, function(err, font) {
        if (err) {
          return console.log(err);
        } else {
          var ctx = document.getElementById('canvas').getContext('2d');
          var path = font.getPath('Moz://a', 0, 150, 72, {
            features: true,
            kern: true
          });
          path.draw(ctx);
        }
      });

(ZILLA_FONT_REGULAR is a data-uri for the font linked to above)

fdb commented 7 years ago

It seems that this is actually a kerning value: screen shot 2017-06-06 at 20 19 46 (this is from Glyphs.app) However, looking through the font, the GPOS table is looking for lookupType 2 (pair adjustment), but the font stores lookupType 9 (extension positioning), which we do not support.

I'm not that familiar with the GPOS / GSUB structures. Maybe @fpirsch or @Jolg42 know more about this?

Jolg42 commented 7 years ago

Interesting!

It looks like the lookup 9 isn't one, it's a fake lookup to do GPOS offsets on 32-bit instead of 16-bit. It should be relatively easy to implement.

I guess the kerning here is a lookup 2 disguised as a lookup 9 (needs to be checked)

fpirsch commented 7 years ago

Hi @fdb and @Jolg42 GPOS lookup 9 and GSUB lookup 7 are actually special extended-offset tables used to break the 32-bit offset limit in big fonts. It is a container for other lookup tables.

Currently, GSUB lookup 7 support is partial (the table is parsed but not used, and not written), and GPOS lookup 9 support is totally absent.

In fact GPOS support was done long ago in a hurry and needs serious refactoring (it is very primitive and inconsistent with what is done for GSUB). I later wrote the GSUB support with this in mind: layout.js, table.js and parser.js are intended to serve both GSUB and GPOS (and GDEF if needed).

Pomax commented 7 years ago

The font used isn't actually all that big, so it's curious that this would use extended offset tables, when the font itself is fairly small. I wonder if that's something to file over on https://github.com/mozilla/zilla-slab

fdb commented 6 years ago

@Pomax not really sure where in their workflow this happens... I quickly looked and I think this happens in the UFO to OTF conversion, so it's something that happens in fonttools, I think.

Pomax commented 6 years ago

I wonder if this is a thing in the UFO instructions, or something @behdad might need filed against fonttools.

fpirsch commented 6 years ago

Looking at ZillaSlab-Regular.otf (v1.002, modified Aug 2017). Only a lookup type 2 table, so opentype definitely reads it. But it has 2 subtables which contradict themselves regarding the "//" kerning value. The 1st subtable (format 1) says to modify xAdvance by -183, the 2nd subtable (format 2) says xAdvance 0.

For some strange reason (I did it...), current GPOS implementation scans subtables backwards and reads the last value. Still, this font has a problem.

fpirsch commented 6 years ago

These tables are so complex that, not only are they a nightmare to parse and to write, but nobody seems to use them correctly :-p

behdad commented 6 years ago

Still, this font has a problem.

No. The first subtable wins. That's correct. And the font is correct.

These tables are so complex that, not only are they a nightmare to parse and to write, but nobody seems to use them correctly :-p

That couldn't be farther from truth. All operating systems and browsers read and use them just fine. By yeah, JS implementations of OpenType tend to underestimate how much work it is to implement them correctly.

fpirsch commented 6 years ago

Then why two subtables with different kerning values for the same glyph pair ? I even thought of something like subtable 2 defines global class kerning, and subtable 1 defines per-glyph exceptions to those generic classes. But no, subtable 2 defines class 92 for a single first glyph "/" and class 77 for a single second glyph "/". So why define 2 classes for a single kerning pair that is already defined in the previous subtable ?

fpirsch commented 6 years ago

OK I get it now, after looking at https://github.com/mozilla/zilla-slab/blob/master/sources/fea/kern/Regular.fea They declare a bunch of pos [ slash ] @someclasses, which define a partial row of the kerning matrix, then a bunch of pos @someclasses [ slash ] which define a partial column. Then the tooling fills the missing values of the kerning matrix with zeroes. Hence the high number of zeroes in this matrix. So the font and tools are fine, this just leaves my bug to be fixed :-p

ghenania commented 6 years ago

Hey @fpirsch, I think your intuition was right:

I even thought of something like subtable 2 defines global class kerning, and subtable 1 defines per-glyph exceptions to those generic classes.

As @behdad told you, subtable 1 overrides subtable 2. It lets you define exceptions for specific pairs within classes. So maybe you should parse subtables backward in Position.prototype.getKerningValue.

By yeah, JS implementations of OpenType tend to underestimate how much work it is to implement them correctly.

@behdad feel free to help!

Pomax commented 6 years ago

Pretty sure @behdad has his hands full with harfbuzz and fonttools already, better to expose this issue as an excellent "help wanted" bug =)

Even better to drill down into the exact part where things go wrong, without trying to fix it, and then file appropriately fine issues with associated small test fonts to fix which taken collectively resolve this particular bug. If anything, that'll also be an excellent way to also ensure there won't be any regression later.