Fix validation error due to native img tag when lightbox and carousel is enabled #7158

thelovekesh · 2022-06-17T17:35:57Z

Summary

This PR aims to fix validation errors by using the native img tag.

Errors:

Lightbox attribute is not identified in <img> tag by amp validator yet.
layout and object-fir attributes are not identified in <img> tag by amp validator yet.

When adding lightbox and carousel with native img used:

Before:

After:

Fixes #7152

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).

…enabled

…attr for native img

github-actions · 2022-06-17T17:42:15Z

Plugin builds for 3e558de are ready 🛎️!

Download development build
Download production build

westonruter · 2022-06-17T19:37:10Z

includes/sanitizers/class-amp-gallery-block-sanitizer.php

+		$img_elements = $this->dom->xpath->query(
+			'.//img[ @layout = "fill" or @object-fit = "cover" ]',
+			$this->dom->body
+		);
+
+		/*
+		* Remove `layout` and `object-fit` attributes from native img as they are not supported by AMP yet.
+		* Add style attribute which is used to set `object-fit` and `layout` attributes.
+		* @see <https://github.com/ampproject/amp-wp/issues/7152#issuecomment-1157933188>
+		* @todo Remove this once `layout` and `object-fit` attributes are supported by AMP spec for native img tag.
+		*/
+		$style_layout_fill = 'position:absolute; left:0; right:0; top:0; bottom: 0; width:100%; height:100%;';
+		$style_object_fit  = 'object-fit:cover;';
+		foreach ( $img_elements as $img_element ) {
+			$remove_layout_attr     = $img_element->removeAttribute( Attribute::LAYOUT );
+			$remove_object_fit_attr = $img_element->removeAttribute( Attribute::OBJECT_FIT );
+			$style_attr_content     = sprintf( '%s %s', $remove_layout_attr ? $style_layout_fill : '', $remove_object_fit_attr ? $style_object_fit : '' );
+
+			if ( ' ' !== $style_attr_content ) {
+				$img_element->setAttribute( Attribute::STYLE, $style_attr_content );
+			}
+		}


I think this logic may make more sense in the image sanitizer, specifically in the adjust_and_replace_node method:

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

Lines 402 to 422 in bbf48d7

private function adjust_and_replace_node( Element $node ) {

if ( $this->args['native_img_used'] || ( $node->parentNode instanceof Element && Tag::PICTURE === $node->parentNode->tagName ) ) {

$attributes = $this->maybe_add_lightbox_attributes( [], $node ); // @todo AMP doesn't support lightbox on <img> yet.

// Set decoding=async by default. See <https://core.trac.wordpress.org/ticket/53232>.

if ( ! $node->hasAttribute( Attribute::DECODING ) ) {

$attributes[ Attribute::DECODING ] = 'async';

}

// @todo This class should really only be added if we actually have to provide dimensions.

$attributes[ Attribute::CLASS_ ] = (string) $node->getAttribute( Attribute::CLASS_ );

if ( ! empty( $attributes[ Attribute::CLASS_ ] ) ) {

$attributes[ Attribute::CLASS_ ] .= ' ';

}

$attributes[ Attribute::CLASS_ ] .= 'amp-wp-enforced-sizes';

foreach ( $attributes as $name => $value ) {

$node->setAttribute( $name, $value );

}

return;

}

Where you're currently adding the lightbox handling.

Where you're currently adding the lightbox handling.

Handling it in the image sanitizer because need to check if an image has lightbox enabled to not.

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

Lines 405 to 418 in 60e2f46

/*

* Mark lightbox as px-verified attribute until it's supported by AMP spec.

* @see <https://github.com/ampproject/amp-wp/issues/7152#issuecomment-1157933188>

* @todo Remove this once lightbox is added in `lightboxable-elements` for native img tag in AMP spec.

*/

if ( isset( $attributes['data-amp-lightbox'] ) ) {

$node->setAttribute( Attribute::LIGHTBOX, 'true' );

$node_attr = $node->getAttributeNode( Attribute::LIGHTBOX );

if ( $node_attr instanceof DOMAttr ) {

ValidationExemption::mark_node_as_px_verified( $node_attr );

}

}

I mean, I think the logic here would all make more sense in the image sanitizer.

Oh, but the problem is that AMP_Gallery_Block_Sanitizer runs after AMP_Img_Sanitizer.

Are you still planning to try to move this code to the img sanitizer by changing the order that the sanitizers run in? That is, if the gallery block sanitizer runs before the image sanitizer then the logic should be able to be consolidated in the img sanitizer.

Are you still planning to try to move this code

I have tried it locally but changing the order of sanitizers causes markup error as the gallery block expects sanitized images.

One more idea that came up to my mind is to create a mini native IMG attributes sanitizer which can be called after gallery block. Doing so can help to sanitize more attributes if required in near future.

I do like your idea of having a new mini native img sanitizer that runs after the gallery sanitizer to specifically handle the lightbox, layout, and object-fit attributes. It would be better yet to have it run in the img sanitizer, but consolidating them into one special sanitizer is better than having them spread out across two sanitizers as they are now.

So, should I create a mini native img attributes sanitizer?

Yes, please create an AMP_Native_Img_Attributes_Sanitizer.

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

Co-authored-by: Weston Ruter <westonruter@google.com>

…anitization

…box-with-native-img * 'develop' of github.com:ampproject/amp-wp: Bump moment from 2.29.3 to 2.29.4 Bump eslint from 8.18.0 to 8.19.0 Bump dependabot/fetch-metadata from 1.3.2 to 1.3.3 Bump dependabot/fetch-metadata from 1.3.1 to 1.3.2 Bump @babel/plugin-proposal-class-properties from 7.17.12 to 7.18.6 Bump @babel/core from 7.18.5 to 7.18.6 Bump eslint-plugin-react from 7.30.0 to 7.30.1 Bump lint-staged from 13.0.2 to 13.0.3 Bump postcss-preset-env from 7.7.1 to 7.7.2 Bump shell-quote from 1.7.2 to 1.7.3 Bump guzzlehttp/guzzle from 6.5.7 to 6.5.8 Bump eslint-plugin-jsdoc from 39.3.2 to 39.3.3 Bump eslint from 8.17.0 to 8.18.0

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

…object-fit

westonruter · 2022-07-07T19:37:44Z

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

+		// Images with object-fit attributes.
+		$img_elements = $this->dom->xpath->query(
+			'.//img[ @object-fit ]',
+			$this->dom->body
+		);
+		if ( $img_elements instanceof DOMNodeList ) {
+			foreach ( $img_elements as $img_element ) {
+				/** @var Element $img_element */
+				$value = $img_element->getAttribute( Attribute::OBJECT_FIT );
+				$img_element->removeAttribute( Attribute::OBJECT_FIT );
+				$img_element->addInlineStyle( sprintf( 'object-fit:%s', $value ) );
+			}
+		}
+
+		// Images with object-position attributes.
+		$img_elements = $this->dom->xpath->query(
+			'.//img[ @object-position ]',
+			$this->dom->body
+		);
+		if ( $img_elements instanceof DOMNodeList ) {
+			foreach ( $img_elements as $img_element ) {
+				/** @var Element $img_element */
+				$value = $img_element->getAttribute( Attribute::OBJECT_POSITION );
+				$img_element->removeAttribute( Attribute::OBJECT_POSITION );
+				$img_element->addInlineStyle( sprintf( 'object-position:%s', $value ) );
+			}
+		}


It turns out that amp-video also supports object-fit and object-position attributes, so it could be that there could end up being some video elements that have these attributes. In this case, the sanitizer could be made more generic instead of being specifically for images. But since we don't have any known cases of this happening for video, this could be done later. Or it could be done now if you want.

Can't think of any such use case. IMO we are amending changes to img tags only so can't we keep it as it is?

I mean, it could be that a theme/plugin author is adding: <video object-fit=cover …> and is then relying on the fact that this is converged to <amp-video object-fit=cover>. But in moderate sandboxing, no conversion is being done, so this would then be a validation error and the attribute should be moved to style as we're doing for img.

Oh. got it now. But I think a new PR should introduce it instead of adding it in the same. Also as mentioned But since we don't have any known cases of this happening for video, this could be done later. IMO adding it with some use case make more sense. Thoughts?

If a user reports this issue we can follow up with the change.

thelovekesh added 8 commits June 17, 2022 21:18

Add lightbox to px-verified-attrs in native img tag when lightbox is …

0afb499

…enabled

Remove layout and object-fit attrs for native img tag and add style attr

7d64732

Add test cases for native img tag px verified lightbox attrs

31cb9a6

Update native_img_used variable with provided default args

90c0fc8

Add test cases for replacing layout and object-fill attrs with style …

22853be

…attr for native img

Add check to find key in args to avoid error in tests

a1d80b0

Fix PHPStan errors

ec01348

Update test case function name

30f7bcd

thelovekesh requested a review from westonruter June 17, 2022 17:36

thelovekesh self-assigned this Jun 17, 2022

Update mentioned method in @Covers

762c309

thelovekesh force-pushed the fix/lightbox-with-native-img branch from 3837bfb to 762c309 Compare June 17, 2022 17:58

westonruter requested changes Jun 17, 2022

View reviewed changes

thelovekesh and others added 3 commits June 18, 2022 01:17

Fix typo

60e2f46

Co-authored-by: Weston Ruter <westonruter@google.com>

Update condition to check if we need to px verify lightbox attr

0755735

Co-authored-by: Weston Ruter <westonruter@google.com>

Update logic of creating node attribute

e4e5396

thelovekesh force-pushed the fix/lightbox-with-native-img branch from 1accc15 to e4e5396 Compare June 17, 2022 20:54

thelovekesh added 2 commits June 20, 2022 11:03

Fix test cases for TikTok embed handler

f0184f1

Fix test cases for YouTube embed handler

d4705ee

thelovekesh force-pushed the fix/lightbox-with-native-img branch from 6a8c84e to d4705ee Compare June 20, 2022 05:35

thelovekesh mentioned this pull request Jun 29, 2022

Eliminate "AMP Settings" panel from Gallery block and move them to AMP advanced settings #7171

Merged

2 tasks

westonruter added this to the v2.3.1 milestone Jun 29, 2022

thelovekesh added 6 commits July 4, 2022 13:39

Remove unused namespace aliases

7c3f6d6

Remove native img attributes sanitization

20e82cb

Add AMP_Native_Img_Attributes_Sanitizer for native image attributes s…

5374854

…anitization

Add AMP_Native_Img_Attributes_Sanitizer in sanitizers

0dc9a1f

Remove test cases for native img attributes sanitization

079f5d8

Add test cases for AMP_Native_Img_Attributes_Sanitizer

ecf255f

thelovekesh requested a review from westonruter July 4, 2022 16:24

westonruter reviewed Jul 7, 2022

View reviewed changes

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

westonruter reviewed Jul 7, 2022

View reviewed changes

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

westonruter added 2 commits July 7, 2022 12:19

Preserve original styles; support object-position; support non-cover …

ee80e10

…object-fit

Add composer-normalize to allowed-plugins before require

3e558de

westonruter reviewed Jul 7, 2022

View reviewed changes

westonruter approved these changes Jul 7, 2022

View reviewed changes

westonruter merged commit 0bf2bbb into develop Jul 7, 2022

westonruter deleted the fix/lightbox-with-native-img branch July 7, 2022 19:58

westonruter unassigned thelovekesh Jul 7, 2022

westonruter mentioned this pull request Jul 25, 2022

Plugin removes lighbox attribute when using native img instead of amp-img #7193

Closed

westonruter modified the milestones: v2.3.1, v2.4 Jan 26, 2023

westonruter added the Changelogged Whether the issue/PR has been added to release notes. label Feb 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix validation error due to native img tag when lightbox and carousel is enabled #7158

Fix validation error due to native img tag when lightbox and carousel is enabled #7158

thelovekesh commented Jun 17, 2022 •

edited

Loading

github-actions bot commented Jun 17, 2022 •

edited

Loading

westonruter Jun 17, 2022

westonruter Jun 17, 2022

thelovekesh Jun 17, 2022

westonruter Jun 17, 2022

westonruter Jun 17, 2022

westonruter Jun 29, 2022

thelovekesh Jun 30, 2022

westonruter Jul 1, 2022

thelovekesh Jul 1, 2022

westonruter Jul 1, 2022

westonruter Jul 7, 2022

thelovekesh Jul 7, 2022

westonruter Jul 7, 2022

thelovekesh Jul 7, 2022

westonruter Jul 7, 2022

	private function adjust_and_replace_node( Element $node ) {
	if ( $this->args['native_img_used'] \|\| ( $node->parentNode instanceof Element && Tag::PICTURE === $node->parentNode->tagName ) ) {
	$attributes = $this->maybe_add_lightbox_attributes( [], $node ); // @todo AMP doesn't support lightbox on <img> yet.

	// Set decoding=async by default. See <https://core.trac.wordpress.org/ticket/53232>.
	if ( ! $node->hasAttribute( Attribute::DECODING ) ) {
	$attributes[ Attribute::DECODING ] = 'async';
	}

	// @todo This class should really only be added if we actually have to provide dimensions.
	$attributes[ Attribute::CLASS_ ] = (string) $node->getAttribute( Attribute::CLASS_ );
	if ( ! empty( $attributes[ Attribute::CLASS_ ] ) ) {
	$attributes[ Attribute::CLASS_ ] .= ' ';
	}
	$attributes[ Attribute::CLASS_ ] .= 'amp-wp-enforced-sizes';

	foreach ( $attributes as $name => $value ) {
	$node->setAttribute( $name, $value );
	}
	return;
	}


	/*
	* Mark lightbox as px-verified attribute until it's supported by AMP spec.
	* @see <https://github.com/ampproject/amp-wp/issues/7152#issuecomment-1157933188>
	* @todo Remove this once lightbox is added in `lightboxable-elements` for native img tag in AMP spec.
	*/
	if ( isset( $attributes['data-amp-lightbox'] ) ) {
	$node->setAttribute( Attribute::LIGHTBOX, 'true' );
	$node_attr = $node->getAttributeNode( Attribute::LIGHTBOX );
	if ( $node_attr instanceof DOMAttr ) {
	ValidationExemption::mark_node_as_px_verified( $node_attr );
	}
	}

Fix validation error due to native img tag when lightbox and carousel is enabled #7158

Fix validation error due to native img tag when lightbox and carousel is enabled #7158

Conversation

thelovekesh commented Jun 17, 2022 • edited Loading

Summary

Checklist

github-actions bot commented Jun 17, 2022 • edited Loading

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thelovekesh commented Jun 17, 2022 •

edited

Loading

github-actions bot commented Jun 17, 2022 •

edited

Loading