Make WordPress Core

Opened 11 months ago

Closed 11 months ago

Last modified 11 months ago

#58672 closed defect (bug) (fixed)

Font asset files not loading within iframed editor

Reported by: ironprogrammer's profile ironprogrammer Owned by: audrasjb's profile audrasjb
Milestone: 6.3 Priority: normal
Severity: normal Version: 6.3
Component: Editor Keywords: has-patch has-testing-info has-screenshots gutenberg-merge
Focuses: Cc:

Description (last modified by ironprogrammer)

The editor may now be iframed in 6.3 unless metaboxes are present, so paths in @font-face definitions in the iframed editor cannot be relative. There was a fix for this (GB PR 51178) merged to Gutenberg, but the Fonts API has undergone significant changes such that it was not included in 6.3.

Change History (14)

This ticket was mentioned in PR #4759 on WordPress/wordpress-develop by @ironprogrammer.


11 months ago
#1

  • Keywords has-patch added

Merge fix from Fonts API: https://github.com/WordPress/gutenberg/pull/51178. To summarize:

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 https://github.com/WordPress/gutenberg/pull/50875 was merged. By making the URLs absolute, it ensures each font asset file does properly relate to the actual location of the files.

Trac ticket: https://core.trac.wordpress.org/ticket/58672

#2 @ironprogrammer
11 months ago

  • Keywords needs-testing has-testing-info has-screenshots gutenberg-merge added

Testing Instructions

For PR #4759.

Setup

  • In a clean install of 6.3-beta2 or from trunk (6.3-beta2-56100-src).
  • The Gutenberg plugin deactivated.

Steps to Reproduce and Test Patch

Perform the following steps in:

  • Site Editor: In Appearance > Editor, activate the editor, and open ◐ Styles > Typography > Headings.
  • Post Editor: In Posts with a new or existing post, add a Heading block and use the Typography > Fonts picker. (You may need to activate the font picker through the three vertical dots to the side of Typography.)
  1. Set the Font to "IBM Plex Mono".
  2. 👀 Observe that the displayed font is the fallback monospace (Figure 1).
  3. 👀 Open the browser's inspector tool and observe that in the editor's iframed document, the style#wp-block-library-inline-css element's @font-face definitions use relative paths, e.g. /wp-content/themes/twentytwentythree/assets/fonts/ibm-plex-mono/IBMPlexMono-Regular.woff2.
  1. 🩹 Apply patch.
  1. Repeat Steps 2-3:
  2. 👀 In Step 2, observe that the displayed font is "IBM Plex Mono" (Figure 2).
  3. 👀 In Step 3, observe that the font file paths are absolute, e.g. http://wp-src.test/wp-content/themes/twentytwentythree/assets/fonts/ibm-plex-mono/IBMPlexMono-Regular.woff2.

Expected Results

  • ✅ After patch, paths to font files should be absolute, and render the correct face in the iframe.

Supplemental Artifacts

Figure 1: Fallback monospace font (on macOS Ventura).

https://cldup.com/yeT1JITorS.png

Figure 2: "IBM Plex Mono" font (specimen at Google Fonts).

https://cldup.com/rAvlimsMXk.png

Additional Information

Note that font faces displayed on the site frontend, as well as in the post editor with meta boxes active, should continue to display as expected. In these contexts the editor is not iframed, so the URLs should work before (relative) and after (absolute).

(Meta boxes can be enabled in a vanilla install from the post editor by clicking the three vertical dots > Preferences > Panels > Additional, then toggling on "Custom fields". Click the panel's displayed reload button to apply the change.)

#3 @ironprogrammer
11 months ago

  • Description modified (diff)

@ndiego commented on PR #4759:


11 months ago
#4

This works as expected. Thanks @ironprogrammer 💪

#5 @antonvlasenko
11 months ago

Test Report

This report validates that the indicated patch addresses the issue.

Patch tested: https://github.com/WordPress/wordpress-develop/pull/4759

Environment

  • WordPress: 6.3-beta2-56100-src
  • PHP: 8.0.27
  • Server: Apache/2.4.55 (Unix) PHP/8.0.27
  • Database: mysqli (Server: 5.7.38 / Client: mysqlnd 8.0.27)
  • Browser: Safari 16.5 (macOS)
  • Theme: Twenty Twenty-Three 1.1
  • MU-Plugins: None activated
  • Plugins: None activated

Steps to test the patch

Please see https://core.trac.wordpress.org/ticket/58672#comment:2.

Actual Results

  • ✅ Issue resolved with patch.

Additional Notes

  • I didn't notice any fallback to the monospace font.
  • The patch seems to fix the issue for both iframed and non-iframed fonts.

Supplemental Artifacts

https://cldup.com/-cVcoDK68i.png
https://cldup.com/cxnhahqtH2.png

#6 @shuvo247
11 months ago

Test Report

This report validates that the indicated patch addresses the issue.

Patch tested: https://github.com/WordPress/wordpress-develop/pull/4759

Environment

  • OS: macOS 11.7.7
  • Web Server: Nginx
  • PHP: 7.4.33
  • WordPress: 6.3-beta3-56130-src
  • Browser: Google Chrome 114.0.5735.198
  • Theme: Twenty Twenty-Three
  • Active Plugins:
    • No Plugins Activated

Actual Results

  • ✅ Issue resolved with patch.

Supplemental Artifacts

Before : https://prnt.sc/iaFZgPdbjG5i
After : https://prnt.sc/VDq3C3DtdTb9

#7 @robinwpdeveloper
11 months ago

  • Keywords needs-unit-tests added

As per the conversation in [6-3-release-leads](https://wordpress.slack.com/archives/C051Z1SKBDZ/p1688390041427749) I am adding needs-unit-tests keyword.

Also it got enough test reports. I think we can remove needs-testing keyword.

Version 0, edited 11 months ago by robinwpdeveloper (next)

This ticket was mentioned in Slack in #core-test by robinwpdeveloper. View the logs.


11 months ago

#9 @oglekler
11 months ago

  • Keywords needs-testing removed

Ticket was discussed during the Test team meeting and it was agreed that because it needs unit tests it is not ready and needs-testing should be removed.

Props to @robinwpdeveloper

#10 @antonvlasenko
11 months ago

In my opinion, this PR may not necessarily need unit tests for the following reasons:

  1. $fn_compile_src is not a public API, so it would be problematic to write unit tests for that function; $fn_compile_src is not accessible from outside (it's an anonymous function);
  2. The code changes made in this PR are considered "harmless." The suggested PR simply removes the code that removes the http* prefix from the src property of a font. I am having difficulty understanding what exactly a unit test for this should cover.
Last edited 11 months ago by antonvlasenko (previous) (diff)

#11 @audrasjb
11 months ago

  • Keywords needs-unit-tests removed

#12 @audrasjb
11 months ago

  • Owner set to audrasjb
  • Status changed from new to reviewing

#13 @audrasjb
11 months ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 56139:

Editor: Fix font paths in iframed editor.

By making font asset files URLs absolute instead of relative, this ensures each font asset file does properly relate to the actual location of the files.

See https://github.com/WordPress/gutenberg/pull/50875.

Props ironprogrammer, antonvlasenko, ndiego, shuvo247, robinwpdeveloper.
Fixes #58672.

Note: See TracTickets for help on using tickets.