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

Add noto fonts to the Docker image #42730

Merged
merged 1 commit into from
Dec 7, 2023
Merged

Add noto fonts to the Docker image #42730

merged 1 commit into from
Dec 7, 2023

Conversation

jgraham
Copy link
Contributor

@jgraham jgraham commented Oct 24, 2023

This ensures that we have fallback glyphs for most/all characters. This affects some tests
e.g. css/css-fonts/size-adjust-unicode-range-system-fallback.html (see https://bugzilla.mozilla.org/show_bug.cgi?id=1860124)

Covering all of unicode does mean that we end up with a rather large increase in the docker image size. If this causes problems we could just install fonts-noto-cjk to fix the known breakage.

@gsnedders
Copy link
Member

c.f. #13203

We probably want full runs of Firefox and Chrome to compare results, because this will change results.

@gsnedders
Copy link
Member

https://packages.ubuntu.com/jammy/fonts-noto for those curious what is included in the fonts-noto meta package.

This ensures that we have fallback glyphs for most/all
characters. This affects some tests
e.g. css/css-fonts/size-adjust-unicode-range-system-fallback.html (see
https://bugzilla.mozilla.org/show_bug.cgi?id=1860124)

Covering all of unicode does mean that we end up with a rather large
increase in the docker image size. If this causes problems we could
just install fonts-noto-cjk to fix the known breakage.
@jgraham
Copy link
Contributor Author

jgraham commented Nov 16, 2023

@jgraham
Copy link
Contributor Author

jgraham commented Nov 16, 2023

@gsnedders
Copy link
Member

I think the things I'm most concerned about are https://wpt.fyi/results/?q=seq%28chrome%3Apass%20chrome%3Afail%29%20edge%3Apass&run_id=5167628194152448&run_id=5110012550053888&run_id=5196165500370944 (i.e., tests that have regressed in Chrome, but which currently pass in Edge—which we have for years ran with much larger font coverage).

  • /css/css-content/quotes-* seems mostly to be anti-aliasing differences,
  • /css/css-fonts/cjk-kerning.html I don't know what the cause might be, especially given it is a testharness test so we don't see what the output is,
  • /css/css-text/white-space/control-chars-* seems to be the most significant change here.

@jfkthame
Copy link
Contributor

/css/css-text/white-space/control-chars-* seems to be the most significant change here.

I suspect this a genuine Chrome bug. The CSS Text spec says that control characters (that are not actually acting as such, like tab or newline) should render a visible glyph, not be invisible.

Last I checked, Blink didn't ensure this; it just rendered the characters with whatever the current font is, which may result in a .notdef tofu box, if that's what the font contains, but may equally well result in nothing at all, if the font maps the control characters to invisible glyphs (or indeed has an invisible .notdef).

So it sounds like Chrome is simply using a Noto font where these control characters are invisible, which fails the tests that explicitly check for CC visibility.

See also w3c/csswg-drafts#8261

@jgraham
Copy link
Contributor Author

jgraham commented Nov 20, 2023

https://wpt.fyi/results/?diff&filter=ADC&run_id=5085224079196160&run_id=5072232675540992 is the diff for Firefox with this change plus installing ffmpeg, which should hopefully not affect any of the same tests.

There are a couple of possible regressions, but overall we end up passing more tests.

Copy link
Member

@gsnedders gsnedders left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This, and the Chrome and Firefox results, look generally good.

@foolip
Copy link
Member

foolip commented Nov 30, 2023

@DanielRyanSmith can you review this for Chrome?

Copy link
Contributor

@DanielRyanSmith DanielRyanSmith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm no expert here, but I am in agreement that there are no glaring issues with the Chrome results. 😅

@jgraham jgraham merged commit 00dee68 into master Dec 7, 2023
58 of 60 checks passed
@jgraham jgraham deleted the noto_fonts branch December 7, 2023 10:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants