Merge pull request #4205 from ampproject/update/reader-mode-style-inc…

…lusion Split Reader mode style template part stylesheet from action-supplied styles
ampproject · Jan 31, 2020 · caa0c8c · caa0c8c
2 parents 17f3594 + d4c266a
commit caa0c8c
Show file tree

Hide file tree

Showing 2 changed files with 13 additions and 25 deletions.
diff --git a/includes/sanitizers/class-amp-style-sanitizer.php b/includes/sanitizers/class-amp-style-sanitizer.php
@@ -1233,13 +1233,8 @@ private function process_style_element( DOMElement $element ) {
 			'imported_font_urls' => $parsed['imported_font_urls'],
 		];
 
-		if ( $element->hasAttribute( 'amp-custom' ) && ! $this->amp_custom_style_element ) {
-			$this->amp_custom_style_element = $element;
-		} else {
-
-			// Remove from DOM since we'll be adding it to amp-custom.
-			$element->parentNode->removeChild( $element );
-		}
+		// Remove from DOM since we'll be adding it to a newly-created style[amp-custom] element later.
+		$element->parentNode->removeChild( $element );
 
 		$this->set_current_node( null );
 	}
@@ -2624,14 +2619,6 @@ private function finalize_styles() {
 
 		// Add style[amp-custom] to document.
 		if ( $stylesheet_groups[ self::STYLE_AMP_CUSTOM_GROUP_INDEX ]['included_count'] > 0 ) {
-
-			// Ensure style[amp-custom] is present in the document.
-			if ( ! $this->amp_custom_style_element ) {
-				$this->amp_custom_style_element = $this->dom->createElement( 'style' );
-				$this->amp_custom_style_element->setAttribute( 'amp-custom', '' );
-				$this->dom->head->appendChild( $this->amp_custom_style_element );
-			}
-
 			/*
 			 * On AMP-first themes when there are new/rejected validation errors present, a parsed stylesheet may include
 			 * @import rules. These must be moved to the beginning to be honored.
@@ -2641,16 +2628,11 @@ private function finalize_styles() {
 			$css .= implode( '', $this->get_stylesheets() );
 			$css .= $stylesheet_groups[ self::STYLE_AMP_CUSTOM_GROUP_INDEX ]['source_map_comment'];
 
-			/*
-			 * 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 );
-			}
+			// Create the style[amp-custom] element and add it to the <head>.
+			$this->amp_custom_style_element = $this->dom->createElement( 'style' );
+			$this->amp_custom_style_element->setAttribute( 'amp-custom', '' );
 			$this->amp_custom_style_element->appendChild( $this->dom->createTextNode( $css ) );
+			$this->dom->head->appendChild( $this->amp_custom_style_element );
 		}
 
 		/*

diff --git a/templates/html-start.php b/templates/html-start.php
@@ -24,6 +24,10 @@
 	<meta charset="utf-8">
 	<meta name="viewport" content="width=device-width,initial-scale=1,minimum-scale=1">
 	<?php
+	// Note: The following two <style> tags are combined into <style amp-custom> via AMP_Style_Sanitizer.
+	// Splitting up the styles into two stylesheets allows for plugin-supplied styles via the amp_post_template_css action
+	// to be excluded from the styles in the style template part, which are more important given they style the overall page.
+
 	/**
 	 * Fires when rendering <head> in Reader mode templates.
 	 *
@@ -33,8 +37,10 @@
 	 */
 	do_action( 'amp_post_template_head', $this );
 	?>
-	<style amp-custom>
+	<style class="style-template-part">
 		<?php $this->load_parts( [ 'style' ] ); ?>
+	</style>
+	<style class="amp-post-template-css-action">
 		<?php
 		/**
 		 * Fires when printing CSS styles in Reader mode templates.