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

Some SVG image previews are clipped or not rendered legibly in GB 17.6 #18

Closed
ironprogrammer opened this issue Feb 2, 2024 · 10 comments · Fixed by #20
Closed

Some SVG image previews are clipped or not rendered legibly in GB 17.6 #18

ironprogrammer opened this issue Feb 2, 2024 · 10 comments · Fixed by #20

Comments

@ironprogrammer
Copy link

This is an updated list (see WordPress/gutenberg#54688) of default font collection SVGs that are not rendered correctly or are clipped, as of Gutenberg 17.6.

chenla
coiny
concert-one
content
cutive
englebert
khmer
liu-jian-mao-cao
material-icons
material-icons-outlined
material-icons-round
material-icons-sharp
material-icons-two-tone
noto-color-emoji
noto-emoji
noto-emoji
noto-kufi-arabic
noto-sans-arabic
noto-sans-lycian
noto-sans-myanmar
noto-sans-phags-pa
noto-sans-signwriting
noto-serif-myanmar
revalia
siemreap
updock
Expand for a list of source SVGs...
https://s.w.org/images/fonts/17.6/previews/chenla/chenla.svg
https://s.w.org/images/fonts/17.6/previews/coiny/coiny.svg
https://s.w.org/images/fonts/17.6/previews/concert-one/concert-one.svg
https://s.w.org/images/fonts/17.6/previews/content/content.svg
https://s.w.org/images/fonts/17.6/previews/cutive/cutive.svg
https://s.w.org/images/fonts/17.6/previews/englebert/englebert.svg
https://s.w.org/images/fonts/17.6/previews/khmer/khmer.svg
https://s.w.org/images/fonts/17.6/previews/liu-jian-mao-cao/liu-jian-mao-cao.svg
https://s.w.org/images/fonts/17.6/previews/material-icons/material-icons.svg
https://s.w.org/images/fonts/17.6/previews/material-icons-outlined/material-icons-outlined.svg
https://s.w.org/images/fonts/17.6/previews/material-icons-round/material-icons-round.svg
https://s.w.org/images/fonts/17.6/previews/material-icons-sharp/material-icons-sharp.svg
https://s.w.org/images/fonts/17.6/previews/material-icons-two-tone/material-icons-two-tone.svg
https://s.w.org/images/fonts/17.6/previews/noto-color-emoji/noto-color-emoji.svg
https://s.w.org/images/fonts/17.6/previews/noto-emoji/noto-emoji.svg
https://s.w.org/images/fonts/17.6/previews/noto-emoji/noto-emoji.svg
https://s.w.org/images/fonts/17.6/previews/noto-kufi-arabic/noto-kufi-arabic.svg
https://s.w.org/images/fonts/17.6/previews/noto-sans-arabic/noto-sans-arabic.svg
https://s.w.org/images/fonts/17.6/previews/noto-sans-lycian/noto-sans-lycian.svg
https://s.w.org/images/fonts/17.6/previews/noto-sans-myanmar/noto-sans-myanmar.svg
https://s.w.org/images/fonts/17.6/previews/noto-sans-phags-pa/noto-sans-phags-pa.svg
https://s.w.org/images/fonts/17.6/previews/noto-sans-signwriting/noto-sans-signwriting.svg
https://s.w.org/images/fonts/17.6/previews/noto-serif-myanmar/noto-serif-myanmar.svg
https://s.w.org/images/fonts/17.6/previews/revalia/revalia.svg
https://s.w.org/images/fonts/17.6/previews/siemreap/siemreap.svg
https://s.w.org/images/fonts/17.6/previews/updock/updock.svg
@pbking
Copy link
Collaborator

pbking commented Feb 5, 2024

I did a spot check for these across releases and there's nothing special about the 17.6 release; these previews seem to have had issues since the first collection stored in this repository (16.7).

@pbking
Copy link
Collaborator

pbking commented Feb 5, 2024

For Chenla (and perhaps others, though not all that are in question) there aren't glyphs to represent the characters we use to generate the preview.

Indeed when "Chenla" is used as a font MOST of the glyphs are rendered using a backup system font. This is why the preview "looks ok" if you use the Font Collection that doesn't include previews (shown below with the glyphs being rendered in 'Times'):

image

I'm not sure that it's appropriate to include fonts that don't cover basic glyphs. My suggestion for that scenario is to filter them out during the "preview generation" process.

@pbking
Copy link
Collaborator

pbking commented Feb 6, 2024

Two issues are at play:

As noted, some fonts (Chenla at least) don't have alpha glyphs (they do have numerical glyphs).

Those fonts shouldn't be included. I believe that we should be inspecting the fonts when we generate the font collection JSON file to determine that any font included has a minimum collection of glyphs.

Secondly the SVGs that are being generated (via text-to-svg) are being rendered with HEIGHT values that are too small. I'm not sure why it's too small, but the value is calculated like this:

  getHeight(fontSize) {
    const fontScale = 1 / this.font.unitsPerEm * fontSize;
    return (this.font.ascender - this.font.descender) * fontScale;
  }

It works for most fonts, but for a handfull of fonts the value is too small and they are shown cut off.
image

Removing the height attribute fixes it. That MIGHT be a fix, but I'm not sure how the previews would render without a height value in the SVG preview...

I can't figure out why the height is calculated incorrectly.

It might be best to (for now) include an "exclusion list" of fonts that we don't want to be included in the collection.

Alternately (or additionally) we could have a list of fonts for which previews aren't generated. This would fix BOTH issues as the backup font would be used for font faces with missing glyphs and the preview would render that way, and the previews that are rendered poorly would be excluded and the browser would do a better job.

@colorful-tones
Copy link
Member

@ironprogrammer @matiasbenedetto do you think that WordPress/gutenberg#54688 captures the visibility and work that needs to be done and we can likely remove this item from the WordPress 6.5 Editor Tasks? It is slightly confusing to have an issue from an external GitHub repo (WordPress/google-fonts-to-wordpress-collection) on the board, and it would be ideal to get progress updates and general triaging against a single source for the release team. 🤔

@getdave
Copy link

getdave commented Feb 12, 2024

@colorful-tones It seems like this is effecting the quality of a feature slated for 6.5, as without accurate previews the feature is somewhat compromised.

Therefore I think it's valid to have this Issue tracked on the 6.5 board.

I'd like to understand if there's been any progress towards a solution. It sounds like a complex problem to which it would be ideal to have:

  • a PR for a fallback solution - maybe having a hardcoded constant of fonts to be excluded
  • a PR for an optimal solution - fixing the rendering

@pbking pbking linked a pull request Feb 12, 2024 that will close this issue
@pbking
Copy link
Collaborator

pbking commented Feb 12, 2024

I forgot to link my PR to this issue, but the "band-aid" solution I propose is to have an excluded list and not render previews for a collection of known offenders.

#20

@pbking
Copy link
Collaborator

pbking commented Feb 12, 2024

I have looked into fixing the rendering issue but couldn't come up with a clean solution. I've tinkered with it well enough to mostly understand what the preview generation is doing, but not why the calculated height isn't correct.

Part of the solution is also not rendering previews for font faces that don't have all the necessary glyphs. That's a little more straightforward, the font face can be inspected to determine it has at least alpha-numeric glyphs before rendering a preview SVG.

@colorful-tones
Copy link
Member

colorful-tones commented Feb 13, 2024

Based on the outcome of the recent async Editor Triage session (#core-editor Slack link) - I'm going to move this into the In Progress column on the WordPress 6.5 Editor Tasks board.

Note: there are no labels applied to this issue, because it can not be labeled, because it exists in a separate wordpress/google-fonts-to-wordpress-collection repo.

@getdave
Copy link

getdave commented Feb 28, 2024

@pbking Are we looking to land the "band aid" solution in #20?

It would need to be in WP 6.5 RC 1 next week 🙏

@getdave
Copy link

getdave commented Feb 29, 2024

Good to see this one wrapped up 👏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
4 participants