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

WARN Caps are averagely centred — v metrics check #4139

Closed
Tracked by #3307
RosaWagner opened this issue May 19, 2023 · 13 comments
Closed
Tracked by #3307

WARN Caps are averagely centred — v metrics check #4139

RosaWagner opened this issue May 19, 2023 · 13 comments
Assignees
Labels
GF's priority list List of high priority issues for google/fonts CI New check proposal We expect new check proposals to include a detailed rationale description and a suggested check-id Profile: Google Fonts

Comments

@RosaWagner
Copy link
Contributor

RosaWagner commented May 19, 2023

capsHeight - hhea/typoAscender ~= - hhea/typoDescender (this is only recommended for fonts with Latin, Greek or Cyrillic as "main" script, I would go for allowing an error margin of 5% of UPM)

We also had the experience with Braah One: google/fonts#6227
So also in case of non-caps fonts, we could have a similar rule.

@RosaWagner RosaWagner changed the title WARN Caps are averagely centred : capsHeight - hhea/typoAscender ~= - hhea/typoDescender (this is only recommended for fonts with Latin, Greek or Cyrillic as "main" script, I would go for allowing an error margin of 5% of UPM) WARN Caps are averagely centred May 19, 2023
@RosaWagner RosaWagner added New check proposal We expect new check proposals to include a detailed rationale description and a suggested check-id Profile: Google Fonts labels May 19, 2023
@RosaWagner RosaWagner changed the title WARN Caps are averagely centred WARN Caps are averagely centred — v metrics check May 19, 2023
@RosaWagner RosaWagner added the GF's priority list List of high priority issues for google/fonts CI label May 19, 2023
@eliheuer
Copy link
Collaborator

This would be a great check, there is a twitter thread going over the rationale in detail here: https://twitter.com/romanshamin_en/status/1562801657691672576

@eliheuer
Copy link
Collaborator

I made a pull request for a new check covering this issue here: #4226

@tphinney
Copy link

tl;dr: you might want to do more testing and investigation before implementing that check?

I just want to point out that there is a big assumption behind this, which I am very skeptical of: that all relevant environments people care about, do vertical centering of text using the same algorithm.

I am especially a bit suspicious of the implied claim that em-square as a distance from the origin is one of the metrics in question. I would also be curious as to WHICH descender metric is relevant, or whether that might vary as well.

In the case of Google’s own Flutter web framework, it appears to rely on sTypo metrics including sTypoAscender, and also possibly on… the baseline (?!). But I did not get enough feedback to be sure what was going on. See
google/material-design-icons#1500

@eliheuer
Copy link
Collaborator

@tphinney Thanks for the feedback! I made a codepen here to try showing why this is a real issue: https://codepen.io/eliheuer/pen/KKrOamg

This kind of vertical centering works on the Web (Chrome) which I would argue is the most relevant environment. And to me it also just intuitively makes sense that you would want to center things this way.

This is only a WARN, so no one is forced to do this. But maybe this is a good best practice if you don't have any reason to do otherwise?

Here is Martian Mono, which is perfectly centered (Top: 42px, Bottom: 42px):

Screenshot 2023-08-13 at 10 07 07 PM

Here is Shantell Sans, which is NOT perfectly centered, but within the threshold to pass this test (Top: 46px, Bottom: 46px):

Screenshot 2023-08-13 at 10 22 28 PM

Here is Amiri, which is off by enough that it would get a WARN, note that this is visibly not centered (Top: 54px, Bottom: 62px):

Screenshot 2023-08-13 at 10 21 45 PM

@tphinney
Copy link

Clarification: I was writing about Roman Shemin’s Twitter thread and his version of the vertical metrics check, which is significantly different than what Rosa proposed. Which one are you doing?

@eliheuer
Copy link
Collaborator

eliheuer commented Aug 14, 2023

I tried to implement the check as described by Rosalie, including the error margin of 5%.

    upm = ttFont["head"].unitsPerEm
    error_margin = upm * 0.05
    cap_height = ttFont["OS/2"].sCapHeight
    ascender = ttFont["hhea"].ascent
    descender = ttFont["hhea"].descent
    top_margin = ascender - cap_height
    difference = abs(top_margin - abs(descender))

    vertically_centered = difference <= error_margin

@felipesanches
Copy link
Collaborator

I understand that the approach described on the Roman Shemin’s twitter thread can be useful. But isn't it too opinionated to expect that all fonts in the world should follow the same approach?

In other words, is this proposal meant to result in a WARN-level universal profile check offering a recommendation for this specific (and optional) approach for vertical metrics? Is everybody here OK with including this recommendation on the universal profile?

@m4rc1e
Copy link
Collaborator

m4rc1e commented Aug 14, 2023

I don't like the use of sCapHeight since it's often set incorrectly. I don't mind this check being included but only as a warn. It's definitely something that should only apply to fonts intended for UI and only include scripts which have similar proportions as Rosa mentioned (Amiri shows exactly why this doesn't work for most Arabic + Latin fonts).

@khaledhosny
Copy link
Contributor

I feel like whatever proposed here is overly Anglo-centeric. You don’t even need Arabic, try some accented text and the perfectly centered fonts are no longer centered. This feel like trying to solve a non-font issue in fonts, since centering depends on the text and the container and it is something should be handled in CSS not in fonts.

@felipesanches
Copy link
Collaborator

I agree with @khaledhosny. To me this proposed check looks like trouble.

@felipesanches
Copy link
Collaborator

The in-person conversations at TypeCon 2023 convinced me it is a reasonable check.

@tphinney
Copy link

Yeah, as long as it is Rosalie’s version and not Roman Shemin’s it is… not crazy. 😛

felipesanches pushed a commit that referenced this issue Aug 20, 2023
If possible, the uppercase glyphs should be vertically centered in the em box.

**com.google.fonts/check/caps_vertically_centered**
Added to the Universal profile.
(issue #4139)
@eliheuer
Copy link
Collaborator

This issue is a great example of this problem: mui/material-ui#29965

felipesanches pushed a commit to emmamarichal/fontbakery that referenced this issue Oct 11, 2024
"If possible, the uppercase glyphs should be vertically centered in the em box."

'caps_vertically_centered'
On the Universal Profile.

(issues fonttools#4139 and fonttools#4274)
felipesanches pushed a commit to emmamarichal/fontbakery that referenced this issue Oct 11, 2024
"If possible, the uppercase glyphs should be vertically centered in the em box."

'caps_vertically_centered'
On the Universal Profile.

(issues fonttools#4139 and fonttools#4274)
felipesanches pushed a commit that referenced this issue Oct 11, 2024
"If possible, the uppercase glyphs should be vertically centered in the em box."

'caps_vertically_centered'
On the Universal Profile.

(issues #4139 and #4274)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GF's priority list List of high priority issues for google/fonts CI New check proposal We expect new check proposals to include a detailed rationale description and a suggested check-id Profile: Google Fonts
Projects
None yet
Development

No branches or pull requests

7 participants