From 0be7466b2096b10e3dca3698d31966d89f5d144b Mon Sep 17 00:00:00 2001 From: Bartosz Gadomski Date: Tue, 2 Nov 2021 10:53:34 +0100 Subject: [PATCH 01/21] The 'font-display' value replaced from 'swap' to 'optional' --- includes/sanitizers/class-amp-style-sanitizer.php | 10 ++++++---- tests/php/test-amp-style-sanitizer.php | 6 +++--- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/includes/sanitizers/class-amp-style-sanitizer.php b/includes/sanitizers/class-amp-style-sanitizer.php index 49b94fcd06a..1c87b963d36 100644 --- a/includes/sanitizers/class-amp-style-sanitizer.php +++ b/includes/sanitizers/class-amp-style-sanitizer.php @@ -2667,15 +2667,17 @@ private function process_font_face_at_rule( AtRuleSet $ruleset, $options ) { } // 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 + /** + * 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 FO * - * If no font-display is already present, add font-display:swap since the font is now being loaded externally. + * If no font-display is already present, add font-display:optional 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' ) ) ) { $font_display_rule = new Rule( 'font-display' ); - $font_display_rule->setValue( 'swap' ); + $font_display_rule->setValue( 'optional' ); $ruleset->addRule( $font_display_rule ); } } diff --git a/tests/php/test-amp-style-sanitizer.php b/tests/php/test-amp-style-sanitizer.php index 37a00975a8b..53f4518269d 100644 --- a/tests/php/test-amp-style-sanitizer.php +++ b/tests/php/test-amp-style-sanitizer.php @@ -1898,20 +1898,20 @@ 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] ); } /** From dfbf6165e324004482d720063feb4fb35aabfe1d Mon Sep 17 00:00:00 2001 From: Bartosz Gadomski Date: Tue, 2 Nov 2021 12:31:05 +0100 Subject: [PATCH 02/21] Preload link added for all font files --- .../sanitizers/class-amp-style-sanitizer.php | 34 +++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/includes/sanitizers/class-amp-style-sanitizer.php b/includes/sanitizers/class-amp-style-sanitizer.php index 1c87b963d36..f3e3f0a4980 100644 --- a/includes/sanitizers/class-amp-style-sanitizer.php +++ b/includes/sanitizers/class-amp-style-sanitizer.php @@ -2680,6 +2680,40 @@ private function process_font_face_at_rule( AtRuleSet $ruleset, $options ) { $font_display_rule->setValue( 'optional' ); $ruleset->addRule( $font_display_rule ); } + + /** + * Preload each font files + */ + foreach ( $ruleset->getRules( 'src' ) as $src_property ) { + $value = $src_property->getValue(); + $font_files = array_filter( + array_map( + function( $path ) { + return substr( $path, 0, strpos( $path, '"' ) ); + }, + explode( 'url("', $value ) + ), + function( $path ) { + return 'http' === substr( $path, 0, 4 ); + } + ); + + if ( ! empty( $font_files ) ) { + foreach ( $font_files as $font_file ) { + $link = AMP_DOM_Utils::create_node( + $this->dom, + 'link', + [ + 'rel' => 'preload', + 'as' => 'font', + 'href' => $font_file, + 'crossorigin' => '', + ] + ); + $this->dom->head->insertBefore( $link ); // Note that \AMP_Theme_Support::ensure_required_markup() will put this in the optimal order. + } + } + } } /** From df8fbc27960bf08d4804b6fe8221fdeb95f492aa Mon Sep 17 00:00:00 2001 From: Bartosz Gadomski Date: Wed, 3 Nov 2021 19:44:51 +0100 Subject: [PATCH 03/21] Test case for font files preloading added --- tests/php/test-amp-style-sanitizer.php | 76 ++++++++++++++++++++++++++ 1 file changed, 76 insertions(+) diff --git a/tests/php/test-amp-style-sanitizer.php b/tests/php/test-amp-style-sanitizer.php index 53f4518269d..ce8589bf9a3 100644 --- a/tests/php/test-amp-style-sanitizer.php +++ b/tests/php/test-amp-style-sanitizer.php @@ -1914,6 +1914,82 @@ public function test_font_data_url_handling_without_file_sources() { $this->assertStringNotContainsString( 'font-display:optional', $actual_stylesheets[2] ); } + /** + * Test that font files are preloaded with element. + * + * @covers AMP_Style_Sanitizer::process_font_face_at_rule() + */ + public function test_font_files_preloading() { + $at_least_one_run = false; + $test_cases = [ + (object) [ + 'theme_slug' => 'twentynineteen', + 'expected_urls' => [ + '/themes/twentynineteen/fonts/NonBreakingSpaceOverride.woff2', + '/themes/twentynineteen/fonts/NonBreakingSpaceOverride.woff', + ], + ], + (object) [ + 'theme_slug' => 'twentytwenty', + 'expected_urls' => [ + '/plugins/amp/assets/fonts/nonbreakingspaceoverride.woff2', + '/plugins/amp/assets/fonts/nonbreakingspaceoverride.woff', + '/themes/twentytwenty/assets/fonts/inter/Inter-upright-var.woff2', + '/themes/twentytwenty/assets/fonts/inter/Inter-italic-var.woff2', + ], + ], + (object) [ + 'theme_slug' => 'twentytwentyone', + 'expected_urls' => [], // Twenty Twenty-One theme uses system font stack, no extra fonts are enqueued. + ], + ]; + + foreach ( $test_cases as $test_case ) { + $theme = new WP_Theme( $test_case->theme_slug, ABSPATH . 'wp-content/themes' ); + if ( ! $theme->errors() ) { + $at_least_one_run = true; + } + + $html = ''; + $html .= sprintf( '', esc_url( $theme->get_stylesheet_directory_uri() . '/style.css' ) ); // phpcs:ignore WordPress.WP.EnqueuedResources.NonEnqueuedStylesheet + $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( $link_elements_count, count( $test_case->expected_urls ) ); + + for ( $i = 0; $i < $link_elements_count; $i++ ) { + $this->assertEquals( + substr_compare( + $link_elements->item( $i )->getAttribute( 'href' ), + $test_case->expected_urls[ $i ], + - strlen( $test_case->expected_urls[ $i ] ) + ), + 0 + ); + $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' ), '' ); + } + } + + if ( false === $at_least_one_run ) { + $this->markTestSkipped( 'None of default themes is installed.' ); + } + } + /** * Test that auto-removal (tree shaking) does not remove rules for classes mentioned in class and [class] attributes. * From 5d282ce88ba79384bfb33e08e0b1380e1c6e3751 Mon Sep 17 00:00:00 2001 From: Bartosz Gadomski Date: Wed, 3 Nov 2021 19:47:26 +0100 Subject: [PATCH 04/21] Fix theme case to not execute assertions if a given theme is not installed --- tests/php/test-amp-style-sanitizer.php | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/tests/php/test-amp-style-sanitizer.php b/tests/php/test-amp-style-sanitizer.php index ce8589bf9a3..4b48dd678f2 100644 --- a/tests/php/test-amp-style-sanitizer.php +++ b/tests/php/test-amp-style-sanitizer.php @@ -1946,10 +1946,12 @@ public function test_font_files_preloading() { foreach ( $test_cases as $test_case ) { $theme = new WP_Theme( $test_case->theme_slug, ABSPATH . 'wp-content/themes' ); - if ( ! $theme->errors() ) { - $at_least_one_run = true; + if ( $theme->errors() ) { + continue; } + $at_least_one_run = true; + $html = ''; $html .= sprintf( '', esc_url( $theme->get_stylesheet_directory_uri() . '/style.css' ) ); // phpcs:ignore WordPress.WP.EnqueuedResources.NonEnqueuedStylesheet $html .= ''; From 1ab2a120d7ff7704d66bfc2fcba21c9e3933c91b Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Wed, 3 Nov 2021 16:30:25 -0700 Subject: [PATCH 05/21] Reuse already parsed URL value and only preload first externalized data: URL --- .../sanitizers/class-amp-style-sanitizer.php | 55 ++++++------------- tests/php/test-amp-style-sanitizer.php | 6 +- 2 files changed, 19 insertions(+), 42 deletions(-) diff --git a/includes/sanitizers/class-amp-style-sanitizer.php b/includes/sanitizers/class-amp-style-sanitizer.php index f3e3f0a4980..5330f60d02b 100644 --- a/includes/sanitizers/class-amp-style-sanitizer.php +++ b/includes/sanitizers/class-amp-style-sanitizer.php @@ -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 ) ) { @@ -2648,7 +2648,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,57 +2662,38 @@ 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:optional to the @font-face - * rule if one isn't already present. This prevents FO + * 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. * * @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( 'optional' ); $ruleset->addRule( $font_display_rule ); } - /** - * Preload each font files - */ - foreach ( $ruleset->getRules( 'src' ) as $src_property ) { - $value = $src_property->getValue(); - $font_files = array_filter( - array_map( - function( $path ) { - return substr( $path, 0, strpos( $path, '"' ) ); - }, - explode( 'url("', $value ) - ), - function( $path ) { - return 'http' === substr( $path, 0, 4 ); - } + // 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 => '', + ] ); - - if ( ! empty( $font_files ) ) { - foreach ( $font_files as $font_file ) { - $link = AMP_DOM_Utils::create_node( - $this->dom, - 'link', - [ - 'rel' => 'preload', - 'as' => 'font', - 'href' => $font_file, - 'crossorigin' => '', - ] - ); - $this->dom->head->insertBefore( $link ); // Note that \AMP_Theme_Support::ensure_required_markup() will put this in the optimal order. - } - } + $this->dom->head->insertBefore( $link ); // Note that \AMP_Theme_Support::ensure_required_markup() will put this in the optimal order. } } diff --git a/tests/php/test-amp-style-sanitizer.php b/tests/php/test-amp-style-sanitizer.php index 4b48dd678f2..69129ae0e61 100644 --- a/tests/php/test-amp-style-sanitizer.php +++ b/tests/php/test-amp-style-sanitizer.php @@ -1926,16 +1926,12 @@ public function test_font_files_preloading() { 'theme_slug' => 'twentynineteen', 'expected_urls' => [ '/themes/twentynineteen/fonts/NonBreakingSpaceOverride.woff2', - '/themes/twentynineteen/fonts/NonBreakingSpaceOverride.woff', ], ], (object) [ 'theme_slug' => 'twentytwenty', 'expected_urls' => [ '/plugins/amp/assets/fonts/nonbreakingspaceoverride.woff2', - '/plugins/amp/assets/fonts/nonbreakingspaceoverride.woff', - '/themes/twentytwenty/assets/fonts/inter/Inter-upright-var.woff2', - '/themes/twentytwenty/assets/fonts/inter/Inter-italic-var.woff2', ], ], (object) [ @@ -1970,7 +1966,7 @@ public function test_font_files_preloading() { $link_elements = $dom->getElementsByTagName( 'link' ); $link_elements_count = $link_elements->length; - $this->assertEquals( $link_elements_count, count( $test_case->expected_urls ) ); + $this->assertEquals( count( $test_case->expected_urls ), $link_elements_count ); for ( $i = 0; $i < $link_elements_count; $i++ ) { $this->assertEquals( From a5e6f6dc43403137f43d115c8a3adf0f32defd3d Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Wed, 3 Nov 2021 16:54:17 -0700 Subject: [PATCH 06/21] Harden collection of font sources --- .../sanitizers/class-amp-style-sanitizer.php | 45 +++++++++++++------ 1 file changed, 32 insertions(+), 13 deletions(-) diff --git a/includes/sanitizers/class-amp-style-sanitizer.php b/includes/sanitizers/class-amp-style-sanitizer.php index 5330f60d02b..7b993d07ce8 100644 --- a/includes/sanitizers/class-amp-style-sanitizer.php +++ b/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] ) ) { @@ -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 = []; From c5de52830d1509aa2b4e51f072ef70a3fe030c7c Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Wed, 3 Nov 2021 16:58:39 -0700 Subject: [PATCH 07/21] Use data provider for test_font_files_preloading --- tests/php/test-amp-style-sanitizer.php | 95 ++++++++++++-------------- 1 file changed, 43 insertions(+), 52 deletions(-) diff --git a/tests/php/test-amp-style-sanitizer.php b/tests/php/test-amp-style-sanitizer.php index 69129ae0e61..25079487d2b 100644 --- a/tests/php/test-amp-style-sanitizer.php +++ b/tests/php/test-amp-style-sanitizer.php @@ -1914,77 +1914,68 @@ public function test_font_data_url_handling_without_file_sources() { $this->assertStringNotContainsString( 'font-display:optional', $actual_stylesheets[2] ); } - /** - * Test that font files are preloaded with element. - * - * @covers AMP_Style_Sanitizer::process_font_face_at_rule() - */ - public function test_font_files_preloading() { - $at_least_one_run = false; - $test_cases = [ - (object) [ + /** @return array */ + public function get_data_to_test_font_files_preloading() { + return [ + 'twentynineteen' => [ 'theme_slug' => 'twentynineteen', 'expected_urls' => [ '/themes/twentynineteen/fonts/NonBreakingSpaceOverride.woff2', ], ], - (object) [ + 'twentytwenty' => [ 'theme_slug' => 'twentytwenty', 'expected_urls' => [ '/plugins/amp/assets/fonts/nonbreakingspaceoverride.woff2', ], ], - (object) [ + 'twentytwentyone' => [ 'theme_slug' => 'twentytwentyone', 'expected_urls' => [], // Twenty Twenty-One theme uses system font stack, no extra fonts are enqueued. ], ]; + } - foreach ( $test_cases as $test_case ) { - $theme = new WP_Theme( $test_case->theme_slug, ABSPATH . 'wp-content/themes' ); - if ( $theme->errors() ) { - continue; - } + /** + * Test that font files are preloaded with 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() ); + } - $at_least_one_run = true; + $html = ''; + $html .= sprintf( '', esc_url( $theme->get_stylesheet_directory_uri() . '/style.css' ) ); // phpcs:ignore WordPress.WP.EnqueuedResources.NonEnqueuedStylesheet + $html .= ''; - $html = ''; - $html .= sprintf( '', esc_url( $theme->get_stylesheet_directory_uri() . '/style.css' ) ); // phpcs:ignore WordPress.WP.EnqueuedResources.NonEnqueuedStylesheet - $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 ); - $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( $test_case->expected_urls ), $link_elements_count ); - - for ( $i = 0; $i < $link_elements_count; $i++ ) { - $this->assertEquals( - substr_compare( - $link_elements->item( $i )->getAttribute( 'href' ), - $test_case->expected_urls[ $i ], - - strlen( $test_case->expected_urls[ $i ] ) - ), - 0 - ); - $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' ), '' ); - } - } + $link_elements = $dom->getElementsByTagName( 'link' ); + $link_elements_count = $link_elements->length; - if ( false === $at_least_one_run ) { - $this->markTestSkipped( 'None of default themes is installed.' ); + $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' ), '' ); } } From 4e0403f2f32ba408e7728817442eb20b3a2074a0 Mon Sep 17 00:00:00 2001 From: Bartosz Gadomski Date: Tue, 9 Nov 2021 12:05:53 +0100 Subject: [PATCH 08/21] Fonts preloading rules improved --- .../sanitizers/class-amp-style-sanitizer.php | 46 ++++++++++++++----- 1 file changed, 34 insertions(+), 12 deletions(-) diff --git a/includes/sanitizers/class-amp-style-sanitizer.php b/includes/sanitizers/class-amp-style-sanitizer.php index 7b993d07ce8..2458e1f88b3 100644 --- a/includes/sanitizers/class-amp-style-sanitizer.php +++ b/includes/sanitizers/class-amp-style-sanitizer.php @@ -2540,6 +2540,9 @@ private function process_font_face_at_rule( AtRuleSet $ruleset, $options ) { $stylesheet_base_url = trailingslashit( $stylesheet_base_url ); } + // Define array of fonts to be preloaded. + $fonts_to_preload = []; + // Attempt to transform data: URLs in src properties to be external file URLs. $converted_data_urls = []; foreach ( $src_properties as $src_property ) { @@ -2684,6 +2687,22 @@ private function process_font_face_at_rule( AtRuleSet $ruleset, $options ) { $converted_data_urls[] = $data_url; } } // End foreach $source_data_url_objects. + + // Loop through fonts loaded from files. + foreach ( $source_file_urls as $source_file_url ) { + $properties = $ruleset->getRules( 'font-display' ); + + if ( ! isset( $properties[0] ) ) { + // If a given font is loaded from file and without font-display property, add font-display:optional and preload it. + $font_display_rule = new Rule( 'font-display' ); + $font_display_rule->setValue( 'optional' ); + $ruleset->addRule( $font_display_rule ); + $fonts_to_preload[] = $source_file_url; + } else if ( 'optional' === $properties[0]->getValue() ) { + // If a given font is loaded from file and with font-display:optional, preload it. + $fonts_to_preload[] = $source_file_url; + } + } // End foreach $source_file_urls. } // End foreach $src_properties. /* @@ -2698,21 +2717,24 @@ private function process_font_face_at_rule( AtRuleSet $ruleset, $options ) { $font_display_rule = new Rule( 'font-display' ); $font_display_rule->setValue( 'optional' ); $ruleset->addRule( $font_display_rule ); + $fonts_to_preload[] = $converted_data_urls[0]->getURL()->getString(); } // 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. + if ( ! empty( $fonts_to_preload ) ) { + foreach ( $fonts_to_preload as $font_to_preload ) { + $link = AMP_DOM_Utils::create_node( + $this->dom, + Tag::LINK, + [ + Attribute::REL => Attribute::REL_PRELOAD, + Attribute::AS_ => 'font', + Attribute::HREF => $font_to_preload, + Attribute::CROSSORIGIN => '', + ] + ); + $this->dom->head->insertBefore( $link ); // Note that \AMP_Theme_Support::ensure_required_markup() will put this in the optimal order. + } } } From 503aa470d1feac3a67bb756cf2581a89ec02ba2e Mon Sep 17 00:00:00 2001 From: Bartosz Gadomski Date: Tue, 9 Nov 2021 12:19:22 +0100 Subject: [PATCH 09/21] Fix failing tests due to font-display:optional --- tests/php/test-amp-style-sanitizer.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/php/test-amp-style-sanitizer.php b/tests/php/test-amp-style-sanitizer.php index 25079487d2b..435b61f0908 100644 --- a/tests/php/test-amp-style-sanitizer.php +++ b/tests/php/test-amp-style-sanitizer.php @@ -213,7 +213,7 @@ public function get_body_style_attribute_data() { '
', '
', [ - '@media screen and ( max-width: 640px ){body{font-size:small}}@font-face{font-family:"Open Sans";src:url("/fonts/OpenSans-Regular-webfont.woff2") format("woff2")}@-moz-document url-prefix(){body{color:red}}@supports (display: grid){div{display:grid}}@-moz-keyframes appear{from{opacity:0}to{opacity:1}}@keyframes appear{from{opacity:0}to{opacity:1}}', + '@media screen and ( max-width: 640px ){body{font-size:small}}@font-face{font-family:"Open Sans";src:url("/fonts/OpenSans-Regular-webfont.woff2") format("woff2");font-display:optional}@-moz-document url-prefix(){body{color:red}}@supports (display: grid){div{display:grid}}@-moz-keyframes appear{from{opacity:0}to{opacity:1}}@keyframes appear{from{opacity:0}to{opacity:1}}', ], ], @@ -2112,7 +2112,7 @@ public function test_large_custom_css_and_rule_removal() { '.b{color:blue}', '#exists{color:white}', 'span{color:white}', - '@font-face{font-family:"Open Sans";src:url("/fonts/OpenSans-Regular-webfont.woff2") format("woff2")}', + '@font-face{font-family:"Open Sans";src:url("/fonts/OpenSans-Regular-webfont.woff2") format("woff2");font-display:optional}', '.b{background:lightblue}', '@media screen and (max-width: 1000px){@supports (display: grid){.b::before{content:"@media screen and (max-width: 1000px) {"}.b::after{content:"}"}}}@media print{@media print{@media print{.b{color:blue}}}}@media screen and (min-width: 750px) and (max-width: 999px){.b::before{content:"@media screen and (max-width: 1000px) {}";content:"@media screen and (max-width: 1000px) {}"}}', ], From 3eda2bc20ed68942dc8650653122d364eaa8333d Mon Sep 17 00:00:00 2001 From: Bartosz Gadomski Date: Wed, 10 Nov 2021 15:51:10 +0100 Subject: [PATCH 10/21] Logic for 'font-display' property and fonts preloading improved --- .../sanitizers/class-amp-style-sanitizer.php | 94 +++++++++---------- tests/php/test-amp-style-sanitizer.php | 54 +++++++---- 2 files changed, 83 insertions(+), 65 deletions(-) diff --git a/includes/sanitizers/class-amp-style-sanitizer.php b/includes/sanitizers/class-amp-style-sanitizer.php index 2458e1f88b3..d150a00a9ec 100644 --- a/includes/sanitizers/class-amp-style-sanitizer.php +++ b/includes/sanitizers/class-amp-style-sanitizer.php @@ -140,6 +140,7 @@ class AMP_Style_Sanitizer extends AMP_Base_Sanitizer { * @type bool $skip_tree_shaking Whether tree shaking should be skipped. * @type bool $allow_excessive_css Whether to allow CSS to exceed the allowed max bytes (without raising validation errors). * @type bool $transform_important_qualifiers Whether !important rules should be transformed. This also necessarily transform inline style attributes. + * @type string[] $font_face_display_overrides Array of the font family names and the font-display value they should each have. * } */ protected $args; @@ -167,6 +168,11 @@ class AMP_Style_Sanitizer extends AMP_Base_Sanitizer { 'skip_tree_shaking' => false, 'allow_excessive_css' => false, 'transform_important_qualifiers' => true, + 'font_face_display_overrides' => [ + 'NonBreakingSpaceOverride' => 'optional', + 'Inter var' => 'optional', + 'Genericons' => 'auto', + ], ]; /** @@ -1656,6 +1662,7 @@ private function get_parsed_stylesheet( $stylesheet, $options = [] ) { 'parsed_cache_variant', 'dynamic_element_selectors', 'transform_important_qualifiers', + 'font_face_display_overrides', ] ), [ @@ -2540,11 +2547,10 @@ private function process_font_face_at_rule( AtRuleSet $ruleset, $options ) { $stylesheet_base_url = trailingslashit( $stylesheet_base_url ); } - // Define array of fonts to be preloaded. - $fonts_to_preload = []; + // Define array of font files. + $font_files = []; // Attempt to transform data: URLs in src properties to be external file URLs. - $converted_data_urls = []; foreach ( $src_properties as $src_property ) { $value = $src_property->getValue(); if ( ! ( $value instanceof RuleValueList ) ) { @@ -2627,6 +2633,7 @@ private function process_font_face_at_rule( AtRuleSet $ruleset, $options ) { $source_data_url_objects[ $format_value ] = $source[0]; } else { $source_file_urls[] = $value; + $font_files[] = $value; } } @@ -2670,7 +2677,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_data_urls[] = $data_url; + $font_files[] = $guessed_url; continue 2; } } @@ -2683,58 +2690,51 @@ private function process_font_face_at_rule( AtRuleSet $ruleset, $options ) { 'genericons.woff', ]; if ( in_array( $font_filename, $bundled_fonts, true ) ) { - $data_url->getURL()->setString( plugin_dir_url( AMP__FILE__ ) . "assets/fonts/$font_filename" ); - $converted_data_urls[] = $data_url; + $font_file = plugin_dir_url( AMP__FILE__ ) . "assets/fonts/$font_filename"; + $data_url->getURL()->setString( $font_file ); + $font_files[] = $font_file; } } // End foreach $source_data_url_objects. - - // Loop through fonts loaded from files. - foreach ( $source_file_urls as $source_file_url ) { - $properties = $ruleset->getRules( 'font-display' ); - - if ( ! isset( $properties[0] ) ) { - // If a given font is loaded from file and without font-display property, add font-display:optional and preload it. - $font_display_rule = new Rule( 'font-display' ); - $font_display_rule->setValue( 'optional' ); - $ruleset->addRule( $font_display_rule ); - $fonts_to_preload[] = $source_file_url; - } else if ( 'optional' === $properties[0]->getValue() ) { - // If a given font is loaded from file and with font-display:optional, preload it. - $fonts_to_preload[] = $source_file_url; - } - } // End foreach $source_file_urls. } // End foreach $src_properties. - /* - * 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. - * - * @see: https://github.com/ampproject/amp-wp/issues/6036 + /** + * Override the 'font-display' property to improve font performance. */ - if ( count( $converted_data_urls ) > 0 && 0 === count( $ruleset->getRules( 'font-display' ) ) ) { + if ( isset( $font_family ) && in_array( $font_family, array_keys( $this->args['font_face_display_overrides'] ), true ) ) { + $properties = $ruleset->getRules( 'font-display' ); + if ( isset( $properties[0] ) ) { + $ruleset->removeRule( $properties[0] ); + } + $font_display_rule = new Rule( 'font-display' ); - $font_display_rule->setValue( 'optional' ); + $font_display_rule->setValue( $this->args['font_face_display_overrides'][ $font_family ] ); $ruleset->addRule( $font_display_rule ); - $fonts_to_preload[] = $converted_data_urls[0]->getURL()->getString(); } - // Preload the first data: URL that was converted to an external file. - if ( ! empty( $fonts_to_preload ) ) { - foreach ( $fonts_to_preload as $font_to_preload ) { - $link = AMP_DOM_Utils::create_node( - $this->dom, - Tag::LINK, - [ - Attribute::REL => Attribute::REL_PRELOAD, - Attribute::AS_ => 'font', - Attribute::HREF => $font_to_preload, - Attribute::CROSSORIGIN => '', - ] - ); - $this->dom->head->insertBefore( $link ); // Note that \AMP_Theme_Support::ensure_required_markup() will put this in the optimal order. - } + /** + * If the font-display is auto, block, or swap then we should automatically add the preload link for the first font file. + */ + $properties = $ruleset->getRules( 'font-display' ); + if ( + ( + ( isset( $properties[0] ) && in_array( $properties[0]->getValue(), [ 'auto', 'block', 'swap' ], true ) ) + || + ( ! isset( $properties[0] ) ) // Defaults to 'auto', hence should be preloaded as well. + ) + && + 1 <= count( $font_files ) + ) { + $link = AMP_DOM_Utils::create_node( + $this->dom, + Tag::LINK, + [ + Attribute::REL => Attribute::REL_PRELOAD, + Attribute::AS_ => 'font', + Attribute::HREF => $font_files[0], + Attribute::CROSSORIGIN => '', + ] + ); + $this->dom->head->insertBefore( $link ); // Note that \AMP_Theme_Support::ensure_required_markup() will put this in the optimal order. } } diff --git a/tests/php/test-amp-style-sanitizer.php b/tests/php/test-amp-style-sanitizer.php index 435b61f0908..eaeb29b0782 100644 --- a/tests/php/test-amp-style-sanitizer.php +++ b/tests/php/test-amp-style-sanitizer.php @@ -213,7 +213,7 @@ public function get_body_style_attribute_data() { '
', '
', [ - '@media screen and ( max-width: 640px ){body{font-size:small}}@font-face{font-family:"Open Sans";src:url("/fonts/OpenSans-Regular-webfont.woff2") format("woff2");font-display:optional}@-moz-document url-prefix(){body{color:red}}@supports (display: grid){div{display:grid}}@-moz-keyframes appear{from{opacity:0}to{opacity:1}}@keyframes appear{from{opacity:0}to{opacity:1}}', + '@media screen and ( max-width: 640px ){body{font-size:small}}@font-face{font-family:"Open Sans";src:url("/fonts/OpenSans-Regular-webfont.woff2") format("woff2")}@-moz-document url-prefix(){body{color:red}}@supports (display: grid){div{display:grid}}@-moz-keyframes appear{from{opacity:0}to{opacity:1}}@keyframes appear{from{opacity:0}to{opacity:1}}', ], ], @@ -1905,13 +1905,13 @@ public function test_font_data_url_handling_without_file_sources() { $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:optional', $actual_stylesheets[1] ); + $this->assertStringContainsString( 'font-display:auto', $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:optional', $actual_stylesheets[2] ); + $this->assertStringNotContainsString( 'font-display:', $actual_stylesheets[2] ); } /** @return array */ @@ -1919,20 +1919,36 @@ public function get_data_to_test_font_files_preloading() { return [ 'twentynineteen' => [ 'theme_slug' => 'twentynineteen', - 'expected_urls' => [ - '/themes/twentynineteen/fonts/NonBreakingSpaceOverride.woff2', - ], + 'expected_urls' => [], // Twenty Nineteen theme uses "NonBreakingSpaceOverride" font which should use 'font-display:optional' property, thus should not be preloaded. ], 'twentytwenty' => [ 'theme_slug' => 'twentytwenty', - 'expected_urls' => [ - '/plugins/amp/assets/fonts/nonbreakingspaceoverride.woff2', - ], + 'expected_urls' => [], // Twenty Twenty theme uses "Inter var" and "NonBreakingSpaceOverride" font which should use 'font-display:optional' property, thus should not be preloaded. ], 'twentytwentyone' => [ 'theme_slug' => 'twentytwentyone', 'expected_urls' => [], // Twenty Twenty-One theme uses system font stack, no extra fonts are enqueued. ], + 'custom_swap' => [ + 'theme_slug' => '', + 'expected_urls' => [ + '/fonts/OpenSans-Regular-webfont.woff2', + ], + 'html' => '', + ], + 'custom_optional' => [ + 'theme_slug' => '', + 'expected_urls' => [], + 'html' => '', + ], + 'custom_combined' => [ + 'theme_slug' => '', + 'expected_urls' => [ + '/fonts/OpenSans-Regular-webfont.woff2', + '/fonts/Lato-Regular-webfont.woff2', + ], + 'html' => '', + ], ]; } @@ -1942,15 +1958,17 @@ public function get_data_to_test_font_files_preloading() { * @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() ); - } + public function test_font_files_preloading( $theme_slug, $expected_urls, $html = '' ) { + if ( ! empty( $theme_slug ) ) { + $theme = new WP_Theme( $theme_slug, ABSPATH . 'wp-content/themes' ); + if ( $theme->errors() ) { + $this->markTestSkipped( $theme->errors()->get_error_message() ); + } - $html = ''; - $html .= sprintf( '', esc_url( $theme->get_stylesheet_directory_uri() . '/style.css' ) ); // phpcs:ignore WordPress.WP.EnqueuedResources.NonEnqueuedStylesheet - $html .= ''; + $html = ''; + $html .= sprintf( '', esc_url( $theme->get_stylesheet_directory_uri() . '/style.css' ) ); // phpcs:ignore WordPress.WP.EnqueuedResources.NonEnqueuedStylesheet + $html .= ''; + } $dom = Document::fromHtml( $html, Options::DEFAULTS ); $error_codes = []; @@ -2112,7 +2130,7 @@ public function test_large_custom_css_and_rule_removal() { '.b{color:blue}', '#exists{color:white}', 'span{color:white}', - '@font-face{font-family:"Open Sans";src:url("/fonts/OpenSans-Regular-webfont.woff2") format("woff2");font-display:optional}', + '@font-face{font-family:"Open Sans";src:url("/fonts/OpenSans-Regular-webfont.woff2") format("woff2")}', '.b{background:lightblue}', '@media screen and (max-width: 1000px){@supports (display: grid){.b::before{content:"@media screen and (max-width: 1000px) {"}.b::after{content:"}"}}}@media print{@media print{@media print{.b{color:blue}}}}@media screen and (min-width: 750px) and (max-width: 999px){.b::before{content:"@media screen and (max-width: 1000px) {}";content:"@media screen and (max-width: 1000px) {}"}}', ], From 49c1103221475a09c9c3d9020083a64c6ea26e93 Mon Sep 17 00:00:00 2001 From: Bartosz Gadomski Date: Wed, 10 Nov 2021 16:26:55 +0100 Subject: [PATCH 11/21] Use LinkManager instead of AMP_DOM_Utils::create_node --- includes/sanitizers/class-amp-style-sanitizer.php | 12 +----------- 1 file changed, 1 insertion(+), 11 deletions(-) diff --git a/includes/sanitizers/class-amp-style-sanitizer.php b/includes/sanitizers/class-amp-style-sanitizer.php index d150a00a9ec..00cc971fe76 100644 --- a/includes/sanitizers/class-amp-style-sanitizer.php +++ b/includes/sanitizers/class-amp-style-sanitizer.php @@ -2724,17 +2724,7 @@ private function process_font_face_at_rule( AtRuleSet $ruleset, $options ) { && 1 <= count( $font_files ) ) { - $link = AMP_DOM_Utils::create_node( - $this->dom, - Tag::LINK, - [ - Attribute::REL => Attribute::REL_PRELOAD, - Attribute::AS_ => 'font', - Attribute::HREF => $font_files[0], - Attribute::CROSSORIGIN => '', - ] - ); - $this->dom->head->insertBefore( $link ); // Note that \AMP_Theme_Support::ensure_required_markup() will put this in the optimal order. + $this->dom->links->addPreload( $font_files[0], 'font' ); } } From 009333be7e2228bd19ae7697ba8d2aec82a8f489 Mon Sep 17 00:00:00 2001 From: Bartosz Gadomski Date: Wed, 10 Nov 2021 16:28:14 +0100 Subject: [PATCH 12/21] Update request destination parameter --- includes/sanitizers/class-amp-style-sanitizer.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/includes/sanitizers/class-amp-style-sanitizer.php b/includes/sanitizers/class-amp-style-sanitizer.php index 00cc971fe76..482b6143db7 100644 --- a/includes/sanitizers/class-amp-style-sanitizer.php +++ b/includes/sanitizers/class-amp-style-sanitizer.php @@ -18,6 +18,7 @@ use AmpProject\Dom\Element; use AmpProject\Exception\FailedToGetFromRemoteUrl; use AmpProject\RemoteGetRequest; +use AmpProject\RequestDestination; use Sabberworm\CSS\RuleSet\DeclarationBlock; use Sabberworm\CSS\CSSList\CSSList; use Sabberworm\CSS\Property\Selector; @@ -2724,7 +2725,7 @@ private function process_font_face_at_rule( AtRuleSet $ruleset, $options ) { && 1 <= count( $font_files ) ) { - $this->dom->links->addPreload( $font_files[0], 'font' ); + $this->dom->links->addPreload( $font_files[0], RequestDestination::FONT ); } } From 4c5b5863ad1ad3ebc9c13674996b0316f99e044b Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Tue, 16 Nov 2021 12:07:36 -0800 Subject: [PATCH 13/21] Use font-display:block for font icon instead of font-display:auto --- includes/sanitizers/class-amp-style-sanitizer.php | 2 +- tests/php/test-amp-style-sanitizer.php | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/includes/sanitizers/class-amp-style-sanitizer.php b/includes/sanitizers/class-amp-style-sanitizer.php index 482b6143db7..9633aa61950 100644 --- a/includes/sanitizers/class-amp-style-sanitizer.php +++ b/includes/sanitizers/class-amp-style-sanitizer.php @@ -172,7 +172,7 @@ class AMP_Style_Sanitizer extends AMP_Base_Sanitizer { 'font_face_display_overrides' => [ 'NonBreakingSpaceOverride' => 'optional', 'Inter var' => 'optional', - 'Genericons' => 'auto', + 'Genericons' => 'block', ], ]; diff --git a/tests/php/test-amp-style-sanitizer.php b/tests/php/test-amp-style-sanitizer.php index eaeb29b0782..454f073fc00 100644 --- a/tests/php/test-amp-style-sanitizer.php +++ b/tests/php/test-amp-style-sanitizer.php @@ -1905,7 +1905,7 @@ public function test_font_data_url_handling_without_file_sources() { $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:auto', $actual_stylesheets[1] ); + $this->assertStringContainsString( 'font-display:block', $actual_stylesheets[1] ); // Check font not included anywhere, so must remain inline. $this->assertStringContainsString( '@font-face{font-family:"Custom";', $actual_stylesheets[2] ); From 97f849790409face27af75950d900d95c86ed2bd Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Tue, 16 Nov 2021 12:14:25 -0800 Subject: [PATCH 14/21] Improve PHP CSS Parser API usage --- .../sanitizers/class-amp-style-sanitizer.php | 28 ++++++++----------- 1 file changed, 12 insertions(+), 16 deletions(-) diff --git a/includes/sanitizers/class-amp-style-sanitizer.php b/includes/sanitizers/class-amp-style-sanitizer.php index 9633aa61950..1402561a9e6 100644 --- a/includes/sanitizers/class-amp-style-sanitizer.php +++ b/includes/sanitizers/class-amp-style-sanitizer.php @@ -2528,10 +2528,12 @@ 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] ) ) { - $font_family = trim( $properties[0]->getValue(), '"\'' ); + $property = end( $properties ); + if ( $property instanceof Rule ) { + $font_family = trim( $property->getValue(), '"\'' ); // Remove all non-word characters from the font family to serve as the filename. $font_basename = preg_replace( '/[^A-Za-z0-9_\-]/', '', $font_family ); // Same as sanitize_key() minus case changes. @@ -2698,29 +2700,23 @@ private function process_font_face_at_rule( AtRuleSet $ruleset, $options ) { } // End foreach $source_data_url_objects. } // End foreach $src_properties. - /** - * Override the 'font-display' property to improve font performance. - */ - if ( isset( $font_family ) && in_array( $font_family, array_keys( $this->args['font_face_display_overrides'] ), true ) ) { - $properties = $ruleset->getRules( 'font-display' ); - if ( isset( $properties[0] ) ) { - $ruleset->removeRule( $properties[0] ); - } - + // Override the 'font-display' property to improve font performance. + if ( $font_family && in_array( $font_family, array_keys( $this->args['font_face_display_overrides'] ), true ) ) { + $ruleset->removeRule( 'font-display' ); $font_display_rule = new Rule( 'font-display' ); $font_display_rule->setValue( $this->args['font_face_display_overrides'][ $font_family ] ); $ruleset->addRule( $font_display_rule ); } - /** - * If the font-display is auto, block, or swap then we should automatically add the preload link for the first font file. - */ + // If the font-display is auto, block, or swap then we should automatically add the preload link for the first font file. $properties = $ruleset->getRules( 'font-display' ); + $property = end( $properties ); // Last since the last property wins in CSS. if ( ( - ( isset( $properties[0] ) && in_array( $properties[0]->getValue(), [ 'auto', 'block', 'swap' ], true ) ) + // Defaults to 'auto', hence should be preloaded as well. + ! $property instanceof Rule || - ( ! isset( $properties[0] ) ) // Defaults to 'auto', hence should be preloaded as well. + in_array( $property->getValue(), [ 'auto', 'block', 'swap' ], true ) ) && 1 <= count( $font_files ) From 7c6afe798054709780f2484f307ac01d2924fd43 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Tue, 16 Nov 2021 12:27:25 -0800 Subject: [PATCH 15/21] Add failing test case for no preload link being added when the first font file is a data: URL --- tests/php/test-amp-style-sanitizer.php | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/tests/php/test-amp-style-sanitizer.php b/tests/php/test-amp-style-sanitizer.php index 454f073fc00..dbf14dd68d4 100644 --- a/tests/php/test-amp-style-sanitizer.php +++ b/tests/php/test-amp-style-sanitizer.php @@ -1949,6 +1949,11 @@ public function get_data_to_test_font_files_preloading() { ], 'html' => '', ], + 'custom_inlined' => [ + 'theme_slug' => '', + 'expected_urls' => [], // No preload link should be present because the data: URL was not converted into an external link. + 'html' => '', + ], ]; } @@ -1984,7 +1989,14 @@ public function test_font_files_preloading( $theme_slug, $expected_urls, $html = $link_elements = $dom->getElementsByTagName( 'link' ); $link_elements_count = $link_elements->length; - $this->assertEquals( count( $expected_urls ), $link_elements_count ); + $actual_urls = array_map( + static function ( Element $link_element ) { + return $link_element->getAttribute( 'href' ); + }, + iterator_to_array( $link_elements ) + ); + + $this->assertEqualSets( $expected_urls, $actual_urls ); for ( $i = 0; $i < $link_elements_count; $i++ ) { $this->assertStringEndsWith( From e02b96ab21e77de1fd2b4e85335fa315959fcea4 Mon Sep 17 00:00:00 2001 From: Bartosz Gadomski Date: Wed, 17 Nov 2021 14:24:31 +0100 Subject: [PATCH 16/21] Font preload logic improved --- .../sanitizers/class-amp-style-sanitizer.php | 29 ++++++++++++++----- 1 file changed, 22 insertions(+), 7 deletions(-) diff --git a/includes/sanitizers/class-amp-style-sanitizer.php b/includes/sanitizers/class-amp-style-sanitizer.php index 1402561a9e6..ea298329ab8 100644 --- a/includes/sanitizers/class-amp-style-sanitizer.php +++ b/includes/sanitizers/class-amp-style-sanitizer.php @@ -2550,8 +2550,9 @@ private function process_font_face_at_rule( AtRuleSet $ruleset, $options ) { $stylesheet_base_url = trailingslashit( $stylesheet_base_url ); } - // Define array of font files. - $font_files = []; + // Obtain the font file path (if any) and the first font src type. + $font_file = ''; + $first_src_type = ''; // Attempt to transform data: URLs in src properties to be external file URLs. foreach ( $src_properties as $src_property ) { @@ -2634,9 +2635,15 @@ private function process_font_face_at_rule( AtRuleSet $ruleset, $options ) { $value = $url->getURL()->getString(); if ( 'data:' === substr( $value, 0, 5 ) ) { $source_data_url_objects[ $format_value ] = $source[0]; + if ( empty( $first_src_type ) ) { + $first_src_type = 'inline'; + } } else { $source_file_urls[] = $value; - $font_files[] = $value; + if ( empty( $first_src_type ) ) { + $first_src_type = 'file'; + $font_file = $value; + } } } @@ -2680,7 +2687,10 @@ 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 ); - $font_files[] = $guessed_url; + if ( 'inline' === $first_src_type ) { + $first_src_type = 'file'; + $font_file = $guessed_url; + } continue 2; } } @@ -2695,7 +2705,10 @@ private function process_font_face_at_rule( AtRuleSet $ruleset, $options ) { if ( in_array( $font_filename, $bundled_fonts, true ) ) { $font_file = plugin_dir_url( AMP__FILE__ ) . "assets/fonts/$font_filename"; $data_url->getURL()->setString( $font_file ); - $font_files[] = $font_file; + if ( 'inline' === $first_src_type ) { + $first_src_type = 'file'; + $font_file = $font_file; + } } } // End foreach $source_data_url_objects. } // End foreach $src_properties. @@ -2719,9 +2732,11 @@ private function process_font_face_at_rule( AtRuleSet $ruleset, $options ) { in_array( $property->getValue(), [ 'auto', 'block', 'swap' ], true ) ) && - 1 <= count( $font_files ) + 'file' === $first_src_type + && + ! empty( $font_file ) ) { - $this->dom->links->addPreload( $font_files[0], RequestDestination::FONT ); + $this->dom->links->addPreload( $font_file, RequestDestination::FONT ); } } From 95c7da2384cb6719625f8755929c8b56121defe0 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Wed, 17 Nov 2021 10:57:30 -0800 Subject: [PATCH 17/21] Add another failing test to ensure preload links are added when parsed stylesheet is cached --- tests/php/test-amp-style-sanitizer.php | 57 ++++++++++++++------------ 1 file changed, 30 insertions(+), 27 deletions(-) diff --git a/tests/php/test-amp-style-sanitizer.php b/tests/php/test-amp-style-sanitizer.php index dbf14dd68d4..b428fd9e876 100644 --- a/tests/php/test-amp-style-sanitizer.php +++ b/tests/php/test-amp-style-sanitizer.php @@ -1975,37 +1975,40 @@ public function test_font_files_preloading( $theme_slug, $expected_urls, $html = $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 ); + // Do the assertions twice in order to ensure that preloads are still added when the CSS is cached. + foreach ( [ 'false', 'true' ] as $is_cached ) { + $dom = Document::fromHtml( $html, Options::DEFAULTS ); + $error_codes = []; + $sanitizer = new AMP_Style_Sanitizer( + $dom, + [ + 'use_document_element' => true, + ] + ); + $sanitizer->sanitize(); + $this->assertEquals( [], $error_codes, "Is cached: $is_cached" ); - $link_elements = $dom->getElementsByTagName( 'link' ); - $link_elements_count = $link_elements->length; + $link_elements = $dom->getElementsByTagName( 'link' ); + $link_elements_count = $link_elements->length; - $actual_urls = array_map( - static function ( Element $link_element ) { - return $link_element->getAttribute( 'href' ); - }, - iterator_to_array( $link_elements ) - ); + $actual_urls = array_map( + static function ( Element $link_element ) { + return $link_element->getAttribute( 'href' ); + }, + iterator_to_array( $link_elements ) + ); - $this->assertEqualSets( $expected_urls, $actual_urls ); + $this->assertEqualSets( $expected_urls, $actual_urls, "Is cached: $is_cached" ); - 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' ), '' ); + 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' ), '' ); + } } } From 6c1611367ad12c831df0f0492c25c06544575dd7 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Wed, 17 Nov 2021 11:00:52 -0800 Subject: [PATCH 18/21] Eliminate redundant code --- includes/sanitizers/class-amp-style-sanitizer.php | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/includes/sanitizers/class-amp-style-sanitizer.php b/includes/sanitizers/class-amp-style-sanitizer.php index ea298329ab8..6f7c5d80008 100644 --- a/includes/sanitizers/class-amp-style-sanitizer.php +++ b/includes/sanitizers/class-amp-style-sanitizer.php @@ -2705,10 +2705,7 @@ private function process_font_face_at_rule( AtRuleSet $ruleset, $options ) { if ( in_array( $font_filename, $bundled_fonts, true ) ) { $font_file = plugin_dir_url( AMP__FILE__ ) . "assets/fonts/$font_filename"; $data_url->getURL()->setString( $font_file ); - if ( 'inline' === $first_src_type ) { - $first_src_type = 'file'; - $font_file = $font_file; - } + $first_src_type = 'file'; } } // End foreach $source_data_url_objects. } // End foreach $src_properties. From 773395e43f5fd86f85e980b6a56c34ae9f6de4b3 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Wed, 17 Nov 2021 11:36:09 -0800 Subject: [PATCH 19/21] Only catch exceptions specific to php-css-parser when parsing --- includes/sanitizers/class-amp-style-sanitizer.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/includes/sanitizers/class-amp-style-sanitizer.php b/includes/sanitizers/class-amp-style-sanitizer.php index 6f7c5d80008..ae3dd00fb86 100644 --- a/includes/sanitizers/class-amp-style-sanitizer.php +++ b/includes/sanitizers/class-amp-style-sanitizer.php @@ -23,6 +23,7 @@ use Sabberworm\CSS\CSSList\CSSList; use Sabberworm\CSS\Property\Selector; use Sabberworm\CSS\RuleSet\RuleSet; +use Sabberworm\CSS\Parsing\SourceException; use Sabberworm\CSS\Property\AtRule; use Sabberworm\CSS\Rule\Rule; use Sabberworm\CSS\CSSList\KeyFrame; @@ -1934,7 +1935,7 @@ static function ( $value ) { ); $important_count = $processed_css_list['important_count']; $imported_font_urls = $processed_css_list['imported_font_urls']; - } catch ( Exception $exception ) { + } catch ( SourceException $exception ) { $error = [ 'code' => self::CSS_SYNTAX_PARSE_ERROR, 'message' => $exception->getMessage(), From b3b6579c5d2cf97b7e885ac98f804d57df55f519 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Wed, 17 Nov 2021 11:49:57 -0800 Subject: [PATCH 20/21] Ensure font preloads are added for cached stylesheets --- .../sanitizers/class-amp-style-sanitizer.php | 151 +++++++++++------- 1 file changed, 95 insertions(+), 56 deletions(-) diff --git a/includes/sanitizers/class-amp-style-sanitizer.php b/includes/sanitizers/class-amp-style-sanitizer.php index ae3dd00fb86..dfb341261b6 100644 --- a/includes/sanitizers/class-amp-style-sanitizer.php +++ b/includes/sanitizers/class-amp-style-sanitizer.php @@ -1427,6 +1427,7 @@ private function process_style_element( DOMElement $element ) { 'imported_font_urls' => $parsed['imported_font_urls'], 'important_count' => $parsed['important_count'], 'kept_error_count' => $parsed['kept_error_count'], + 'preload_font_urls' => $parsed['preload_font_urls'], ]; // Remove from DOM since we'll be adding it to a newly-created style[amp-custom] element later. @@ -1531,6 +1532,7 @@ private function process_link_element( DOMElement $element ) { 'imported_font_urls' => $parsed['imported_font_urls'], 'important_count' => $parsed['important_count'], 'kept_error_count' => $parsed['kept_error_count'], + 'preload_font_urls' => $parsed['preload_font_urls'], ]; // Remove now that styles have been processed. @@ -1626,16 +1628,17 @@ private function fetch_external_stylesheet( $url ) { * @return array { * Processed stylesheet. * - * @type array $tokens Stylesheet tokens, where arrays are tuples for declaration blocks. - * @type string $hash MD5 hash of the parsed stylesheet. - * @type array $validation_results Validation results, array containing arrays with error and sanitized keys. - * @type array $imported_font_urls Imported font stylesheet URLs. - * @type int $priority The priority of the stylesheet. - * @type float $parse_time The time duration it took to parse the stylesheet, in milliseconds. - * @type float $shake_time The time duration it took to tree-shake the stylesheet, in milliseconds. - * @type bool $cached Whether the parsed stylesheet was cached. - * @type int $important_count Number of !important qualifiers. - * @type int $kept_error_count Number of instances of invalid markup causing validation errors which are kept. + * @type array $tokens Stylesheet tokens, where arrays are tuples for declaration blocks. + * @type string $hash MD5 hash of the parsed stylesheet. + * @type array $validation_results Validation results, array containing arrays with error and sanitized keys. + * @type array $imported_font_urls Imported font stylesheet URLs. + * @type int $priority The priority of the stylesheet. + * @type float $parse_time The time duration it took to parse the stylesheet, in milliseconds. + * @type float $shake_time The time duration it took to tree-shake the stylesheet, in milliseconds. + * @type bool $cached Whether the parsed stylesheet was cached. + * @type int $important_count Number of !important qualifiers. + * @type int $kept_error_count Number of instances of invalid markup causing validation errors which are kept. + * @type string[] $preload_font_urls Font URLs to preload. * } */ private function get_parsed_stylesheet( $stylesheet, $options = [] ) { @@ -1757,10 +1760,11 @@ private function should_use_transient_caching() { * @return array { * Results. * - * @type array $validation_results Validation results. - * @type array $imported_font_urls Imported font URLs. - * @type array $viewport_rules Extracted viewport rules. - * @type int $important_count Number of !important qualifiers. + * @type array $validation_results Validation results. + * @type array $imported_font_urls Imported font URLs. + * @type array $viewport_rules Extracted viewport rules. + * @type int $important_count Number of !important qualifiers. + * @type string[] $preload_font_urls Font URLs to preload. * } */ private function splice_imported_stylesheet( Import $item, CSSList $css_list, $options ) { @@ -1771,6 +1775,7 @@ private function splice_imported_stylesheet( Import $item, CSSList $css_list, $o $location = array_shift( $at_rule_args ); $media_query = array_shift( $at_rule_args ); $important_count = 0; + $preload_font_urls = []; if ( isset( $options['stylesheet_url'] ) ) { $this->real_path_urls( [ $location ], $options['stylesheet_url'] ); @@ -1781,7 +1786,7 @@ private function splice_imported_stylesheet( Import $item, CSSList $css_list, $o // Prevent importing something that has already been imported, and avoid infinite recursion. if ( isset( $this->processed_imported_stylesheet_urls[ $import_stylesheet_url ] ) ) { $css_list->remove( $item ); - return compact( 'validation_results', 'imported_font_urls', 'viewport_rules', 'important_count' ); + return compact( 'validation_results', 'imported_font_urls', 'viewport_rules', 'important_count', 'preload_font_urls' ); } $this->processed_imported_stylesheet_urls[ $import_stylesheet_url ] = true; @@ -1804,7 +1809,7 @@ private function splice_imported_stylesheet( Import $item, CSSList $css_list, $o '1.0' ); - return compact( 'validation_results', 'imported_font_urls', 'viewport_rules', 'important_count' ); + return compact( 'validation_results', 'imported_font_urls', 'viewport_rules', 'important_count', 'preload_font_urls' ); } $stylesheet = $this->get_stylesheet_from_url( $import_stylesheet_url ); @@ -1821,7 +1826,7 @@ private function splice_imported_stylesheet( Import $item, CSSList $css_list, $o } $validation_results[] = compact( 'error', 'sanitized' ); - return compact( 'validation_results', 'imported_font_urls', 'viewport_rules', 'important_count' ); + return compact( 'validation_results', 'imported_font_urls', 'viewport_rules', 'important_count', 'preload_font_urls' ); } if ( $media_query ) { @@ -1834,6 +1839,7 @@ private function splice_imported_stylesheet( Import $item, CSSList $css_list, $o $validation_results = array_merge( $validation_results, $parsed_stylesheet['validation_results'] ); $viewport_rules = $parsed_stylesheet['viewport_rules']; $important_count = $parsed_stylesheet['important_count']; + $preload_font_urls = $parsed_stylesheet['preload_font_urls']; if ( ! empty( $parsed_stylesheet['css_document'] ) ) { /** @@ -1848,7 +1854,7 @@ private function splice_imported_stylesheet( Import $item, CSSList $css_list, $o $css_list->remove( $item ); } - return compact( 'validation_results', 'imported_font_urls', 'viewport_rules', 'important_count' ); + return compact( 'validation_results', 'imported_font_urls', 'viewport_rules', 'important_count', 'preload_font_urls' ); } /** @@ -1893,6 +1899,7 @@ private function replace_inside_css_list( CSSList $css_list, $old_item, $new_ite * @type array $imported_font_urls Imported font URLs. * @type array $viewport_rules Extracted viewport rules. * @type int $important_count Number of !important qualifiers. + * @type string[] $preload_font_urls Font URLs to preload. * } */ private function create_validated_css_document( $stylesheet_string, $options ) { @@ -1900,6 +1907,7 @@ private function create_validated_css_document( $stylesheet_string, $options ) { $imported_font_urls = []; $viewport_rules = []; $important_count = 0; + $preload_font_urls = []; $css_document = null; // Note that there is no known case where an exception can be thrown here since PHP-CSS-Parser is using lenient parsing. @@ -1935,6 +1943,7 @@ static function ( $value ) { ); $important_count = $processed_css_list['important_count']; $imported_font_urls = $processed_css_list['imported_font_urls']; + $preload_font_urls = array_merge( $preload_font_urls, $processed_css_list['preload_font_urls'] ); } catch ( SourceException $exception ) { $error = [ 'code' => self::CSS_SYNTAX_PARSE_ERROR, @@ -1951,7 +1960,7 @@ static function ( $value ) { $validation_results[] = compact( 'error', 'sanitized' ); } - return compact( 'validation_results', 'css_document', 'imported_font_urls', 'viewport_rules', 'important_count' ); + return compact( 'validation_results', 'css_document', 'imported_font_urls', 'viewport_rules', 'important_count', 'preload_font_urls' ); } /** @@ -1968,12 +1977,13 @@ static function ( $value ) { * @return array { * Parsed stylesheet. * - * @type array $tokens Stylesheet tokens, where arrays are tuples for declaration blocks. - * @type string $hash MD5 hash of the parsed stylesheet. - * @type array $validation_results Validation results, array containing arrays with error and sanitized keys. - * @type array $imported_font_urls Imported font stylesheet URLs. - * @type float $parse_time The time duration it took to parse the stylesheet, in milliseconds. - * @type int $important_count Number of !important qualifiers. + * @type array $tokens Stylesheet tokens, where arrays are tuples for declaration blocks. + * @type string $hash MD5 hash of the parsed stylesheet. + * @type array $validation_results Validation results, array containing arrays with error and sanitized keys. + * @type array $imported_font_urls Imported font stylesheet URLs. + * @type float $parse_time The time duration it took to parse the stylesheet, in milliseconds. + * @type int $important_count Number of !important qualifiers. + * @type string[] $preload_font_urls Font URLs to preload. * } */ private function parse_stylesheet( $stylesheet_string, $options = [] ) { @@ -2166,6 +2176,7 @@ static function( $matches ) use ( $selector, &$selectors_parsed ) { 'parse_time' => ( microtime( true ) - $start_time ), 'viewport_rules' => $parsed_stylesheet['viewport_rules'], 'important_count' => $parsed_stylesheet['important_count'], + 'preload_font_urls' => $parsed_stylesheet['preload_font_urls'], ] ); } @@ -2246,16 +2257,18 @@ static function( $matches ) { * @return array { * Processed CSS list. * - * @type array $validation_results Validation results. - * @type array $viewport_rules Extracted viewport rules. - * @type array $imported_font_urls Imported font URLs. - * @type int $important_count Number of !important qualifiers. + * @type array $validation_results Validation results. + * @type array $viewport_rules Extracted viewport rules. + * @type array $imported_font_urls Imported font URLs. + * @type int $important_count Number of !important qualifiers. + * @type string[] $preload_font_urls Font URLs to preload. * } */ private function process_css_list( CSSList $css_list, $options ) { $validation_results = []; $viewport_rules = []; $imported_font_urls = []; + $preload_font_urls = []; $important_count = 0; foreach ( $css_list->getContents() as $css_item ) { @@ -2264,6 +2277,7 @@ private function process_css_list( CSSList $css_list, $options ) { $processed = $this->process_css_declaration_block( $css_item, $css_list, $options ); $important_count += $processed['important_count']; + $preload_font_urls = array_merge( $preload_font_urls, $processed['preload_font_urls'] ); $validation_results = array_merge( $validation_results, $processed['validation_results'] @@ -2283,6 +2297,7 @@ private function process_css_list( CSSList $css_list, $options ) { $processed = $this->process_css_list( $css_item, $options ); $viewport_rules = array_merge( $viewport_rules, $processed['viewport_rules'] ); $important_count += $processed['important_count']; + $preload_font_urls = array_merge( $preload_font_urls, $processed['preload_font_urls'] ); $validation_results = array_merge( $validation_results, $processed['validation_results'] @@ -2292,6 +2307,7 @@ private function process_css_list( CSSList $css_list, $options ) { $imported_stylesheet = $this->splice_imported_stylesheet( $css_item, $css_list, $options ); $imported_font_urls = array_merge( $imported_font_urls, $imported_stylesheet['imported_font_urls'] ); $validation_results = array_merge( $validation_results, $imported_stylesheet['validation_results'] ); + $preload_font_urls = array_merge( $preload_font_urls, $imported_stylesheet['preload_font_urls'] ); $viewport_rules = array_merge( $viewport_rules, $imported_stylesheet['viewport_rules'] ); $important_count += $imported_stylesheet['important_count']; } elseif ( $css_item instanceof AtRuleSet ) { @@ -2321,6 +2337,7 @@ private function process_css_list( CSSList $css_list, $options ) { $processed = $this->process_css_declaration_block( $css_item, $css_list, $options ); $validation_results = array_merge( $validation_results, $processed['validation_results'] ); $important_count += $processed['important_count']; + $preload_font_urls = array_merge( $preload_font_urls, $processed['preload_font_urls'] ); } } elseif ( $css_item instanceof KeyFrame ) { if ( ! in_array( 'keyframes', $options['allowed_at_rules'], true ) ) { @@ -2374,7 +2391,7 @@ private function process_css_list( CSSList $css_list, $options ) { } } - return compact( 'validation_results', 'imported_font_urls', 'viewport_rules', 'important_count' ); + return compact( 'validation_results', 'imported_font_urls', 'viewport_rules', 'important_count', 'preload_font_urls' ); } /** @@ -2433,13 +2450,15 @@ private function real_path_urls( $urls, $stylesheet_url ) { * @return array { * Results. * - * @type array $validation_results Validation results. - * @type int $important_count Number of !important qualifiers. + * @type array $validation_results Validation results. + * @type int $important_count Number of !important qualifiers. + * @type string[] $preload_font_urls Font URLs to preload. * } */ private function process_css_declaration_block( RuleSet $ruleset, CSSList $css_list, $options ) { $validation_results = []; $important_count = 0; + $preload_font_urls = []; if ( $ruleset instanceof DeclarationBlock ) { $validation_results = array_merge( @@ -2448,7 +2467,7 @@ private function process_css_declaration_block( RuleSet $ruleset, CSSList $css_l ); if ( 0 === count( $ruleset->getSelectors() ) ) { $css_list->remove( $ruleset ); - return compact( 'validation_results', 'important_count' ); + return compact( 'validation_results', 'important_count', 'preload_font_urls' ); } } @@ -2493,7 +2512,7 @@ private function process_css_declaration_block( RuleSet $ruleset, CSSList $css_l } if ( $ruleset instanceof AtRuleSet && 'font-face' === $ruleset->atRuleName() ) { - $this->process_font_face_at_rule( $ruleset, $options ); + $preload_font_urls = $this->process_font_face_at_rule( $ruleset, $options ); } $transformed = $this->transform_important_qualifiers( $ruleset, $css_list, $options ); @@ -2507,7 +2526,7 @@ private function process_css_declaration_block( RuleSet $ruleset, CSSList $css_l if ( 0 === count( $ruleset->getRules() ) ) { $css_list->remove( $ruleset ); } - return compact( 'validation_results', 'important_count' ); + return compact( 'validation_results', 'important_count', 'preload_font_urls' ); } /** @@ -2521,13 +2540,16 @@ private function process_css_declaration_block( RuleSet $ruleset, CSSList $css_l * * @type string $stylesheet_url Stylesheet URL, if available. * } + * @return string[] Font URLs to preload. */ private function process_font_face_at_rule( AtRuleSet $ruleset, $options ) { $src_properties = $ruleset->getRules( 'src' ); if ( empty( $src_properties ) ) { - return; + return []; } + $preload_font_urls = []; + // Obtain the font-family name to guess the filename. $font_family = null; $font_basename = null; @@ -2734,8 +2756,10 @@ private function process_font_face_at_rule( AtRuleSet $ruleset, $options ) { && ! empty( $font_file ) ) { - $this->dom->links->addPreload( $font_file, RequestDestination::FONT ); + $preload_font_urls[] = $font_file; } + + return $preload_font_urls; } /** @@ -2973,20 +2997,21 @@ private function collect_inline_styles( DOMElement $element ) { if ( $parsed['tokens'] ) { $this->pending_stylesheets[] = [ - 'group' => self::STYLE_AMP_CUSTOM_GROUP_INDEX, - 'original_size' => strlen( $rule ), - 'final_size' => null, - 'element' => $element, - 'origin' => 'style_attribute', - 'sources' => $this->current_sources, - 'priority' => $this->get_stylesheet_priority( $attr_node ), - 'tokens' => $parsed['tokens'], - 'hash' => $parsed['hash'], - 'parse_time' => $parsed['parse_time'], - 'shake_time' => null, - 'cached' => $parsed['cached'], - 'important_count' => $parsed['important_count'], - 'kept_error_count' => $parsed['kept_error_count'], + 'group' => self::STYLE_AMP_CUSTOM_GROUP_INDEX, + 'original_size' => strlen( $rule ), + 'final_size' => null, + 'element' => $element, + 'origin' => 'style_attribute', + 'sources' => $this->current_sources, + 'priority' => $this->get_stylesheet_priority( $attr_node ), + 'tokens' => $parsed['tokens'], + 'hash' => $parsed['hash'], + 'parse_time' => $parsed['parse_time'], + 'shake_time' => null, + 'cached' => $parsed['cached'], + 'important_count' => $parsed['important_count'], + 'kept_error_count' => $parsed['kept_error_count'], + 'preload_font_urls' => $parsed['preload_font_urls'], ]; if ( $element->hasAttribute( 'class' ) ) { @@ -3009,6 +3034,8 @@ private function collect_inline_styles( DOMElement $element ) { * @see https://www.ampproject.org/docs/fundamentals/spec#keyframes-stylesheet */ private function finalize_styles() { + $preload_font_urls = []; + $stylesheet_groups = [ self::STYLE_AMP_CUSTOM_GROUP_INDEX => [ 'source_map_comment' => "\n\n/*# sourceURL=amp-custom.css */", @@ -3018,6 +3045,7 @@ private function finalize_styles() { 'important_count' => 0, 'kept_error_count' => 0, 'is_excessive_size' => false, + 'preload_font_urls' => [], ], self::STYLE_AMP_KEYFRAMES_GROUP_INDEX => [ 'source_map_comment' => "\n\n/*# sourceURL=amp-keyframes.css */", @@ -3027,6 +3055,7 @@ private function finalize_styles() { 'important_count' => 0, 'kept_error_count' => 0, 'is_excessive_size' => false, + 'preload_font_urls' => [], ], ]; @@ -3059,6 +3088,13 @@ private function finalize_styles() { return; } + // Add the font preloads. + foreach ( $stylesheet_groups as $stylesheet_group ) { + foreach ( $stylesheet_group['preload_font_urls'] as $preload_font_url ) { + $this->dom->links->addPreload( $preload_font_url, RequestDestination::FONT ); + } + } + // Add style[amp-custom] to document. if ( $stylesheet_groups[ self::STYLE_AMP_CUSTOM_GROUP_INDEX ]['included_count'] > 0 ) { /* @@ -3516,10 +3552,11 @@ static function ( $class_name ) { * @return array { * Finalized group info. * - * @type int $included_count Number of included stylesheets in group. - * @type bool $is_excessive_size Whether the total is greater than the max bytes allowed. - * @type int $important_count Number of !important qualifiers. - * @type int $kept_error_count Number of validation errors whose markup was kept. + * @type int $included_count Number of included stylesheets in group. + * @type bool $is_excessive_size Whether the total is greater than the max bytes allowed. + * @type int $important_count Number of !important qualifiers. + * @type int $kept_error_count Number of validation errors whose markup was kept. + * @type string[] $preload_font_urls Font URLs to preload. * } */ private function finalize_stylesheet_group( $group, $group_config ) { @@ -3529,6 +3566,7 @@ private function finalize_stylesheet_group( $group, $group_config ) { $concatenated_size = 0; $important_count = 0; $kept_error_count = 0; + $preload_font_urls = []; $previously_seen_stylesheet_index = []; foreach ( $this->pending_stylesheets as $pending_stylesheet_index => &$pending_stylesheet ) { @@ -3743,6 +3781,7 @@ function ( $a, $b ) { $this->pending_stylesheets[ $i ]['included'] = true; $included_count++; $concatenated_size += $this->pending_stylesheets[ $i ]['final_size']; + $preload_font_urls = array_merge( $preload_font_urls, $this->pending_stylesheets[ $i ]['preload_font_urls'] ); if ( $is_stylesheet_excessive ) { $is_excessive_size = true; @@ -3755,7 +3794,7 @@ function ( $a, $b ) { } } - return compact( 'included_count', 'is_excessive_size', 'important_count', 'kept_error_count' ); + return compact( 'included_count', 'is_excessive_size', 'important_count', 'kept_error_count', 'preload_font_urls' ); } /** From 96b748bfbe29a2367231b936ec181d282039f7fd Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Wed, 17 Nov 2021 11:51:55 -0800 Subject: [PATCH 21/21] Bust stylesheet cache group --- includes/sanitizers/class-amp-style-sanitizer.php | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/includes/sanitizers/class-amp-style-sanitizer.php b/includes/sanitizers/class-amp-style-sanitizer.php index dfb341261b6..323740f1ec7 100644 --- a/includes/sanitizers/class-amp-style-sanitizer.php +++ b/includes/sanitizers/class-amp-style-sanitizer.php @@ -1642,10 +1642,8 @@ private function fetch_external_stylesheet( $url ) { * } */ private function get_parsed_stylesheet( $stylesheet, $options = [] ) { - $parsed = null; - $cache_key = null; $cached = true; - $cache_group = 'amp-parsed-stylesheet-v37'; // This should be bumped whenever the PHP-CSS-Parser is updated or parsed format is updated. + $cache_group = 'amp-parsed-stylesheet-v38'; // This should be bumped whenever the PHP-CSS-Parser is updated or parsed format is updated. $use_transients = $this->should_use_transient_caching(); // @todo If ValidationExemption::is_px_verified_for_node( $this->current_node ) then keep !important.