Split Reader mode style template part stylesheet from action-supplied styles

[westonruter]

Summary

Splitting up stylesheets in Reader mode

The Reader mode templates have for a long time output all styles in a single <style amp-custom> tag. This was needed because the AMP_Style_Sanitizer was originally not able to handle the parsing of stylesheets but only style attributes. Now that the Reader mode templates are passed through the post-processor (via #2202) all style elements will be combined into a single style[amp-custom] element automatically. It is advantageous to split styles into multiple stylesheets because when a stylesheet goes above the limit of 50KB, it will get excluded. If a plugin adds a lot of styles via the amp_post_template_head action, the result can be that all of the styles in the style template part would also get excluded.

By splitting the two sets of styles into separate stylesheets, only the styles in amp_post_template_head will be excluded if they are excessive. This is better because those styles are less important than those being included in the style template part, which control the overall layout and design of the template. Otherwise, there is no difference in how the styles are included in the page.

The inclusion of the class names on the style elements is purely for the purpose of debugging, when/if Reader mode templates get shown as Validated URLs in the admin, with the new stylesheet analysis (see #2169).

Here's some example code that causes too much CSS to be added in amp_post_template_css:

add_action( 'amp_post_template_css', function() {
	printf( 'body:after{ content: "%s"; }', str_repeat( 'a', 50000 ) );
} );

See also #2044.

Preventing invalid style[amp-custom] from passing through

I also noticed that in the develop branch this code actually does not get sanitized at all. In fact, in Reader mode more than 50KB of CSS are getting added to the page:

The reason is that when there is only one single single style on the page and this stylesheet is greater than 50KB, then this condition is not entered since there are no included stylesheets:

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

Lines 2625 to 2626 in 0d3ce78

	// Add style[amp-custom] to document.
	if ( $stylesheet_groups[ self::STYLE_AMP_CUSTOM_GROUP_INDEX ]['included_count'] > 0 ) {

When that condition is not entered, then the previously-captured amp_custom_style_element element is not subsequently emptied out to populate with the processed CSS (since there is none):

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

Lines 2644 to 2653 in 0d3ce78

	/*
	* Let the style[amp-custom] be populated with the concatenated CSS.
	* !important: Updating the contents of this style element by setting textContent is not
	* reliable across PHP/libxml versions, so this is why the children are removed and the
	* text node is then explicitly added containing the CSS.
	*/
	while ( $this->amp_custom_style_element->firstChild ) {
	$this->amp_custom_style_element->removeChild( $this->amp_custom_style_element->firstChild );
	}
	$this->amp_custom_style_element->appendChild( $this->dom->createTextNode( $css ) );

So the fix is simply to discontinue capturing any original style[amp-custom] in favor of just creating a fresh empty new one to add to the document only if there are valid stylesheets to include.

Checklist

• My pull request is addressing an open issue (please create one otherwise).
• My code is tested and passes existing tests.
• My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

[kienstra]

Looks Good

Hi @westonruter,
Nice, this looks good.

Like you showed in your screenshots, I also saw that styling added on amp_post_template_css with your callback is now handled correctly.

Before

After

[kienstra]

This makes sense, given that the document is passed through the sanitizers.