googlefonts / ufo2ft

A bridge from UFOs to FontTools objects (and therefore, OTFs and TTFs).
MIT License
152 stars 43 forks source link

Fix ligatures with unnumbered contextual anchors having their mark attachment dropped #890

Closed RickyDaMa closed 2 days ago

RickyDaMa commented 1 week ago

If a ligature glyph has a contextual anchor whose name doesn't have a number, it's silently dropped currently. This PR instead sets that anchor number to 1 to be able to keep it around

Previously when using glyphsLib's own implementation of the ContextualMarkFeatureWriter (glyphsLib v6.7.1 tested), this was not an issue and the mark attachment worked as expected

Leaving as draft as to confirm this is an okay/safe approach to fixing the problem (maybe some assertions should be added to ensure this isn't clobbering another anchor?). Happy to add unit testing and/or a reproducer as desired

cc @belluzj @jackmcDaMa (who helped me track this down)

khaledhosny commented 1 week ago

glyphsLib’s ContextualMarkFeatureWriter didn’t support ligatures at all and everything was assumed to be base glyphs. GlyphsApp does not support contextual ligature anchors as well, but I don’t know what it does in the case of a non-ligature contextual anchor over a ligature.

khaledhosny commented 1 week ago

Another thing to consider: what happen to non-ligature non-contextual anchors on a ligature glyph (e.g. top on an f_i glyph), do we drop it or assume the component number is one or add it to mark2base lookup? It might be good idea to be consistent in treatment here between contextual and non-contextual anchors.

RickyDaMa commented 1 week ago

As discussed, here's a refactored approach that 'coerces' contextual numberless ligature anchors to mark2base, consistent with the behaviour of non-contextual anchors

I had to change the function from operating in two passes / being called twice (one for mark2base, one for mark2liga) to producing both at once in order to do this

If we like this approach I'll update/add tests

If the type hints feel like clutter I can remove them, but I'm a fan personally

anthrotype commented 1 week ago

the question is, does it work when shaping to have the ligature glyph in a markToBase lookup?

behdad commented 1 week ago

the question is, does it work when shaping to have the ligature glyph in a markToBase lookup?

HarfBuzz has no issue with that. I don't know about other systems.

khaledhosny commented 1 week ago

It works or not depending on what the type designers/font engineer intentions are. Say we have a a_a ligature with a mark2base anchor, the input sequence a a dotabovecomb and a dotabovecomb a will render the same (i.e. the dot will attach at the same place in the ligature). This may or may not be the intended output.

RickyDaMa commented 5 days ago

Okay I've rewritten this PR a couple of times now, but there's the reference I used for the latest version, which passes the existing tests locally (which is what I wanted, as I don't believe they cover numberless ligature anchors)

image

Bolded are the cells changed

I'll now look to add tests to cover the edge case that I've changed the behaviour for

Feel free to code golf me on the implementation of _makeContextualAttachments, I just tried to keep it readable with a couple of inner functions so that the main conditionals were as readable as possible

RickyDaMa commented 2 days ago

Any further refinements I need to make here, or is this all good to get merged?