Make WordPress Core

Opened 5 months ago

Last modified 5 days ago

#63568 accepted defect (bug)

WP_Font_Face: Font names that contain single quotes are not wrapped in double quotes

Reported by: wildworks's profile wildworks Owned by: audrasjb's profile audrasjb
Milestone: 7.0 Priority: normal
Severity: normal Version: 6.4
Component: Editor Keywords: has-patch has-test-info has-screenshots has-unit-tests needs-testing dev-feedback
Focuses: ui Cc:

Description

Originally reported in GB 70426.

The WP_Font_Face::build_font_face_css method checks the font name and wraps it in double quotes if it contains spaces. However, if the font name contains a single quote, it will be erroneously detected as already "wrapped" and the wrapping process will not be performed.

As a result, the correct CSS will not be generated, and the font will not be applied:

<style class='wp-fonts-local'>
@font-face{
  font-family:O'Reilly Sans;
}
</style>

This code should be:

<style class='wp-fonts-local'>
@font-face{
  font-family:"O'Reilly Sans";
}
</style>

On the other hand, CSS variable values ​​are correctly wrapped in double quotes. This is because it is properly handled by the WP_Font_Utils::maybe_add_quotes method:

<style id='global-styles-inline-css'>
:root{
  --wp--preset--font-family--oreilly-sans: "O'Reilly Sans"
}
</style>

The solution should probably be one of the following:

  • Make the logic of the build_font_face_css method the same as the maybe_add_quotes method.
  • Deprecate the build_font_face_css method and use the maybe_add_quotes method instead.

Testing Instructions

  1. Open the site editor.
  2. Click on the Styles button in the top right.
  3. Typography > Fonts section > click the Manage fonts button
  4. Click the Upload tab
  5. Upload the O-Reilly-Sans-Regular.ttf font file attached to this ticket. This font is Savate font, an open-licensed font published on Google Fonts, and I changed the font name to "O'Reilly Sans" using FontForge.
  6. Close the font modal.
  7. Elements section > Text > Typography section
  8. Select the "O'Reilly Sans" font from the Font dropdown.
  9. The font will be applied throughout the site editor, most likely because the font is being loaded in JS and not via PHP.
  10. Click the save button and reload your browser. The font should no longer be applied.
  11. The font should no longer be applied on the front end.

Attachments (3)

O-Reilly-Sans-Regular.ttf (170.5 KB) - added by wildworks 5 months ago.
Savate font, but the font name has been changed to "O'Reilly Sans" using FontForge
O-Reilly-Sans-Regular-double-quotes-included.ttf (170.3 KB) - added by wildworks 2 months ago.
Savate font, but the font name has been changed to O"Reilly Sans using FontForge (contains double quotes)
O-Reilly-Sans-Regular-double-quotes-included-inspector.png (66.0 KB) - added by wildworks 2 months ago.
"O-Reilly-Sans-Regular-double-quotes-included.ttf" file information. The font name contains double quotes.

Download all attachments as: .zip

Change History (51)

@wildworks
5 months ago

Savate font, but the font name has been changed to "O'Reilly Sans" using FontForge

#1 @audrasjb
5 months ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 6.9
  • Owner set to audrasjb
  • Status changed from new to accepted
  • Version set to 6.4

Thanks for the Trac report, I'm able to reproduce the issue using the provided font.

Introduced in [56012] / WP 6.4.

I moved this to milestone 6.9 but I wouldn't be opposed to ship it in the next report, depending on how self-contained and straightforward is the proposed patch.

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


5 months ago
#2

  • Keywords has-patch added; needs-patch removed

I have created a patch for the following Trac ticket:
Trac ticket: https://core.trac.wordpress.org/ticket/63568

Details for the bug reproduction can be found here.

In this patch, the quotes are added by using the logic of maybe_add_quotes method in the build_font_face_css function.

The comparison of input and output before the patch and after the patch is:
https://github.com/user-attachments/assets/937a3473-9721-4e23-a04b-14ecb4ee01f7

Here is the before patch (bug reproduction video):-
https://github.com/user-attachments/assets/d93736c0-d410-46da-84fa-38900f843471

After patch (test report video):-
https://github.com/user-attachments/assets/4a610657-5c09-4de1-92a2-f161c9910812

@wildworks commented on PR #8982:


5 months ago
#3

Thanks for the PR!

I'm thinking it might be better to expose the maybe_add_quotes method rather than duplicating it. Because the method belongs to a utility class.

That way, it should be available in the form of WP_Font_Utils::maybe_add_quotes().

@sandeepdahiya commented on PR #8982:


5 months ago
#4

I'm thinking it might be better to expose the maybe_add_quotes method rather than duplicating it. Because the method belongs to a utility class.

To me, this seems better choice as well without duplication/repeatations.

@wildworks commented on PR #8982:


5 months ago
#5

Thanks for the update! I think we can commit this PR once the PHP lint errors are fixed.

@jonsurrell commented on PR #8982:


5 months ago
#6

It seems good to always process the provided font family name, however I wonder whether we should improve on maybe_add_quotes to something like normalize_quoted_font_family_name.

I believe there are a number of edge cases that are unhandled here. Most of these are edge cases and not likely to appear in popular font names, however it seems ideal if the system behaves well under any conditions. For example, what happens if the string contains " instead of '?

This section of the CSS standard is interesting and relevant, in particular the notes:

means most punctuation characters and digits at the start of each token must be escaped in unquoted font family names.
if you really have a font whose name is the same as one of the <generic-family> names, or the system font names, or the CSS-wide keywords, it must be quoted.

I'd like to consider always using the quoted form of the font family name. This way they are handeled as CSS strings, and handling a few characters should ensure that font names are correctly represented in the resulting CSS.

A quoted form of the name seems ideal where some escaping inside the quoted CSS string would be necessary, namely:

  • Change \ to \\
  • Change the quote character to its escaped form, so if " is the quote used then " inside the string becomes \".
  • Newlines should become \A .

---

I'm not familiar with the fonts API so this may be problematic in some ways I'm unaware of, but from the CSS perspective this should be more robust.

@jonsurrell commented on PR #8982:


5 months ago
#7

My comment is not intended to block this PR which seems to be an incremental improvement.

@wildworks commented on PR #8982:


5 months ago
#8

@sirreal Thanks for the feedback!

It seems good to always process the provided font family name, however I wonder whether we should improve on maybe_add_quotes to something like normalize_quoted_font_family_name.

I think this is a nice suggestion. This method was previously private, it should be safe to change the method name.

I believe there are a number of edge cases that are unhandled here.

This is true. There are also no unit tests and they will need to be added, but I think those can be addressed in a follow-up.

@sandeepdahiya commented on PR #8982:


5 months ago
#9

I believe there are a number of edge cases that are unhandled here. Most of these are edge cases and not likely to appear in popular font names, however it seems ideal if the system behaves well under any conditions. For example, what happens if the string contains " instead of '?

Yes,  there’s zero downside to being more defensive here.

I think this is a nice suggestion. This method was previously private, it should be safe to change the method name.

I agree, this is a good suggestion. I've gone ahead and added some edge cases as mentioned above by @sirreal and renamed the method to normalize_quoted_font_family_name since it better reflects what the function is doing now.

@wildworks commented on PR #8982:


5 months ago
#10

@iamsandeepdahiya Thanks for the update. If we change the logic of the method itself, I feel like we need unit testing to prevent regressions. This method is very hard to test manually.

@wildworks commented on PR #8982:


5 months ago
#11

My recommendation would be to land the simpler fix and then create a new PR with the additional improvements.

I agree with this suggestion. I think it would be better to make only minimal changes here and address the logic changes and adding unit tests in a follow-up.

The feedback left by @sirreal will be useful when creating a follow-up PR. Looking at the feedback, it seems that there are many edge cases that need to be tested carefully 😅

@sandeepdahiya commented on PR #8982:


5 months ago
#12

I agree with this suggestion. I think it would be better to make only minimal changes here and address the logic changes and adding unit tests in a follow-up.

I have reverted the last commit as discussed and the recent changes have been undone. The branch is now back to the previous state. Please let me know if any further adjustments are needed. ✅

#13 @rollybueno
5 months ago

  • Keywords needs-testing added

#14 @rollybueno
5 months ago

  • Focuses ui added
  • Keywords has-test-info has-screenshots added

Test Report

Description

This report validates whether the indicated patch works as expected.

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

Environment

  • WordPress: 6.9-alpha-60093-src
  • PHP: 8.2.28
  • Server: nginx/1.27.5
  • Database: mysqli (Server: 8.4.5 / Client: mysqlnd 8.2.28)
  • Browser: Chrome 136.0.0.0
  • OS: Linux
  • Theme: Twenty Twenty-Five 1.2
  • MU Plugins: None activated
  • Plugins:
    • Test Reports 1.2.0

Actual Results

  1. ✅ Issue resolved with patch.
  • Custom font named "O'Reilly Sans" is now correctly wrapped in double quotes in the generated @font-face CSS: https://i.imgur.com/HBERAlO.png
  • The font remains applied after refreshing the Site Editor and is correctly rendered on the frontend.

Additional Notes

  • With the patch applied, the quoting issue is fixed, resulting in valid CSS output.

Supplemental Artifacts

Video recording: https://www.awesomescreenshot.com/video/41086843?key=c613daad2490e76793493416500628f5

Script: https://i.imgur.com/UFhMNNT.png

#15 @rollybueno
5 months ago

Leaving the needs-testng keywords for more testing from other contributors, not sure if we need unit-test though. I'll leave that open for discussions

#16 @rollybueno
5 months ago

Okay update:

Out of cusriosity, I have cloned the font renamed it to include more single quote on the name. The result is the same.

  • Font correctly wrapped in double quote
  • Font working as expected on the front-end

https://i.imgur.com/aT8VLPN.png

@jonsurrell commented on PR #8982:


5 months ago
#17

Please don't wait for additional review from me, I'm not familiar with the font system and am satisfied with @t-hamano's approval.

#18 @jonsurrell
5 months ago

For work to improve normalize_css_font_family_name, I want to link PR 7857 / #62653. That PR has remaining work to be done, but contains some CSS parsing methods (WP_CSS_Selector_Parser_Matcher::parse_string) and related tests that may be interesting for the implementation or tests.

#19 @SirLouen
5 months ago

  • Keywords reporter-feedback added; needs-testing removed

Reproduction Report

Description

❌ This report can't validate that the issue can be reproduced.

Environment

  • WordPress: 6.9-alpha-60093-src
  • PHP: 7.4.33
  • Server: nginx/1.27.5
  • Database: mysqli (Server: 8.4.5 / Client: mysqlnd 7.4.33)
  • Browser: Firefox 139.0
  • OS: Windows 10/11
  • Theme: Twenty Twenty-Five 1.2
  • MU Plugins: None activated
  • Plugins:
    • Test Reports 1.2.0

Reproduction Steps

  • Following the ones provided in the OP

Actual Results

  1. ❌ Error conditions is not occurring, check screencast

Additional Notes

@wildworks it seems that there has been some validation already, but I can't seem to validate this, following your steps. Can you check my screencast and give me some feedback on what I'm missing? Maybe an environment difference?

Supplemental Artifacts

Screencast. https://f003.backblazeb2.com/file/wordpress-videos/wp-videos/2025/06/63568.mp4

#20 follow-up: @wildworks
5 months ago

@SirLouen Thanks for the testing.

After further testing, I found this test to be very unstable.

My guess is that this is an issue, possibly with file uploads.

After following the steps I have shown you, check the source code on the frontend - the font family itself should be defined correctly.

<style class='wp-fonts-local'>
<!-- Some other fonts definisions -->
@font-face{font-family:"O'Reilly Sans";font-style:normal;font-weight:400;font-display:fallback;src:url('http://localhost:8889/wp-content/uploads/fonts/O-Reilly-Sans-Regular.ttf') format('truetype');}
</style>

Try accessing http://localhost:8889/wp-content/uploads/fonts/O-Reilly-Sans-Regular.ttf, which is indicated by this definition. In my environment, for some reason, the font could not be found.

I used the Font Manager plugin to monitor whether the font file was actually uploaded. Strangely, the uploaded font may disappear immediately. See the video below:

https://i.gyazo.com/a10898228bc4290f0b77442a5449de7e.mp4

Here is what I discovered:

  • This problem does not occur with common font files that do not contain single quotes.
  • After repeatedly deleting and updating the font, the font becomes available for upload.
  • This problem also occurs in trunk.

In conclusion, I think this behavior is either a niche bug related to the font file upload process that has existed in trunk, or there is some mistake in the font I prepared.

Either way, this patch does not change anything in the font file upload process, so I think it is safe.

#21 in reply to: ↑ 20 @SirLouen
5 months ago

Replying to wildworks:

Try accessing http://localhost:8889/wp-content/uploads/fonts/O-Reilly-Sans-Regular.ttf, which is indicated by this definition. In my environment, for some reason, the font could not be found.

I've tried this, but I can find the font
https://f003.backblazeb2.com/file/wordpress-videos/wp-videos/2025/06/63568-2.mp4

https://i.gyazo.com/a10898228bc4290f0b77442a5449de7e.mp4

It's so weird how the font disappears from your disk. I've also tried the same protocol
https://f003.backblazeb2.com/file/wordpress-videos/wp-videos/2025/06/63568-3.mp4

But it doesnt dissapear from my disk.

Btw? Have you tried reproducing with WP Playground? We cannot access the filesystem but you can see visually the result if its expected or not.

#22 follow-up: @wildworks
5 months ago

@SirLouen

Thanks for the test. The disappearance of the font file may be a problem with my environment.

I tested it again using Playground, but I was unable to reproduce the problem you are encountering. I used the following two browsers:

  • Chrome 137.0.7151.119
  • Firefox 139.0.4

On the front end, could you make sure the following code is present in your source? If they are all present after running the test steps correctly, the font should be applied correctly.

<style class='wp-fonts-local'>
@font-face{
  font-family:"O'Reilly Sans";
  font-style:normal;
  font-weight:400;
  font-display:fallback;
  src:url('/path/to/wp-content/uploads/fonts/O-Reilly-Sans-Regular.ttf') format('truetype');
}
</style>

<style id='global-styles-inline-css'>
:root{
  --wp--preset--font-family--oreilly-sans: "O'Reilly Sans";
}
body{
  font-family: var(--wp--preset--font-family--oreilly-sans);
}
</style>

#23 in reply to: ↑ 22 ; follow-up: @siliconforks
5 months ago

Replying to wildworks:

Thanks for the test. The disappearance of the font file may be a problem with my environment.

What is your environment, exactly? Are you running npm run dev, with the file watcher?

I've noticed before that npm run dev will sometimes (not always and not very reproducibly, but sometimes) delete uploaded files.

#24 in reply to: ↑ 23 ; follow-up: @wildworks
5 months ago

Replying to siliconforks:

Are you running npm run dev, with the file watcher?

That's right. I've tried it in various environments, but the problem seems to occur when running the wordpress-develop repository by running npm run dev. This issue may need to be investigated in more depth in a separate ticket.

#25 in reply to: ↑ 24 ; follow-up: @siliconforks
5 months ago

Replying to wildworks:

That's right. I've tried it in various environments, but the problem seems to occur when running the wordpress-develop repository by running npm run dev. This issue may need to be investigated in more depth in a separate ticket.

I just created ticket #63605 for this issue.

#26 in reply to: ↑ 25 ; follow-up: @SirLouen
5 months ago

What I don't really get is how this patch is going to solve #63605
After all the font disappears after uploading. It won't be never loaded regardless of the code changes.
I feel that the code is addressing another problem with another reproduction instructions right?
These instructions?

Last edited 5 months ago by SirLouen (previous) (diff)

#27 in reply to: ↑ 26 ; follow-up: @wildworks
5 months ago

Replying to SirLouen:

This patch (This PR) does not solve #63605. The problem of disappearing font files is not related to this patch. This patch just wraps the font family name with proper double quotes so that the font-face is generated correctly.

However, it is true that disappearing font files make it difficult to test this patch accurately.

#28 in reply to: ↑ 27 ; follow-up: @siliconforks
5 months ago

Replying to wildworks:

However, it is true that disappearing font files make it difficult to test this patch accurately.

I believe npm run build:dev is the same thing as npm run dev but without running the file watcher? So it should be possible to use that as a workaround to avoid running into #63605.

There's also npm run build which gives you a production build (in the build/ directory) instead of a development build (again without running the file watcher).

#29 in reply to: ↑ 28 @SirLouen
5 months ago

  • Keywords reporter-feedback removed

Test Report

Description

✅ This report validates that the indicated patch works as expected.

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

Environment

  • WordPress: 6.9-alpha-60093-src
  • PHP: 8.2.28
  • Server: nginx/1.27.5
  • Database: mysqli (Server: 8.4.5 / Client: mysqlnd 8.2.28)
  • Browser: Chrome 137.0.0.0
  • OS: Windows 10/11
  • Theme: Twenty Twenty-Five 1.2
  • MU Plugins: None activated
  • Plugins:
    • Test Reports 1.2.0

Testing Instructions

  1. Loading the font proposed here, as commented in OP instructions
  2. Set the font as default Text font
  3. Go to the front end and check the code. Look for the name of the font. In this case, "Reilly Sans". You might spot it in two places: in the local fonts, and in the CSS vars (check supplemental artifacts)
  4. The CSS var is correctly set with double quotes
  5. 🐞 The local font lacks of the double quotes

Expected Results

  • Ideally, the local fonts should also display the font within double quotes

Actual Results

  1. ✅ With the patch the font is displayed with double quotes, check supplemental artifacts.

Additional Notes

Replying to siliconforks:

Replying to wildworks:

However, it is true that disappearing font files make it difficult to test this patch accurately.

I believe npm run build:dev is the same thing as npm run dev but without running the file watcher? So it should be possible to use that as a workaround to avoid running into #63605.

There's also npm run build which gives you a production build (in the build/ directory) instead of a development build (again without running the file watcher).

Let me recap

The problem here is that originally, the font was not being shown to @wildworks. This was because the font was disappearing and not correctly set with double quotes, 50/50.
Then I checked the bug, and it was working for me (I always run npm run build:dev because I rarely work with JS files live), so the font was showing for me for some reason.

But now I see that with the patch, the font displays consistently both in the front end and in the back end (when it's not deleted!). This was my confusion. And as @wildworks suggested, this patch adds a degree of correctness.

✅ So I think that this patch is safe to ship

Supplemental Artifacts

Global Styles

https://i.imgur.com/nZUl5ju.png

Fonts Local Before patch

https://i.imgur.com/TLTU3Fj.png

Fonts Local After patch

https://i.imgur.com/28P0XZP.png

#30 @wildworks
5 months ago

  • Keywords commit added

#31 @peterwilsoncc
2 months ago

@wildworks Are you able to upload an open-source font modified with FontForge to contain a double quote in the name? I tried to figure out how to do it myself but without success, I'm afraid.

@peterwilsoncc commented on PR #8982:


2 months ago
#32

@iamsandeepdahiya Are you able to add some tests using the modified font file on the ticket that contains a single quote?

@wildworks
2 months ago

Savate font, but the font name has been changed to O"Reilly Sans using FontForge (contains double quotes)

@wildworks
2 months ago

"O-Reilly-Sans-Regular-double-quotes-included.ttf" file information. The font name contains double quotes.

#33 @peterwilsoncc
8 weeks ago

Thanks @wildworks.

A font with double quotes in the name will fail during upload due to a JavaScript error, so I am unclear if the use case is resolved. However the use case isn't included in the font family test suite on the spec so I am not overly concerned.

#34 @wildworks
8 weeks ago

A font with double quotes in the name will fail during upload due to a JavaScript error

I was able to upload a font with double quotes in the site editor, but the @font-face definition inculudes unescaped double quotes:

@font-face {
  font-family: O"Reilly Sans;
  font-style: normal;
  font-weight: 400;
  font-display: fallback;
  src: url('http://localhost:8888/wp-content/uploads/fonts/O-Reilly-Sans-Regular-double-quotes-included.ttf') format('truetype');
}

As a result, most of the global styles inline CSS will break and no styles will be applied to the site.

I don't have a strong opinion whether we should address this problem in PR 8982. As a follow-up, we can also improve the normalize_css_font_family_name method to escape double quotes.

#35 @jonsurrell
8 weeks ago

I started to work on improvements to the proposed normalization function and have some findings to share. I'm unfamiliar with the font system in WordPress, so I may have misinterpreted or overlooked some things. In particular, I'm not familiar with the system as a whole and am mostly interpreting things from the CSS perspective.


The linked PR touches font families in two places:

  • WP_Font_Face::build_font_face_css() (source)
  • WP_Font_Utils::sanitize_font_family() (source)

I believe these serve different purposes while both dealing with font-family. This is because ::build_font_face_css() corresponds to an @font-face at-rule. This @font-family font-family takes a single value. It is not the same as the font-family CSS property that takes a comma-separated list of values to match font-family.

On the linked PR, I suggested always quoting font names. That must not be applied in both places. I believe it should be applied in ::build_font_face_css() but not ::sanitize_font_family().


The main problem being addressed here is in WP_Font_Face::build_font_face_css() where a simple font family string is provided (as part of a larger $font_face array). This is a PHP string of a font family name, not a CSS string.

Assuming the input is a PHP string (not an escaped or quoted CSS string) the provided font-family name can and should be normalized. Always quoting the value simplifies escaping and prevents collisions with generic family names. See the specification.

Normalization might look something like this:

<?php
function normalize_font_family_name( string $font_family ): string {
  return '"' . strtr(
    trim( $font_family, " \t\r\f\n"),
    array(
      /*
       * Normalize preprocessed whitespace.
       * https://www.w3.org/TR/css-syntax-3/#input-preprocessing
       */
      "\r" => '\\A ',
      "\f" => '\\A ',
      "\r\n" => '\\A ',

      /*
       * CSS unicode escaping for problematic characters.
       * https://www.w3.org/TR/css-syntax-3/#escaping
       */
      "\n" => '\\A ',
      '\\' => '\\5C ',
      ',' => '\\2C ',
      '"' => '\\22 ',
    )
  ) . '"';
}

The linked PR also touches WP_Font_Utils::sanitize_font_family(). This function takes a string that is a CSS font-family declaration value. I recommended leaving this entire class untouched in the PR. Handling a single @font-face font family name and a font-family declaration value have different considerations and should not be conflated.

It splits a font-family description at ,, applies some functions (including normalization) to the parts, then joins the parts again with , .

The applied functions are sanitize_text_field (which is filterable) and possibly ::maybe_add_quotes() renamed to ::normalize_css_font_family_name() in the linked PR.

The sanitization function works on some expected inputs but will break some other valid inputs. Unfortunately, without some basic CSS parsing it's difficult to improve. Notably, the , character could be part of the CSS font name, in which case the sanitization will mangle the intended result. I doubt that most common font-family names use , in the name because it complicates use in CSS, so maybe we can ignore this point for now.

Quoting in this sanitization function cannot be done indiscriminately. Quotes must not be applied to generic font family names like sans-serif or they will not be treated as generic families.

This function is problematic in edge cases, but I think it's best left alone until we're prepared to implement some improved parsing. HTML API: Add CSS selector support #7857 may be helpful if folks are interested.

@jonsurrell commented on PR #8982:


8 weeks ago
#36

I've started work suggested changes in https://github.com/WordPress/wordpress-develop/pull/9951.

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


7 weeks ago
#37

  • Keywords has-unit-tests added

This includes tests to verify correct and expected behavior. It's not as straightforward to verify that the normalized font works correctly in the browser.

I've prepared a demonstration available test and compare how these encodings work. It includes some examples from the test suite and can be used to verify that, for example, these font families match:

@font-face {
  font-family:"BS\\Quot\"Apos'Semi;Comma,Newline\ALT<Oh😵My!";
}
* {
  font-family: "BS\5C Quot\22 Apos\'Semi;Comma\2C Newline\A LT\3C Oh😵My!";
}

The behavior can be tested for regressions with the playground. The default twentytwentyfive theme will use the font families Manrope and Fira Code that can be tested on the playground.

Before (unquoted): https://playground.wordpress.net/?wp=6.8
After (quoted): https://playground.wordpress.net/?core-pr=9951

-@font-face{font-family:Manrope;font-style:normal;font-weight:200 800;font-display:fallback;src:url('h/wp-content/themes/twentytwentyfive/assets/fonts/manrope/Manrope-VariableFont_wght.woff2') format('woff2');}
+@font-face{font-family:"Manrope";font-style:normal;font-weight:200 800;font-display:fallback;src:url('/wp-content/themes/twentytwentyfive/assets/fonts/manrope/Manrope-VariableFont_wght.woff2') format('woff2');}
 @font-face{font-family:"Fira Code";font-style:normal;font-weight:300 700;font-display:fallback;src:url('/wp-content/themes/twentytwentyfive/assets/fonts/fira-code/FiraCode-VariableFont_wght.woff2') format('woff2');}

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

#38 @jonsurrell
7 weeks ago

  • Keywords needs-testing dev-feedback added; commit removed

I'd love to get review from someone more familiar with the font system on PR 9951. Testing would also be welcome.

@jonsurrell commented on PR #9951:


7 weeks ago
#39

I'd love to get feedback from folks that worked on Font Face CSS support. Based on history, @hellofromtonya and @oandregal may be candidates.

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


12 days ago

#41 @jonsurrell
6 days ago

PR 9951 is a patch to address this. It's ready for review, but I'm not sure it's a candidate for 6.9 at this point in the release cycle.

#42 @wildworks
6 days ago

I apologize for the late reply. I don't have much bandwidth available, but I'll try my best to review the PR9951 this week.

I'd also like to send a message to @dmsnell about this matter. That's because he has a deep understanding of these kinds of problems.

To summarize this ticket, the goal is to properly escape the characters included in the font family name using the new normalize_css_font_family() function and enclose them in double quotes.

Actual:

<style class='wp-fonts-local'>
@font-face{
  font-family: O'Reilly Sans;
}
</style>

Expected:

<style class='wp-fonts-local'>
@font-face{
  font-family: "O'Reilly Sans";
}
</style>

#43 @dmsnell
6 days ago

Thanks @wildworks for the ping. I will try and follow-up with this within a few days, but in the meantime I fully trust what @jonsurrell has to say on the matter. He’s even more knowledgeable on these mixed-environment scenarios.

It does seem like the issue isn’t so much that font names with an apostrophe cause problems, but rather that somewhere we have overlooked the fact that CSS strings may need to be quoted and escaped, and I like the direction he’s proposing to handle the generation of CSS strings given a raw PHP input value.

In #9299 I was working along a similar idea with $wpdb to properly escape “schema name identifiers” (table names, column names, etc…) and reject outright those which are truly invalid according to MySQL. There could be invalid or unrepresentable font names, and sometimes it’s best to put that understanding into the code, which also has the benefit of being able to reliably skip quoting when unnecessary.

@jonsurrell commented on PR #9951:


5 days ago
#44

I've been doing some testing and I have concerns about this approach. This has focused on fonts registered via theme.json, however after reviewing the issue and testing I've realized that font uploads are not processed by the functions that this PR touches.

I need to investigate how font uploads are processed. I've tested uploading a font named O"Reilly,Sans and it breaks the styling because of the quote wrapping. I'd expect that to work in this PR if set in a theme.json file, but it is not handled correctly as an uploaded font.

That case is interesting because we know it's a plain font name (not CSS) so there's no question of whether or not the value has been quoted or escaped already (see https://github.com/WordPress/wordpress-develop/pull/9951#discussion_r2491124999).

#45 @faisal03
5 days ago

I agree with the approach of keeping this fix minimal for now and handling broader normalization/escaping logic in a follow-up PR (as suggested earlier).

It might also be helpful to add a dedicated unit test for the normalize_quoted_font_family_name() method in the next iteration, to prevent regressions for edge cases like escaped quotes.

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


5 days ago

#47 @krupajnanda
5 days ago

Given that we're only a few days away from RC1, could we get a quick final review? Since it affects how font names are escaped and output in CSS, it would be safer to validate all edge cases before committing.
If we're unable to complete testing and review in the next day or two, I'd recommend punting this to the 7.0 milestone so we have enough time to add test coverage and confirm there are no regressions.

#48 @wildworks
5 days ago

  • Milestone changed from 6.9 to 7.0

Let's punt this to 7.0.

As far as I know, fonts with single quotes in their names are very rare, and problems only occur in a limited number of environments, so I don't think this is essential for the 6.9 release. Also, as mentioned here, we need to investigate the font upload logic as well.

Note: See TracTickets for help on using tickets.