Persist `img` of `picture` tag instead of removing it during sanitization. #6896

dhaval-parekh · 2022-02-07T07:03:01Z

Summary

These PR changes prevent the img tag inside the picture tag from being removed if native_img_used is false. and will mark the picture tag and its child element as px-verified-tag if native_img_used is true.

Checklist

My code is tested and passes existing tests.
My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

github-actions · 2022-02-07T09:05:33Z

Plugin builds for 0d63423 are ready 🛎️!

Download development build
Download production build

includes/sanitizers/class-amp-img-sanitizer.php

westonruter · 2022-02-16T00:02:01Z

I've asked if this will address the concerns by a support forum user: https://wordpress.org/support/topic/plugin-deleted-all-tags-picture/#post-15369498

That user wasn't able to use @milindmore22's mini plugin successfully, but I believe that is due in large part that they missed the required picture > img element. I tried adding a picture with only source children and this was flagged as an error by Validator.nu:

So we should require that picture > img to be present for the sanitizer to do anything.

dhaval-parekh · 2022-02-18T11:16:10Z

So we should require that picture > img to be present for the sanitizer to do anything.

you mean to only enable sanitization when there picture > img in the body content. But that will also prevent tag to be converted into an AMP component. And I think it should be implemented after #6805

westonruter · 2022-02-18T16:43:25Z

What I mean is that if there is a picture without a child img, then our picture sanitizer wouldn't be able to do anything if we're looking to transform into amp-img.

dhaval-parekh · 2022-02-21T10:32:13Z

What I mean is that if there is a picture without a child img, then our picture sanitizer wouldn't be able to do anything if we're looking to transform into amp-img.

I have added that check in recent commit 39a57e6

…allow-child-img-of-picture * 'develop' of github.com:ampproject/amp-wp: (135 commits) Bump eslint-plugin-react from 7.29.2 to 7.29.3 Add tests for new specs Update tests for new component versions Add support for Bento extensions being defined in bundles.config.bento.json Update amphtml to 2202230359001 and add to latest-extension-versions Fix check_for_page_caching to account for wp_remote_retrieve_header() returning arrays Bump `@wordpress/scripts` to 22.1.0 Revert "Prevent copying PHP files from `src/` into `assets/`" Use links as exported from PHP, split out functions, and make list item matching more robust Bump postcss-preset-env from 7.4.1 to 7.4.2 Update namespace for Attribute and Tag interfaces Remove obsolete debug code Defer creation of post fixture until test Add assertion for post data being created before test Add yet more assertions to debug oEmbed problem on GHA Add oembed_request_post_id filter sooner Add more assertions Add assertions Ensure video shortcode is registered to test_process_text_widgets Use oembed_request_post_id for test ...

westonruter · 2022-03-05T00:11:47Z

includes/sanitizers/class-amp-img-sanitizer.php

+			return;
+		}
+
+		$picture_nodes = $this->dom->getElementsByTagName( Tag::PICTURE );


This shouldn't be needed because there is an XPath query right above.

westonruter · 2022-03-05T00:12:28Z

includes/sanitizers/class-amp-img-sanitizer.php

+	 */
+	protected function process_picture_elements() {
+
+		$nodes = $this->dom->xpath->query( '//picture//img' );


This could be made to select the direct descendant:

Suggested change

$nodes = $this->dom->xpath->query( '//picture//img' );

$nodes = $this->dom->xpath->query( '//picture/img' );

Or better to select picture elements that have img children:

Suggested change

$nodes = $this->dom->xpath->query( '//picture//img' );

$nodes = $this->dom->xpath->query( '//picture[ img ]' );

Or or no, the first is better, as then you can loop over the img elements and then to get the picture you just get the parentNode.

westonruter · 2022-03-05T00:28:45Z

tests/php/test-amp-img-sanitizer.php

+				'expected' => '<h1>Page heading</h1><ul><li>Item 1</li><li>Item 2</li></ul>',
+			],
+			'picture_without_img_allow_picture_false' => [
+				'input'    => '<picture><div class="screen-reader-text"></div><source srcset="https://interactive-examples.mdn.mozilla.net/media/cc0-images/surfer-240-200.jpg" media="(min-width: 800px)"></picture>',


Humm. Why is there a <div class="screen-reader-text"></div> as a child of the picture? I thought only source and img elements were allowed as children.

westonruter · 2022-03-05T00:29:18Z

tests/php/test-amp-img-sanitizer.php

+				'args'     => [
+					'allow_picture' => false,
+				],
+				'expected' => '<picture><div class="screen-reader-text"></div><source srcset="https://interactive-examples.mdn.mozilla.net/media/cc0-images/surfer-240-200.jpg" media="(min-width: 800px)"></picture>',


Since allow_picture is false, why isn't the picture converted? Oh, it's because there is no img?

…ve_img_used

delawski

This looks good to me. I left some minor feedback.

delawski · 2022-03-07T14:35:04Z

includes/sanitizers/class-amp-img-sanitizer.php

+		if ( 0 === $picture_img_query->length ) {
+			return;
+		}


I guess this is redundant. The $this->dom->xpath->query() returns a DOMNodeList. If it's an empty list, the foreach loop below will do 0 iterations.

delawski · 2022-03-07T14:38:35Z

includes/sanitizers/class-amp-img-sanitizer.php

+				$picture_element->removeChild( $img_element );
+				$picture_element->parentNode->replaceChild( $img_element, $picture_element );


I don't have much experience with the DOM API in PHP but is it necessary to remove the IMG tag first from the PICTURE and then replace the entire PICTURE with an IMG? Wouldn't just the replaceChild() suffice?

I recall this has been necessary since replacing a parent with a child while the child is still in the DOM could cause an error, if I remember correctly.

…tion (#6896) Co-authored-by: Dhaval Parekh <[email protected]>

dhaval-parekh added 3 commits February 7, 2022 12:16

Keep img tag from picture instead of removing it while sanitizing

1b52d70

Add unit test cases

327d2ba

Fix phpstan issue

50a45b7

dhaval-parekh requested review from milindmore22 and delawski February 7, 2022 08:58

dhaval-parekh marked this pull request as ready for review February 7, 2022 09:00

dhaval-parekh self-assigned this Feb 7, 2022

westonruter requested changes Feb 7, 2022

View reviewed changes

includes/sanitizers/class-amp-img-sanitizer.php Outdated Show resolved Hide resolved

includes/sanitizers/class-amp-img-sanitizer.php Outdated Show resolved Hide resolved

includes/sanitizers/class-amp-img-sanitizer.php Outdated Show resolved Hide resolved

maitreyie-chavan added this to the v2.2.2 milestone Feb 9, 2022

Improve picture tag to img conversion.

8c14fa9

dhaval-parekh requested a review from westonruter February 11, 2022 11:12

Add check for existence of picture > img in body content

39a57e6

westonruter reviewed Mar 5, 2022

View reviewed changes

westonruter added 2 commits March 4, 2022 16:19

Restore PHP import of ValidationExemption after merging #6803

8e7b44a

Improve DOM querying

7e8fb15

westonruter reviewed Mar 5, 2022

View reviewed changes

westonruter added 6 commits March 4, 2022 16:31

Add indentation to improve test data readability

c612ab7

Remove div.screen-reader-text child of picture in tests

306056d

Add missing img child to test allow_picture arg

4386784

Add support for width/height attributes on picture/source elements

7627f98

Add width/height attributes to picture img test data

a8a27e2

Discontinue marking picture>img as PX-verified and process as if mati…

1bfc3a5

…ve_img_used

Add tests for when there is a picture with no img child

cb39fa5

westonruter approved these changes Mar 5, 2022

View reviewed changes

delawski approved these changes Mar 7, 2022

View reviewed changes

Improve test coverage

0d63423

westonruter merged commit f7c7230 into develop Mar 7, 2022

westonruter deleted the bug/6676-allow-child-img-of-picture branch March 7, 2022 18:59

westonruter added a commit that referenced this pull request Mar 7, 2022

Persist img of picture tag instead of removing it during sanitiza…

620933e

…tion (#6896) Co-authored-by: Dhaval Parekh <[email protected]>

westonruter added the Changelogged Whether the issue/PR has been added to release notes. label Apr 4, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Persist `img` of `picture` tag instead of removing it during sanitization. #6896

Persist `img` of `picture` tag instead of removing it during sanitization. #6896

dhaval-parekh commented Feb 7, 2022

github-actions bot commented Feb 7, 2022 •

edited

Loading

westonruter commented Feb 16, 2022

dhaval-parekh commented Feb 18, 2022

westonruter commented Feb 18, 2022

dhaval-parekh commented Feb 21, 2022

westonruter Mar 5, 2022

westonruter Mar 5, 2022

westonruter Mar 5, 2022

westonruter Mar 5, 2022

westonruter Mar 5, 2022

westonruter Mar 5, 2022

delawski left a comment

delawski Mar 7, 2022

delawski Mar 7, 2022

westonruter Mar 7, 2022

	$nodes = $this->dom->xpath->query( '//picture//img' );
	$nodes = $this->dom->xpath->query( '//picture/img' );

		$picture_element->removeChild( $img_element );
		$picture_element->parentNode->replaceChild( $img_element, $picture_element );

Persist img of picture tag instead of removing it during sanitization. #6896

Persist img of picture tag instead of removing it during sanitization. #6896

Conversation

dhaval-parekh commented Feb 7, 2022

Summary

Checklist

github-actions bot commented Feb 7, 2022 • edited Loading

westonruter commented Feb 16, 2022

dhaval-parekh commented Feb 18, 2022

westonruter commented Feb 18, 2022

dhaval-parekh commented Feb 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

delawski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Persist `img` of `picture` tag instead of removing it during sanitization. #6896

Persist `img` of `picture` tag instead of removing it during sanitization. #6896

github-actions bot commented Feb 7, 2022 •

edited

Loading