diff --git a/includes/sanitizers/class-amp-style-sanitizer.php b/includes/sanitizers/class-amp-style-sanitizer.php index 2f447637a06..f9da47b754d 100644 --- a/includes/sanitizers/class-amp-style-sanitizer.php +++ b/includes/sanitizers/class-amp-style-sanitizer.php @@ -47,6 +47,42 @@ class AMP_Style_Sanitizer extends AMP_Base_Sanitizer { */ const INLINE_SPECIFICITY_MULTIPLIER = 5; // @todo The correctness of using "5" should be validated. + /** + * Array index for tag names extracted from a selector. + * + * @private + * @since 1.1 + * @see \AMP_Style_Sanitizer::prepare_stylesheet() + */ + const SELECTOR_EXTRACTED_TAGS = 0; + + /** + * Array index for class names extracted from a selector. + * + * @private + * @since 1.1 + * @see \AMP_Style_Sanitizer::prepare_stylesheet() + */ + const SELECTOR_EXTRACTED_CLASSES = 1; + + /** + * Array index for IDs extracted from a selector. + * + * @private + * @since 1.1 + * @see \AMP_Style_Sanitizer::prepare_stylesheet() + */ + const SELECTOR_EXTRACTED_IDS = 2; + + /** + * Array index for attributes extracted from a selector. + * + * @private + * @since 1.1 + * @see \AMP_Style_Sanitizer::prepare_stylesheet() + */ + const SELECTOR_EXTRACTED_ATTRIBUTES = 3; + /** * Array of flags used to control sanitization. * @@ -169,10 +205,25 @@ class AMP_Style_Sanitizer extends AMP_Base_Sanitizer { /** * Attributes used in the document. * + * This is initially populated with boolean attributes which can be mutated by AMP at runtime, + * since they can by dynamically added at any time. + * * @since 1.1 * @var array */ - private $used_attributes = array(); + private $used_attributes = array( + 'autofocus' => true, + 'checked' => true, + 'controls' => true, + 'disabled' => true, + 'hidden' => true, + 'loop' => true, + 'multiple' => true, + 'open' => true, + 'readonly' => true, + 'required' => true, + 'selected' => true, + ); /** * Tag names used in document. @@ -558,7 +609,7 @@ private function has_used_tag_names( $tag_names ) { * Check whether the attributes exist. * * @since 1.1 - * @todo Make $attribute_names into $attributes as an associative array and implement lookups of specific values. + * @todo Make $attribute_names into $attributes as an associative array and implement lookups of specific values. Since attribute values can vary (e.g. with amp-bind), this may not be feasible. * * @param string[] $attribute_names Attribute names. * @return bool Whether all supplied attributes are used. @@ -1106,7 +1157,7 @@ private function fetch_external_stylesheet( $url ) { private function process_stylesheet( $stylesheet, $options = array() ) { $parsed = null; $cache_key = null; - $cache_group = 'amp-parsed-stylesheet-v15'; // This should be bumped whenever the PHP-CSS-Parser is updated. + $cache_group = 'amp-parsed-stylesheet-v16'; // This should be bumped whenever the PHP-CSS-Parser is updated or parsed format is updated. $cache_impacting_options = array_merge( wp_array_slice_assoc( @@ -1127,7 +1178,7 @@ private function process_stylesheet( $stylesheet, $options = array() ) { if ( wp_using_ext_object_cache() ) { $parsed = wp_cache_get( $cache_key, $cache_group ); } else { - $parsed = get_transient( $cache_key . $cache_group ); + $parsed = get_transient( $cache_group . '-' . $cache_key ); } /* @@ -1156,7 +1207,7 @@ private function process_stylesheet( $stylesheet, $options = array() ) { wp_cache_set( $cache_key, $parsed, $cache_group ); } else { // The expiration is to ensure transient doesn't stick around forever since no LRU flushing like with external object cache. - set_transient( $cache_key . $cache_group, $parsed, MONTH_IN_SECONDS ); + set_transient( $cache_group . '-' . $cache_key, $parsed, MONTH_IN_SECONDS ); } } @@ -1456,41 +1507,64 @@ function( $selector ) { $selectors = explode( $between_selectors . ',', $split_stylesheet[ ++$i ] ); $declaration = $split_stylesheet[ ++$i ]; + // @todo The following logic could be made much more robust if PHP-CSS-Parser did parsing of selectors. See and . $selectors_parsed = array(); foreach ( $selectors as $selector ) { $selectors_parsed[ $selector ] = array(); // Remove :not() and pseudo selectors to eliminate false negatives, such as with `body:not(.title-tagline-hidden) .site-branding-text`. - $reduced_selector = preg_replace( '/:[a-zA-Z0-9_-]+(\(.+?\))?/', '', $selector ); - - // Remove attribute selectors to eliminate false negative, such as with `.social-navigation a[href*="example.com"]:before`. - $reduced_selector = preg_replace( '/\[\w.*?\]/', '', $reduced_selector ); + $reduced_selector = preg_replace( '/::?[a-zA-Z0-9_-]+(\(.+?\))?/', '', $selector ); // Ignore any selector terms that occur under a dynamic selector. if ( $dynamic_selector_pattern ) { $reduced_selector = preg_replace( '#((?:' . $dynamic_selector_pattern . ')(?:\.[a-z0-9_-]+)*)[^a-z0-9_-].*#si', '$1', $reduced_selector . ' ' ); } + /* + * Gather attribute names while removing attribute selectors to eliminate false negative, + * such as with `.social-navigation a[href*="example.com"]:before`. + */ + $reduced_selector = preg_replace_callback( + '/\[([A-Za-z0-9_:-]+)(\W?=[^\]]+)?\]/', + function( $matches ) use ( $selector, &$selectors_parsed ) { + $selectors_parsed[ $selector ][ self::SELECTOR_EXTRACTED_ATTRIBUTES ][] = $matches[1]; + return ''; + }, + $reduced_selector + ); + + // Extract class names. $reduced_selector = preg_replace_callback( '/\.([a-zA-Z0-9_-]+)/', function( $matches ) use ( $selector, &$selectors_parsed ) { - $selectors_parsed[ $selector ]['classes'][] = $matches[1]; + $selectors_parsed[ $selector ][ self::SELECTOR_EXTRACTED_CLASSES ][] = $matches[1]; return ''; }, $reduced_selector ); + + // Extract IDs. $reduced_selector = preg_replace_callback( '/#([a-zA-Z0-9_-]+)/', function( $matches ) use ( $selector, &$selectors_parsed ) { - $selectors_parsed[ $selector ]['ids'][] = $matches[1]; + $selectors_parsed[ $selector ][ self::SELECTOR_EXTRACTED_IDS ][] = $matches[1]; return ''; }, $reduced_selector ); - if ( preg_match_all( '/[a-zA-Z0-9_-]+/', $reduced_selector, $matches ) ) { - $selectors_parsed[ $selector ]['tags'] = $matches[0]; - } + // Extract tag names. + $reduced_selector = preg_replace_callback( + '/[a-zA-Z0-9_-]+/', + function( $matches ) use ( $selector, &$selectors_parsed ) { + $selectors_parsed[ $selector ][ self::SELECTOR_EXTRACTED_TAGS ][] = $matches[0]; + return ''; + }, + $reduced_selector + ); + + // At this point, $reduced_selector should contain just the remnants of the selector, primarily combinators. + unset( $reduced_selector ); } $stylesheet[] = array( @@ -2586,18 +2660,18 @@ private function finalize_stylesheet_set( $stylesheet_set ) { ( // If all class names are used in the doc. ( - empty( $parsed_selector['classes'] ) + empty( $parsed_selector[ self::SELECTOR_EXTRACTED_CLASSES ] ) || - $this->has_used_class_name( $parsed_selector['classes'] ) + $this->has_used_class_name( $parsed_selector[ self::SELECTOR_EXTRACTED_CLASSES ] ) ) && // If all IDs are used in the doc. ( - empty( $parsed_selector['ids'] ) + empty( $parsed_selector[ self::SELECTOR_EXTRACTED_IDS ] ) || 0 === count( array_filter( - $parsed_selector['ids'], + $parsed_selector[ self::SELECTOR_EXTRACTED_IDS ], function( $id ) use ( $dom ) { return ! $dom->getElementById( $id ); } @@ -2607,9 +2681,16 @@ function( $id ) use ( $dom ) { && // If tag names are present in the doc. ( - empty( $parsed_selector['tags'] ) + empty( $parsed_selector[ self::SELECTOR_EXTRACTED_TAGS ] ) + || + $this->has_used_tag_names( $parsed_selector[ self::SELECTOR_EXTRACTED_TAGS ] ) + ) + && + // If all attribute names are used in the doc. + ( + empty( $parsed_selector[ self::SELECTOR_EXTRACTED_ATTRIBUTES ] ) || - $this->has_used_tag_names( $parsed_selector['tags'] ) + $this->has_used_attributes( $parsed_selector[ self::SELECTOR_EXTRACTED_ATTRIBUTES ] ) ) ) ); diff --git a/tests/test-amp-style-sanitizer.php b/tests/test-amp-style-sanitizer.php index 98ef72f9279..cf8b8efb049 100644 --- a/tests/test-amp-style-sanitizer.php +++ b/tests/test-amp-style-sanitizer.php @@ -318,7 +318,7 @@ public function get_link_and_style_test_data() { ) ), array( - 'form [submit-success] b,div[submit-failure] b{color:green}', + 'form [submit-success] b{color:green}', // The [submit-failure] selector is removed because there is no div[submit-failure]. 'amp-live-list li .highlighted{background:yellow}', '', 'body amp-list .portland{color:blue}', @@ -358,7 +358,7 @@ public function get_link_and_style_test_data() { array(), ), 'unamerican_lang_attribute_selectors_removed' => array( // USA is used for convenience here. No political statement intended. - 'Test', + 'Test', array( 'html[lang=en-US]{color:red}html[lang="en-US"]{color:white}html[lang^=en]{color:blue}', ), @@ -592,6 +592,11 @@ public function get_amp_selector_data() { 'div img.logo{border:solid 1px red}', '', // The selector is removed because there is no div element. ), + 'attribute_selectors' => array( + '
Top', + '[type="button"], [type="reset"], [type^="submit"] {color:red} a[href^=http]:after, a[href^="#"]:after { color:blue } span[hidden] {display:none}#content[tabindex="-1"]:focus{ outline: solid 1px red; }', + '[type="button"],[type="reset"],[type^="submit"]{color:red}a[href^=http]:after,a[href^="#"]:after{color:blue}span[hidden]{display:none}#content[tabindex="-1"]:focus{outline:solid 1px red}', // Any selector mentioning [type] or [href] will persist since value is not used for tree shaking. + ), 'playbuzz' => array( '

hello

', 'p + div.pb_feed{border:solid 1px blue}', @@ -655,6 +660,120 @@ public function test_amp_selector_conversion( $markup, $input, $output ) { $this->assertEquals( $output, $stylesheets[0] ); } + /** + * Provide data for attribute selector test. + * + * @return array Data. + */ + public function get_attribute_selector_data() { + return array( + 'type_attribute' => array( + '', + // All selectors remain because only the existence of the attribute is examined. + array( + '[type="button"]' => true, + '[type*="reset"]' => true, + '[type^="submit"]' => true, + '[type$="button"]' => true, + ), + ), + 'tabindex_attribute' => array( + '', + // The div[tabindex] is removed because there is no div. The span[tabindex^=2] remains because value is not considered. + array( + 'div[tabindex]' => false, + 'span[tabindex]' => true, + 'span[tabindex=-1]' => true, + 'span[tabindex^=2]' => true, + ), + ), + 'href_attribute' => array( + 'Foo', + array( + 'a[href^=http]:after' => true, + 'a[href^="#"]:after' => true, + ), + ), + 'hidden_attribute' => array( + 'not hidden', + // Only div[hidden] should be removed because there is no div element; the other [hidden] selectors remain because it can be dynamically added. + array( + 'span[hidden]' => true, + '[hidden]' => true, + 'div[hidden]' => false, + 'span:not([hidden])' => true, + ), + ), + 'selected_readonly_disabled_multiple_autofocus_required' => array( + '', + array( + '[autofocus]' => true, + '[checked]' => true, + '[disabled]' => true, + '[multiple]' => true, + '[readonly]' => true, + '[required]' => true, + '[selected]' => true, + ), + ), + 'open_attribute' => array( + '
MoreDetails
', + array( + '[open]' => true, + 'amp-lightbox[open]' => false, + 'details[open]' => true, + ), + ), + 'media_attributes' => array( + '', + array( + '[loop]' => true, + '[controls]' => true, + ), + ), + ); + } + + /** + * Test attribute selector tree shaking. + * + * @dataProvider get_attribute_selector_data + * + * @param string $markup Source HTML markup. + * @param array $selectors Mapping of selectors to whether they are expected. + */ + public function test_attribute_selector( $markup, $selectors ) { + $style = implode( + '', + array_map( + function ( $selector ) { + return sprintf( '%s{ color: red; }', $selector ); + }, + array_keys( $selectors ) + ) + ); + + $html = "$markup"; + $dom = AMP_DOM_Utils::get_dom( $html ); + + $sanitizer_classes = amp_get_content_sanitizers(); + $sanitizer_classes['AMP_Style_Sanitizer']['remove_unused_rules'] = 'always'; + + $sanitized = AMP_Content_Sanitizer::sanitize_document( + $dom, + $sanitizer_classes, + array( + 'use_document_element' => true, + ) + ); + + $stylesheets = array_values( $sanitized['stylesheets'] ); + + $actual_selectors = array_values( array_filter( preg_split( '/{.+?}/s', $stylesheets[0] ) ) ); + $expected_selectors = array_keys( array_filter( $selectors ) ); + $this->assertEqualSets( $expected_selectors, $actual_selectors ); + } + /** * Data for testing CSS hack removal. * @@ -1045,7 +1164,7 @@ public function test_large_custom_css_and_rule_removal() { @media screen {} '; - $html .= '...'; + $html .= '...'; $dom = AMP_DOM_Utils::get_dom( $html ); $error_codes = array();