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.

Sign up for GitHub

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

Jump to bottom

LibWeb/CSS: Add valid default font if no font can be found #2955

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

LibWeb/CSS: Add valid default font if no font can be found #2955

wants to merge 3 commits into from
+34 −2

Conversation

manuel-za
Copy link
Contributor

@manuel-za manuel-za commented Dec 17, 2024 •
edited
Loading

Fixes #2787

  • Before: If a font cannot be computed based on style values, e.g. if the family is incorrect (try 'lolol'), then the list of found fonts contains only the default emoji font. Text styled with an invalid font is rendered using the metrics of the emoji font (weird spacing).
  • After: The default font of the Font plugin is added to the top of an empty list of computed fonts, before the emoji font. This way, we're failing gracefully on author error, and we still display emojis :]m

@manuel-za
Copy link
Contributor Author

I was expecting my unit test to fail on macOS. Wow! It passes!!

@manuel-za manuel-za marked this pull request as ready for review December 18, 2024 03:00
@manuel-za manuel-za requested a review from AtkinsSJ as a code owner December 18, 2024 03:00
@AtkinsSJ
Copy link
Member

This looks like it solves the issue, but it's a little messy: There are now basically 2 "last resort" fonts: The one set as last resort, and this extra one appended to the fonts list.

Maybe we just shouldn't have a last_resort_font in the font list? I'm honestly not sure.

@manuel-za
Copy link
Contributor Author

manuel-za commented Dec 19, 2024 •
edited
Loading

There are now basically 2 "last resort" fonts: The one set as last resort, and this extra one appended to the fonts list.

My read:

  1. font_list contains legitimate fonts; try them in order to paint the content. Example (the way it works "after"):
    1. If a font face is found, it will be added on top of the list (what the author wanted)
    2. If a font is not found, the [legitimate] default font will be used (likely author error: specifying the font)
    3. Emoji fonts are added nonetheless (emojis may be inserted in any content)
  2. last_resort_font comes in when you don't know how to paint [portions of the text] with legitimate fonts at your disposal (in font_list). Example (the way it worked "before"):
    1. Assume text is written in emoji (incl. metrics)
    2. In case of non-compatible content ("space"), assume fragments of another font
    3. In case another compatible font is not on the list, use the "last resort" font (likely author error: font/content compatibility)

Maybe we just shouldn't have a last_resort_font in the font list? I'm honestly not sure.

Opinion:

  • There seem to be two legitimate use cases ☝️. I haven't found specs... Are there any?
  • At a high level, it seems to be what Firefox is doing: emoji content or fragments show in their emoji font (not specified by author); content in invalid font shows in their default font
  • Outstanding issue: different default fonts in Default.css and Platform::FontPlugin::the().default_font() -- serif vs. sans
    • I would replace the FontPlugin one with serif (a la Firefox) 👈 include this in the PR?

@github-actions GitHub Actions
Copy link

Your pull request has conflicts that need to be resolved before it can be reviewed and merged. Make sure to rebase your branch on top of the latest master.

@github-actions github-actions bot added the conflicts Pull request has merge conflicts that need resolution label Dec 22, 2024
@manuel-za
LibWeb/CSS: Add valid default font if no font can be found
2827b0f 
- Before: If a font cannot found based on style values, e.g. if the
         family is incorrect (try 'lolol'), then the list of found
         fonts contains only the default emoji font.
- After:  The default font of the Font plugin is added to the top of
          an empty list of computed fonts. This way, we're failing
          gracefully on author error, and we still display emojis :]m
@github-actions github-actions bot removed the conflicts Pull request has merge conflicts that need resolution label Dec 22, 2024
@manuel-za
LibWeb/CSS: Rename found_font to last_resort_font
d75a085 
The name `found_font` was misleading.

It was probably an artifact from simpler times, when only one font
could be found; before `font_list`,
@manuel-za
LibWeb/CSS: Unit test: layout text with invalid font
4045be8 
- Before: the default emoji font was picked up. The text was split
          into five fragments (3 text; 2 whitespace) and the whitespace
          was probably extended to match the font metrics.
- After: the whole text is layed out into one fragment.

NOTE:
- This test may not work on the Apple platform: expect font metrics for
  the default emoji font to be different from Ubuntu, although the
  layout should be the same.
- The test works reliably in the test runner `headless-browser -R`,
  however, it generates different layout metrics compared to running
  stand-alone in dump mode `headless-browser -d`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AtkinsSJ AtkinsSJ Awaiting requested review from AtkinsSJ AtkinsSJ is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Wrong font metrics used when font-family doesn't exist
2 participants
@manuel-za @AtkinsSJ