search

fonttools / fontbakery

🧁 A font quality assurance tool for everyone
https://fontbakery.readthedocs.io
Apache License 2.0
558 stars 103 forks source link

FAIL when detect legacy accent in composite glyphs #3959

Open RosaWagner opened 2 years ago

RosaWagner commented 2 years ago

What needs to be checked?

Presence of these 13 legacy accents in composite/accented glyphs: 00A8, 02D9, 0060, 00B4, 02DD, 02C6, 02C7, 02D8, 02DA, 02DC, 00AF, 00B8, 02DB (dieresis, dotaccent, grave, acute, hungarumlaut, circumflex, caron, breve, ring, tilde, macron, cedilla, ogonek)

Detailed description of the problem

Legacy accents should not be used in accented glyphs. The use of legacy accents in accented glyphs breaks the mark to mark combining feature that allows a font to stack diacritics over one glyph.

Optional fix

Use combining marks instead as a component in composite glyphs. Make sure your combining marks are having all necessary anchors; one to get attached to the base glyphs, and one for another mark to attach to this one. Legacy accents should not have anchors.

The 13 counter combining marks are: 0308, 0307, 0300, 0301, 030B, 0302, 030C, 0306, 030A, 0303, 0304, 0327, 0328 (dieresiscomb, dotaccentcomb, gravecomb, acutecomb, hungarumlautcomb, circumflexcomb, caroncomb, brevecomb, ringcomb, tildecomb, macroncomb, cedillacomb, ogonekcomb)

Resources and exact process needed to replicate

This issue follow up https://github.com/googlefonts/fontbakery/issues/3296 since it is a little bit different, but same files can be use.

Expected Profile

Please suggest in which profile should it be included. (You could inspect the Profiles's list at https://font-bakery.readthedocs.io/en/stable/fontbakery/profiles/index.html). Most common are:

Expected Result

Which log result level should it have:

Severity assessment

4

guidoferreyra commented 1 year ago

Related to #3490, maybe is useful for a rationale. At least on MacOs, when typing to get accented characters, fonts that have 0 width legacy diacritics are rendered incorrectly like in the video below.

https://github.com/googlefonts/fontbakery/assets/1755590/51d8344c-a3c4-4893-969a-69a949845df5

yanone commented 1 year ago

Thank you @guidoferreyra

I've updated the rationale like this:

Legacy accents should not be used in accented glyphs. The use of legacy accents in accented glyphs breaks
the mark to mark combining feature that allows a font to stack diacritics over one glyph.
Use combining marks instead as a component in composite glyphs.

Legacy accents should not have anchors and positive width (at least as wide as their bounding box).
They are often used independently of a letter, either as a placeholder for an expected
combined mark+letter combination in MacOS, or separately. For instance, the ´ is often used
(mistakenly) as an apostrophe, the ` is used in Markdown to notify code blocks, and ^ is used
as an exponential operator in maths.
moyogo commented 1 year ago

Considering not every font aims to support all the languages supported by their character sets, this shouldn't be universal.

A font could very well use a legacy mark as a component and have an equivalent combining mark that uses it as a component as well. This doesn't mean it is broken for what it was designed for. The font may very well be functioning for the languages it was designed for.

It should WARN in the universal profile or whatever shared profile it ends up in, may even be irrelevant to some designers, and should be FAIL in the profiles that want to enforce this.

yanone commented 1 year ago

I’m not sure I fully agree. Let's take it apart. This check emits three different FAILs:

  1. legacy-accents-component FAILs when legacy accents are detected in composites.
  2. legacy-accents-width FAILs when legacy accents are of zero width.
  3. legacy-accents-gdef FAILs when legacy accents are defined as marks in GDEF.

Given how legacy accents are used as standalone glyphs (see rationale), I do think the FAIL status 2) and 3) are warranted for universal use. 2) is self-explanatory, and 3) means the same because as soon as glyphs are defined as marks in GDEF, Harfbuzz (and possibly other shapers) enforces zero-width for all marks except some (or all) Indic vowel marks. I confirmed that with Behdad.

I agree that technically 1) could be downgraded to WARN for universal. But consider this: As soon as a glyph has _top or _bottom anchors, our production toolchain defines them as a marks in GDEF, so their width is enforced as zero by the shaper.

It's true that other font production tools may choose to not automatically define legacy marks with anchors as marks in GDEF, therefore not leaving them with zero width to be enforced by the shaper, and allow them as part of pre-composed components, and have a technically and visually functioning font.

I just wonder how important this conduct is in practice. I would find a font like that rather unusual. I'm happy to downgrade this to WARN with a FAIL override for Google Fonts if this should be an allowed conduct.

yanone commented 1 year ago

Also, a font could have no anchors whatsoever in legacy accents, with those marks be used as pre-composed components, and with legacy accents still having positive width when used standalone. This should be allowed. So I'm downgrading 1) to WARN

moyogo commented 1 year ago

I’m not sure I fully agree. Let's take it apart. This check emits three different FAILs:

1. `legacy-accents-component` FAILs when legacy accents are detected in composites.

2. `legacy-accents-width` FAILs when legacy accents are of zero width.

3. `legacy-accents-gdef` FAILs when legacy accents are defined as marks in GDEF.

@yanone Thanks for breaking it down. Sorry I was commenting on the "What needs to be checked: Presence of these 13 legacy accents in composite/accented glyphs [...]". 2 and 3 make sense to be universal FAIL. 1 legacy-accents-component by itself should not FAIL, having legacy accents as components is not Something that must be addressed for the propper functioning of the font. Sure it’s a dated practice at best, a bad practice at worst (some of these legacy accent should be designed differently than the components used in accented letters in many styles). But yes, 2 legacy-accents-width and 3 legacy-accents-gdef are and should likely FAIL.

I just wonder how important this conduct is in practice. I would find a font like that rather unusual. I'm happy to downgrade this to WARN with a FAIL override for Google Fonts if this should be an allowed conduct.

I’d say only downgrade 1 to WARN, keep 2 and 3 as FAIL.

moyogo commented 1 year ago

Also, a font could have no anchors whatsoever in legacy accents, with those marks be used as pre-composed components, and with legacy accents still having positive width when used standalone. This should be allowed. So I'm downgrading 1) to WARN

Sorry for not seeing this comment, that's the perfect use case for a WARN.

simoncozens commented 1 year ago

Felipe asked me to explain why I disagree with this check. I feel like it's a good idea but I'm still not completely convinced by the implementation.

It feels to me like we are trying to ask several different questions: "Is this font well-made?" "Will mark-to-mark anchors work on unencoded, unprecomposed glyphs?" "Does the designer know and understand the difference between legacy accents and combining accents?" These are good questions!

But what this test actually measures is a completely different question: Which glyphs do the contours in precomposed glyphs come from? And I don't really care about that question. Whether the outlines come from a combining mark glyphs or a legacy mark glyphs doesn't answer any of the good questions in the previous paragraph.

moyogo commented 1 year ago

This check reports false negatives.

The Glyphs source file of Questrial uses Gdotaccent is a composite of G+dotaccentcomb (build from anchors), dotaccentcomb with anchors is a composite of dotaccent without anchors, gftools-build flattens Gdotaccent to G+dotaccent (visually identical to G+dotaccentcomb).

The designer knew what they were doing.

khaledhosny commented 1 year ago

The test as it is now should be a source format test not a binary font test. A test for binary fonts should be structured differently and shouldn’t be concerned about components at all.

simoncozens commented 1 year ago

Yes, 100% what Khaled just said. I think we need to start identifying checks (and proposals) which are better off tested on the sources, and I'll work on them in "sourcebakery" or whatever it's going to be next year.

In the meantime, can we please disable this check? It's leading to many false FAILs.

RosaWagner commented 1 year ago

Or move it to the experimental checks until it is better formatted? Most of the designers we work with don't really know what they do when it comes to diacritics and this check is helpful to spot problems.

felipesanches commented 1 year ago

I'll work on them in "sourcebakery" or whatever it's going to be next year.

That can be done in fontbakery itself. In theory we already can have source-level checks in fontbakery, we just did not implement them yet because it was not considered a priority.

felipesanches commented 9 months ago

Yes, 100% what Khaled just said. I think we need to start identifying checks (and proposals) which are better off tested on the sources, and I'll work on them in "sourcebakery" or whatever it's going to be next year.

In the meantime, can we please disable this check? It's leading to many false FAILs.

The check was temporarily disabled now via PR #4567

felipesanches commented 9 months ago

We should also address the comments made at Discussion https://github.com/fonttools/fontbakery/discussions/4555 whenever we decide to re-enable this check.

felipesanches commented 9 months ago

At https://github.com/fonttools/fontbakery/pull/4567#issuecomment-1973590613 @khaledhosny said:

The half of the check that dealt with soacing marks being given GDEF mark class was actually useful, the other part was just noise.