[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:  Future
                                                 |  Release
Component:  Editor                               |     Version:  6.4
 Severity:  normal                               |  Resolution:
 Keywords:  has-patch has-test-info has-         |     Focuses:  ui, css
  screenshots has-unit-tests needs-testing dev-  |
  feedback                                       |
-------------------------------------------------+-------------------------
Changes (by jonsurrell):

 * focuses:  ui => ui, css


Comment:

 [https://github.com/WordPress/wordpress-
 develop/pull/9951#discussion_r2491124999 There is conversation in the
 linked PR that's worth considering here.] Some of the discussion initiated
 on lines of code and is not copied into this thread.

 On the surface, this may seem like a simple issue. However, it raises a
 lot of questions. This type of problem arises from dealing working with
 CSS with no interface for parsing or manipulation. There are many
 parallels with HTML and the HTML API. Every day I see more motivation to
 introduce a CSS API.

 -----

 Some observations from my investigation on this ticket:

 - Uploading a font with an apostrophe in its like `O'Reilly Sans` results
 in broken CSS like the declaration `font-family: O'Reilly Sans;`. There
 are many ways to fix this declaration, the simplest and most common is to
 quote `font-family: "O'Reilly Sans";`
 - The font name could contain virtually any text as long as it is quoted
 and or escaped correctly.
 - [https://github.com/WordPress/wordpress-
 develop/blob/0c3e42c238fc2ca907bbe649160598f39267d812/src/wp-
 includes/fonts/class-wp-font-face.php#L361-L371 WordPress has a mechanism
 that attempts to quote font families.]
 - [https://github.com/WordPress/wordpress-
 develop/blob/0c3e42c238fc2ca907bbe649160598f39267d812/src/wp-
 includes/fonts/class-wp-font-utils.php#L60-L78 WordPress also attempts to
 sanitize font-family values] and [https://github.com/WordPress/wordpress-
 develop/blob/0c3e42c238fc2ca907bbe649160598f39267d812/src/wp-
 includes/fonts/class-wp-font-utils.php#L32-L41 maybe quote them].
 - Surrounding a value in `"` or `'` covers some cases but is insufficient.
 Values can contain `'` and or `"`.
 - Splitting a comma-delimited list of values on `,` is also insufficient.
 `,` can appear inside values in the list.
 - There are several `font-family` in CSS:
   - In `@font-face` rules to [https://developer.mozilla.org/en-
 US/docs/Web/CSS/Reference/At-rules/@font-face/font-family specify the font
 family for the font.] This takes a ''single value''.
   - [https://developer.mozilla.org/en-US/docs/Web/CSS/Reference/Properties
 /font-family As a style property], which takes a ''comma-delimited list of
 values''.
   - [https://developer.mozilla.org/en-US/docs/Web/CSS/Reference/At-rules
 /@font-palette-values/font-family I just learned it's also a descriptor of
 @font-pallete-values].
 - `font-family` values can be ''identifiers'' or ''strings'' (quoted
 values). Most font-family values can be represented in either way. One
 exception are generic fonts which must use the unquoted identifier form.
 String values can be used with the same font-family name as a generic,
 where the value type will differentiate.
 - Storing pieces of CSS in JSON is confusing and error-prone,
 ''especially'' when humans are authoring the JSON. For example:
   - [https://github.com/WordPress/wordpress-
 develop/blob/0c3e42c238fc2ca907bbe649160598f39267d812/src/wp-
 content/themes/twentytwentyfive/theme.json#L180 twentytwentyfive uses] `{
 "fontFamily": "\"Fira Code\"" }`. **This appears to be correct.** The
 quoted CSS string `"Fira Code"` is encoded as the JSON string `"\"Fira
 Code\""`.
   - [https://github.com/WordPress/wordpress-
 develop/blob/0c3e42c238fc2ca907bbe649160598f39267d812/src/wp-
 content/themes/twentytwentythree/theme.json#L142 twentytwentythree uses]
 `{ "fontFamily": "DM Sans" }` (invalid value `DM Sans` after JSON
 decoding) [https://github.com/WordPress/wordpress-
 develop/blob/0c3e42c238fc2ca907bbe649160598f39267d812/src/wp-
 content/themes/twentytwentythree/theme.json#L151 but also uses] `{
 "fontFamily": "\"DM Sans\", sans-serif" }` (valid font-family value `"DM
 Sans", sans-serif` after decoding).
 - [https://developer.wordpress.org/themes/global-settings-and-
 styles/settings/typography/ The typography documentation is unclear about
 expectations:]
   - "fontFamily: (Required) A valid value that will map to the CSS font-
 family value. Generally, this will be a font stack (a list of families
 that the browser will try to use in order)."
   - "fontFamily: A valid CSS font-family descriptor."
   - One example uses `"fontFamily": "'Open Sans', sans-serif"`, while
 another is `"fontFamily": "Open Sans"`. I'd expect the latter to be
 `"fontFamily": "'Open Sans'"`, `"fontFamily": "\"Open Sans\"", or similar.
 - It's very difficult to try to normalize, correct, fix, or sanitize
 snippets of CSS values that are not ready to use when JSON decoded. The
 existing quoting and sanitization behavior complicates things further
 because efforts to correct the system likely need to maintain some of the
 behavior to avoid introducing regressions.

 -----

 How does this move forward?

 I'd like for this work to start by clarifying what the values in JSON
 actually are. If they're JSON-encoded CSS values, then the examples and
 Core themes should be updated accordingly, for example `Open Sans` should
 always be quoted.

 The sanitization that splits on `,`, maybe quotes, and joins needs to be
 re-evaluated. It may not be necessary at all or may do more harm than
 good.

 It should not be necessary to strip tags from CSS. Since [61418],
 `<style>` tags should be "safe" with regards to HTML.

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