Noto Sans Arabic v2.011: FontBakery QA FAIL: OS/2.usWinAscent value should be equal or greater than 1431

[eliheuer]

Font

Noto Sans Arabic v2.011

Where the font came from, and when

QA report from this PR: https://github.com/google/fonts/pull/6859

Issue

There is one FontBakery vertical metrics fail, is there a good reason to ignore this? Or should it be fixed and the PR updated before merging?

🔥 FAIL: Checking OS/2 usWinAscent & usWinDescent. (com.google.fonts/check/family/win_ascent_and_descent) 🔥 FAIL OS/2.usWinAscent value should be equal or greater than 1431, but got 1374 instead [code: ascent]

I looked around in previous PRs and issue for this font, but couldn't find any prior discussion of this.

[simoncozens]

Generally speaking the good reason for ignoring vertical metrics fails is "Don't mess about with the vertical metrics of fonts which are already onboarded unless you're really sure you're not going to break things!" But there is an argument that with USE_TYPO_METRICS on, changing the OS/2 winAscent/winDescent is not going to break things. It just depends how sure you are and how brave you are... (When it comes to large and well-used families like Noto Sans Arabic, I tend to err on the side of ~cowardice~ caution.)

[eliheuer]

I'm not 100% sure fixing this fail is the right thing to do, so why don't we leave it for now. It can always be updated in a separate PR if a vertical metrics issues comes up.

I just needed an explantation for why the fail is being ignored.

[eliheuer]

Closing this for now

[khaledhosny]

I think we should update the win metrics to avoid clipping on Windows (it will also change the line spacing in some windows apps but 🤷🏾).

[eliheuer]

@khaledhosny Ok, I reopened this issue, that would be great if you want to make the change to be equal or greater than 1431.

It's only 57 units, and I can do some testing after Simon's pull request is merged to make sure this does not introduce any problems.