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

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

Conversation

RickyDaMa
Copy link
Contributor

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
Copy link
Collaborator

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
Copy link
Collaborator

khaledhosny commented Nov 20, 2024

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 RickyDaMa force-pushed the fix-dropped-contextual-ligature-mark-attachment branch from f86ea2b to 03d831f Compare November 21, 2024 16:35
@RickyDaMa
Copy link
Contributor Author

RickyDaMa commented Nov 21, 2024

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
Copy link
Member

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

@behdad
Copy link
Collaborator

behdad commented Nov 22, 2024

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
Copy link
Collaborator

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.

Make attachments for mark2base and mark2liga in the same pass
Coerce numberless liga anchors to mark2base
@RickyDaMa RickyDaMa force-pushed the fix-dropped-contextual-ligature-mark-attachment branch from 03d831f to f0798ae Compare November 25, 2024 13:27
@RickyDaMa
Copy link
Contributor Author

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 RickyDaMa marked this pull request as ready for review November 25, 2024 17:37
@RickyDaMa
Copy link
Contributor Author

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

@khaledhosny khaledhosny merged commit f8a01e5 into googlefonts:main Nov 28, 2024
7 checks passed
@RickyDaMa RickyDaMa deleted the fix-dropped-contextual-ligature-mark-attachment branch November 28, 2024 14:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants