-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Font Library: fixes installed font families not rendering in the editor or frontend. #59019
Conversation
…amily values according to CSS spec
…f the fonts from font collections
…re that CSS properties are valid
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress. If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged. If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack. Thank you! ❤️ View changed files❔ lib/compat/wordpress-6.5/fonts/class-wp-font-collection.php ❔ lib/compat/wordpress-6.5/fonts/class-wp-font-utils.php |
Size Change: -164 B (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
Flaky tests detected in f85650a. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7906532926
|
It seems to be a difference on the quotes needed in Chrome vs Firefox, I'll add a fix for that. |
Chromium-based browsers (Chrome, Edge, Brave, etc) and WebKit browsers (Epiphany, Safari, etc) seem to be working in the same way: Font faces loaded by JS should not have quotes in their names. Gecko browsers (Firefox) are the exception to this. they need quotes on the font face names loaded by JS. I added a conditional to cover both cases and a test assertion based on the user agent here: 9758385 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, @matiasbenedetto! The fix works as expected. Tested in Chrome and Firefox. Code looks good to me, left a couple nits.
Screen.Recording.2024-02-14.at.19.16.31.mov
output = output.replace( /^["']|["']$/g, '' ); | ||
|
||
// Firefox needs the font name to be wrapped in double quotes meanwhile other browsers don't. | ||
if ( window.navigator.userAgent.toLowerCase().includes( 'firefox' ) ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Should we check for gecko
to make it more broad and cover not only Firefox but all gecko browsers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that Firefox on iOS uses the Safari/Webkit engine (as do all iOS browsers, as currently required by Apple), so I'm guessing it will need the same treatment as Safari (no quotes), I'll see if I can test it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call; I didn't know that. I'm unable to test on iOS, but if it's not Gecko engine, we should not include it among the exceptions. I removed that from the conditional in this PR: #59037
Reference: https://firefox-source-docs.mozilla.org/overview/ios.html
.replace( /^["']|["']$/g, '' ); | ||
} | ||
// removes leading and trailing quotes. | ||
output = output.replace( /^["']|["']$/g, '' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: We could think of abstracting this replacement in a function as we use it in different places.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, I removed one of the calls that was redundant: 8abd359
Co-authored-by: Juan Aldasoro <252415+juanfra@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@matiasbenedetto I left several code comments for things that may come up in Core review, in case you want to address them now.
@creativecoder I added all the nits you pointer in this PR: #59103 |
There is already a ticket, see https://core.trac.wordpress.org/ticket/60537 |
The ticket was recently closed stating it was reported upstream. |
@nith53 Well, yes, but a ticket is still required for adding this PR here into core :) |
Reopened for now. Thank you for the clarification. |
…or or frontend. (#59019) * Improves the sanitize_font_family function to output CSS valid font-family values according to CSS spec * add font-family css specific sanitization for fontFamily properties of the fonts from font collections * use _wp_to_kebab_case to format the font slug property of fonts from font collections * format comment * improving fontFontFamily and add formatFontFaceName functions to ensure that CSS properties are valid * load font face in both iframe and document * use the wordpress function to generate slugs for font files uploads * lint variable names * add exception for firefox in the font face name formatting * format php * improve php check * format php * replace firefox by gecko to cover all gecko engine browsers Co-authored-by: Juan Aldasoro <252415+juanfra@users.noreply.github.com> * remove not needed repeated call * using firefox and fxios to detect firefox browser user agent --------- Co-authored-by: Juan Aldasoro <252415+juanfra@users.noreply.github.com> Co-authored-by: matiasbenedetto <mmaattiiaass@git.wordpress.org> Co-authored-by: juanfra <juanfra@git.wordpress.org> Co-authored-by: arthur791004 <arthur791004@git.wordpress.org> Co-authored-by: richtabor <richtabor@git.wordpress.org> Co-authored-by: pbking <pbking@git.wordpress.org> Co-authored-by: getdave <get_dave@git.wordpress.org> Co-authored-by: okmttdhr <okat@git.wordpress.org> Co-authored-by: nith53 <nithins53@git.wordpress.org>
I just cherry-picked this PR to the cherry-pick-beta-2 branch to get it included in the next release: de9eb98 |
…or or frontend. (#59019) * Improves the sanitize_font_family function to output CSS valid font-family values according to CSS spec * add font-family css specific sanitization for fontFamily properties of the fonts from font collections * use _wp_to_kebab_case to format the font slug property of fonts from font collections * format comment * improving fontFontFamily and add formatFontFaceName functions to ensure that CSS properties are valid * load font face in both iframe and document * use the wordpress function to generate slugs for font files uploads * lint variable names * add exception for firefox in the font face name formatting * format php * improve php check * format php * replace firefox by gecko to cover all gecko engine browsers Co-authored-by: Juan Aldasoro <252415+juanfra@users.noreply.github.com> * remove not needed repeated call * using firefox and fxios to detect firefox browser user agent --------- Co-authored-by: Juan Aldasoro <252415+juanfra@users.noreply.github.com> Co-authored-by: matiasbenedetto <mmaattiiaass@git.wordpress.org> Co-authored-by: juanfra <juanfra@git.wordpress.org> Co-authored-by: arthur791004 <arthur791004@git.wordpress.org> Co-authored-by: richtabor <richtabor@git.wordpress.org> Co-authored-by: pbking <pbking@git.wordpress.org> Co-authored-by: getdave <get_dave@git.wordpress.org> Co-authored-by: okmttdhr <okat@git.wordpress.org> Co-authored-by: nith53 <nithins53@git.wordpress.org>
What?
Fixes installed font families not rendering in the editor or frontend.
This is an alternative fix #58853
Why?
Fixes: #58765
Fixes: #59010
How?
It follows the recommendations from the CSS spec: https://www.w3.org/TR/css-fonts-4/#font-family-prop
theme.json
presets renderer uses that format)Testing Instructions
Test with Firefox, Chrome and Safari
Use font faces that demonstrated to produce errors i.e.: Abril Fatface, Rock 3D, DotGothic16, Slabo 27px, Baloo 2, etc.
Screenshots or screencast
Screencast of the fix working:
2024-02-14.11-25-45.mp4