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

Font Library: fixes installed font families not rendering in the editor or frontend. #59019

Merged
merged 15 commits into from
Feb 14, 2024
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions lib/compat/wordpress-6.5/fonts/class-wp-font-collection.php
Original file line number Diff line number Diff line change
Expand Up @@ -221,8 +221,10 @@ private static function get_sanitization_schema() {
array(
'font_family_settings' => array(
'name' => 'sanitize_text_field',
'slug' => 'sanitize_title',
'fontFamily' => 'sanitize_text_field',
'slug' => static function ( $value ) {
return _wp_to_kebab_case( sanitize_title( $value ) );
},
'fontFamily' => 'WP_Font_Utils::sanitize_font_family',
'preview' => 'sanitize_url',
'fontFace' => array(
array(
Expand Down
61 changes: 42 additions & 19 deletions lib/compat/wordpress-6.5/fonts/class-wp-font-utils.php
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,41 @@
* @access private
*/
class WP_Font_Utils {

/**
* Adds surrounding quotes to font family names that contain special characters.
*
* It follows the recommendations from the CSS Fonts Module Level 4.
* @link https://www.w3.org/TR/css-fonts-4/#font-family-prop
*
* @since 6.5.0
* @access private
matiasbenedetto marked this conversation as resolved.
Show resolved Hide resolved
*
* @see sanitize_font_family()
matiasbenedetto marked this conversation as resolved.
Show resolved Hide resolved
*
* @param string $item A font family name.
* @return string The font family name with surrounding quotes if necessary.
matiasbenedetto marked this conversation as resolved.
Show resolved Hide resolved
*/
private static function maybe_add_quotes( $item ) {
// Match any non alphabetic characters (a-zA-Z), dashes -, or parenthesis ().
$regex = '/[^a-zA-Z\-()]+/';
matiasbenedetto marked this conversation as resolved.
Show resolved Hide resolved
$item = trim( $item );
if ( preg_match( $regex, $item ) ) {
// Removes leading and trailing quotes.
$item = preg_replace( '/^["\']|["\']$/', '', $item );
matiasbenedetto marked this conversation as resolved.
Show resolved Hide resolved
return "\"$item\"";
matiasbenedetto marked this conversation as resolved.
Show resolved Hide resolved
}
return $item;
}

/**
* Sanitizes and formats font family names.
*
* - Applies `sanitize_text_field`
* - Adds surrounding quotes to names that contain spaces and are not already quoted
* - Adds surrounding quotes to names that special
matiasbenedetto marked this conversation as resolved.
Show resolved Hide resolved
*
* It follows the recommendations from the CSS Fonts Module Level 4.
* @link https://www.w3.org/TR/css-fonts-4/#font-family-prop
*
* @since 6.5.0
* @access private
Expand All @@ -39,26 +69,19 @@ public static function sanitize_font_family( $font_family ) {
return '';
}

$font_family = sanitize_text_field( $font_family );
$font_families = explode( ',', $font_family );
$wrapped_font_families = array_map(
function ( $family ) {
$trimmed = trim( $family );
if ( ! empty( $trimmed ) && str_contains( $trimmed, ' ' ) && ! str_contains( $trimmed, "'" ) && ! str_contains( $trimmed, '"' ) ) {
return '"' . $trimmed . '"';
$output = trim( sanitize_text_field( $font_family ) );
matiasbenedetto marked this conversation as resolved.
Show resolved Hide resolved
$formatted_items = array();
if ( str_contains( $output, ',' ) ) {
$items = explode( ',', $output );
foreach ( $items as $item ) {
$formatted_item = self::maybe_add_quotes( $item );
if ( ! empty( $formatted_item ) ) {
$formatted_items[] = $formatted_item;
}
return $trimmed;
},
$font_families
);

if ( count( $wrapped_font_families ) === 1 ) {
$font_family = $wrapped_font_families[0];
} else {
$font_family = implode( ', ', $wrapped_font_families );
}
return implode( ', ', $formatted_items );
}

return $font_family;
return self::maybe_add_quotes( $output );
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -392,7 +392,7 @@ function FontLibraryProvider( { children } ) {
loadFontFaceInBrowser(
face,
getDisplaySrcFromFontFace( face.src ),
'iframe'
'all'
);
} );
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import { privateApis as componentsPrivateApis } from '@wordpress/components';
import { FONT_WEIGHTS, FONT_STYLES } from './constants';
import { unlock } from '../../../../lock-unlock';
import { fetchInstallFontFace } from '../resolvers';
import { formatFontFamily } from './preview-styles';
import { formatFontFaceName } from './preview-styles';

/**
* Browser dependencies
Expand Down Expand Up @@ -99,7 +99,7 @@ export async function loadFontFaceInBrowser( fontFace, source, addTo = 'all' ) {
}

const newFont = new window.FontFace(
formatFontFamily( fontFace.fontFamily ),
formatFontFaceName( fontFace.fontFamily ),
dataSource,
{
style: fontFace.fontStyle,
Expand Down
Original file line number Diff line number Diff line change
@@ -1,10 +1,22 @@
/**
* WordPress dependencies
*/
import { privateApis as componentsPrivateApis } from '@wordpress/components';

/**
* Internal dependencies
*/
import { unlock } from '../../../../lock-unlock';

const { kebabCase } = unlock( componentsPrivateApis );

export default function makeFamiliesFromFaces( fontFaces ) {
const fontFamiliesObject = fontFaces.reduce( ( acc, item ) => {
if ( ! acc[ item.fontFamily ] ) {
acc[ item.fontFamily ] = {
name: item.fontFamily,
fontFamily: item.fontFamily,
slug: item.fontFamily.replace( /\s+/g, '-' ).toLowerCase(),
slug: kebabCase( item.fontFamily.toLowerCase() ),
fontFace: [],
};
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,22 +30,81 @@ function extractFontWeights( fontFaces ) {
return result;
}

/*
* Format the font family to use in the CSS font-family property of a CSS rule.
*
* The input can be a string with the font family name or a string with multiple font family names separated by commas.
* It follows the recommendations from the CSS Fonts Module Level 4.
* https://www.w3.org/TR/css-fonts-4/#font-family-prop
*
* @param {string} input - The font family.
* @return {string} The formatted font family.
*
* Example:
* formatFontFamily( "Open Sans, Font+Name, sans-serif" ) => '"Open Sans", "Font+Name", sans-serif'
* formatFontFamily( "'Open Sans', sans-serif" ) => '"Open Sans", sans-serif'
* formatFontFamily( "DotGothic16, Slabo 27px, serif" ) => '"DotGothic16","Slabo 27px",serif'
* formatFontFamily( "Mine's, Moe's Typography" ) => `"mine's","Moe's Typography"`
*/
export function formatFontFamily( input ) {
return input
.split( ',' )
.map( ( font ) => {
font = font.trim(); // Remove any leading or trailing white spaces
// If the font doesn't start with quotes and contains a space, then wrap in quotes.
// Check that string starts with a single or double quote and not a space
if (
! ( font.startsWith( '"' ) || font.startsWith( "'" ) ) &&
font.indexOf( ' ' ) !== -1
) {
return `"${ font }"`;
}
return font; // Return font as is if no transformation is needed
} )
.join( ', ' );
// Matchs any non alphabetic characters (a-zA-Z), dashes - , or parenthesis ()
const regex = /[^a-zA-Z\-()]+/;
const output = input.trim();

const formatItem = ( item ) => {
item = item.trim();
if ( item.match( regex ) ) {
// removes leading and trailing quotes.
item = item.replace( /^["']|["']$/g, '' );
return `"${ item }"`;
}
return item;
};

if ( output.includes( ',' ) ) {
return output
.split( ',' )
.map( formatItem )
.filter( ( item ) => item !== '' )
.join( ', ' );
}

return formatItem( output );
}

/*
* Format the font face name to use in the font-family property of a font face.
*
* The input can be a string with the font face name or a string with multiple font face names separated by commas.
* It removes the leading and trailing quotes from the font face name.
*
* @param {string} input - The font face name.
* @return {string} The formatted font face name.
*
* Example:
* formatFontFaceName("Open Sans") => "Open Sans"
* formatFontFaceName("'Open Sans', sans-serif") => "Open Sans"
* formatFontFaceName(", 'Open Sans', 'Helvetica Neue', sans-serif") => "Open Sans"
*/
export function formatFontFaceName( input ) {
let output = input.trim();
if ( output.includes( ',' ) ) {
output = output
.split( ',' )
// finds the first item that is not an empty string.
.find( ( item ) => item.trim() !== '' )
.trim()
// removes leading and trailing quotes.
.replace( /^["']|["']$/g, '' );
}
// removes leading and trailing quotes.
output = output.replace( /^["']|["']$/g, '' );
Copy link
Member

Choose a reason for hiding this comment

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

Nit: We could think of abstracting this replacement in a function as we use it in different places.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, I removed one of the calls that was redundant: 8abd359


// Firefox needs the font name to be wrapped in double quotes meanwhile other browsers don't.
if ( window.navigator.userAgent.toLowerCase().includes( 'firefox' ) ) {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Should we check for gecko to make it more broad and cover not only Firefox but all gecko browsers?

Copy link
Contributor Author

@matiasbenedetto matiasbenedetto Feb 14, 2024

Choose a reason for hiding this comment

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

I tried with 'gecko' 2f3a0df, but after that, I noticed that "Gecko" string is in a wide range of user-agents, so I think we should use "firefox" and "fxios"(firefox for ios). Added that here: f85650a

Reference:

Copy link
Contributor

Choose a reason for hiding this comment

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

Note that Firefox on iOS uses the Safari/Webkit engine (as do all iOS browsers, as currently required by Apple), so I'm guessing it will need the same treatment as Safari (no quotes), I'll see if I can test it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call; I didn't know that. I'm unable to test on iOS, but if it's not Gecko engine, we should not include it among the exceptions. I removed that from the conditional in this PR: #59037

Reference: https://firefox-source-docs.mozilla.org/overview/ios.html

output = `"${ output }"`;
}
return output;
}

export function getFamilyPreviewStyle( family ) {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,11 @@
/**
* Internal dependencies
*/
import { getFamilyPreviewStyle, formatFontFamily } from '../preview-styles';
import {
getFamilyPreviewStyle,
formatFontFamily,
formatFontFaceName,
} from '../preview-styles';

describe( 'getFamilyPreviewStyle', () => {
it( 'should return default fontStyle and fontWeight if fontFace is not provided', () => {
Expand Down Expand Up @@ -139,7 +143,7 @@ describe( 'formatFontFamily', () => {
"Seravek, 'Gill Sans Nova', Ubuntu, Calibri, 'DejaVu Sans', source-sans-pro, sans-serif"
)
).toBe(
"Seravek, 'Gill Sans Nova', Ubuntu, Calibri, 'DejaVu Sans', source-sans-pro, sans-serif"
'Seravek, "Gill Sans Nova", Ubuntu, Calibri, "DejaVu Sans", source-sans-pro, sans-serif'
);
} );

Expand All @@ -153,9 +157,50 @@ describe( 'formatFontFamily', () => {
);
} );

it( 'should wrap only those font names with spaces which are not already quoted', () => {
expect( formatFontFamily( 'Baloo Bhai 2, Arial' ) ).toBe(
'"Baloo Bhai 2", Arial'
it( 'should wrap names with special characters in quotes', () => {
expect(
formatFontFamily(
'Font+Name, Font*Name, _Font_Name_, generic(kai), sans-serif'
)
).toBe(
'"Font+Name", "Font*Name", "_Font_Name_", generic(kai), sans-serif'
);
} );

it( 'should fix empty wrong formatted font family', () => {
expect( formatFontFamily( ', Abril Fatface,Times,serif' ) ).toBe(
'"Abril Fatface", Times, serif'
);
} );
} );

describe( 'formatFontFaceName', () => {
it( 'should remove leading and trailing quotes', () => {
expect( formatFontFaceName( '"Open Sans"' ) ).toBe( 'Open Sans' );
} );

it( 'should remove leading and trailing quotes from multiple font face names', () => {
expect(
formatFontFaceName( "'Open Sans', 'Helvetica Neue', sans-serif" )
).toBe( 'Open Sans' );
} );

it( 'should remove leading and trailing quotes even from names with spaces and special characters', () => {
expect( formatFontFaceName( "'Font+Name 24', sans-serif" ) ).toBe(
'Font+Name 24'
);
} );

it( 'should ouput the font face name with quotes on Firefox', () => {
const mockUserAgent =
'Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:122.0) Gecko/20100101 Firefox/122.0';

// Mock the userAgent for this test
Object.defineProperty( window.navigator, 'userAgent', {
value: mockUserAgent,
configurable: true,
} );

expect( formatFontFaceName( 'Open Sans' ) ).toBe( '"Open Sans"' );
} );
} );
Loading