Merge pull request #1111 from Automattic/update/dynamic-element-selec…

…tors Prevent CSS tree shaking from removing selectors with classes mentioned in [class] amp-bind attributes
ampproject · May 3, 2018 · 857e403 · 857e403
2 parents 8f0c5ca + 52b3616
commit 857e403
Show file tree

Hide file tree

Showing 5 changed files with 71 additions and 5 deletions.
diff --git a/dev-lib b/dev-lib
diff --git a/includes/sanitizers/class-amp-style-sanitizer.php b/includes/sanitizers/class-amp-style-sanitizer.php
@@ -211,7 +211,7 @@ public function get_stylesheets() {
 	}
 
 	/**
-	 * Get list of all the class names used in the document.
+	 * Get list of all the class names used in the document, including those used in [class] attributes.
 	 *
 	 * @since 1.0
 	 * @return array Used class names.
@@ -222,6 +222,14 @@ private function get_used_class_names() {
 			foreach ( $this->xpath->query( '//*/@class' ) as $class_attribute ) {
 				$classes .= ' ' . $class_attribute->nodeValue;
 			}
+
+			// Find all [class] attributes and capture the contents of any single- or double-quoted strings.
+			foreach ( $this->xpath->query( '//*/@' . AMP_DOM_Utils::get_amp_bind_placeholder_prefix() . 'class' ) as $bound_class_attribute ) {
+				if ( preg_match_all( '/([\'"])([^\1]*?)\1/', $bound_class_attribute->nodeValue, $matches ) ) {
+					$classes .= ' ' . implode( ' ', $matches[2] );
+				}
+			}
+
 			$this->used_class_names = array_unique( array_filter( preg_split( '/\s+/', trim( $classes ) ) ) );
 		}
 		return $this->used_class_names;

diff --git a/includes/utils/class-amp-dom-utils.php b/includes/utils/class-amp-dom-utils.php
@@ -240,8 +240,13 @@ public static function convert_amp_bind_attributes( $html ) {
 			return '<' . $tag_matches['name'] . $new_attrs . '>';
 		};
 
+		/*
+		 * Match all start tags that probably contain a binding attribute.
+		 * @todo Warning: The following pattern is brittle since it truncates HTML tags that have attributes that contain ">" or "=>".
+		 * For example, if there exists: `<body class="foo" [class]="bodyClasses.concat( isBar ? 'bar' : '' ).filter( className => '' != className )">`
+		 * Then this results in the following being output as the first child fo the body: `'' != className )">`
+		 */
 		$converted = preg_replace_callback(
-			// Match all start tags that probably contain a binding attribute.
 			'#<(?P<name>[a-zA-Z0-9_\-]+)(?P<attrs>\s[^>]+\]=[^>]+)>#',
 			$replace_callback,
 			$html

diff --git a/tests/test-amp-style-sanitizer.php b/tests/test-amp-style-sanitizer.php
@@ -388,6 +388,58 @@ public function test_font_data_url_handling() {
 		$this->assertContains( '.dashicons-format-chat:before', $actual_stylesheets[0] );
 	}
 
+	/**
+	 * Test that auto-removal (tree shaking) does not remove rules for classes mentioned in class and [class] attributes.
+	 *
+	 * @covers AMP_Style_Sanitizer::get_used_class_names()
+	 * @covers AMP_Style_Sanitizer::finalize_stylesheet_set()
+	 */
+	public function test_class_amp_bind_preservation() {
+		ob_start();
+		?>
+		<html amp>
+			<head>
+				<meta charset="utf-8">
+				<style>.sidebar1 { display:none }</style>
+				<style>.sidebar1.expanded { display:block }</style>
+				<style>.sidebar2{ visibility:hidden }</style>
+				<style>.sidebar2.visible { display:block }</style>
+				<style>.nothing { visibility:hidden; }</style>
+				</style>
+			</head>
+			<body>
+				<amp-state id="mySidebar">
+					<script type="application/json">
+						{
+							"expanded": false
+						}
+					</script>
+				</amp-state>
+				<aside class="sidebar1" [class]="! mySidebar.expanded ? '' : 'expanded'">...</aside>
+				<aside class="sidebar2" [class]='mySidebar.expanded ? "visible" : ""'>...</aside>
+			</body>
+		</html>
+		<?php
+		$dom = AMP_DOM_Utils::get_dom( ob_get_clean() );
+
+		$error_codes = array();
+		$sanitizer   = new AMP_Style_Sanitizer( $dom, array(
+			'use_document_element'      => true,
+			'remove_unused_rules'       => 'always',
+			'validation_error_callback' => function( $error ) use ( &$error_codes ) {
+				$error_codes[] = $error['code'];
+			},
+		) );
+		$sanitizer->sanitize();
+		$this->assertEquals( array(), $error_codes );
+		$actual_stylesheets = array_values( $sanitizer->get_stylesheets() );
+		$this->assertEquals( '.sidebar1{display:none;}', $actual_stylesheets[0] );
+		$this->assertEquals( '.sidebar1.expanded{display:block;}', $actual_stylesheets[1] );
+		$this->assertEquals( '.sidebar2{visibility:hidden;}', $actual_stylesheets[2] );
+		$this->assertEquals( '.sidebar2.visible{display:block;}', $actual_stylesheets[3] );
+		$this->assertEmpty( $actual_stylesheets[4] );
+	}
+
 	/**
 	 * Test that auto-removal is performed when remove_unused_rules=sometimes (the default), and that excessive CSS will be removed entirely.
 	 *

diff --git a/tests/test-class-amp-theme-support.php b/tests/test-class-amp-theme-support.php
@@ -1024,7 +1024,7 @@ public function test_prepare_response() {
 		$this->assertContains( '<meta charset="' . get_bloginfo( 'charset' ) . '">', $sanitized_html );
 		$this->assertContains( '<meta name="viewport" content="width=device-width,minimum-scale=1">', $sanitized_html );
 		$this->assertContains( '<style amp-boilerplate>', $sanitized_html );
-		$this->assertContains( '<style amp-custom>body{background:black;}', $sanitized_html );
+		$this->assertRegExp( '#<style amp-custom>.*?body{background:black;}.*?</style>#s', $sanitized_html );
 		$this->assertContains( '<script type="text/javascript" src="https://cdn.ampproject.org/v0.js" async></script>', $sanitized_html ); // phpcs:ignore WordPress.WP.EnqueuedResources.NonEnqueuedScript
 		$this->assertContains( '<script type="text/javascript" src="https://cdn.ampproject.org/v0/amp-list-latest.js" async custom-element="amp-list"></script>', $sanitized_html ); // phpcs:ignore WordPress.WP.EnqueuedResources.NonEnqueuedScript
 		$this->assertContains( '<script type="text/javascript" src="https://cdn.ampproject.org/v0/amp-mathml-latest.js" async custom-element="amp-mathml"></script>', $sanitized_html ); // phpcs:ignore WordPress.WP.EnqueuedResources.NonEnqueuedScript
@@ -1041,9 +1041,10 @@ public function test_prepare_response() {
 		$this->assertContains( '<script type=\'text/javascript\' src=\'https://cdn.ampproject.org/v0/amp-ad-latest.js\' async custom-element="amp-ad"></script>', $sanitized_html ); // phpcs:ignore WordPress.WP.EnqueuedResources.NonEnqueuedScript
 
 		$this->assertContains( '<button>no-onclick</button>', $sanitized_html );
-		$this->assertCount( 4, $removed_nodes );
+		$this->assertCount( 3, $removed_nodes );
 		$this->assertInstanceOf( 'DOMElement', $removed_nodes['script'] );
 		$this->assertInstanceOf( 'DOMAttr', $removed_nodes['onclick'] );
+		$this->assertInstanceOf( 'DOMAttr', $removed_nodes['handle'] );
 	}
 
 	/**