Improve page experience for fonts in core themes

includes/sanitizers/class-amp-style-sanitizer.php

@@ -29,6 +29,7 @@
 use Sabberworm\CSS\OutputFormat;
 use Sabberworm\CSS\Property\Import;
 use Sabberworm\CSS\CSSList\AtRuleBlockList;
+use Sabberworm\CSS\Value\CSSFunction;
 use Sabberworm\CSS\Value\RuleValueList;
 use Sabberworm\CSS\Value\URL;
 use Sabberworm\CSS\Value\Value;
@@ -2519,7 +2520,6 @@ private function process_font_face_at_rule( AtRuleSet $ruleset, $options ) {
 		}
 
 		// Obtain the font-family name to guess the filename.
-		$font_family   = null;
 		$font_basename = null;
 		$properties    = $ruleset->getRules( 'font-family' );
 		if ( isset( $properties[0] ) ) {
@@ -2541,7 +2541,7 @@ private function process_font_face_at_rule( AtRuleSet $ruleset, $options ) {
 		}
 
 		// Attempt to transform data: URLs in src properties to be external file URLs.
-		$converted_count = 0;
+		$converted_data_urls = [];
 		foreach ( $src_properties as $src_property ) {
 			$value = $src_property->getValue();
 			if ( ! ( $value instanceof RuleValueList ) ) {
@@ -2599,24 +2599,43 @@ private function process_font_face_at_rule( AtRuleSet $ruleset, $options ) {
 			 * @var URL[]    $source_data_url_objects
 			 */
 			$source_data_url_objects = [];
-			foreach ( $sources as $i => $source ) {
-				if ( $source[0] instanceof URL ) {
-					$value = $source[0]->getURL()->getString();
-					if ( 'data:' === substr( $value, 0, 5 ) ) {
-						$source_data_url_objects[ $i ] = $source[0];
-					} else {
-						$source_file_urls[ $i ] = $value;
-					}
+			foreach ( $sources as $source ) {
+				if ( count( $source ) !== 2 ) {
+					continue;
+				}
+				list( $url, $format ) = $source;
+				if (
+					! $url instanceof URL
+					||
+					! $format instanceof CSSFunction
+					||
+					$format->getName() !== 'format'
+					||
+					count( $format->getArguments() ) !== 1
+				) {
+					continue;
+				}
+
+				list( $format_value ) = $format->getArguments();
+				$format_value         = trim( $format_value, '"\'' );
+
+				$value = $url->getURL()->getString();
+				if ( 'data:' === substr( $value, 0, 5 ) ) {
+					$source_data_url_objects[ $format_value ] = $source[0];
+				} else {
+					$source_file_urls[] = $value;
 				}
 			}
 
 			// Convert data: URLs into regular URLs, assuming there will be a file present (e.g. woff fonts in core themes).
-			foreach ( $source_data_url_objects as $i => $data_url ) {
+			foreach ( $source_data_url_objects as $format => $data_url ) {
 				$mime_type = strtok( substr( $data_url->getURL()->getString(), 5 ), ';' );
-				if ( ! $mime_type ) {
-					continue;
+				if ( $mime_type ) {
+					$extension = preg_replace( ':.+/(.+-)?:', '', $mime_type );
+				} else {
+					$extension = $format;
 				}
-				$extension = preg_replace( ':.+/(.+-)?:', '', $mime_type );
+				$extension = sanitize_key( $extension );
 
 				$guessed_urls = [];
 
@@ -2648,7 +2667,7 @@ private function process_font_face_at_rule( AtRuleSet $ruleset, $options ) {
 					$path = $this->get_validated_url_file_path( $guessed_url, [ 'woff', 'woff2', 'ttf', 'otf', 'svg' ] );
 					if ( ! is_wp_error( $path ) ) {
 						$data_url->getURL()->setString( $guessed_url );
-						$converted_count++;
+						$converted_data_urls[] = $data_url;
 						continue 2;
 					}
 				}
@@ -2662,22 +2681,39 @@ private function process_font_face_at_rule( AtRuleSet $ruleset, $options ) {
 				];
 				if ( in_array( $font_filename, $bundled_fonts, true ) ) {
 					$data_url->getURL()->setString( plugin_dir_url( AMP__FILE__ ) . "assets/fonts/$font_filename" );
-					$converted_count++;
+					$converted_data_urls[] = $data_url;
 				}
 			} // End foreach $source_data_url_objects.
 		} // End foreach $src_properties.
 
 		/*
-		 * If a data: URL has been replaced with an external file URL, then we add a font-display:swap to the @font-face
-		 * rule if one isn't already present. This prevents FO
+		 * If a data: URL has been replaced with an external file URL, then we add a font-display:optional to the @font-face
+		 * rule if one isn't already present. This prevents a flash of unstyled text (FOUT).
+		 *
+		 * If no font-display is already present, add font-display:optional since the font is now being loaded externally.
 		 *
-		 *  If no font-display is already present, add font-display:swap since the font is now being loaded externally.
+		 * @see: https://github.com/ampproject/amp-wp/issues/6036
 		 */
-		if ( $converted_count && 0 === count( $ruleset->getRules( 'font-display' ) ) ) {
+		if ( count( $converted_data_urls ) > 0 && 0 === count( $ruleset->getRules( 'font-display' ) ) ) {
 			$font_display_rule = new Rule( 'font-display' );
-			$font_display_rule->setValue( 'swap' );
+			$font_display_rule->setValue( 'optional' );
 			$ruleset->addRule( $font_display_rule );
 		}
+
+		// Preload the first data: URL that was converted to an external file.
+		if ( isset( $converted_data_urls[0] ) ) {
+			$link = AMP_DOM_Utils::create_node(
+				$this->dom,
+				Tag::LINK,
+				[
+					Attribute::REL         => Attribute::REL_PRELOAD,
+					Attribute::AS_         => 'font',
+					Attribute::HREF        => $converted_data_urls[0]->getURL()->getString(),
+					Attribute::CROSSORIGIN => '',
+				]
+			);
+			$this->dom->head->insertBefore( $link ); // Note that \AMP_Theme_Support::ensure_required_markup() will put this in the optimal order.
+		}
 	}
 
 	/**

tests/php/test-amp-style-sanitizer.php

@@ -1898,20 +1898,85 @@ public function test_font_data_url_handling_without_file_sources() {
 		$this->assertStringNotContainsString( 'data:', $actual_stylesheets[0] );
 		$this->assertStringContainsString( 'fonts/NonBreakingSpaceOverride.woff2', $actual_stylesheets[0] );
 		$this->assertStringContainsString( 'fonts/NonBreakingSpaceOverride.woff', $actual_stylesheets[0] );
-		$this->assertStringContainsString( 'font-display:swap', $actual_stylesheets[0] );
+		$this->assertStringContainsString( 'font-display:optional', $actual_stylesheets[0] );
 
 		// Check font not included in theme, but included in plugin.
 		$this->assertStringContainsString( '@font-face{font-family:"Genericons";', $actual_stylesheets[1] );
 		$this->assertStringContainsString( 'format("woff")', $actual_stylesheets[1] );
 		$this->assertStringNotContainsString( 'data:', $actual_stylesheets[1] );
 		$this->assertStringContainsString( 'assets/fonts/genericons.woff', $actual_stylesheets[1] );
-		$this->assertStringContainsString( 'font-display:swap', $actual_stylesheets[1] );
+		$this->assertStringContainsString( 'font-display:optional', $actual_stylesheets[1] );
 
 		// Check font not included anywhere, so must remain inline.
 		$this->assertStringContainsString( '@font-face{font-family:"Custom";', $actual_stylesheets[2] );
 		$this->assertStringContainsString( 'url("data:application/x-font-woff;charset=utf-8;base64,d09GRgABAAA")', $actual_stylesheets[2] );
 		$this->assertStringContainsString( 'format("woff")', $actual_stylesheets[2] );
-		$this->assertStringNotContainsString( 'font-display:swap', $actual_stylesheets[2] );
+		$this->assertStringNotContainsString( 'font-display:optional', $actual_stylesheets[2] );
+	}
+
+	/** @return array */
+	public function get_data_to_test_font_files_preloading() {
+		return [
+			'twentynineteen' => [
+				'theme_slug'    => 'twentynineteen',
+				'expected_urls' => [
+					'/themes/twentynineteen/fonts/NonBreakingSpaceOverride.woff2',
+				],
+			],
+			'twentytwenty' => [
+				'theme_slug'    => 'twentytwenty',
+				'expected_urls' => [
+					'/plugins/amp/assets/fonts/nonbreakingspaceoverride.woff2',
+				],
+			],
+			'twentytwentyone' => [
+				'theme_slug'    => 'twentytwentyone',
+				'expected_urls' => [], // Twenty Twenty-One theme uses system font stack, no extra fonts are enqueued.
+			],
+		];
+	}
+
+	/**
+	 * Test that font files are preloaded with <link> element.
+	 *
+	 * @dataProvider get_data_to_test_font_files_preloading
+	 * @covers AMP_Style_Sanitizer::process_font_face_at_rule()
+	 */
+	public function test_font_files_preloading( $theme_slug, $expected_urls ) {
+		$theme = new WP_Theme( $theme_slug, ABSPATH . 'wp-content/themes' );
+		if ( $theme->errors() ) {
+			$this->markTestSkipped( $theme->errors()->get_error_message() );
+		}
+
+		$html  = '<html amp><head><meta charset="utf-8">';
+		$html .= sprintf( '<link rel="stylesheet" href="%s">', esc_url( $theme->get_stylesheet_directory_uri() . '/style.css' ) ); // phpcs:ignore WordPress.WP.EnqueuedResources.NonEnqueuedStylesheet
+		$html .= '</head><body></body></html>';
+
+		$dom         = Document::fromHtml( $html, Options::DEFAULTS );
+		$error_codes = [];
+		$sanitizer   = new AMP_Style_Sanitizer(
+			$dom,
+			[
+				'use_document_element' => true,
+			]
+		);
+		$sanitizer->sanitize();
+		$this->assertEquals( [], $error_codes );
+
+		$link_elements       = $dom->getElementsByTagName( 'link' );
+		$link_elements_count = $link_elements->length;
+
+		$this->assertEquals( count( $expected_urls ), $link_elements_count );
+
+		for ( $i = 0; $i < $link_elements_count; $i++ ) {
+			$this->assertStringEndsWith(
+				$expected_urls[ $i ],
+				$link_elements->item( $i )->getAttribute( 'href' )
+			);
+			$this->assertEquals( $link_elements->item( $i )->getAttribute( 'rel' ), 'preload' );
+			$this->assertEquals( $link_elements->item( $i )->getAttribute( 'as' ), 'font' );
+			$this->assertEquals( $link_elements->item( $i )->getAttribute( 'crossorigin' ), '' );
+		}
 	}
 
 	/**