-
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
[Fonts API] Make local font asset file URL absolute #51178
Conversation
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.
Thanks for the quick fix @hellofromtonya. Everything is now working for me.
In the gif below, I have applied the IBM Plex Mono font in TT3 to all text. Then manually removing the specification in CSS reverts the text to monospace, which confirms the font is correctly getting applied.
Flaky tests detected in 6de50d6. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5146490128
|
Update: This test report is invalid for this PR. Why? It's testing with a Google Fonts Provider, not a local font. Doh. Test Report: Test editing a postEnv:
Test Instructions
Test Results✅ Playfair Display font does appear in the Network tab's font list with a 200 status 🟢 ✅ Merriweather font also appears in the Network tab's font list with a 200 status 🟢 Inspecting the iframe's HTML, the It's working as expected in my testing. |
Hmm, it's working for me in the post/page Editor with WordPress 6.2.2 and this patch. @ryandejaegher are you using 6.2.2, and are you able to test if the same issue occurs using the included fonts in Twenty Twenty-Three? |
Oh silly me, this test report is invalid as it's using a different fonts provider. Please ignore it. I'm now testing in the post editor with a local font. |
Test Report: Test editing a postEnv:
Test Instructions
Test Results✅ Source Serif Pro font does appear in the Network tab's font list with a 200 status 🟢 ✅ IBM Plex Mono font also appears in the Network tab's font list with a 200 status 🟢 Inspecting the iframe's HTML, the It's working as expected in my testing. |
@ryandejaegher It works for me too. Can you share how you're testing it including your testing environment set up such as the WordPress version, what plugins are activated, which theme, etc? That information can help us. |
Hey @hellofromtonya sorry for the delay. After playing around with plugins it looks like the issue is related to ACF ACF: 6.1.6 Tested on Safari 16.4 If I disable ACF, my fonts load correctly in the editor. |
@ryandejaegher I'm glad you found the issue on your site and that this PR is working for you too. Thank you so much for investigating and reporting back. |
Seems this PR is good to go. Thank you everyone for your contributions! I've marked this PR for 15.9.1 release. cc @c4rl0sbr4v0 |
Ensures local font asset file URLs also work in iframes.
Cherry picked for 15.9.1 |
Ensures local font asset file URLs also work in iframes.
@ryandejaegher I should have caught this earlier 🤦♂️, but the issue you experienced is not isolated to ACF. It's actually any plugin that adds metaboxes to the Editor, which causes the Editor to no longer be iframed. I have logged the issue here. |
I encountered the same issue. But it's Yoast (20.9) that prevents the fonts from loading for some reason :( |
@hellofromtonya @tellthemachines @ramonjd it does not look like this ever made it into 6.3 Beta 2. If you comment out these lines, the fonts load as expected. When testing make sure that Gutenberg is not active. |
Merge fix from Fonts API: WordPress/gutenberg#51178
@hellofromtonya I think we need advice here. My understanding was that the Font API was not going to be part of 6.3. |
Hi, @ramonjd -- this is being addressed in https://core.trac.wordpress.org/ticket/58672, which will merge this change over in a separate PR. That's correct about the Fonts API not going into this release. It's changing significantly to support the Font Library, as explained starting here: #41479 (comment). |
Ah, got it. Thanks for the clarification @ironprogrammer 🙇🏻 |
Ensures local font asset file URLs also work in iframes.
Fixes #50965
What?
Makes all local font asset file URL absolute, rather than local.
Why?
The font asset files are not being loaded within an iframe. Why? The relative URL to the asset files are relative to the iframe's #50875 was merged. By making the URLs absolute, it ensures each font asset file does properly relate to the actual location of the files.
How?
In the
WP_Fonts_Provider_Local::compile_src()
, the code for making the URLs relative is removed. In this way, all URLs should be absolute, assuming their src path was defined as absolute or it usedfile:./
placeholder (where the API handles the conversion).Testing Instructions
Before using 15.8.1
Outfit-Variable.woff2
file should be listed.Notice the font is listed using 15.8.1 - it works ✅
Before: Reproduce the bug with Gutenberg 15.9.0:
Outfit-Variable.woff2
file should be listed, but it's not. This is the bug / regression ❌ 🐞After: Bugfix
Outfit-Variable.woff2
should be listed.Expectation: The font file from the Frost theme, i.e.
Outfit-Variable.woff2
should appear in the Network tab with a 200 status 🟢