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

Ensure font-face styles are printed in iframe editors. #54313

Merged

Conversation

hellofromtonya
Copy link
Contributor

@hellofromtonya hellofromtonya commented Sep 8, 2023

What?

In the plugin, ensure wp_print_font_faces() is invoked and added to the iframed editors' styles.

Why?

Gutenberg overrides Core's _wp_get_iframed_editor_assets() styles results. Font Face's wp_print_font_faces() is in Core's _wp_get_iframed_editor_assets(). But Gutenberg overrides the results of that function in _gutenberg_get_iframed_editor_assets() (for WP 6.3) and _gutenberg_get_iframed_editor_assets_6_4() (for WP 6.4).

How?

Loads the plugin's fonts.php file and moves the hooked code for 'block_editor_settings_all' out of the guard.

Testing Instructions

  1. Set FONT_LIBRARY_ENABLE constant to true in your test site's config.php:
define( 'FONT_LIBRARY_ENABLE', true );
  1. Activate the Gutenberg plugin.
  2. Go to the Site Editor.
  3. Open your browser's dev tools and search the HTML for wp-fonts-local.

Expected: After applying this PR, it should also exist in the iframe.

Gutenberg overrides Core's _wp_get_iframed_editor_assets() styles
results. As Font Face is merged into Core, wp_print_font_faces()
was not being invoked.

This commit ensures it is invoked for printing the `@font-face` styles
in the iframed editors.
@hellofromtonya hellofromtonya removed the request for review from spacedmonkey September 8, 2023 17:01
@hellofromtonya hellofromtonya added [Type] Bug An existing feature does not function as intended Gutenberg Plugin Issues or PRs related to Gutenberg Plugin management related efforts labels Sep 8, 2023
@github-actions
Copy link

github-actions bot commented Sep 8, 2023

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.4/fonts/fonts.php
❔ lib/load.php

@ironprogrammer
Copy link
Contributor

Thanks for the PR!

Test Report

Steps to Test

  1. Configure environment to use wordpress-develop:trunk (current 6.4-alpha branch).
  2. In Appearance > Editor, navigate to Styles > Typography > Headings and set the font to IBM Plex Mono. Then Save the global styles settings.
  3. Observe that the headings display with the fallback face monospace. (This fallback may appear differently depending on your system. If unsure, use the browser inspector to override an element with font-family: monospace and note that they visually match.)
  4. Open the browser inspector and observe that the editor's iframe does not include <style id="wp-fonts-local"> element in the document's <head>. The theme's @font-face definitions are missing.
  5. Apply patch/check out PR.
  6. Refresh the editor.
  7. Observe that the headings display in the correct font. (Also refer to the IBM Plex Mono specimen page for visual confirmation.)
  8. Observe that the <style id="wp-fonts-local"> element exists in the document's <head>. The tag includes @font-face definitions for active fonts.

Environment

  • OS: macOS 13.5.1
  • Browser: Safari 16.6
  • PHP: 8.2.10
  • WordPress: 6.4-alpha-56267-src
  • Theme: twentytwentythree v1.2
  • Active Plugins:
    • gutenberg v16.6.0

Actual Results

Reproduce Issue

  • ✅ Fallback to monospace issue reproduced, Figure 1.
  • ✅ The #wp-fonts-local element is missing from the iframed document's <head>.

Test Patch

  • ✅ Heading displays with correct font face after patch, Figure 2.
  • ✅ The <head> in the iframe includes the #wp-fonts-local element, and includes the @font-face definitions.

Supplemental Artifacts

Figure 1 - before patch:
heading in monospace

Figure 2 - after patch:
heading in IBM Plex Mono

Copy link
Contributor

@ironprogrammer ironprogrammer left a comment

Choose a reason for hiding this comment

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

This looks good to me. I've got one small comment question, but it's not worth holding things up, as it'll be obvious when merging to Core.

lib/compat/wordpress-6.4/fonts/fonts.php Show resolved Hide resolved
@anton-vlasenko
Copy link
Contributor

anton-vlasenko commented Sep 11, 2023

Test Report

Environment

  • WordPress: 6.4-alpha-56267-src
  • PHP: 7.3.33
  • Server: Apache/2.4.57 (Unix) PHP/7.3.33
  • Database: mysqli (Server: 5.7.43)
  • Browser: Safari 16.6 (macOS)
  • Theme: Twenty Twenty-Three 1.2
  • MU-Plugins: None activated
  • Plugins:
    • Gutenberg

Steps to Test

  1. Activate Gutenberg.
  2. Check out the trunk branch.
  3. Navigate to the Site Editor.
  4. Open your browser's dev tools and search the HTML for #wp-fonts-local elements.
  5. Note the number of #wp-fonts-local elements.
  6. Check out this PR.
  7. Navigate to the Site Editor again.
  8. Open your browser's dev tools and search the HTML for #wp-fonts-local elements.
  9. Note the number of #wp-fonts-local elements.

Expected Results

  • ✅ After applying this PR, #wp-fonts-local should exist in the iframe.

Actual Results

  • ❌ With Gutenberg enabled, I observe a similar number of #wp-fonts-local elements regardless of whether I'm on the trunk branch or have checked out this PR. In both scenarios (i.e. step #5 vs step #9), I see 2 #wp-fonts-local elements in the root <head> element and 12 #wp-fonts-local elements within iframe elements.
    In other words, the changes in this PR do not seem to affect the quantity of #wp-fonts-local elements.

@hellofromtonya
Copy link
Contributor Author

@anton-vlasenko did you set the FONT_LIBRARY_ENABLE constant in your test site's wp-config.php file?

define( 'FONT_LIBRARY_ENABLE', true );

This constant turns on the Font Library and Font Face code in the plugin and turns off the Fonts API.

@hellofromtonya hellofromtonya enabled auto-merge (squash) September 12, 2023 14:04
@anton-vlasenko
Copy link
Contributor

Test Report

Environment

  • WordPress: 6.4-alpha-56267-src
  • PHP: 7.3.33
  • Server: Apache/2.4.57 (Unix) PHP/7.3.33
  • Database: mysqli (Server: 5.7.43)
  • Browser: Safari 16.6 (macOS)
  • Theme: Twenty Twenty-Three 1.2
  • MU-Plugins: None activated
  • Plugins:
    • Gutenberg

Steps to Test

  1. Set the FONT_LIBRARY_ENABLE constant to true in your test site's config.php.
  2. Activate Gutenberg.
  3. Check out the trunk branch.
  4. Navigate to the Site Editor.
  5. Open your browser's dev tools and search the HTML for #wp-fonts-local elements.
  6. Note the number of #wp-fonts-local elements.
  7. Check out this PR.
  8. Navigate to the Site Editor again.
  9. Open your browser's dev tools and search the HTML for #wp-fonts-local elements.
  10. Note the number of #wp-fonts-local elements.

Expected Results

  • ✅ After applying this PR, #wp-fonts-local should exist in the iframe.

Actual Results

  • ✅ After applying this PR, #wp-fonts-local elements are present in the frame elements on the page.
    There were no #wp-fonts-local elements in the iframe elements before applying this patch.

Copy link
Contributor

@anton-vlasenko anton-vlasenko left a comment

Choose a reason for hiding this comment

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

After retesting the PR with the FONT_LIBRARY_ENABLE constant set to enabled, it functions as intended: #54313 (comment)

LGTM!

@hellofromtonya hellofromtonya merged commit 0cf7898 into trunk Sep 12, 2023
52 of 54 checks passed
@hellofromtonya hellofromtonya deleted the fix/load-font-faces-into-iframe-editor-for-6.4-override branch September 12, 2023 21:15
@github-actions github-actions bot added this to the Gutenberg 16.7 milestone Sep 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Gutenberg Plugin Issues or PRs related to Gutenberg Plugin management related efforts [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants