From 407c26c3923e05432509df1c6e30cfaa605f7152 Mon Sep 17 00:00:00 2001 From: Alberto Medina Date: Fri, 6 Apr 2018 18:23:34 -0700 Subject: [PATCH 1/4] Convert width attribute of col tag to equivalent CSS rule --- .../sanitizers/class-amp-style-sanitizer.php | 13 +++++++++++ tests/test-amp-style-sanitizer.php | 22 +++++++++++++++++++ 2 files changed, 35 insertions(+) diff --git a/includes/sanitizers/class-amp-style-sanitizer.php b/includes/sanitizers/class-amp-style-sanitizer.php index 8440c097369..79a6cfef9c2 100644 --- a/includes/sanitizers/class-amp-style-sanitizer.php +++ b/includes/sanitizers/class-amp-style-sanitizer.php @@ -258,6 +258,19 @@ public function sanitize() { $elements[] = $element; } + // If 'width' attribute is present for 'col' tag, convert to proper CSS rule. + foreach ( $this->dom->getElementsByTagName( 'col' ) as $col ) { + $width_attr = $col->getAttribute( 'width' ); + if ( ! empty( $width_attr ) && ( false === strpos( $width_attr, '*' ) ) ) { + $width_style = 'width: ' . $width_attr; + if ( is_numeric( $width_attr ) ) { + $width_style .= 'px'; + } + $col->setAttribute( 'style', $width_style ); + $col->removeAttribute( 'width' ); + } + } + /** * Element. * diff --git a/tests/test-amp-style-sanitizer.php b/tests/test-amp-style-sanitizer.php index 5c7cd0a1afe..756a4a39393 100644 --- a/tests/test-amp-style-sanitizer.php +++ b/tests/test-amp-style-sanitizer.php @@ -156,6 +156,28 @@ public function get_body_style_attribute_data() { ':root:not(#_):not(#_):not(#_):not(#_):not(#_):not(#_):not(#_):not(#_):not(#_):not(#_):not(#_) .amp-wp-ab79d9e{color:purple;}', ), ), + + 'col_with_width_attribute' => array( + '
', + '
', + array( + ':root:not(#_):not(#_):not(#_):not(#_):not(#_) .amp-wp-cbcb5c2{max-width:253px;}', + ), + ), + + 'col_with_percent_width_attribute' => array( + '
', + '
', + array( + ':root:not(#_):not(#_):not(#_):not(#_):not(#_) .amp-wp-cd7753e{max-width:50%;}', + ), + ), + + 'col_with_star_width_attribute' => array( + '
', + '
', + array(), + ), ); } From b09bb46b97bd26dbfd9e2ba0ba9f25d60a487201 Mon Sep 17 00:00:00 2001 From: Alberto Medina Date: Mon, 9 Apr 2018 17:20:25 -0700 Subject: [PATCH 2/4] Revert changes to convert width to max-width automatically Reverts PR #495 for Issue #494. --- .../sanitizers/class-amp-style-sanitizer.php | 32 ++++++------------- tests/test-amp-style-sanitizer.php | 12 ++----- 2 files changed, 11 insertions(+), 33 deletions(-) diff --git a/includes/sanitizers/class-amp-style-sanitizer.php b/includes/sanitizers/class-amp-style-sanitizer.php index 79a6cfef9c2..d7a780fca05 100644 --- a/includes/sanitizers/class-amp-style-sanitizer.php +++ b/includes/sanitizers/class-amp-style-sanitizer.php @@ -472,7 +472,6 @@ private function process_link_element( DOMElement $element ) { * @type bool $class_selector_tree_shaking Whether to perform tree shaking to delete rules that reference class names not extant in the current document. * @type string[] $property_whitelist Exclusively-allowed properties. * @type string[] $property_blacklist Disallowed properties. - * @type bool $convert_width_to_max_width Convert width to max-width. * @type string $stylesheet_url Original URL for stylesheet when originating via link (or @import?). * @type string $stylesheet_path Original filesystem path for stylesheet when originating via link (or @import?). * @type array $allowed_at_rules Allowed @-rules. @@ -483,7 +482,7 @@ private function process_link_element( DOMElement $element ) { private function process_stylesheet( $stylesheet, $node, $options = array() ) { $cache_impacting_options = wp_array_slice_assoc( $options, - array( 'property_whitelist', 'property_blacklist', 'convert_width_to_max_width', 'stylesheet_url', 'allowed_at_rules' ) + array( 'property_whitelist', 'property_blacklist', 'stylesheet_url', 'allowed_at_rules' ) ); $cache_key = md5( $stylesheet . serialize( $cache_impacting_options ) ); // phpcs:ignore WordPress.PHP.DiscouragedPHPFunctions.serialize_serialize @@ -532,17 +531,16 @@ private function parse_stylesheet( $stylesheet_string, $options = array() ) { $options = array_merge( array( - 'allowed_at_rules' => array(), - 'convert_width_to_max_width' => false, - 'property_blacklist' => array( + 'allowed_at_rules' => array(), + 'property_blacklist' => array( // See . 'behavior', '-moz-binding', ), - 'property_whitelist' => array(), - 'validate_keyframes' => false, - 'stylesheet_url' => null, - 'stylesheet_path' => null, + 'property_whitelist' => array(), + 'validate_keyframes' => false, + 'stylesheet_url' => null, + 'stylesheet_path' => null, ), $options ); @@ -802,17 +800,6 @@ private function process_css_declaration_block( RuleSet $ruleset, CSSList $css_l $this->transform_important_qualifiers( $ruleset, $css_list ) ); - // Convert width to max-width when requested. See . - if ( $options['convert_width_to_max_width'] ) { - $properties = $ruleset->getRules( 'width' ); - foreach ( $properties as $property ) { - $width_property = new Rule( 'max-width' ); - $width_property->setValue( $property->getValue() ); - $ruleset->removeRule( $property ); - $ruleset->addRule( $width_property, $property ); - } - } - // Remove the ruleset if it is now empty. if ( 0 === count( $ruleset->getRules() ) ) { $css_list->remove( $ruleset ); @@ -1085,9 +1072,8 @@ private function collect_inline_styles( $element ) { $rule = sprintf( '%s .%s { %s }', $root, $class, $style_attribute->nodeValue ); $stylesheet = $this->process_stylesheet( $rule, $style_attribute, array( - 'convert_width_to_max_width' => true, - 'allowed_at_rules' => array(), - 'property_whitelist' => $this->style_custom_cdata_spec['css_spec']['allowed_declarations'], + 'allowed_at_rules' => array(), + 'property_whitelist' => $this->style_custom_cdata_spec['css_spec']['allowed_declarations'], ) ); if ( empty( $stylesheet ) ) { diff --git a/tests/test-amp-style-sanitizer.php b/tests/test-amp-style-sanitizer.php index 756a4a39393..b0ad11899bf 100644 --- a/tests/test-amp-style-sanitizer.php +++ b/tests/test-amp-style-sanitizer.php @@ -49,14 +49,6 @@ public function get_body_style_attribute_data() { ), ), - 'width_to_max-width' => array( - '
', - '
', - array( - ':root:not(#_):not(#_):not(#_):not(#_):not(#_) .amp-wp-343bce0{max-width:300px;}', - ), - ), - 'span_display_none' => array( 'Kses-banned properties are allowed since Kses will have already applied if user does not have unfiltered_html.', 'Kses-banned properties are allowed since Kses will have already applied if user does not have unfiltered_html.', @@ -161,7 +153,7 @@ public function get_body_style_attribute_data() { '
', '
', array( - ':root:not(#_):not(#_):not(#_):not(#_):not(#_) .amp-wp-cbcb5c2{max-width:253px;}', + ':root:not(#_):not(#_):not(#_):not(#_):not(#_) .amp-wp-cbcb5c2{width:253px;}', ), ), @@ -169,7 +161,7 @@ public function get_body_style_attribute_data() { '
', '
', array( - ':root:not(#_):not(#_):not(#_):not(#_):not(#_) .amp-wp-cd7753e{max-width:50%;}', + ':root:not(#_):not(#_):not(#_):not(#_):not(#_) .amp-wp-cd7753e{width:50%;}', ), ), From 11293d4a6b487ccf18d6aa18af9cc5419e77fed0 Mon Sep 17 00:00:00 2001 From: Alberto Medina Date: Mon, 9 Apr 2018 18:02:00 -0700 Subject: [PATCH 3/4] Update 'style' attribute if it exists, or create it if it doesn't --- includes/sanitizers/class-amp-style-sanitizer.php | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/includes/sanitizers/class-amp-style-sanitizer.php b/includes/sanitizers/class-amp-style-sanitizer.php index d7a780fca05..459e8542cbf 100644 --- a/includes/sanitizers/class-amp-style-sanitizer.php +++ b/includes/sanitizers/class-amp-style-sanitizer.php @@ -266,7 +266,11 @@ public function sanitize() { if ( is_numeric( $width_attr ) ) { $width_style .= 'px'; } - $col->setAttribute( 'style', $width_style ); + if ( $col->hasAttribute( 'style' ) ) { + $col->setAttribute( 'style', $col->getAttribute( 'style' ) . ';' . $width_style ); + } else { + $col->setAttribute( 'style', $width_style ); + } $col->removeAttribute( 'width' ); } } From a1eda204e29cac74ce0d30207a81f48179986fe0 Mon Sep 17 00:00:00 2001 From: Alberto Medina Date: Mon, 16 Apr 2018 17:46:02 -0700 Subject: [PATCH 4/4] Add test case with existing style attribute --- includes/sanitizers/class-amp-style-sanitizer.php | 2 +- tests/test-amp-style-sanitizer.php | 8 ++++++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/includes/sanitizers/class-amp-style-sanitizer.php b/includes/sanitizers/class-amp-style-sanitizer.php index 459e8542cbf..ac1e6f77069 100644 --- a/includes/sanitizers/class-amp-style-sanitizer.php +++ b/includes/sanitizers/class-amp-style-sanitizer.php @@ -267,7 +267,7 @@ public function sanitize() { $width_style .= 'px'; } if ( $col->hasAttribute( 'style' ) ) { - $col->setAttribute( 'style', $col->getAttribute( 'style' ) . ';' . $width_style ); + $col->setAttribute( 'style', $width_style . ';' . $col->getAttribute( 'style' ) ); } else { $col->setAttribute( 'style', $width_style ); } diff --git a/tests/test-amp-style-sanitizer.php b/tests/test-amp-style-sanitizer.php index b0ad11899bf..c588493902c 100644 --- a/tests/test-amp-style-sanitizer.php +++ b/tests/test-amp-style-sanitizer.php @@ -170,6 +170,14 @@ public function get_body_style_attribute_data() { '
', array(), ), + + 'col_with_width_attribute_and_existing_style' => array( + '
', + '
', + array( + ':root:not(#_):not(#_):not(#_):not(#_):not(#_) .amp-wp-c8aa9e9{width:50px;width:60px;background-color:red;}', + ), + ), ); }