From c4dd0ec94564e4640bd546fdb866450f1e81ec6c Mon Sep 17 00:00:00 2001 From: Ryan Kienstra Date: Mon, 7 Oct 2019 18:47:32 -0500 Subject: [PATCH 1/9] Allow images that are wrapped in to have a lightbox There was an issue where images that are wrapped in didn't support the lightbox feature, as clicking on them simply caused the browser to go to that link. So this recognizes when an image is wrapped in , and it strips that. --- .../sanitizers/class-amp-img-sanitizer.php | 18 ++++++++++++++---- tests/php/test-amp-img-sanitizer.php | 5 +++++ 2 files changed, 19 insertions(+), 4 deletions(-) diff --git a/includes/sanitizers/class-amp-img-sanitizer.php b/includes/sanitizers/class-amp-img-sanitizer.php index 284c1db04e7..9dec5c45d15 100644 --- a/includes/sanitizers/class-amp-img-sanitizer.php +++ b/includes/sanitizers/class-amp-img-sanitizer.php @@ -368,12 +368,17 @@ private function adjust_and_replace_node( $node ) { */ private function maybe_add_lightbox_attributes( $attributes, $node ) { $parent_node = $node->parentNode; - if ( ! ( $parent_node instanceof DOMElement ) || 'figure' !== $parent_node->tagName ) { + if ( ! ( $parent_node instanceof DOMElement ) ) { return $attributes; } - // Account for blocks that include alignment. - // In that case, the structure changes from figure.wp-block-image > img + $is_node_wrapped_in_anchor = 'a' === $parent_node->tagName && $parent_node->parentNode instanceof DOMElement && 'figure' === $parent_node->parentNode->tagName; + if ( 'figure' !== $parent_node->tagName && ! $is_node_wrapped_in_anchor ) { + return $attributes; + } + + // Account for blocks that include alignment or images that are wrapped in . + // With alignment, the structure changes from figure.wp-block-image > img // to div.wp-block-image > figure > img and the amp-lightbox attribute // can be found on the wrapping div instead of the figure element. $grand_parent = $parent_node->parentNode; @@ -393,13 +398,18 @@ private function maybe_add_lightbox_attributes( $attributes, $node ) { $attributes['tabindex'] = 0; $this->maybe_add_amp_image_lightbox_node(); + + if ( $is_node_wrapped_in_anchor ) { + // Remove the that the image is wrapped in, as it can prevent the lightbox from working. + $node->parentNode->parentNode->replaceChild( $node, $node->parentNode ); + } } return $attributes; } /** - * Determines is a URL is considered a GIF URL + * Determines if a URL is considered a GIF URL * * @since 0.2 * diff --git a/tests/php/test-amp-img-sanitizer.php b/tests/php/test-amp-img-sanitizer.php index 73b4fee4b00..5f7a5066b15 100644 --- a/tests/php/test-amp-img-sanitizer.php +++ b/tests/php/test-amp-img-sanitizer.php @@ -339,6 +339,11 @@ public function get_data() { '
', ], + 'image_block_wrapped_in_a_with_lightbox' => [ + '
', + '
', + ], + 'aligned_image_block_with_lightbox' => [ '
', '
', From 237cde9cfe8b56caa7a8059ebbeb2fb463392965 Mon Sep 17 00:00:00 2001 From: Ryan Kienstra Date: Tue, 15 Oct 2019 17:00:21 -0500 Subject: [PATCH 2/9] Use amp-lightbox-gallery for Core Image block It sounds like this is the recommended component for lightboxes, not the previous amp-image-ligthbox. --- includes/sanitizers/class-amp-img-sanitizer.php | 6 +----- tests/php/test-amp-img-sanitizer.php | 8 ++++---- 2 files changed, 5 insertions(+), 9 deletions(-) diff --git a/includes/sanitizers/class-amp-img-sanitizer.php b/includes/sanitizers/class-amp-img-sanitizer.php index 9dec5c45d15..d3a873546b3 100644 --- a/includes/sanitizers/class-amp-img-sanitizer.php +++ b/includes/sanitizers/class-amp-img-sanitizer.php @@ -393,11 +393,7 @@ private function maybe_add_lightbox_attributes( $attributes, $node ) { if ( isset( $parent_attributes['data-amp-lightbox'] ) && true === filter_var( $parent_attributes['data-amp-lightbox'], FILTER_VALIDATE_BOOLEAN ) ) { $attributes['data-amp-lightbox'] = ''; - $attributes['on'] = 'tap:' . self::AMP_IMAGE_LIGHTBOX_ID; - $attributes['role'] = 'button'; - $attributes['tabindex'] = 0; - - $this->maybe_add_amp_image_lightbox_node(); + $attributes['lightbox'] = ''; if ( $is_node_wrapped_in_anchor ) { // Remove the that the image is wrapped in, as it can prevent the lightbox from working. diff --git a/tests/php/test-amp-img-sanitizer.php b/tests/php/test-amp-img-sanitizer.php index 5f7a5066b15..5de995ab749 100644 --- a/tests/php/test-amp-img-sanitizer.php +++ b/tests/php/test-amp-img-sanitizer.php @@ -216,7 +216,7 @@ public function get_data() { 'image_with_custom_lightbox_attrs' => [ '
', - '
', + '
', ], 'wide_image' => [ @@ -336,17 +336,17 @@ public function get_data() { 'image_block_with_lightbox' => [ '
', - '
', + '
', ], 'image_block_wrapped_in_a_with_lightbox' => [ '
', - '
', + '
', ], 'aligned_image_block_with_lightbox' => [ '
', - '
', + '
', ], 'test_with_dev_mode' => [ From 9ee7d2e42556a43a024dd706b2d80e0a8c694256 Mon Sep 17 00:00:00 2001 From: Ryan Kienstra Date: Tue, 15 Oct 2019 18:09:34 -0500 Subject: [PATCH 3/9] Add a comment that code can be removed if an issue is implemented If the amphtml feature is added, the code can probably be removed. --- includes/sanitizers/class-amp-img-sanitizer.php | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/includes/sanitizers/class-amp-img-sanitizer.php b/includes/sanitizers/class-amp-img-sanitizer.php index d3a873546b3..6e2afed5648 100644 --- a/includes/sanitizers/class-amp-img-sanitizer.php +++ b/includes/sanitizers/class-amp-img-sanitizer.php @@ -395,8 +395,11 @@ private function maybe_add_lightbox_attributes( $attributes, $node ) { $attributes['data-amp-lightbox'] = ''; $attributes['lightbox'] = ''; + /* + * Remove the if the image is wrapped in one, as it can prevent the lightbox from working. + * If this feature is added, this can probably be removed: https://github.com/ampproject/amphtml/issues/25021 + */ if ( $is_node_wrapped_in_anchor ) { - // Remove the that the image is wrapped in, as it can prevent the lightbox from working. $node->parentNode->parentNode->replaceChild( $node, $node->parentNode ); } } From 9c7fd7b87116b2c6a51740479e9a706e85fbe996 Mon Sep 17 00:00:00 2001 From: Ryan Kienstra Date: Wed, 16 Oct 2019 21:59:25 -0500 Subject: [PATCH 4/9] Only remove the if it goes to the media file, not the attachment page As Weston mentioned, the lightbox is a better experience than simply going to the URL of the media file. But if it's for the attachment page, don't add the lightbox attribute, and don't strip the . --- .../sanitizers/class-amp-img-sanitizer.php | 19 ++++++++--- tests/php/test-amp-img-sanitizer.php | 33 +++++++++++++++++-- 2 files changed, 45 insertions(+), 7 deletions(-) diff --git a/includes/sanitizers/class-amp-img-sanitizer.php b/includes/sanitizers/class-amp-img-sanitizer.php index 6e2afed5648..246ad8801aa 100644 --- a/includes/sanitizers/class-amp-img-sanitizer.php +++ b/includes/sanitizers/class-amp-img-sanitizer.php @@ -372,8 +372,18 @@ private function maybe_add_lightbox_attributes( $attributes, $node ) { return $attributes; } - $is_node_wrapped_in_anchor = 'a' === $parent_node->tagName && $parent_node->parentNode instanceof DOMElement && 'figure' === $parent_node->parentNode->tagName; - if ( 'figure' !== $parent_node->tagName && ! $is_node_wrapped_in_anchor ) { + // This should be true for links to the actual media file, but not for links to the attachment page. + $is_node_wrapped_in_media_file_link = ( + 'a' === $parent_node->tagName + && + $parent_node->parentNode instanceof DOMElement + && + 'figure' === $parent_node->parentNode->tagName + && + attachment_url_to_postid( $parent_node->getAttribute( 'href' ) ) + ); + + if ( 'figure' !== $parent_node->tagName && ! $is_node_wrapped_in_media_file_link ) { return $attributes; } @@ -396,10 +406,11 @@ private function maybe_add_lightbox_attributes( $attributes, $node ) { $attributes['lightbox'] = ''; /* - * Remove the if the image is wrapped in one, as it can prevent the lightbox from working. + * Removes the if the image is wrapped in one, as it can prevent the lightbox from working. + * But this only removes the if it links to the media file, not the attachment page. * If this feature is added, this can probably be removed: https://github.com/ampproject/amphtml/issues/25021 */ - if ( $is_node_wrapped_in_anchor ) { + if ( $is_node_wrapped_in_media_file_link ) { $node->parentNode->parentNode->replaceChild( $node, $node->parentNode ); } } diff --git a/tests/php/test-amp-img-sanitizer.php b/tests/php/test-amp-img-sanitizer.php index 5de995ab749..7ea16176eb5 100644 --- a/tests/php/test-amp-img-sanitizer.php +++ b/tests/php/test-amp-img-sanitizer.php @@ -339,9 +339,9 @@ public function get_data() { '
', ], - 'image_block_wrapped_in_a_with_lightbox' => [ - '
', - '
', + 'image_block_link_attach_page_lightbox' => [ + '
', + '
', ], 'aligned_image_block_with_lightbox' => [ @@ -443,4 +443,31 @@ public function test_no_gif_image_scripts() { ); $this->assertEquals( $expected, $scripts ); } + + /** + * Test an Image block wrapped in an , that links to the media file, with 'lightbox' selected. + * + * This should have the stripped, as it interferes with the lightbox. + */ + public function test_image_block_link_to_media_file_with_lightbox() { + $attachment_id = $this->factory()->attachment->create_object( + 'example-image.jpeg', + $this->factory()->post->create(), + [ + 'post_mime_type' => 'image/jpeg', + 'post_type' => 'attachment', + ] + ); + $source = sprintf( '
', wp_get_attachment_image_url( $attachment_id ) ); + $expected = '
'; + + $dom = AMP_DOM_Utils::get_dom_from_content( $source ); + $sanitizer = new AMP_Img_Sanitizer( $dom ); + $sanitizer->sanitize(); + + $sanitizer = new AMP_Tag_And_Attribute_Sanitizer( $dom ); + $sanitizer->sanitize(); + $content = AMP_DOM_Utils::get_content_from_dom( $dom ); + $this->assertEquals( $expected, $content ); + } } From 9a08c8ac55dc901ee3c96833acfb6efd1d8d1b62 Mon Sep 17 00:00:00 2001 From: Ryan Kienstra Date: Thu, 17 Oct 2019 17:39:45 -0500 Subject: [PATCH 5/9] Allow lightboxes in Image blocks with links and alignment Before, there was handling for links or alignment, but not both. So add a way to get the great-grandparent of the . --- .../sanitizers/class-amp-img-sanitizer.php | 32 +++++-- tests/php/test-amp-img-sanitizer.php | 94 +++++++++++++++++-- 2 files changed, 107 insertions(+), 19 deletions(-) diff --git a/includes/sanitizers/class-amp-img-sanitizer.php b/includes/sanitizers/class-amp-img-sanitizer.php index 246ad8801aa..4b16c3a6558 100644 --- a/includes/sanitizers/class-amp-img-sanitizer.php +++ b/includes/sanitizers/class-amp-img-sanitizer.php @@ -368,7 +368,7 @@ private function adjust_and_replace_node( $node ) { */ private function maybe_add_lightbox_attributes( $attributes, $node ) { $parent_node = $node->parentNode; - if ( ! ( $parent_node instanceof DOMElement ) ) { + if ( ! ( $parent_node instanceof DOMElement ) || ! ( $parent_node->parentNode instanceof DOMElement ) ) { return $attributes; } @@ -376,9 +376,7 @@ private function maybe_add_lightbox_attributes( $attributes, $node ) { $is_node_wrapped_in_media_file_link = ( 'a' === $parent_node->tagName && - $parent_node->parentNode instanceof DOMElement - && - 'figure' === $parent_node->parentNode->tagName + ( 'figure' === $parent_node->tagName || 'figure' === $parent_node->parentNode->tagName ) && attachment_url_to_postid( $parent_node->getAttribute( 'href' ) ) ); @@ -392,11 +390,10 @@ private function maybe_add_lightbox_attributes( $attributes, $node ) { // to div.wp-block-image > figure > img and the amp-lightbox attribute // can be found on the wrapping div instead of the figure element. $grand_parent = $parent_node->parentNode; - if ( $grand_parent instanceof DOMElement ) { - $classes = preg_split( '/\s+/', $grand_parent->getAttribute( 'class' ) ); - if ( in_array( 'wp-block-image', $classes, true ) ) { - $parent_node = $grand_parent; - } + if ( $this->does_node_have_block_class( $grand_parent ) ) { + $parent_node = $grand_parent; + } elseif ( isset( $grand_parent->parentNode ) && $this->does_node_have_block_class( $grand_parent->parentNode ) ) { + $parent_node = $grand_parent->parentNode; } $parent_attributes = AMP_DOM_Utils::get_node_attributes_as_assoc_array( $parent_node ); @@ -418,6 +415,23 @@ private function maybe_add_lightbox_attributes( $attributes, $node ) { return $attributes; } + /** + * Gets whether a node has the class 'wp-block-image', meaning it is a wrapper for an Image block. + * + * @param DOMElement $node A node to evaluate. + * @return bool Whether the node has the class 'wp-block-image'. + */ + private function does_node_have_block_class( $node ) { + if ( $node instanceof DOMElement ) { + $classes = preg_split( '/\s+/', $node->getAttribute( 'class' ) ); + if ( in_array( 'wp-block-image', $classes, true ) ) { + return true; + } + } + + return false; + } + /** * Determines if a URL is considered a GIF URL * diff --git a/tests/php/test-amp-img-sanitizer.php b/tests/php/test-amp-img-sanitizer.php index 7ea16176eb5..fe627adca0f 100644 --- a/tests/php/test-amp-img-sanitizer.php +++ b/tests/php/test-amp-img-sanitizer.php @@ -450,16 +450,8 @@ public function test_no_gif_image_scripts() { * This should have the stripped, as it interferes with the lightbox. */ public function test_image_block_link_to_media_file_with_lightbox() { - $attachment_id = $this->factory()->attachment->create_object( - 'example-image.jpeg', - $this->factory()->post->create(), - [ - 'post_mime_type' => 'image/jpeg', - 'post_type' => 'attachment', - ] - ); - $source = sprintf( '
', wp_get_attachment_image_url( $attachment_id ) ); - $expected = '
'; + $source = sprintf( '
', wp_get_attachment_image_url( $this->get_new_attachment_id() ) ); + $expected = '
'; $dom = AMP_DOM_Utils::get_dom_from_content( $source ); $sanitizer = new AMP_Img_Sanitizer( $dom ); @@ -470,4 +462,86 @@ public function test_image_block_link_to_media_file_with_lightbox() { $content = AMP_DOM_Utils::get_content_from_dom( $dom ); $this->assertEquals( $expected, $content ); } + + /** + * Test an Image block wrapped in an , that has right-alignment, links to the media file, and has 'lightbox' selected. + * + * This should have the stripped, as it interferes with the lightbox. + */ + public function test_image_block_link_to_media_file_and_alignment_with_lightbox() { + $source = sprintf( '
', wp_get_attachment_image_url( $this->get_new_attachment_id() ) ); + $expected = '
'; + + $dom = AMP_DOM_Utils::get_dom_from_content( $source ); + $sanitizer = new AMP_Img_Sanitizer( $dom ); + $sanitizer->sanitize(); + + $sanitizer = new AMP_Tag_And_Attribute_Sanitizer( $dom ); + $sanitizer->sanitize(); + $content = AMP_DOM_Utils::get_content_from_dom( $dom ); + $this->assertEquals( $expected, $content ); + } + + /** + * Gets test data for test_does_node_have_block_class(), using a
element. + * + * @see AMP_Img_Sanitizer_Test::test_does_node_have_block_class() + * @return array Test data for function. + */ + public function get_data_for_node_block_class_test() { + return [ + 'has_no_class' => [ + '
', + false, + ], + 'has_wrong_class' => [ + '
', + false, + ], + 'only_has_part_of_class' => [ + '
', + false, + ], + 'has_correct_class' => [ + '
', + true, + ], + ]; + } + + /** + * Test does_node_have_block_class. + * + * @dataProvider get_data_for_node_block_class_test + * @covers \AMP_Img_Sanitizer::does_node_have_block_class() + * + * @param string $source The source markup to test. + * @param string $expected The expected return of the tested function, using the source markup. + * @throws ReflectionException If invoking the private method fails. + */ + public function test_does_node_have_block_class( $source, $expected ) { + $dom = AMP_DOM_Utils::get_dom_from_content( $source ); + $sanitizer = new AMP_Img_Sanitizer( $dom ); + $figures = $dom->getElementsByTagName( 'figure' ); + $method = new ReflectionMethod( $sanitizer, 'does_node_have_block_class' ); + + $method->setAccessible( true ); + $this->assertEquals( $expected, $method->invoke( $figures[0] ) ); + } + + /** + * Creates a new image attachment, and gets the ID. + * + * @return int|WP_Error The new attachment ID, or WP_Error. + */ + public function get_new_attachment_id() { + return $this->factory()->attachment->create_object( + 'example-image.jpeg', + $this->factory()->post->create(), + [ + 'post_mime_type' => 'image/jpeg', + 'post_type' => 'attachment', + ] + ); + } } From 88b8efcf817bcc8fff31a7a3162c83f2949fb07e Mon Sep 17 00:00:00 2001 From: Ryan Kienstra Date: Thu, 17 Oct 2019 17:53:47 -0500 Subject: [PATCH 6/9] Add a parameter to the invoke() method in a test This doesn't pass without passing the object to it. --- tests/php/test-amp-img-sanitizer.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/php/test-amp-img-sanitizer.php b/tests/php/test-amp-img-sanitizer.php index fe627adca0f..86856fa20c4 100644 --- a/tests/php/test-amp-img-sanitizer.php +++ b/tests/php/test-amp-img-sanitizer.php @@ -526,7 +526,7 @@ public function test_does_node_have_block_class( $source, $expected ) { $method = new ReflectionMethod( $sanitizer, 'does_node_have_block_class' ); $method->setAccessible( true ); - $this->assertEquals( $expected, $method->invoke( $figures[0] ) ); + $this->assertEquals( $expected, $method->invoke( $sanitizer, $figures[0] ) ); } /** From bd1906ba541d16b44775cd076dbbe2b8a52956db Mon Sep 17 00:00:00 2001 From: Ryan Kienstra Date: Thu, 17 Oct 2019 18:00:35 -0500 Subject: [PATCH 7/9] Remove the link to the amphtml issue, as it's closed It looks like it won't be implemented for backwards-compatibility reasons. --- 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 4b16c3a6558..50c2e01ffe3 100644 --- a/includes/sanitizers/class-amp-img-sanitizer.php +++ b/includes/sanitizers/class-amp-img-sanitizer.php @@ -405,7 +405,6 @@ private function maybe_add_lightbox_attributes( $attributes, $node ) { /* * Removes the if the image is wrapped in one, as it can prevent the lightbox from working. * But this only removes the if it links to the media file, not the attachment page. - * If this feature is added, this can probably be removed: https://github.com/ampproject/amphtml/issues/25021 */ if ( $is_node_wrapped_in_media_file_link ) { $node->parentNode->parentNode->replaceChild( $node, $node->parentNode ); From d2433264ce297e21c19f56439d5a07be477b9ecc Mon Sep 17 00:00:00 2001 From: Ryan Kienstra Date: Thu, 17 Oct 2019 18:39:49 -0500 Subject: [PATCH 8/9] Access a DOMNodeList item with ->item() instead of [0] This caused an error in PHP 5.4 and 5.5. --- tests/php/test-amp-img-sanitizer.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/php/test-amp-img-sanitizer.php b/tests/php/test-amp-img-sanitizer.php index 86856fa20c4..627d51d8405 100644 --- a/tests/php/test-amp-img-sanitizer.php +++ b/tests/php/test-amp-img-sanitizer.php @@ -526,7 +526,7 @@ public function test_does_node_have_block_class( $source, $expected ) { $method = new ReflectionMethod( $sanitizer, 'does_node_have_block_class' ); $method->setAccessible( true ); - $this->assertEquals( $expected, $method->invoke( $sanitizer, $figures[0] ) ); + $this->assertEquals( $expected, $method->invoke( $sanitizer, $figures->item( 0 ) ) ); } /** From 783a8d8e239657f3f2eb265fe6a43b9313a28c84 Mon Sep 17 00:00:00 2001 From: Ryan Kienstra Date: Fri, 18 Oct 2019 17:35:28 -0500 Subject: [PATCH 9/9] Use Weston's method of finding if the link is to a file Use the line that Weston suggested, instead of attachment_url_to_postid(). --- includes/sanitizers/class-amp-img-sanitizer.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/includes/sanitizers/class-amp-img-sanitizer.php b/includes/sanitizers/class-amp-img-sanitizer.php index 50c2e01ffe3..9530665a0f2 100644 --- a/includes/sanitizers/class-amp-img-sanitizer.php +++ b/includes/sanitizers/class-amp-img-sanitizer.php @@ -372,13 +372,13 @@ private function maybe_add_lightbox_attributes( $attributes, $node ) { return $attributes; } - // This should be true for links to the actual media file, but not for links to the attachment page. + $is_file_url = preg_match( '/\.\w+$/', wp_parse_url( $parent_node->getAttribute( 'href' ), PHP_URL_PATH ) ); $is_node_wrapped_in_media_file_link = ( 'a' === $parent_node->tagName && ( 'figure' === $parent_node->tagName || 'figure' === $parent_node->parentNode->tagName ) && - attachment_url_to_postid( $parent_node->getAttribute( 'href' ) ) + $is_file_url // This should be a link to the media file, not the attachment page. ); if ( 'figure' !== $parent_node->tagName && ! $is_node_wrapped_in_media_file_link ) {