From 87e850168d4112412eb696226843f55bfda9fcf0 Mon Sep 17 00:00:00 2001 From: Ryan Kienstra Date: Tue, 6 Feb 2018 15:30:02 -0600 Subject: [PATCH 01/18] Issue #864: Remove 'sizes' attribute for and . Before, there was a workaround to ensure these didn't overflow. I added style="max-width:1005" This removes that workaround, And instead sets the layout to 'responsive.' Also, this updates the PHPUnit tests. --- .../sanitizers/class-amp-base-sanitizer.php | 9 +++-- .../sanitizers/class-amp-iframe-sanitizer.php | 9 ++--- .../sanitizers/class-amp-video-sanitizer.php | 9 ++--- tests/test-amp-iframe-sanitizer.php | 36 +++++++++---------- tests/test-amp-video-sanitizer.php | 26 +++++++------- tests/test-class-amp-base-sanitizer.php | 10 +++--- 6 files changed, 53 insertions(+), 46 deletions(-) diff --git a/includes/sanitizers/class-amp-base-sanitizer.php b/includes/sanitizers/class-amp-base-sanitizer.php index ab0695fd9c9..3f9141abb8a 100644 --- a/includes/sanitizers/class-amp-base-sanitizer.php +++ b/includes/sanitizers/class-amp-base-sanitizer.php @@ -190,7 +190,7 @@ public function sanitize_dimension( $value, $dimension ) { } /** - * Enforce fixed height. + * Sets the layout, and possibly the 'height' and 'width' attributes. * * @param string[] $attributes { * Attributes. @@ -203,15 +203,18 @@ public function sanitize_dimension( $value, $dimension ) { * } * @return string[] */ - public function enforce_fixed_height( $attributes ) { + public function set_layout( $attributes ) { if ( empty( $attributes['height'] ) ) { unset( $attributes['width'] ); $attributes['height'] = self::FALLBACK_HEIGHT; + $attributes['layout'] = 'fixed-height'; } - if ( empty( $attributes['width'] ) ) { $attributes['layout'] = 'fixed-height'; } + if ( ! empty( $attributes['width'] ) && ! empty( $attributes['height'] ) ) { + $attributes['layout'] = 'responsive'; + } return $attributes; } diff --git a/includes/sanitizers/class-amp-iframe-sanitizer.php b/includes/sanitizers/class-amp-iframe-sanitizer.php index e841596e642..a3dced0ce13 100644 --- a/includes/sanitizers/class-amp-iframe-sanitizer.php +++ b/includes/sanitizers/class-amp-iframe-sanitizer.php @@ -80,8 +80,10 @@ public function sanitize() { $this->did_convert_elements = true; - $new_attributes = $this->enforce_fixed_height( $new_attributes ); - $new_attributes = $this->enforce_sizes_attribute( $new_attributes ); + $new_attributes = $this->set_layout( $new_attributes ); + if ( isset( $new_attributes['width'] ) && isset( $new_attributes['height'] ) ) { + $this->add_or_append_attribute( $new_attributes, 'class', 'amp-wp-enforced-sizes' ); + } $new_node = AMP_DOM_Utils::create_node( $this->dom, 'amp-iframe', $new_attributes ); @@ -126,8 +128,7 @@ public function sanitize() { * @return array Returns HTML attributes; removes any not specifically declared above from input. */ private function filter_attributes( $attributes ) { - $out = array(); - $out['style'] = 'max-width:100%'; // AMP_Style_Sanitizer will move this to the amp-custom style. + $out = array(); foreach ( $attributes as $name => $value ) { switch ( $name ) { diff --git a/includes/sanitizers/class-amp-video-sanitizer.php b/includes/sanitizers/class-amp-video-sanitizer.php index 87bf0df6924..02b2da5d0c5 100644 --- a/includes/sanitizers/class-amp-video-sanitizer.php +++ b/includes/sanitizers/class-amp-video-sanitizer.php @@ -50,8 +50,10 @@ public function sanitize() { $new_attributes = $this->filter_attributes( $old_attributes ); - $new_attributes = $this->enforce_fixed_height( $new_attributes ); - $new_attributes = $this->enforce_sizes_attribute( $new_attributes ); + $new_attributes = $this->set_layout( $new_attributes ); + if ( isset( $new_attributes['width'] ) && isset( $new_attributes['height'] ) ) { + $this->add_or_append_attribute( $new_attributes, 'class', 'amp-wp-enforced-sizes' ); + } $new_node = AMP_DOM_Utils::create_node( $this->dom, 'amp-video', $new_attributes ); @@ -124,8 +126,7 @@ public function sanitize() { * @return array Returns HTML attributes; removes any not specifically declared above from input. */ private function filter_attributes( $attributes ) { - $out = array(); - $out['style'] = 'max-width:100%'; // Note that this will get moved to amp-custom style by AMP_Style_Sanitizer. + $out = array(); foreach ( $attributes as $name => $value ) { switch ( $name ) { diff --git a/tests/test-amp-iframe-sanitizer.php b/tests/test-amp-iframe-sanitizer.php index 1bdda928309..0313b73667c 100644 --- a/tests/test-amp-iframe-sanitizer.php +++ b/tests/test-amp-iframe-sanitizer.php @@ -10,57 +10,57 @@ public function get_data() { 'simple_iframe' => array( '', - '', + '', ), 'force_https' => array( '', - '', + '', ), 'iframe_without_dimensions' => array( '', - '', + '', ), 'iframe_with_height_only' => array( '', - '', + '', ), 'iframe_with_width_only' => array( '', - '', + '', ), 'iframe_with_invalid_frameborder' => array( '', - '', + '', ), 'iframe_with_1_frameborder' => array( '', - '', + '', ), 'simple_iframe_with_sandbox' => array( '', - '', + '', ), 'iframe_with_blacklisted_attribute' => array( '', - '', + '', ), 'iframe_with_sizes_attribute_is_overridden' => array( - '', - '', + '', + '', ), 'iframe_with_protocol_relative_url' => array( '', - '', + '', ), 'multiple_same_iframe' => array( @@ -69,7 +69,7 @@ public function get_data() { ', - '', + '', ), 'multiple_different_iframes' => array( @@ -78,19 +78,19 @@ public function get_data() { ', - '', + '', ), 'iframe_in_p_tag' => array( '

', - '', + '', ), 'multiple_iframes_in_p_tag' => array( '

', - '', + '', ), 'multiple_iframes_and_contents_in_p_tag' => array( '

contents

', - '

contents

', + '

contents

', ), ); } @@ -160,7 +160,7 @@ public function test_get_scripts__did_convert() { public function test__args__placeholder() { $source = ''; - $expected = '
'; + $expected = '
'; $dom = AMP_DOM_Utils::get_dom_from_content( $source ); $sanitizer = new AMP_Iframe_Sanitizer( $dom, array( diff --git a/tests/test-amp-video-sanitizer.php b/tests/test-amp-video-sanitizer.php index f08f194db9e..917be213f5d 100644 --- a/tests/test-amp-video-sanitizer.php +++ b/tests/test-amp-video-sanitizer.php @@ -10,42 +10,42 @@ public function get_data() { 'simple_video' => array( '', - '', + '', ), 'video_without_dimensions' => array( '', - '', + '', ), 'autoplay_attribute' => array( '', - '', + '', ), 'autoplay_attribute__false' => array( '', - '', + '', ), 'video_with_whitelisted_attributes__enabled' => array( '', - '', + '', ), 'video_with_whitelisted_attributes__disabled' => array( '', - '', + '', ), 'video_with_blacklisted_attribute' => array( '', - '', + '', ), 'video_with_sizes_attribute_is_overridden' => array( - '', - '', + '', + '', ), 'video_with_children' => array( @@ -53,7 +53,7 @@ public function get_data() { ', - '', + '', ), 'multiple_same_video' => array( @@ -61,19 +61,19 @@ public function get_data() { ', - '', + '', ), 'multiple_different_videos' => array( ' ', - '', + '', ), 'https_not_required' => array( '', - '', + '', ), ); } diff --git a/tests/test-class-amp-base-sanitizer.php b/tests/test-class-amp-base-sanitizer.php index 4e135d4fbde..87575059e86 100644 --- a/tests/test-class-amp-base-sanitizer.php +++ b/tests/test-class-amp-base-sanitizer.php @@ -112,12 +112,14 @@ public function get_data() { return array( 'both_dimensions_included' => array( array( - 'width' => 100, + 'width' => 100, 'height' => 100, + 'layout' => 'responsive', ), array( - 'width' => 100, + 'width' => 100, 'height' => 100, + 'layout' => 'responsive', ), ), @@ -165,9 +167,9 @@ public function get_data() { /** * @dataProvider get_data */ - public function test_enforce_fixed_height( $source_attributes, $expected_attributes, $args = array() ) { + public function test_set_layout( $source_attributes, $expected_attributes, $args = array() ) { $sanitizer = new AMP_Test_Stub_Sanitizer( new DOMDocument, $args ); - $returned_attributes = $sanitizer->enforce_fixed_height( $source_attributes ); + $returned_attributes = $sanitizer->set_layout( $source_attributes ); $this->assertEquals( $expected_attributes, $returned_attributes ); } From 936f302a936401be6390635e35c22a4a4b19bbf1 Mon Sep 17 00:00:00 2001 From: Ryan Kienstra Date: Tue, 6 Feb 2018 15:46:17 -0600 Subject: [PATCH 02/18] Issue #864: Remove extra line in set_layout(). I had copied this line into the block above. But it's not needed there. --- includes/sanitizers/class-amp-base-sanitizer.php | 1 - 1 file changed, 1 deletion(-) diff --git a/includes/sanitizers/class-amp-base-sanitizer.php b/includes/sanitizers/class-amp-base-sanitizer.php index 3f9141abb8a..a94852fbdd6 100644 --- a/includes/sanitizers/class-amp-base-sanitizer.php +++ b/includes/sanitizers/class-amp-base-sanitizer.php @@ -207,7 +207,6 @@ public function set_layout( $attributes ) { if ( empty( $attributes['height'] ) ) { unset( $attributes['width'] ); $attributes['height'] = self::FALLBACK_HEIGHT; - $attributes['layout'] = 'fixed-height'; } if ( empty( $attributes['width'] ) ) { $attributes['layout'] = 'fixed-height'; From 6848541e9aa680f8756719d338d9358d1c4e516a Mon Sep 17 00:00:00 2001 From: Ryan Kienstra Date: Mon, 12 Feb 2018 22:47:59 -0600 Subject: [PATCH 03/18] Issue #864: Remove the 'sizes' attribute from . On Paul Baukus and Weston Ruter's suggestion. The sizes attribute isn't relevant to this WP plugin, as Paul Baukus mentioned. --- .../sanitizers/class-amp-base-sanitizer.php | 36 ------ .../sanitizers/class-amp-img-sanitizer.php | 2 +- tests/test-amp-img-sanitizer.php | 24 ++-- tests/test-class-amp-base-sanitizer.php | 107 ------------------ 4 files changed, 13 insertions(+), 156 deletions(-) diff --git a/includes/sanitizers/class-amp-base-sanitizer.php b/includes/sanitizers/class-amp-base-sanitizer.php index a94852fbdd6..aa958caa344 100644 --- a/includes/sanitizers/class-amp-base-sanitizer.php +++ b/includes/sanitizers/class-amp-base-sanitizer.php @@ -218,42 +218,6 @@ public function set_layout( $attributes ) { return $attributes; } - /** - * This is our workaround to enforce max sizing with layout=responsive. - * - * We want elements to not grow beyond their width and shrink to fill the screen on viewports smaller than their width. - * - * See https://github.com/ampproject/amphtml/issues/1280#issuecomment-171533526 - * See https://github.com/Automattic/amp-wp/issues/101 - * - * @param string[] $attributes { - * Attributes. - * - * @type int $height - * @type int $width - * @type string $sizes - * @type string $class - * @type string $layout - * } - * @return string[] - */ - public function enforce_sizes_attribute( $attributes ) { - if ( ! isset( $attributes['width'], $attributes['height'] ) ) { - return $attributes; - } - - $max_width = $attributes['width']; - if ( isset( $this->args['content_max_width'] ) && $max_width >= $this->args['content_max_width'] ) { - $max_width = $this->args['content_max_width']; - } - - $attributes['sizes'] = sprintf( '(min-width: %1$dpx) %1$dpx, 100vw', absint( $max_width ) ); - - $this->add_or_append_attribute( $attributes, 'class', 'amp-wp-enforced-sizes' ); - - return $attributes; - } - /** * Adds or appends key and value to list of attributes * diff --git a/includes/sanitizers/class-amp-img-sanitizer.php b/includes/sanitizers/class-amp-img-sanitizer.php index 8007aaa7152..22242a2fa0a 100644 --- a/includes/sanitizers/class-amp-img-sanitizer.php +++ b/includes/sanitizers/class-amp-img-sanitizer.php @@ -192,7 +192,7 @@ private function adjust_and_replace_nodes_in_array_map( $node_lists ) { private function adjust_and_replace_node( $node ) { $old_attributes = AMP_DOM_Utils::get_node_attributes_as_assoc_array( $node ); $new_attributes = $this->filter_attributes( $old_attributes ); - $new_attributes = $this->enforce_sizes_attribute( $new_attributes ); + $this->add_or_append_attribute( $new_attributes, 'class', 'amp-wp-enforced-sizes' ); if ( $this->is_gif_url( $new_attributes['src'] ) ) { $this->did_convert_elements = true; diff --git a/tests/test-amp-img-sanitizer.php b/tests/test-amp-img-sanitizer.php index 2c437b7deff..557b9faa675 100644 --- a/tests/test-amp-img-sanitizer.php +++ b/tests/test-amp-img-sanitizer.php @@ -29,47 +29,47 @@ public function get_data() { 'image_with_self_closing_tag' => array( 'Placeholder!', - '', + '', ), 'image_with_no_end_tag' => array( 'Placeholder!', - '', + '', ), 'image_with_end_tag' => array( 'Placeholder!', - '', + '', ), 'image_with_on_attribute' => array( '', - '', + '', ), 'image_with_blacklisted_attribute' => array( '', - '', + '', ), 'image_with_no_dimensions_is_forced_dimensions' => array( '', - '', + '', ), 'image_with_sizes_attribute_is_overridden' => array( - '', - '', + '', + '', ), 'gif_image_conversion' => array( 'Placeholder!', - '', + '', ), 'gif_image_url_with_querystring' => array( 'Placeholder!', - '', + '', ), 'multiple_same_image' => array( @@ -78,7 +78,7 @@ public function get_data() { ', - '', + '', ), 'multiple_different_images' => array( @@ -86,7 +86,7 @@ public function get_data() { ', - '', + '', ), ); } diff --git a/tests/test-class-amp-base-sanitizer.php b/tests/test-class-amp-base-sanitizer.php index 87575059e86..2f26ff6c64a 100644 --- a/tests/test-class-amp-base-sanitizer.php +++ b/tests/test-class-amp-base-sanitizer.php @@ -1,112 +1,5 @@ array( - array( - 'sizes' => 'blah', - ), - array( - 'sizes' => 'blah', - ), - ), - - 'empty' => array( - array(), - array(), - ), - - 'no_width' => array( - array( - 'height' => 100, - ), - array( - 'height' => 100, - ), - ), - - 'no_height' => array( - array( - 'width' => 200, - ), - array( - 'width' => 200, - ), - ), - - 'enforce_sizes_no_class' => array( - array( - 'width' => 200, - 'height' => 100, - ), - array( - 'width' => 200, - 'height' => 100, - 'sizes' => '(min-width: 200px) 200px, 100vw', - 'class' => 'amp-wp-enforced-sizes', - ), - ), - - 'enforce_sizes_has_class' => array( - array( - 'width' => 200, - 'height' => 100, - 'class' => 'my-class', - ), - array( - 'width' => 200, - 'height' => 100, - 'sizes' => '(min-width: 200px) 200px, 100vw', - 'class' => 'my-class amp-wp-enforced-sizes', - ), - ), - - 'enforce_sizes_with_bigger_content_max_width' => array( - array( - 'width' => 250, - 'height' => 100, - ), - array( - 'width' => 250, - 'height' => 100, - 'sizes' => '(min-width: 250px) 250px, 100vw', - 'class' => 'amp-wp-enforced-sizes', - ), - array( - 'content_max_width' => 500, - ), - ), - - 'enforce_sizes_with_smaller_content_max_width' => array( - array( - 'width' => 800, - 'height' => 350, - ), - array( - 'width' => 800, - 'height' => 350, - 'sizes' => '(min-width: 675px) 675px, 100vw', - 'class' => 'amp-wp-enforced-sizes', - ), - array( - 'content_max_width' => 675, - ), - ), - ); - } - - /** - * @dataProvider get_data - */ - public function test_enforce_sizes_attribute( $source_attributes, $expected_attributes, $args = array() ) { - $sanitizer = new AMP_Test_Stub_Sanitizer( new DOMDocument, $args ); - $returned_attributes = $sanitizer->enforce_sizes_attribute( $source_attributes ); - - $this->assertEquals( $expected_attributes, $returned_attributes ); - } -} - class AMP_Base_Sanitizer__Enforce_Fixed_Height__Test extends WP_UnitTestCase { public function get_data() { return array( From 11405b3c40346605fe6ba08b72f67f46d02be261 Mon Sep 17 00:00:00 2001 From: Ryan Kienstra Date: Tue, 13 Feb 2018 22:19:23 -0600 Subject: [PATCH 04/18] Issue #864: Remove layout="responsive" from . This could lead to unexpected results. If the intended width or height is less than the container, This will increase it to fill the container. This fixed the issue of overflowing in widgets. But it could create an issue in other places. --- .../sanitizers/class-amp-base-sanitizer.php | 3 -- .../sanitizers/class-amp-iframe-sanitizer.php | 3 +- .../sanitizers/class-amp-video-sanitizer.php | 2 +- tests/test-amp-iframe-sanitizer.php | 34 +++++++++---------- tests/test-amp-video-sanitizer.php | 22 ++++++------ 5 files changed, 31 insertions(+), 33 deletions(-) diff --git a/includes/sanitizers/class-amp-base-sanitizer.php b/includes/sanitizers/class-amp-base-sanitizer.php index fcf784d0678..43cbcc2b5aa 100644 --- a/includes/sanitizers/class-amp-base-sanitizer.php +++ b/includes/sanitizers/class-amp-base-sanitizer.php @@ -211,9 +211,6 @@ public function set_layout( $attributes ) { if ( empty( $attributes['width'] ) ) { $attributes['layout'] = 'fixed-height'; } - if ( ! empty( $attributes['width'] ) && ! empty( $attributes['height'] ) ) { - $attributes['layout'] = 'responsive'; - } return $attributes; } diff --git a/includes/sanitizers/class-amp-iframe-sanitizer.php b/includes/sanitizers/class-amp-iframe-sanitizer.php index a2acdd0de76..c3bd35dea30 100644 --- a/includes/sanitizers/class-amp-iframe-sanitizer.php +++ b/includes/sanitizers/class-amp-iframe-sanitizer.php @@ -80,7 +80,8 @@ public function sanitize() { $this->did_convert_elements = true; - $new_attributes = $this->set_layout( $new_attributes ); + $new_attributes = $this->set_layout( $new_attributes ); + $new_attributes['style'] = 'max-width:100%'; if ( isset( $new_attributes['width'] ) && isset( $new_attributes['height'] ) ) { $this->add_or_append_attribute( $new_attributes, 'class', 'amp-wp-enforced-sizes' ); } diff --git a/includes/sanitizers/class-amp-video-sanitizer.php b/includes/sanitizers/class-amp-video-sanitizer.php index a4a7c2b56cc..08e7762839f 100644 --- a/includes/sanitizers/class-amp-video-sanitizer.php +++ b/includes/sanitizers/class-amp-video-sanitizer.php @@ -52,7 +52,7 @@ public function sanitize() { $new_attributes = $this->set_layout( $new_attributes ); if ( isset( $new_attributes['width'] ) && isset( $new_attributes['height'] ) ) { - $this->add_or_append_attribute( $new_attributes, 'class', 'amp-wp-enforced-sizes' ); + $new_attributes['layout'] = 'responsive'; } $new_node = AMP_DOM_Utils::create_node( $this->dom, 'amp-video', $new_attributes ); diff --git a/tests/test-amp-iframe-sanitizer.php b/tests/test-amp-iframe-sanitizer.php index 7f02bafc3da..b3fdd23e5d6 100644 --- a/tests/test-amp-iframe-sanitizer.php +++ b/tests/test-amp-iframe-sanitizer.php @@ -10,57 +10,57 @@ public function get_data() { 'simple_iframe' => array( '', - '', + '', ), 'force_https' => array( '', - '', + '', ), 'iframe_without_dimensions' => array( '', - '', + '', ), 'iframe_with_height_only' => array( '', - '', + '', ), 'iframe_with_width_only' => array( '', - '', + '', ), 'iframe_with_invalid_frameborder' => array( '', - '', + '', ), 'iframe_with_1_frameborder' => array( '', - '', + '', ), 'simple_iframe_with_sandbox' => array( '', - '', + '', ), 'iframe_with_blacklisted_attribute' => array( '', - '', + '', ), 'iframe_with_sizes_attribute_is_overridden' => array( '', - '', + '', ), 'iframe_with_protocol_relative_url' => array( '', - '', + '', ), 'multiple_same_iframe' => array( @@ -69,7 +69,7 @@ public function get_data() { ', - '', + '', ), 'multiple_different_iframes' => array( @@ -78,19 +78,19 @@ public function get_data() { ', - '', + '', ), 'iframe_in_p_tag' => array( '

', - '', + '', ), 'multiple_iframes_in_p_tag' => array( '

', - '', + '', ), 'multiple_iframes_and_contents_in_p_tag' => array( '

contents

', - '

contents

', + '

contents

', ), ); } @@ -160,7 +160,7 @@ public function test_get_scripts__did_convert() { public function test__args__placeholder() { $source = ''; - $expected = '
'; + $expected = '
'; $dom = AMP_DOM_Utils::get_dom_from_content( $source ); $sanitizer = new AMP_Iframe_Sanitizer( $dom, array( diff --git a/tests/test-amp-video-sanitizer.php b/tests/test-amp-video-sanitizer.php index 917be213f5d..1a0ae62413d 100644 --- a/tests/test-amp-video-sanitizer.php +++ b/tests/test-amp-video-sanitizer.php @@ -10,7 +10,7 @@ public function get_data() { 'simple_video' => array( '', - '', + '', ), 'video_without_dimensions' => array( @@ -20,32 +20,32 @@ public function get_data() { 'autoplay_attribute' => array( '', - '', + '', ), 'autoplay_attribute__false' => array( '', - '', + '', ), 'video_with_whitelisted_attributes__enabled' => array( '', - '', + '', ), 'video_with_whitelisted_attributes__disabled' => array( '', - '', + '', ), 'video_with_blacklisted_attribute' => array( '', - '', + '', ), 'video_with_sizes_attribute_is_overridden' => array( '', - '', + '', ), 'video_with_children' => array( @@ -53,7 +53,7 @@ public function get_data() { ', - '', + '', ), 'multiple_same_video' => array( @@ -61,19 +61,19 @@ public function get_data() { ', - '', + '', ), 'multiple_different_videos' => array( ' ', - '', + '', ), 'https_not_required' => array( '', - '', + '', ), ); } From f95c0be97e24355fd29df393e313a143c5c89e34 Mon Sep 17 00:00:00 2001 From: Ryan Kienstra Date: Wed, 14 Feb 2018 00:32:24 -0600 Subject: [PATCH 05/18] Issue #864: Add comment after addition of 'style.' This was present earlier. Weston added this, and it describes where it will appear. --- includes/sanitizers/class-amp-iframe-sanitizer.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/includes/sanitizers/class-amp-iframe-sanitizer.php b/includes/sanitizers/class-amp-iframe-sanitizer.php index c3bd35dea30..0f758bfb6d3 100644 --- a/includes/sanitizers/class-amp-iframe-sanitizer.php +++ b/includes/sanitizers/class-amp-iframe-sanitizer.php @@ -81,7 +81,7 @@ public function sanitize() { $this->did_convert_elements = true; $new_attributes = $this->set_layout( $new_attributes ); - $new_attributes['style'] = 'max-width:100%'; + $new_attributes['style'] = 'max-width:100%'; // AMP_Style_Sanitizer will move this to the amp-custom style. if ( isset( $new_attributes['width'] ) && isset( $new_attributes['height'] ) ) { $this->add_or_append_attribute( $new_attributes, 'class', 'amp-wp-enforced-sizes' ); } From 6bd4326a88d60539b8568e2551f774e278f0720c Mon Sep 17 00:00:00 2001 From: Ryan Kienstra Date: Wed, 14 Feb 2018 18:10:59 -0600 Subject: [PATCH 06/18] Issue #864: Remove sizes attribute if present. An earlier commit only prevented adding it. This removes the attribute it it's present. --- includes/sanitizers/class-amp-img-sanitizer.php | 1 - 1 file changed, 1 deletion(-) diff --git a/includes/sanitizers/class-amp-img-sanitizer.php b/includes/sanitizers/class-amp-img-sanitizer.php index 90908a17414..4f4169b729d 100644 --- a/includes/sanitizers/class-amp-img-sanitizer.php +++ b/includes/sanitizers/class-amp-img-sanitizer.php @@ -119,7 +119,6 @@ private function filter_attributes( $attributes ) { case 'alt': case 'class': case 'srcset': - case 'sizes': case 'on': case 'attribution': $out[ $name ] = $value; From 613f16f6c164a0ac9688eafb99b1f0364e5dbf27 Mon Sep 17 00:00:00 2001 From: Ryan Kienstra Date: Wed, 14 Feb 2018 23:06:16 -0600 Subject: [PATCH 07/18] Issue #864: Add layout="responsive" to . Iframes from WordPress embeds usually have width and height. In AMP, the inferred value layout for this is fixed. This means that even if you apply max-width:100%, The height won't adjust to the new ratio. Setting the 'layout' to 'responsive' seems to be the only way to ensure the same aspect ratio. Of course, this has a risk that iframes will expand to fill their container, where they didn't before. --- .../sanitizers/class-amp-base-sanitizer.php | 4 ++- .../sanitizers/class-amp-iframe-sanitizer.php | 4 +-- tests/test-amp-iframe-sanitizer.php | 34 +++++++++---------- tests/test-class-amp-base-sanitizer.php | 20 ++++++++--- 4 files changed, 37 insertions(+), 25 deletions(-) diff --git a/includes/sanitizers/class-amp-base-sanitizer.php b/includes/sanitizers/class-amp-base-sanitizer.php index 43cbcc2b5aa..bf4a73fc85f 100644 --- a/includes/sanitizers/class-amp-base-sanitizer.php +++ b/includes/sanitizers/class-amp-base-sanitizer.php @@ -208,7 +208,9 @@ public function set_layout( $attributes ) { unset( $attributes['width'] ); $attributes['height'] = self::FALLBACK_HEIGHT; } - if ( empty( $attributes['width'] ) ) { + if ( ! empty( $attributes['width'] ) && ! empty( $attributes['height'] ) ) { + $attributes['layout'] = 'responsive'; + } elseif ( empty( $attributes['width'] ) ) { $attributes['layout'] = 'fixed-height'; } diff --git a/includes/sanitizers/class-amp-iframe-sanitizer.php b/includes/sanitizers/class-amp-iframe-sanitizer.php index 0f758bfb6d3..ed767018310 100644 --- a/includes/sanitizers/class-amp-iframe-sanitizer.php +++ b/includes/sanitizers/class-amp-iframe-sanitizer.php @@ -79,9 +79,7 @@ public function sanitize() { } $this->did_convert_elements = true; - - $new_attributes = $this->set_layout( $new_attributes ); - $new_attributes['style'] = 'max-width:100%'; // AMP_Style_Sanitizer will move this to the amp-custom style. + $new_attributes = $this->set_layout( $new_attributes ); if ( isset( $new_attributes['width'] ) && isset( $new_attributes['height'] ) ) { $this->add_or_append_attribute( $new_attributes, 'class', 'amp-wp-enforced-sizes' ); } diff --git a/tests/test-amp-iframe-sanitizer.php b/tests/test-amp-iframe-sanitizer.php index b3fdd23e5d6..7f02bafc3da 100644 --- a/tests/test-amp-iframe-sanitizer.php +++ b/tests/test-amp-iframe-sanitizer.php @@ -10,57 +10,57 @@ public function get_data() { 'simple_iframe' => array( '', - '', + '', ), 'force_https' => array( '', - '', + '', ), 'iframe_without_dimensions' => array( '', - '', + '', ), 'iframe_with_height_only' => array( '', - '', + '', ), 'iframe_with_width_only' => array( '', - '', + '', ), 'iframe_with_invalid_frameborder' => array( '', - '', + '', ), 'iframe_with_1_frameborder' => array( '', - '', + '', ), 'simple_iframe_with_sandbox' => array( '', - '', + '', ), 'iframe_with_blacklisted_attribute' => array( '', - '', + '', ), 'iframe_with_sizes_attribute_is_overridden' => array( '', - '', + '', ), 'iframe_with_protocol_relative_url' => array( '', - '', + '', ), 'multiple_same_iframe' => array( @@ -69,7 +69,7 @@ public function get_data() { ', - '', + '', ), 'multiple_different_iframes' => array( @@ -78,19 +78,19 @@ public function get_data() { ', - '', + '', ), 'iframe_in_p_tag' => array( '

', - '', + '', ), 'multiple_iframes_in_p_tag' => array( '

', - '', + '', ), 'multiple_iframes_and_contents_in_p_tag' => array( '

contents

', - '

contents

', + '

contents

', ), ); } @@ -160,7 +160,7 @@ public function test_get_scripts__did_convert() { public function test__args__placeholder() { $source = ''; - $expected = '
'; + $expected = '
'; $dom = AMP_DOM_Utils::get_dom_from_content( $source ); $sanitizer = new AMP_Iframe_Sanitizer( $dom, array( diff --git a/tests/test-class-amp-base-sanitizer.php b/tests/test-class-amp-base-sanitizer.php index e7fac24cf61..3ea82f6d92c 100644 --- a/tests/test-class-amp-base-sanitizer.php +++ b/tests/test-class-amp-base-sanitizer.php @@ -16,7 +16,7 @@ public function get_data() { ), ), - 'both_dimensions_missing' => array( + 'both_dimensions_missing' => array( array(), array( 'height' => 400, @@ -24,7 +24,7 @@ public function get_data() { ), ), - 'both_dimensions_empty' => array( + 'both_dimensions_empty' => array( array( 'width' => '', 'height' => '', @@ -35,7 +35,7 @@ public function get_data() { ), ), - 'no_width' => array( + 'no_width' => array( array( 'height' => 100, ), @@ -45,7 +45,7 @@ public function get_data() { ), ), - 'no_height' => array( + 'no_height' => array( array( 'width' => 200, ), @@ -54,6 +54,18 @@ public function get_data() { 'layout' => 'fixed-height', ), ), + + 'no_layout_specified' => array( + array( + 'width' => 100, + 'height' => 100, + ), + array( + 'width' => 100, + 'height' => 100, + 'layout' => 'responsive', + ), + ), ); } From 878e023274622a4683a7338478af1c1958f070a9 Mon Sep 17 00:00:00 2001 From: Ryan Kienstra Date: Thu, 1 Mar 2018 13:53:40 -0600 Subject: [PATCH 08/18] Issue #864: Convert 'data-amp-layout' to 'layout.' In the image preprocessor, Convert a 'data-amp-layout' attribute to 'layout.' As Weston mentioned, this won't change the styling on non-AMP pages, As the preprocessor won't modify them. --- .../sanitizers/class-amp-img-sanitizer.php | 4 +++ tests/test-amp-img-sanitizer.php | 29 +++++++++++-------- 2 files changed, 21 insertions(+), 12 deletions(-) diff --git a/includes/sanitizers/class-amp-img-sanitizer.php b/includes/sanitizers/class-amp-img-sanitizer.php index 4f4169b729d..45d4e3c9b3a 100644 --- a/includes/sanitizers/class-amp-img-sanitizer.php +++ b/includes/sanitizers/class-amp-img-sanitizer.php @@ -129,6 +129,10 @@ private function filter_attributes( $attributes ) { $out[ $name ] = $this->sanitize_dimension( $value, $name ); break; + case 'data-amp-layout': + $out['layout'] = $value; + break; + default: break; } diff --git a/tests/test-amp-img-sanitizer.php b/tests/test-amp-img-sanitizer.php index 557b9faa675..52889dd1097 100644 --- a/tests/test-amp-img-sanitizer.php +++ b/tests/test-amp-img-sanitizer.php @@ -12,42 +12,47 @@ public function setUp() { public function get_data() { return array( - 'no_images' => array( + 'no_images' => array( '

Lorem Ipsum Demet Delorit.

', '

Lorem Ipsum Demet Delorit.

', ), - 'image_without_src' => array( + 'image_without_src' => array( '

', '

', ), - 'image_with_empty_src' => array( + 'image_with_empty_src' => array( '

', '

', ), - 'image_with_self_closing_tag' => array( + 'image_with_layout' => array( + '', + '', + ), + + 'image_with_self_closing_tag' => array( 'Placeholder!', '', ), - 'image_with_no_end_tag' => array( + 'image_with_no_end_tag' => array( 'Placeholder!', '', ), - 'image_with_end_tag' => array( + 'image_with_end_tag' => array( 'Placeholder!', '', ), - 'image_with_on_attribute' => array( + 'image_with_on_attribute' => array( '', '', ), - 'image_with_blacklisted_attribute' => array( + 'image_with_blacklisted_attribute' => array( '', '', ), @@ -62,17 +67,17 @@ public function get_data() { '', ), - 'gif_image_conversion' => array( + 'gif_image_conversion' => array( 'Placeholder!', '', ), - 'gif_image_url_with_querystring' => array( + 'gif_image_url_with_querystring' => array( 'Placeholder!', '', ), - 'multiple_same_image' => array( + 'multiple_same_image' => array( ' @@ -81,7 +86,7 @@ public function get_data() { '', ), - 'multiple_different_images' => array( + 'multiple_different_images' => array( ' From 037f6efda336e52de1d77cdfe46fa661ac5f30a8 Mon Sep 17 00:00:00 2001 From: Ryan Kienstra Date: Thu, 1 Mar 2018 15:00:50 -0600 Subject: [PATCH 09/18] Issue #864: Allow 'data-amp-layout' in wp_kses() for 'post.' Add this attribute to the allowed list for . The sanitizer now converts this to 'layout.' --- includes/class-amp-theme-support.php | 1 + includes/utils/class-amp-wp-utils.php | 24 ++++++++++++++++++++++++ tests/test-amp-wp-utils.php | 22 ++++++++++++++++++++++ 3 files changed, 47 insertions(+) diff --git a/includes/class-amp-theme-support.php b/includes/class-amp-theme-support.php index acda180917f..22eb7f85579 100644 --- a/includes/class-amp-theme-support.php +++ b/includes/class-amp-theme-support.php @@ -183,6 +183,7 @@ public static function register_hooks() { add_filter( 'comment_reply_link', array( __CLASS__, 'filter_comment_reply_link' ), 10, 4 ); add_filter( 'cancel_comment_reply_link', array( __CLASS__, 'filter_cancel_comment_reply_link' ), 10, 3 ); add_action( 'comment_form', array( __CLASS__, 'add_amp_comment_form_templates' ), 100 ); + add_filter( 'wp_kses_allowed_html', 'AMP_WP_Utils::add_layout', 10, 2 ); // @todo Add character conversion. } diff --git a/includes/utils/class-amp-wp-utils.php b/includes/utils/class-amp-wp-utils.php index bacb72458af..014dabab1d5 100644 --- a/includes/utils/class-amp-wp-utils.php +++ b/includes/utils/class-amp-wp-utils.php @@ -58,4 +58,28 @@ protected static function _wp_translate_php_url_constant_to_key( $constant ) { return false; } + + /** + * Adds 'data-amp-layout' to the allowed attributes for wp_kses() in a 'post' context. + * + * @since 0.7 + * + * @param array $context Allowed tags and their allowed attributes. + * @param string $context_type Type of context. + * @return array $context Filtered allowed tags and attributes. + */ + public static function add_layout( $context, $context_type ) { + if ( 'post' !== $context_type ) { + return $context; + } + $img = isset( $context['img'] ) ? $context['img'] : array(); + $context['img'] = array_merge( + $img, + array( + 'data-amp-layout' => true, + ) + ); + return $context; + } + } diff --git a/tests/test-amp-wp-utils.php b/tests/test-amp-wp-utils.php index 43f3b3a572e..93e0faa4fc7 100644 --- a/tests/test-amp-wp-utils.php +++ b/tests/test-amp-wp-utils.php @@ -44,4 +44,26 @@ function test__method( $url, $expected, $component ) { $this->assertEquals( $expected, $actual ); } + + /** + * Test AMP_WP_Utils::add_layout(). + * + * @see AMP_WP_Utils::add_layout() + */ + public function test_add_layout() { + $this->assertEquals( array(), AMP_WP_Utils::add_layout( array(), 'explicit' ) ); + $this->assertEquals( + array( + 'img' => array( + 'data-amp-layout' => true, + ), + ), + AMP_WP_Utils::add_layout( array(), 'post' ) + ); + + add_filter( 'wp_kses_allowed_html', 'AMP_WP_Utils::add_layout', 10, 2 ); + $image = ''; + $this->assertEquals( $image, wp_kses_post( $image ) ); + } + } From a233b59e1d4088eae1f614fc4ed856c700959558 Mon Sep 17 00:00:00 2001 From: Ryan Kienstra Date: Thu, 1 Mar 2018 15:28:06 -0600 Subject: [PATCH 10/18] Issue #864: Change logic for adding 'data-amp-layout'. Per feedback, this should apply to all contexts, As long as they allow with a width and height. Uses Weston's snippet in the filter callback. @todo: move it to AMP_Theme_Support. --- includes/utils/class-amp-wp-utils.php | 11 ++--------- tests/test-amp-wp-utils.php | 27 ++++++++++++++++++++------- 2 files changed, 22 insertions(+), 16 deletions(-) diff --git a/includes/utils/class-amp-wp-utils.php b/includes/utils/class-amp-wp-utils.php index 014dabab1d5..737e79a3c94 100644 --- a/includes/utils/class-amp-wp-utils.php +++ b/includes/utils/class-amp-wp-utils.php @@ -69,16 +69,9 @@ protected static function _wp_translate_php_url_constant_to_key( $constant ) { * @return array $context Filtered allowed tags and attributes. */ public static function add_layout( $context, $context_type ) { - if ( 'post' !== $context_type ) { - return $context; + if ( ! empty( $context['img']['width'] ) && ! empty( $context['img']['height'] ) ) { + $context['img']['data-amp-layout'] = true; } - $img = isset( $context['img'] ) ? $context['img'] : array(); - $context['img'] = array_merge( - $img, - array( - 'data-amp-layout' => true, - ) - ); return $context; } diff --git a/tests/test-amp-wp-utils.php b/tests/test-amp-wp-utils.php index 93e0faa4fc7..685915022c0 100644 --- a/tests/test-amp-wp-utils.php +++ b/tests/test-amp-wp-utils.php @@ -51,15 +51,28 @@ function test__method( $url, $expected, $component ) { * @see AMP_WP_Utils::add_layout() */ public function test_add_layout() { - $this->assertEquals( array(), AMP_WP_Utils::add_layout( array(), 'explicit' ) ); - $this->assertEquals( - array( - 'img' => array( - 'data-amp-layout' => true, - ), + $attribute = 'data-amp-layout'; + $image_no_dimensions = array( + 'img' => array( + $attribute => true, ), - AMP_WP_Utils::add_layout( array(), 'post' ) ); + $image_with_dimensions = array_merge( + $image_no_dimensions, + array( + 'height' => '100', + 'width' => '100', + ) + ); + + $this->assertEquals( array(), AMP_WP_Utils::add_layout( array(), 'explicit' ) ); + $this->assertEquals( $image_no_dimensions, AMP_WP_Utils::add_layout( $image_no_dimensions, 'post' ) ); + + $context = AMP_WP_Utils::add_layout( $image_with_dimensions, 'post' ); + $this->assertTrue( $context['img'][ $attribute ] ); + + $context = AMP_WP_Utils::add_layout( $image_with_dimensions, 'explicit' ); + $this->assertTrue( $context['img'][ $attribute ] ); add_filter( 'wp_kses_allowed_html', 'AMP_WP_Utils::add_layout', 10, 2 ); $image = ''; From 011e47838e5154bb77af4f36010ff56d8bedd9eb Mon Sep 17 00:00:00 2001 From: Ryan Kienstra Date: Thu, 1 Mar 2018 15:56:43 -0600 Subject: [PATCH 11/18] Issue #864: Move add_layout() to AMP_Theme_Support. Also, remove $context_type parameter. It's no longer used. Otherwise, there's no change to the function. --- includes/class-amp-theme-support.php | 18 ++++++++++++- includes/utils/class-amp-wp-utils.php | 17 ------------- tests/test-amp-wp-utils.php | 35 -------------------------- tests/test-class-amp-theme-support.php | 35 ++++++++++++++++++++++++++ 4 files changed, 52 insertions(+), 53 deletions(-) diff --git a/includes/class-amp-theme-support.php b/includes/class-amp-theme-support.php index 22eb7f85579..71237df7aa3 100644 --- a/includes/class-amp-theme-support.php +++ b/includes/class-amp-theme-support.php @@ -183,7 +183,7 @@ public static function register_hooks() { add_filter( 'comment_reply_link', array( __CLASS__, 'filter_comment_reply_link' ), 10, 4 ); add_filter( 'cancel_comment_reply_link', array( __CLASS__, 'filter_cancel_comment_reply_link' ), 10, 3 ); add_action( 'comment_form', array( __CLASS__, 'add_amp_comment_form_templates' ), 100 ); - add_filter( 'wp_kses_allowed_html', 'AMP_WP_Utils::add_layout', 10, 2 ); + add_filter( 'wp_kses_allowed_html', array( __CLASS__, 'add_layout' ), 10 ); // @todo Add character conversion. } @@ -834,4 +834,20 @@ public static function prepare_response( $response, $args = array() ) { return $response; } + + /** + * Adds 'data-amp-layout' to the allowed attributes for wp_kses(). + * + * @since 0.7 + * + * @param array $context Allowed tags and their allowed attributes. + * @return array $context Filtered allowed tags and attributes. + */ + public static function add_layout( $context ) { + if ( ! empty( $context['img']['width'] ) && ! empty( $context['img']['height'] ) ) { + $context['img']['data-amp-layout'] = true; + } + return $context; + } + } diff --git a/includes/utils/class-amp-wp-utils.php b/includes/utils/class-amp-wp-utils.php index 737e79a3c94..bacb72458af 100644 --- a/includes/utils/class-amp-wp-utils.php +++ b/includes/utils/class-amp-wp-utils.php @@ -58,21 +58,4 @@ protected static function _wp_translate_php_url_constant_to_key( $constant ) { return false; } - - /** - * Adds 'data-amp-layout' to the allowed attributes for wp_kses() in a 'post' context. - * - * @since 0.7 - * - * @param array $context Allowed tags and their allowed attributes. - * @param string $context_type Type of context. - * @return array $context Filtered allowed tags and attributes. - */ - public static function add_layout( $context, $context_type ) { - if ( ! empty( $context['img']['width'] ) && ! empty( $context['img']['height'] ) ) { - $context['img']['data-amp-layout'] = true; - } - return $context; - } - } diff --git a/tests/test-amp-wp-utils.php b/tests/test-amp-wp-utils.php index 685915022c0..43f3b3a572e 100644 --- a/tests/test-amp-wp-utils.php +++ b/tests/test-amp-wp-utils.php @@ -44,39 +44,4 @@ function test__method( $url, $expected, $component ) { $this->assertEquals( $expected, $actual ); } - - /** - * Test AMP_WP_Utils::add_layout(). - * - * @see AMP_WP_Utils::add_layout() - */ - public function test_add_layout() { - $attribute = 'data-amp-layout'; - $image_no_dimensions = array( - 'img' => array( - $attribute => true, - ), - ); - $image_with_dimensions = array_merge( - $image_no_dimensions, - array( - 'height' => '100', - 'width' => '100', - ) - ); - - $this->assertEquals( array(), AMP_WP_Utils::add_layout( array(), 'explicit' ) ); - $this->assertEquals( $image_no_dimensions, AMP_WP_Utils::add_layout( $image_no_dimensions, 'post' ) ); - - $context = AMP_WP_Utils::add_layout( $image_with_dimensions, 'post' ); - $this->assertTrue( $context['img'][ $attribute ] ); - - $context = AMP_WP_Utils::add_layout( $image_with_dimensions, 'explicit' ); - $this->assertTrue( $context['img'][ $attribute ] ); - - add_filter( 'wp_kses_allowed_html', 'AMP_WP_Utils::add_layout', 10, 2 ); - $image = ''; - $this->assertEquals( $image, wp_kses_post( $image ) ); - } - } diff --git a/tests/test-class-amp-theme-support.php b/tests/test-class-amp-theme-support.php index b71c4d41228..a339bd76d89 100644 --- a/tests/test-class-amp-theme-support.php +++ b/tests/test-class-amp-theme-support.php @@ -249,4 +249,39 @@ public function test_get_amp_scripts() { $scripts ); } + + /** + * Test AMP_Theme_Support::add_layout(). + * + * @see AMP_Theme_Support::add_layout() + */ + public function test_add_layout() { + $attribute = 'data-amp-layout'; + $image_no_dimensions = array( + 'img' => array( + $attribute => true, + ), + ); + $image_with_dimensions = array_merge( + $image_no_dimensions, + array( + 'height' => '100', + 'width' => '100', + ) + ); + + $this->assertEquals( array(), AMP_Theme_Support::add_layout( array() ) ); + $this->assertEquals( $image_no_dimensions, AMP_Theme_Support::add_layout( $image_no_dimensions ) ); + + $context = AMP_Theme_Support::add_layout( $image_with_dimensions ); + $this->assertTrue( $context['img'][ $attribute ] ); + + $context = AMP_Theme_Support::add_layout( $image_with_dimensions ); + $this->assertTrue( $context['img'][ $attribute ] ); + + add_filter( 'wp_kses_allowed_html', 'AMP_Theme_Support::add_layout', 10, 2 ); + $image = ''; + $this->assertEquals( $image, wp_kses_post( $image ) ); + } + } From e344418132bc56fbf4d0c6083f399061f0ab4976 Mon Sep 17 00:00:00 2001 From: Ryan Kienstra Date: Thu, 1 Mar 2018 16:32:55 -0600 Subject: [PATCH 12/18] Rename to whitelist_layout_in_wp_kses_allowed_html(). Change the function name from: AMP_Theme_Support::add_layout(). --- includes/class-amp-theme-support.php | 4 ++-- tests/test-class-amp-theme-support.php | 16 ++++++++-------- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/includes/class-amp-theme-support.php b/includes/class-amp-theme-support.php index 0623d32355c..c9ccfdda178 100644 --- a/includes/class-amp-theme-support.php +++ b/includes/class-amp-theme-support.php @@ -215,7 +215,7 @@ public static function register_hooks() { add_filter( 'cancel_comment_reply_link', array( __CLASS__, 'filter_cancel_comment_reply_link' ), 10, 3 ); add_action( 'comment_form', array( __CLASS__, 'add_amp_comment_form_templates' ), 100 ); remove_action( 'comment_form', 'wp_comment_form_unfiltered_html_nonce' ); - add_filter( 'wp_kses_allowed_html', array( __CLASS__, 'add_layout' ), 10 ); + add_filter( 'wp_kses_allowed_html', array( __CLASS__, 'whitelist_layout_in_wp_kses_allowed_html' ), 10 ); // @todo Add character conversion. } @@ -1017,7 +1017,7 @@ public static function prepare_response( $response, $args = array() ) { * @param array $context Allowed tags and their allowed attributes. * @return array $context Filtered allowed tags and attributes. */ - public static function add_layout( $context ) { + public static function whitelist_layout_in_wp_kses_allowed_html( $context ) { if ( ! empty( $context['img']['width'] ) && ! empty( $context['img']['height'] ) ) { $context['img']['data-amp-layout'] = true; } diff --git a/tests/test-class-amp-theme-support.php b/tests/test-class-amp-theme-support.php index 7c2bb3ae5f6..2ae8824ee6f 100644 --- a/tests/test-class-amp-theme-support.php +++ b/tests/test-class-amp-theme-support.php @@ -332,11 +332,11 @@ public function test_handle_xhr_request() { } /** - * Test AMP_Theme_Support::add_layout(). + * Test AMP_Theme_Support::whitelist_layout_in_wp_kses_allowed_html(). * - * @see AMP_Theme_Support::add_layout() + * @see AMP_Theme_Support::whitelist_layout_in_wp_kses_allowed_html() */ - public function test_add_layout() { + public function test_whitelist_layout_in_wp_kses_allowed_html() { $attribute = 'data-amp-layout'; $image_no_dimensions = array( 'img' => array( @@ -351,16 +351,16 @@ public function test_add_layout() { ) ); - $this->assertEquals( array(), AMP_Theme_Support::add_layout( array() ) ); - $this->assertEquals( $image_no_dimensions, AMP_Theme_Support::add_layout( $image_no_dimensions ) ); + $this->assertEquals( array(), AMP_Theme_Support::whitelist_layout_in_wp_kses_allowed_html( array() ) ); + $this->assertEquals( $image_no_dimensions, AMP_Theme_Support::whitelist_layout_in_wp_kses_allowed_html( $image_no_dimensions ) ); - $context = AMP_Theme_Support::add_layout( $image_with_dimensions ); + $context = AMP_Theme_Support::whitelist_layout_in_wp_kses_allowed_html( $image_with_dimensions ); $this->assertTrue( $context['img'][ $attribute ] ); - $context = AMP_Theme_Support::add_layout( $image_with_dimensions ); + $context = AMP_Theme_Support::whitelist_layout_in_wp_kses_allowed_html( $image_with_dimensions ); $this->assertTrue( $context['img'][ $attribute ] ); - add_filter( 'wp_kses_allowed_html', 'AMP_Theme_Support::add_layout', 10, 2 ); + add_filter( 'wp_kses_allowed_html', 'AMP_Theme_Support::whitelist_layout_in_wp_kses_allowed_html', 10, 2 ); $image = ''; $this->assertEquals( $image, wp_kses_post( $image ) ); } From adce3086b2732e32eee42b518724c6b39506be98 Mon Sep 17 00:00:00 2001 From: Mike Crantea Date: Thu, 29 Mar 2018 17:42:33 +0300 Subject: [PATCH 13/18] 864 - Use intrinsic as default layout for images --- includes/sanitizers/class-amp-img-sanitizer.php | 3 +++ 1 file changed, 3 insertions(+) diff --git a/includes/sanitizers/class-amp-img-sanitizer.php b/includes/sanitizers/class-amp-img-sanitizer.php index 97a3cb89fdb..2b4e0245df3 100644 --- a/includes/sanitizers/class-amp-img-sanitizer.php +++ b/includes/sanitizers/class-amp-img-sanitizer.php @@ -208,6 +208,9 @@ private function adjust_and_replace_node( $node ) { $old_attributes = AMP_DOM_Utils::get_node_attributes_as_assoc_array( $node ); $new_attributes = $this->filter_attributes( $old_attributes ); $this->add_or_append_attribute( $new_attributes, 'class', 'amp-wp-enforced-sizes' ); + if ( empty( $new_attributes['layout'] ) && ! empty( $new_attributes['height'] ) && ! empty( $new_attributes['width'] ) ) { + $new_attributes['layout'] = 'intrinsic'; + } if ( $this->is_gif_url( $new_attributes['src'] ) ) { $this->did_convert_elements = true; From bc5c4073b2101a30ed3344759bf31123fe5ca363 Mon Sep 17 00:00:00 2001 From: Ryan Kienstra Date: Thu, 29 Mar 2018 14:01:13 -0600 Subject: [PATCH 14/18] Update unit tests for layout. Add layout="intrinsic" to some tags. We might later consider using that value for other tags as a default value. --- tests/test-amp-img-sanitizer.php | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/tests/test-amp-img-sanitizer.php b/tests/test-amp-img-sanitizer.php index a65a18713dc..6a8bb176024 100644 --- a/tests/test-amp-img-sanitizer.php +++ b/tests/test-amp-img-sanitizer.php @@ -59,52 +59,52 @@ public function get_data() { 'image_with_decimal_width' => array( '

', - '

', + '

', ), 'image_with_self_closing_tag' => array( 'Placeholder!', - '', + '', ), 'image_with_no_end_tag' => array( 'Placeholder!', - '', + '', ), 'image_with_end_tag' => array( 'Placeholder!', - '', + '', ), 'image_with_on_attribute' => array( '', - '', + '', ), 'image_with_blacklisted_attribute' => array( '', - '', + '', ), 'image_with_no_dimensions_is_forced_dimensions' => array( '', - '', + '', ), 'image_with_sizes_attribute_is_overridden' => array( '', - '', + '', ), 'gif_image_conversion' => array( 'Placeholder!', - '', + '', ), 'gif_image_url_with_querystring' => array( 'Placeholder!', - '', + '', ), 'multiple_same_image' => array( @@ -113,7 +113,7 @@ public function get_data() { ', - '', + '', ), 'multiple_different_images' => array( @@ -121,7 +121,7 @@ public function get_data() { ', - '', + '', ), ); } From ece0f8650c9096f6372269c406e85a8717f2f093 Mon Sep 17 00:00:00 2001 From: Ryan Kienstra Date: Thu, 29 Mar 2018 14:07:26 -0600 Subject: [PATCH 15/18] Align equals signs, use () after class. In response to Travis errors. Use parentheses in class instantiation. --- tests/test-class-amp-base-sanitizer.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test-class-amp-base-sanitizer.php b/tests/test-class-amp-base-sanitizer.php index 72fa047d564..cc30670eec6 100644 --- a/tests/test-class-amp-base-sanitizer.php +++ b/tests/test-class-amp-base-sanitizer.php @@ -95,7 +95,7 @@ public function get_data() { * @covers AMP_Base_Sanitizer::enforce_fixed_height() */ public function test_set_layout( $source_attributes, $expected_attributes, $args = array() ) { - $sanitizer = new AMP_Test_Stub_Sanitizer( new DOMDocument, $args ); + $sanitizer = new AMP_Test_Stub_Sanitizer( new DOMDocument(), $args ); $returned_attributes = $sanitizer->set_layout( $source_attributes ); $this->assertEquals( $expected_attributes, $returned_attributes ); } From 02db2293d2d13fb3c8c64a6fe56461581c92bd1a Mon Sep 17 00:00:00 2001 From: Ryan Kienstra Date: Thu, 29 Mar 2018 15:00:54 -0600 Subject: [PATCH 16/18] Set default layout of and to intrinsic. Inspired by Mike Cranteas earlier commit, which applied to Before, this set the layout of those elements to responsive if the height and width weren't empty. This does the same thing, only with 'intrinsic.' Initial testing on Native AMP and paired mode shows that this displays as expected. According to the documentation, 'This layout works very well for most AMP elements, including amp-img, amp-video,' @see https://www.ampproject.org/docs/design/responsive/control_layout#what-if-the-layout-attribute-isn%E2%80%99t-specified? --- .../sanitizers/class-amp-base-sanitizer.php | 2 +- tests/test-amp-iframe-sanitizer.php | 26 +++++++++---------- tests/test-class-amp-base-sanitizer.php | 6 ++--- 3 files changed, 17 insertions(+), 17 deletions(-) diff --git a/includes/sanitizers/class-amp-base-sanitizer.php b/includes/sanitizers/class-amp-base-sanitizer.php index baea21ee265..f6d837a4899 100644 --- a/includes/sanitizers/class-amp-base-sanitizer.php +++ b/includes/sanitizers/class-amp-base-sanitizer.php @@ -220,7 +220,7 @@ public function set_layout( $attributes ) { $attributes['height'] = self::FALLBACK_HEIGHT; } if ( ! empty( $attributes['width'] ) && ! empty( $attributes['height'] ) ) { - $attributes['layout'] = 'responsive'; + $attributes['layout'] = 'intrinsic'; } elseif ( empty( $attributes['width'] ) ) { $attributes['layout'] = 'fixed-height'; } diff --git a/tests/test-amp-iframe-sanitizer.php b/tests/test-amp-iframe-sanitizer.php index 3b4659c3c56..be2f00becfd 100644 --- a/tests/test-amp-iframe-sanitizer.php +++ b/tests/test-amp-iframe-sanitizer.php @@ -10,12 +10,12 @@ public function get_data() { 'simple_iframe' => array( '', - '', + '', ), 'force_https' => array( '', - '', + '', ), 'iframe_without_dimensions' => array( @@ -35,27 +35,27 @@ public function get_data() { 'iframe_with_invalid_frameborder' => array( '', - '', + '', ), 'iframe_with_1_frameborder' => array( '', - '', + '', ), 'simple_iframe_with_sandbox' => array( '', - '', + '', ), 'iframe_with_blacklisted_attribute' => array( '', - '', + '', ), 'iframe_with_sizes_attribute_is_overridden' => array( '', - '', + '', ), 'iframe_with_protocol_relative_url' => array( @@ -69,7 +69,7 @@ public function get_data() { ', - '', + '', ), 'multiple_different_iframes' => array( @@ -78,19 +78,19 @@ public function get_data() { ', - '', + '', ), 'iframe_in_p_tag' => array( '

', - '', + '', ), 'multiple_iframes_in_p_tag' => array( '

', - '', + '', ), 'multiple_iframes_and_contents_in_p_tag' => array( '

contents

', - '

contents

', + '

contents

', ), ); } @@ -160,7 +160,7 @@ public function test_get_scripts__did_convert() { public function test__args__placeholder() { $source = ''; - $expected = '
'; + $expected = '
'; $dom = AMP_DOM_Utils::get_dom_from_content( $source ); $sanitizer = new AMP_Iframe_Sanitizer( $dom, array( diff --git a/tests/test-class-amp-base-sanitizer.php b/tests/test-class-amp-base-sanitizer.php index cc30670eec6..eb7a3fae197 100644 --- a/tests/test-class-amp-base-sanitizer.php +++ b/tests/test-class-amp-base-sanitizer.php @@ -28,7 +28,7 @@ public function get_data() { array( 'width' => 100, 'height' => 100, - 'layout' => 'responsive', + 'layout' => 'intrinsic', ), ), @@ -79,14 +79,14 @@ public function get_data() { array( 'width' => 100, 'height' => 100, - 'layout' => 'responsive', + 'layout' => 'intrinsic', ), ), ); } /** - * Test AMP_Base_Sanitizer::enforce_fixed_height(). + * Test AMP_Base_Sanitizer::set_layout(). * * @dataProvider get_data * @param array $source_attributes Source Attrs. From f983029f01f544420a65f3b4e626f74bc0ce8874 Mon Sep 17 00:00:00 2001 From: Ryan Kienstra Date: Sat, 31 Mar 2018 13:05:23 -0600 Subject: [PATCH 17/18] Remove my duplicate layout assignment in AMP_Video_Sanitizer. This already conditionally sets a layout of responsive in set_layout(). And the removed logic sometimes reassigned the layout to responsive. So remove this extra logic. --- .../sanitizers/class-amp-video-sanitizer.php | 4 ---- tests/test-amp-video-sanitizer.php | 22 +++++++++---------- 2 files changed, 11 insertions(+), 15 deletions(-) diff --git a/includes/sanitizers/class-amp-video-sanitizer.php b/includes/sanitizers/class-amp-video-sanitizer.php index 08e7762839f..fe4ecba8ed3 100644 --- a/includes/sanitizers/class-amp-video-sanitizer.php +++ b/includes/sanitizers/class-amp-video-sanitizer.php @@ -49,11 +49,7 @@ public function sanitize() { $old_attributes = AMP_DOM_Utils::get_node_attributes_as_assoc_array( $node ); $new_attributes = $this->filter_attributes( $old_attributes ); - $new_attributes = $this->set_layout( $new_attributes ); - if ( isset( $new_attributes['width'] ) && isset( $new_attributes['height'] ) ) { - $new_attributes['layout'] = 'responsive'; - } $new_node = AMP_DOM_Utils::create_node( $this->dom, 'amp-video', $new_attributes ); diff --git a/tests/test-amp-video-sanitizer.php b/tests/test-amp-video-sanitizer.php index 5a0f989db05..249520cb27a 100644 --- a/tests/test-amp-video-sanitizer.php +++ b/tests/test-amp-video-sanitizer.php @@ -10,7 +10,7 @@ public function get_data() { 'simple_video' => array( '', - '', + '', ), 'video_without_dimensions' => array( @@ -20,32 +20,32 @@ public function get_data() { 'autoplay_attribute' => array( '', - '', + '', ), 'autoplay_attribute__false' => array( '', - '', + '', ), 'video_with_whitelisted_attributes__enabled' => array( '', - '', + '', ), 'video_with_whitelisted_attributes__disabled' => array( '', - '', + '', ), 'video_with_blacklisted_attribute' => array( '', - '', + '', ), 'video_with_sizes_attribute_is_overridden' => array( '', - '', + '', ), 'video_with_children' => array( @@ -53,7 +53,7 @@ public function get_data() { ', - '', + '', ), 'multiple_same_video' => array( @@ -61,19 +61,19 @@ public function get_data() { ', - '', + '', ), 'multiple_different_videos' => array( ' ', - '', + '', ), 'https_not_required' => array( '', - '', + '', ), ); } From f5a3c7380abdc427abd2eae8e93be633ff2d7414 Mon Sep 17 00:00:00 2001 From: Ryan Kienstra Date: Sat, 31 Mar 2018 14:21:08 -0600 Subject: [PATCH 18/18] Conditionally set layout to 'responsive' for . This used to be a layout of 'intrinsic.' Strangely, the layout documentation says about 'intrinsic': 'This layout works very well for most of AMP elements, including amp-img, amp-video' @see https://www.ampproject.org/docs/design/responsive/control_layout But the spec for doesn't allow layout="intrinsic". So use layout=responsive for Still use layout=intrinsic for --- .../sanitizers/class-amp-base-sanitizer.php | 4 +--- .../sanitizers/class-amp-iframe-sanitizer.php | 3 ++- .../sanitizers/class-amp-video-sanitizer.php | 3 +++ tests/test-amp-video-sanitizer.php | 22 +++++++++---------- tests/test-class-amp-base-sanitizer.php | 3 +-- 5 files changed, 18 insertions(+), 17 deletions(-) diff --git a/includes/sanitizers/class-amp-base-sanitizer.php b/includes/sanitizers/class-amp-base-sanitizer.php index f6d837a4899..0fc9a697085 100644 --- a/includes/sanitizers/class-amp-base-sanitizer.php +++ b/includes/sanitizers/class-amp-base-sanitizer.php @@ -219,9 +219,7 @@ public function set_layout( $attributes ) { unset( $attributes['width'] ); $attributes['height'] = self::FALLBACK_HEIGHT; } - if ( ! empty( $attributes['width'] ) && ! empty( $attributes['height'] ) ) { - $attributes['layout'] = 'intrinsic'; - } elseif ( empty( $attributes['width'] ) ) { + if ( empty( $attributes['width'] ) ) { $attributes['layout'] = 'fixed-height'; } diff --git a/includes/sanitizers/class-amp-iframe-sanitizer.php b/includes/sanitizers/class-amp-iframe-sanitizer.php index ed767018310..07c07875f72 100644 --- a/includes/sanitizers/class-amp-iframe-sanitizer.php +++ b/includes/sanitizers/class-amp-iframe-sanitizer.php @@ -80,7 +80,8 @@ public function sanitize() { $this->did_convert_elements = true; $new_attributes = $this->set_layout( $new_attributes ); - if ( isset( $new_attributes['width'] ) && isset( $new_attributes['height'] ) ) { + if ( empty( $new_attributes['layout'] ) && ! empty( $new_attributes['width'] ) && ! empty( $new_attributes['height'] ) ) { + $new_attributes['layout'] = 'intrinsic'; $this->add_or_append_attribute( $new_attributes, 'class', 'amp-wp-enforced-sizes' ); } diff --git a/includes/sanitizers/class-amp-video-sanitizer.php b/includes/sanitizers/class-amp-video-sanitizer.php index fe4ecba8ed3..0512da17754 100644 --- a/includes/sanitizers/class-amp-video-sanitizer.php +++ b/includes/sanitizers/class-amp-video-sanitizer.php @@ -50,6 +50,9 @@ public function sanitize() { $new_attributes = $this->filter_attributes( $old_attributes ); $new_attributes = $this->set_layout( $new_attributes ); + if ( empty( $new_attributes['layout'] ) && ! empty( $new_attributes['width'] ) && ! empty( $new_attributes['height'] ) ) { + $new_attributes['layout'] = 'responsive'; + } $new_node = AMP_DOM_Utils::create_node( $this->dom, 'amp-video', $new_attributes ); diff --git a/tests/test-amp-video-sanitizer.php b/tests/test-amp-video-sanitizer.php index 249520cb27a..5a0f989db05 100644 --- a/tests/test-amp-video-sanitizer.php +++ b/tests/test-amp-video-sanitizer.php @@ -10,7 +10,7 @@ public function get_data() { 'simple_video' => array( '', - '', + '', ), 'video_without_dimensions' => array( @@ -20,32 +20,32 @@ public function get_data() { 'autoplay_attribute' => array( '', - '', + '', ), 'autoplay_attribute__false' => array( '', - '', + '', ), 'video_with_whitelisted_attributes__enabled' => array( '', - '', + '', ), 'video_with_whitelisted_attributes__disabled' => array( '', - '', + '', ), 'video_with_blacklisted_attribute' => array( '', - '', + '', ), 'video_with_sizes_attribute_is_overridden' => array( '', - '', + '', ), 'video_with_children' => array( @@ -53,7 +53,7 @@ public function get_data() { ', - '', + '', ), 'multiple_same_video' => array( @@ -61,19 +61,19 @@ public function get_data() { ', - '', + '', ), 'multiple_different_videos' => array( ' ', - '', + '', ), 'https_not_required' => array( '', - '', + '', ), ); } diff --git a/tests/test-class-amp-base-sanitizer.php b/tests/test-class-amp-base-sanitizer.php index eb7a3fae197..8902bb297cf 100644 --- a/tests/test-class-amp-base-sanitizer.php +++ b/tests/test-class-amp-base-sanitizer.php @@ -28,7 +28,7 @@ public function get_data() { array( 'width' => 100, 'height' => 100, - 'layout' => 'intrinsic', + 'layout' => 'responsive', ), ), @@ -79,7 +79,6 @@ public function get_data() { array( 'width' => 100, 'height' => 100, - 'layout' => 'intrinsic', ), ), );