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

Fix font printing regression when metabox exists #52343

Merged
merged 1 commit into from
Jul 5, 2023
Merged

Fix font printing regression when metabox exists #52343

merged 1 commit into from
Jul 5, 2023

Conversation

hellofromtonya
Copy link
Contributor

@hellofromtonya hellofromtonya commented Jul 5, 2023 •
edited
Loading

Fixes #51209

What?

Fixes the font printing regression in the post editor when a metabox exists.

Why?

The change that introduced the regression in Gutenberg:

This was done for performance to avoid printing the @font-face styles more than once, i.e. in the main doc and in the iframe. But consideration was not given to when metabox is present, causing the editor no longer be in an iframe.

How?

Removes the if not a block theme guard around the 'admin_print_styles' hook. This means,@font-facestyles will be printed in each admin's main doc` and in each iframed editor.

Testing Instructions

  1. Install and activate the Yoast SEO plugin, which will add a metabox to each post.
  2. Change the Heading blocks to use IBM Plex Mono:
    a. Go to the Site Editor > Styles > Typography > Headings.
    b. In "Font", select "IBM Plex Mono".
    c. In "Appearance", select "Bold Italic.
    d. Click/select the "Save" button twice.

Site Editor > Styles > Typography > Headings UI

  1. Create a new post and add the following the content:
    a. Title: "Mindblowing: a blog about philosophy."
    b. Paragraph block: whatever content you want.
  2. In your favorite editor:
    a. Open its dev tools to the Network tab.
    b. Select "Fonts".
    c. Disable the cache, if available.
    d. Refresh the page.

Expectations:

IBM Plex Mono font looks like this on the Google Fonts UI page

@hellofromtonya hellofromtonya added [Type] Regression Related to a regression in the latest release [Feature] Fonts API [Feature] Typography Font and typography-related issues and PRs labels Jul 5, 2023
@hellofromtonya hellofromtonya self-assigned this Jul 5, 2023
@hellofromtonya
Copy link
Contributor Author

hellofromtonya commented Jul 5, 2023 •
edited
Loading

The failing CI job (PHPUnit tests running on PHP 5.6) is not related to this PR.

Today, WordPress Core made a change to raise the minimum PHP version to 7.0. Thus, the PHP 5.6 job is no longer needed. But changing that is out of scope for this PR.

I opened #52344 to address it.

@hellofromtonya hellofromtonya marked this pull request as ready for review July 5, 2023 19:48
@hellofromtonya hellofromtonya mentioned this pull request Jul 5, 2023
Closed
@hellofromtonya hellofromtonya removed the request for review from spacedmonkey July 5, 2023 19:49
ndiego
ndiego approved these changes Jul 5, 2023
View reviewed changes
Copy link
Member

@ndiego ndiego left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can confirm this PR fixes #51209. Fonts are loaded properly when the Editor is not iframe due to metaboxes being present (Yoast). 🙌

Gutenberg Trunk (not fixed) This PR (Fixed)
image image

@hellofromtonya hellofromtonya mentioned this pull request Jul 5, 2023
Closed
3 tasks
@hellofromtonya hellofromtonya force-pushed the fix/post-editor-font-printing-when-metabox-exists branch from c1cfa34 to 6200aaa Compare July 5, 2023 22:02
@hellofromtonya
Copy link
Contributor Author

Rebased this PR against trunk which brings in removing the PHP 5.6 CI jobs (via d9022ff).

No changes were made to this PR since @ndiego approved it.

@ironprogrammer
Copy link
Contributor

ironprogrammer commented Jul 5, 2023 •
edited
Loading

Test Report

Another confidence check! Thanks for the PR, @hellofromtonya!

Environment

  • Hardware: MacBook Pro Apple M1 Pro
  • OS: macOS 13.4.1
  • Browser: Safari 16.5.1
  • Server: nginx/1.25.1
  • PHP: 7.4.33
  • WordPress: 6.3-beta3-56130-src
  • Theme: twentytwentythree v1.1
  • Active Plugins:
    • gutenberg v16.2.0-rc.1

Actual Results

  • ✅ With patch applied, @font-face declarations are written into a <style id="wp-fonts-local"> element in the HTML document <head>, as they are in the iframed editor.

Additional Notes

  • For my test steps, I enabled the editor's "Custom fields" panel (via Options > Preferences > Panels > Additional) in order to add a metabox to the editor.
  • Also confirmed the patch works with a classic theme with a custom font set in theme.json.

@hellofromtonya
Copy link
Contributor Author

Restarted the failed e2e test. Hmm seeing a high number of e2e CI failures on Core too. What's going on? 🤔

@hellofromtonya
Copy link
Contributor Author

Awesome! Thank you @ironprogrammer and @ndiego :) Love test reports!!! As soon as the e2e CI job is happy, I'll merge this PR.

@hellofromtonya hellofromtonya merged commit 72b2b28 into trunk Jul 5, 2023
@hellofromtonya hellofromtonya deleted the fix/post-editor-font-printing-when-metabox-exists branch July 5, 2023 23:29
@github-actions github-actions bot added this to the Gutenberg 16.3 milestone Jul 5, 2023
@mikachan mikachan added the Needs PHP backport Needs PHP backport to Core label Sep 6, 2023
@mikachan mikachan mentioned this pull request Sep 6, 2023
Closed
86 tasks
@hellofromtonya hellofromtonya removed the Needs PHP backport Needs PHP backport to Core label Sep 6, 2023
@hellofromtonya
Copy link
Contributor Author

Removed Needs PHP backport label as this PR will not be backported to Core. The Fonts API is deprecated within the plugin.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ndiego ndiego ndiego approved these changes

@ironprogrammer ironprogrammer Awaiting requested review from ironprogrammer

@anton-vlasenko anton-vlasenko Awaiting requested review from anton-vlasenko

Assignees

@hellofromtonya hellofromtonya

Labels
[Feature] Typography Font and typography-related issues and PRs [Type] Regression Related to a regression in the latest release
Projects
None yet
Milestone
Gutenberg 16.3
Development

Successfully merging this pull request may close these issues.

Local font asset files not loading in post Editor when metaboxes are enabled (i.e. Editor is not iframed)
4 participants
@hellofromtonya @ironprogrammer @ndiego @mikachan