[wp-trac] [WordPress Trac] #63568: WP_Font_Face: Font names that contain single quotes are not wrapped in double quotes

[WordPress Trac]

#63568: WP_Font_Face: Font names that contain single quotes are not wrapped in
double quotes
-------------------------------------------------+-------------------------
 Reporter:  wildworks                            |       Owner:  audrasjb
     Type:  defect (bug)                         |      Status:  accepted
 Priority:  normal                               |   Milestone:  6.9
Component:  Editor                               |     Version:  6.4
 Severity:  normal                               |  Resolution:
 Keywords:  has-patch has-test-info has-         |     Focuses:  ui
  screenshots commit                             |
-------------------------------------------------+-------------------------

Comment (by jonsurrell):

 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()`
 [https://core.trac.wordpress.org/browser/tags/6.8.2/src/wp-includes/fonts
 /class-wp-font-face.php#L358 (source)]
 - `WP_Font_Utils::sanitize_font_family()`
 [https://core.trac.wordpress.org/browser/tags/6.8.2/src/wp-includes/fonts
 /class-wp-font-utils.php#L60 (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 [https://developer.mozilla.org/en-US/docs/Web/CSS/@font-face
 /font-family not the same] as the `font-family`
 [https://developer.mozilla.org/en-US/docs/Web/CSS/font-family CSS
 property] that takes a comma-separated list of values to match font-
 family.

 On the linked PR, [https://github.com/WordPress/wordpress-
 develop/pull/8982#issuecomment-2976190329 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. [https://www.w3.org/TR/css-fonts-4/#family-name-syntax See the
 specification.]

 Normalization might look something like this:

 {{{#!php
 <?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.
 [https://github.com/WordPress/wordpress-develop/pull/7857 HTML API: Add
 CSS selector support #7857] may be helpful if folks are interested.

-- 
Ticket URL: <https://core.trac.wordpress.org/ticket/63568#comment:35>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform