Only add all Google Font style rules in editor context by westonruter · Pull Request #3328 · ampproject/amp-wp

westonruter · 2019-09-23T17:20:12Z

I noticed there is an excessive_css problem with stories:

<!--
The style[amp-custom] element is populated with:
   361 B (3%): link#wp-block-library-css[rel=stylesheet][href=https://stories-labs.dev/wp-content/plugins/gutenberg/build/block-library/style.css?ver=1568297729][type=text/css][media=all]
   543 B (47%): link#amp-stories-css[rel=stylesheet][href=https://stories-labs.dev/wp-content/plugins/amp/assets/css/amp-stories.css?ver=1.3-RC1-20190918T205018Z-820046ad][type=text/css][media=all]
   455 B (32%): link#amp-stories-frontend-css[rel=stylesheet][href=https://stories-labs.dev/wp-content/plugins/amp/assets/css/amp-stories-frontend.css?ver=1.3-RC1-20190918T205018Z-820046ad][type=text/css][media=]
   129 B: div.amp-story-block-wrapper amp-wp-2b4f3e6
   128 B: div.amp-story-block-wrapper amp-wp-a96837a
   127 B: div.amp-story-block-wrapper amp-wp-1d1a90b
    90 B: p.wp-block-amp-amp-story-text amp-text-content has-text-color amp-wp-7340773
   129 B: div.amp-story-block-wrapper amp-wp-0624f06
   122 B: h1.wp-block-amp-amp-story-text has-text-color has-background amp-wp-d164d38[data-font-family=Georgia]
    84 B: amp-story-page#14d45f4c-554e-49be-ac09-9b447acaba08.wp-block-amp-amp-story-page amp-wp-74885c3
   129 B: div.amp-story-block-wrapper amp-wp-2ad3080
    86 B: h2.wp-block-amp-amp-story-text has-text-color amp-wp-5ff4663[data-font-family=Georgia]
   127 B: div.amp-story-block-wrapper amp-wp-2040580
   110 B: h2.wp-block-amp-amp-story-text is-style-rounded has-text-color has-background has-vivid-red-color has-vivid-red-background-color amp-wp-2678cb0[data-font-family=Georgia]
   127 B: div.amp-story-block-wrapper amp-wp-20fe417
   121 B: h2.wp-block-amp-amp-story-text is-style-rounded has-text-color has-background has-vivid-red-background-color amp-wp-e8c9813[data-font-family=Georgia]
   128 B: div.amp-story-block-wrapper amp-wp-62c28cc
    90 B: h1.wp-block-amp-amp-story-text amp-text-content has-text-color amp-wp-5bc4f93[data-font-family=Georgia]
Total included size: 3,086 bytes (21% of 14,538 total after tree shaking)

The following stylesheets are too large to be included in style[amp-custom]:
 66365 B: style#amp-stories-inline-css[type=text/css]
Total excluded size: 66,365 bytes (100% of 66,365 total after tree shaking)

Total combined size: 69,451 bytes (85% of 80,903 total after tree shaking)
-->

Notice the amp-stories-inline-css here, being 66KB+.

I rejected the validation errors to see what the underlying markup was prior to sanitizing, and it was many rules like this:

[data-font-family="Alef"] {
	font-family: "Alef", "sans-serif"
}

So the issue is custom font styles being added with this code:

amp-wp/includes/class-amp-story-post-type.php

Lines 833 to 839 in b44ac6a

    
           $fonts = self::get_fonts(); 
        
           foreach ( $fonts as $font ) { 
        
           	wp_add_inline_style( 
        
           		self::AMP_STORIES_STYLE_HANDLE, 
        
           		self::get_inline_font_style_rule( $font ) 
        
           	); 
        
           }

The problem is that only the style rules for fonts that are actually being used should be added to the page. As @swissspidy this is already being done:

amp-wp/includes/class-amp-story-post-type.php

Lines 1386 to 1418 in b44ac6a

    
           	/** 
        
           	 * Include any required Google Font styles when rendering a block in AMP Stories. 
        
           	 * 
        
           	 * @param string $block_content The block content about to be appended. 
        
           	 * @param array  $block         The full block, including name and attributes. 
        
           	 * @return string Block content. 
        
           	 */ 
        
           	public static function render_block_with_google_fonts( $block_content, $block ) { 
        
           		$font_family_attribute = 'ampFontFamily'; 
        
           		// Short-circuit if no font family present. 
        
           		if ( empty( $block['attrs'][ $font_family_attribute ] ) ) { 
        
           			return $block_content; 
        
           		} 
        
           		// Short-circuit if there is no Google Font or the font is already enqueued. 
        
           		$font = self::get_font( $block['attrs'][ $font_family_attribute ] ); 
        
           		if ( ! isset( $font['handle'], $font['src'] ) || ! $font || wp_style_is( $font['handle'] ) ) { 
        
           			return $block_content; 
        
           		} 
        
           		if ( ! wp_style_is( $font['handle'], 'registered' ) ) { 
        
           			wp_register_style( $font['handle'], $font['src'], [], null, 'all' ); // phpcs:ignore WordPress.WP.EnqueuedResourceParameters.MissingVersion 
        
           		} 
        
           		wp_enqueue_style( $font['handle'] ); 
        
           		wp_add_inline_style( 
        
           			$font['handle'], 
        
           			self::get_inline_font_style_rule( $font ) 
        
           		); 
        
           		return $block_content; 
        
           	}

Therefore, it appears these style rules were being added redundantly in the admin editor context and they can just be moved to only be added there.

The reason why this issue would only sometimes occur is because it would only happen when someone picked a custom font. At that point, a data-font-family attribute would appear in the page and the style rules using selectors like [data-font-family="..."] would no longer be tree-shaken.

Before

<!--
The style[amp-custom] element is populated with:
  4343 B (30%): link#wp-block-library-css[rel=stylesheet][href=https://wordpressdev.lndo.site/content/plugins/gutenberg/build/block-library/style.css?ver=1569254363][media=all]
   658 B (57%): link#amp-stories-css[rel=stylesheet][href=https://wordpressdev.lndo.site/content/plugins/amp/assets/css/amp-stories.css?ver=1.3-RC1][media=all]
  1195 B (69%): link#amp-stories-frontend-css[rel=stylesheet][href=https://wordpressdev.lndo.site/content/plugins/amp/assets/css/amp-stories-frontend.css?ver=1.3-RC1][media=]
    75 B: style#bubblegum-sans-font-inline-css
    63 B: style#boogaloo-font-inline-css
    63 B: style#balthazar-font-inline-css
   129 B: div.amp-story-block-wrapper amp-wp-b3afd51
   126 B: div.amp-story-block-wrapper amp-wp-3612edb
   130 B: div.amp-story-block-wrapper amp-wp-5cc6f64
   130 B: h1.wp-block-amp-amp-story-text has-background has-very-light-gray-background-color amp-wp-13be987[data-font-family=Boogaloo]
   119 B: div.wp-block-cover has-background-dim alignwide amp-wp-d28a8c9
    79 B: p.amp-wp-f6e3d7f
    88 B: amp-img.wp-smiley amp-wp-enforced-sizes amp-wp-843f19c[src=https://s.w.org/images/core/emoji/12.0.0-1/72x72/1f642.png][alt=🙂][width=72][height=72][noloading=][layout=intrinsic]
    80 B: p.amp-wp-13076d9
    84 B: amp-story-page#3a7fb829-6179-4ccb-93a3-7ecda29a1b92.wp-block-amp-amp-story-page amp-wp-74885c3
   123 B: div.amp-story-block-wrapper amp-wp-7b64e67
    75 B: h1.wp-block-amp-amp-story-text amp-wp-f7dbba7[data-font-family=Balthazar]
Total included size: 7,560 bytes (40% of 18,658 total after tree shaking)

The following stylesheets are too large to be included in style[amp-custom]:
 66365 B: style#amp-stories-inline-css
Total excluded size: 66,365 bytes (100% of 66,365 total after tree shaking)

Total combined size: 73,925 bytes (86% of 85,023 total after tree shaking)
-->

After

<!--
The style[amp-custom] element is populated with:
  4343 B (30%): link#wp-block-library-css[rel=stylesheet][href=https://wordpressdev.lndo.site/content/plugins/gutenberg/build/block-library/style.css?ver=1569254363][media=all]
   658 B (57%): link#amp-stories-css[rel=stylesheet][href=https://wordpressdev.lndo.site/content/plugins/amp/assets/css/amp-stories.css?ver=1.3-RC1][media=all]
  1195 B (69%): link#amp-stories-frontend-css[rel=stylesheet][href=https://wordpressdev.lndo.site/content/plugins/amp/assets/css/amp-stories-frontend.css?ver=1.3-RC1][media=]
    75 B: style#bubblegum-sans-font-inline-css
    63 B: style#boogaloo-font-inline-css
    63 B: style#balthazar-font-inline-css
   129 B: div.amp-story-block-wrapper amp-wp-b3afd51
   126 B: div.amp-story-block-wrapper amp-wp-3612edb
   130 B: div.amp-story-block-wrapper amp-wp-5cc6f64
   130 B: h1.wp-block-amp-amp-story-text has-background has-very-light-gray-background-color amp-wp-13be987[data-font-family=Boogaloo]
   119 B: div.wp-block-cover has-background-dim alignwide amp-wp-d28a8c9
    79 B: p.amp-wp-f6e3d7f
    88 B: amp-img.wp-smiley amp-wp-enforced-sizes amp-wp-843f19c[src=https://s.w.org/images/core/emoji/12.0.0-1/72x72/1f642.png][alt=🙂][width=72][height=72][noloading=][layout=intrinsic]
    80 B: p.amp-wp-13076d9
    84 B: amp-story-page#3a7fb829-6179-4ccb-93a3-7ecda29a1b92.wp-block-amp-amp-story-page amp-wp-74885c3
   123 B: div.amp-story-block-wrapper amp-wp-7b64e67
    75 B: h1.wp-block-amp-amp-story-text amp-wp-f7dbba7[data-font-family=Balthazar]
Total included size: 7,560 bytes (40% of 18,658 total after tree shaking)
-->

This also fixes blatant error in test_render_post_filters which was introduced a long time ago, but for some reason it only showed up now. Fixes #3327.

Also fix blatant error in test_render_post_filters

Also fix blatant error in test_render_post_filters Merges #3328 into 1.3 branch

swissspidy · 2019-09-26T20:50:36Z

QA fort this is part of #3344.

Only add all Google Font style rules in editor context

8bd750b

Also fix blatant error in test_render_post_filters

westonruter added the Size: XS label Sep 23, 2019

westonruter added this to the v1.3 milestone Sep 23, 2019

westonruter requested a review from swissspidy September 23, 2019 17:20

googlebot added the cla: yes Signed the Google CLA label Sep 23, 2019

westonruter added the AMP Stories label Sep 23, 2019

This was referenced Sep 23, 2019

Investigate broken test against WordPress trunk #3327

Closed

Bump 1.3-RC2 #3335

Merged

swissspidy approved these changes Sep 24, 2019

View reviewed changes

swissspidy merged commit bb35223 into develop Sep 24, 2019

swissspidy deleted the fix/font-inclusion-on-frontend branch September 24, 2019 06:11

westonruter added a commit that referenced this pull request Sep 24, 2019

Only add all Google Font style rules in editor context

9389eef

Also fix blatant error in test_render_post_filters Merges #3328 into 1.3 branch

swissspidy mentioned this pull request Sep 25, 2019

Inline styles not added for blocks with custom font #3344

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Only add all Google Font style rules in editor context#3328

Only add all Google Font style rules in editor context#3328
swissspidy merged 1 commit intodevelopfrom
fix/font-inclusion-on-frontend

westonruter commented Sep 23, 2019 •

edited

Loading

Uh oh!

swissspidy commented Sep 26, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	$fonts = self::get_fonts();
	foreach ( $fonts as $font ) {
	wp_add_inline_style(
	self::AMP_STORIES_STYLE_HANDLE,
	self::get_inline_font_style_rule( $font )
	);
	}

	/**
	* Include any required Google Font styles when rendering a block in AMP Stories.
	*
	* @param string $block_content The block content about to be appended.
	* @param array $block The full block, including name and attributes.
	* @return string Block content.
	*/
	public static function render_block_with_google_fonts( $block_content, $block ) {
	$font_family_attribute = 'ampFontFamily';

	// Short-circuit if no font family present.
	if ( empty( $block['attrs'][ $font_family_attribute ] ) ) {
	return $block_content;
	}

	// Short-circuit if there is no Google Font or the font is already enqueued.
	$font = self::get_font( $block['attrs'][ $font_family_attribute ] );
	if ( ! isset( $font['handle'], $font['src'] ) \|\| ! $font \|\| wp_style_is( $font['handle'] ) ) {
	return $block_content;
	}

	if ( ! wp_style_is( $font['handle'], 'registered' ) ) {
	wp_register_style( $font['handle'], $font['src'], [], null, 'all' ); // phpcs:ignore WordPress.WP.EnqueuedResourceParameters.MissingVersion
	}

	wp_enqueue_style( $font['handle'] );
	wp_add_inline_style(
	$font['handle'],
	self::get_inline_font_style_rule( $font )
	);

	return $block_content;
	}

Conversation

westonruter commented Sep 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Before

After

Uh oh!

swissspidy commented Sep 26, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

westonruter commented Sep 23, 2019 •

edited

Loading