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

feat: Add Font Fallback #1833

Closed
wants to merge 10 commits into from

Conversation

ahmed-novalabs
Copy link
Contributor

@ahmed-novalabs ahmed-novalabs commented May 28, 2022

Should fix #1771, #499.
Is part of #933, #856.

@ahmed-novalabs ahmed-novalabs changed the title Add Font Fallback feat: Add Font Fallback May 28, 2022
@diegomura
Copy link
Owner

Thanks @ahmed-novalabs for working on this! Can you provide a test plan for this PR?

If I understood correctly, user will need to provide a unicodeRange to each font definition in the form of a regex. I find this a bit odd, in the sense that if feels like it's exposing something to the user that should be possible to compute inside. Isn't it? Most people doesn't know what unicode range the font they are using covers, so by exposing this API I feel many will be confused. Thoughts?

@ahmed-novalabs
Copy link
Contributor Author

ahmed-novalabs commented May 30, 2022

@diegomura I think the main benefit is optimization by only loading the used fonts based on the text and unicodeRange.
The behavior should be to load all the used fonts that don't have a unicodeRange and only load fonts with unicodeRange if they are used and the regex matches at least one char in the text.

There is a similar CSS property.
I am not sure if there is a better way to achieve this.

For the testing plan, I don't have a lot of experience with testing, so I am not sure how to make or approach that. If you can provide an example or a guide of what you expect, I can work on it.

While resolving assets, the first call to load the font will await loading the font correctly, while subsequent calls to load the font will finish before the font is loaded. This sometimes leads to characters not rendering correctly.
@ahmed-novalabs
Copy link
Contributor Author

What's in this PR

  • Added unicodeRange to the font object to support loading only needed fonts
  • Added support for multiple fonts per <Text/> by modifying the font substitution stage to split the font by , and going through the list of fonts one by one till a font that supports the current glyph is found.

Things to test:

  • Pass multiple fonts in the form of Font1, Font2 with characters that aren't supported by the first font and see if the character is rendered in Font2 correctly. (Also with multiple languages)
  • Set a limited unicodeRange for a font and try rendering the document once with at least one character that matches the range and another time with no characters that match the range. The font should be loaded the first time, but not the 2nd time.
  • Try rendering text with one font, and then adding more text that needs another font to be rendered and update using the update function in React and see if it works correctly (Causes a weird bug).
  • Test in node.js

@ghost
Copy link

ghost commented Aug 17, 2022

Any update on this?

@diegomura @ahmed-novalabs

@ghost
Copy link

ghost commented Aug 22, 2022

@ahmed-novalabs Is there specific reason to go with new parameter fontStack than allowing fontFamily to be a string or array?

@chasovskiy
Copy link

Any updates on this PR?

@ahmedwalid05
Copy link

I lost access to the ahmed-novalabs account.

@chathu-novade I am not sure I understand the question, fontStack is an internal variable that holds the font stack, it's not for use by the user.
The user can just pass multiple fonts using the fontFamily attribute just like HTML.
e.g.:

fontFamily: "MyFont1, MyFont2"

@nikgraf
Copy link
Contributor

nikgraf commented Feb 7, 2024

@diegomura the unicode-range is a standard CSS feature https://developer.mozilla.org/en-US/docs/Web/CSS/@font-face/unicode-range. For example in one project we use it to make sure all numbers are rendered with the monospaced version of the font for better readability.

That said it already would be very helpful to allow to support multiple fonts to support various glyphs e.g. arabic, cyrillic. Most fonts offer dedicated fonts for arabic and so on, because apparently there is a limit of 64K glyphs for a TrueType file (notofonts/noto-fonts#13 (comment)). So you can get Noto Sans and Noto Sans Arabic. Combining multiple fonts is one solution, but at some point you hit the 64K limit again.

The easiest going forward afaik would be to allow defining multiple fonts e.g.
fontFamily: "NotoSans, NotoSansArabic" or fontFamily: ["NotoSans", "NotoSansArabic"]. If an arabic glyph is not found in NotoSans it would fall back to NotoSansArabic.

If you think this is a good idea I would be happy to extract this part from the PR and create a new one.

@diegomura
Copy link
Owner

@nikgraf i think it is!

@nikgraf
Copy link
Contributor

nikgraf commented Feb 13, 2024

great, will work on it in the coming days

@nikgraf
Copy link
Contributor

nikgraf commented Feb 14, 2024

PR is ready: #2640

@nikgraf
Copy link
Contributor

nikgraf commented Mar 15, 2024

@diegomura wanted to ask if you could review the PR 🔝

@diegomura
Copy link
Owner

We already support this. Closing this

@diegomura diegomura closed this Sep 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Font not rendering special characters
5 participants