Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 23 commits into from
Jul 7, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
0afb499
Add lightbox to px-verified-attrs in native img tag when lightbox is …
thelovekesh Jun 17, 2022
7d64732
Remove layout and object-fit attrs for native img tag and add style attr
thelovekesh Jun 17, 2022
31cb9a6
Add test cases for native img tag px verified lightbox attrs
thelovekesh Jun 17, 2022
90c0fc8
Update native_img_used variable with provided default args
thelovekesh Jun 17, 2022
22853be
Add test cases for replacing layout and object-fill attrs with style …
thelovekesh Jun 17, 2022
a1d80b0
Add check to find key in args to avoid error in tests
thelovekesh Jun 17, 2022
ec01348
Fix PHPStan errors
thelovekesh Jun 17, 2022
30f7bcd
Update test case function name
thelovekesh Jun 17, 2022
762c309
Update mentioned method in @covers
thelovekesh Jun 17, 2022
60e2f46
Fix typo
thelovekesh Jun 17, 2022
0755735
Update condition to check if we need to px verify lightbox attr
thelovekesh Jun 17, 2022
e4e5396
Update logic of creating node attribute
thelovekesh Jun 17, 2022
f0184f1
Fix test cases for TikTok embed handler
thelovekesh Jun 20, 2022
d4705ee
Fix test cases for YouTube embed handler
thelovekesh Jun 20, 2022
7c3f6d6
Remove unused namespace aliases
thelovekesh Jul 4, 2022
20e82cb
Remove native img attributes sanitization
thelovekesh Jul 4, 2022
5374854
Add AMP_Native_Img_Attributes_Sanitizer for native image attributes s…
thelovekesh Jul 4, 2022
0dc9a1f
Add AMP_Native_Img_Attributes_Sanitizer in sanitizers
thelovekesh Jul 4, 2022
079f5d8
Remove test cases for native img attributes sanitization
thelovekesh Jul 4, 2022
ecf255f
Add test cases for AMP_Native_Img_Attributes_Sanitizer
thelovekesh Jul 4, 2022
144b65f
Merge branch 'develop' of github.com:ampproject/amp-wp into fix/light…
westonruter Jul 7, 2022
ee80e10
Preserve original styles; support object-position; support non-cover …
westonruter Jul 7, 2022
3e558de
Add composer-normalize to allowed-plugins before require
westonruter Jul 7, 2022
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/build-test-measure.yml
Original file line number Diff line number Diff line change
Expand Up @@ -197,8 +197,8 @@ jobs:

- name: Normalize composer.json
run: |
composer require --no-interaction --dev ergebnis/composer-normalize --ignore-platform-reqs
composer config --no-interaction --no-plugins allow-plugins.ergebnis/composer-normalize true
composer require --no-interaction --dev ergebnis/composer-normalize --ignore-platform-reqs
composer --no-interaction normalize --dry-run

#-----------------------------------------------------------------------------------------------------------------------
Expand Down
48 changes: 26 additions & 22 deletions includes/amp-helper-functions.php
Original file line number Diff line number Diff line change
Expand Up @@ -1522,12 +1522,12 @@ function amp_get_content_sanitizers( $post = null ) {

$sanitizers = [
// Embed sanitization must come first because it strips out custom scripts associated with embeds.
AMP_Embed_Sanitizer::class => [
AMP_Embed_Sanitizer::class => [
'amp_to_amp_linking_enabled' => $amp_to_amp_linking_enabled,
],
AMP_O2_Player_Sanitizer::class => [],
AMP_Playbuzz_Sanitizer::class => [],
AMP_Core_Theme_Sanitizer::class => [
AMP_O2_Player_Sanitizer::class => [],
AMP_Playbuzz_Sanitizer::class => [],
AMP_Core_Theme_Sanitizer::class => [
'template' => get_template(),
'stylesheet' => get_stylesheet(),
'theme_features' => [
Expand All @@ -1536,50 +1536,53 @@ function amp_get_content_sanitizers( $post = null ) {
'native_img_used' => $native_img_used,
],

AMP_Comments_Sanitizer::class => [
AMP_Comments_Sanitizer::class => [
'comments_live_list' => ! empty( $theme_support_args['comments_live_list'] ),
],

AMP_Bento_Sanitizer::class => [],
AMP_Bento_Sanitizer::class => [],

// The AMP_PWA_Script_Sanitizer run before AMP_Script_Sanitizer, to prevent the script tags
// from getting removed in PWA plugin offline/500 templates.
AMP_PWA_Script_Sanitizer::class => [],
AMP_PWA_Script_Sanitizer::class => [],

// The AMP_Script_Sanitizer runs here because based on whether it allows custom scripts
// to be kept, it may impact the behavior of other sanitizers. For example, if custom
// scripts are kept then this is a signal that tree shaking in AMP_Style_Sanitizer cannot be
// performed.
AMP_Script_Sanitizer::class => [],
AMP_Script_Sanitizer::class => [],

AMP_Srcset_Sanitizer::class => [],
AMP_Img_Sanitizer::class => [
AMP_Srcset_Sanitizer::class => [],
AMP_Img_Sanitizer::class => [
'align_wide_support' => current_theme_supports( 'align-wide' ),
'native_img_used' => $native_img_used,
],
AMP_Form_Sanitizer::class => [],
AMP_Video_Sanitizer::class => [],
AMP_Audio_Sanitizer::class => [],
AMP_Object_Sanitizer::class => [],
AMP_Iframe_Sanitizer::class => [
AMP_Form_Sanitizer::class => [],
AMP_Video_Sanitizer::class => [],
AMP_Audio_Sanitizer::class => [],
AMP_Object_Sanitizer::class => [],
AMP_Iframe_Sanitizer::class => [
'add_placeholder' => true,
'current_origin' => $current_origin,
'align_wide_support' => current_theme_supports( 'align-wide' ),
],
AMP_Gallery_Block_Sanitizer::class => [ // Note: Gallery block sanitizer must come after image sanitizers since itś logic is using the already sanitized images.
AMP_Gallery_Block_Sanitizer::class => [ // Note: Gallery block sanitizer must come after image sanitizers since itś logic is using the already sanitized images.
'carousel_required' => ! is_array( $theme_support_args ), // For back-compat.
'native_img_used' => $native_img_used,
],
AMP_Block_Sanitizer::class => [], // Note: Block sanitizer must come after embed / media sanitizers since its logic is using the already sanitized content.
AMP_Style_Sanitizer::class => [
AMP_Native_Img_Attributes_Sanitizer::class => [ // Note: Native img attributes sanitizer must come after image sanitizers since its logic is sanitizing the already sanitized images attributes.
'native_img_used' => $native_img_used,
],
AMP_Block_Sanitizer::class => [], // Note: Block sanitizer must come after embed / media sanitizers since its logic is using the already sanitized content.
AMP_Style_Sanitizer::class => [
'skip_tree_shaking' => is_customize_preview(),
'allow_excessive_css' => is_customize_preview(),
],
AMP_Meta_Sanitizer::class => [],
AMP_Layout_Sanitizer::class => [],
AMP_Accessibility_Sanitizer::class => [],
AMP_Meta_Sanitizer::class => [],
AMP_Layout_Sanitizer::class => [],
AMP_Accessibility_Sanitizer::class => [],
// Note: This validating sanitizer must come at the end to clean up any remaining issues the other sanitizers didn't catch.
AMP_Tag_And_Attribute_Sanitizer::class => [
AMP_Tag_And_Attribute_Sanitizer::class => [
'prefer_bento' => amp_is_bento_enabled(),
],
];
Expand Down Expand Up @@ -1723,6 +1726,7 @@ function amp_get_content_sanitizers( $post = null ) {
AMP_Object_Sanitizer::class,
AMP_Iframe_Sanitizer::class,
AMP_Gallery_Block_Sanitizer::class,
AMP_Native_Img_Attributes_Sanitizer::class, // Must come after gallery block sanitizer since it sanitizes img attributes.
AMP_Block_Sanitizer::class,
AMP_Accessibility_Sanitizer::class,
AMP_Layout_Sanitizer::class,
Expand Down
14 changes: 14 additions & 0 deletions includes/sanitizers/class-amp-img-sanitizer.php
Original file line number Diff line number Diff line change
Expand Up @@ -403,6 +403,20 @@ 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.

/*
* 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->hasAttribute( Attribute::LIGHTBOX ) ) {
$node_attr = $node->getAttributeNode( Attribute::LIGHTBOX );
if ( ! $node_attr instanceof DOMAttr ) {
$node_attr = $this->dom->createAttribute( Attribute::LIGHTBOX );
$node->setAttributeNode( $node_attr );
}
ValidationExemption::mark_node_as_px_verified( $node_attr );
}

// Set decoding=async by default. See <https://core.trac.wordpress.org/ticket/53232>.
if ( ! $node->hasAttribute( Attribute::DECODING ) ) {
$attributes[ Attribute::DECODING ] = 'async';
Expand Down
91 changes: 91 additions & 0 deletions includes/sanitizers/class-amp-native-img-attributes-sanitizer.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
<?php
/**
* Native img attributes sanitizer.
*
* @since 2.3.1
* @package AMP
*/

use AmpProject\Html\Attribute;
use AmpProject\Dom\Element;
use AmpProject\Layout;

/**
* Class AMP_Native_Img_Attributes_Sanitizer
*
* @since 2.3.1
* @internal
*/
class AMP_Native_Img_Attributes_Sanitizer extends AMP_Base_Sanitizer {

/**
* Array of flags used to control sanitization.
*
* @var array {
* @type bool $native_img_used Whether native img is being used.
* }
*/
protected $args;

/**
* Default args.
*
* @var array
*/
protected $DEFAULT_ARGS = [
'native_img' => false,
];

/**
* Sanitize the Native img attributes.
*
* @since 2.3
*/
public function sanitize() {
// Bail if native img is not being used.
if ( ! isset( $this->args['native_img_used'] ) || ! $this->args['native_img_used'] ) {
return;
}

// Images with layout=fill.
$img_elements = $this->dom->xpath->query(
'.//img[ @layout = "fill" ]',
$this->dom->body
);
if ( $img_elements instanceof DOMNodeList ) {
foreach ( $img_elements as $img_element ) {
/** @var Element $img_element */
$img_element->removeAttribute( Attribute::LAYOUT );
$img_element->addInlineStyle( 'position:absolute;left:0;right:0;top:0;bottom:0;width:100%;height:100%' );
}
}

// 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 ) );
}
}
Comment on lines +63 to +89
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
}
2 changes: 0 additions & 2 deletions includes/sanitizers/class-amp-pwa-script-sanitizer.php
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,6 @@
*/

use AmpProject\AmpWP\ValidationExemption;
use AmpProject\Dom\Element;
use AmpProject\Html\Attribute;

/**
* Class AMP_PWA_Script_Sanitizer
Expand Down
19 changes: 19 additions & 0 deletions tests/php/test-amp-img-sanitizer.php
Original file line number Diff line number Diff line change
Expand Up @@ -776,6 +776,25 @@ public function test_image_block_link_to_media_file_with_lightbox() {
$this->assertEquals( $expected, $content );
}

/**
* Test an native img tag has px-verified lightbox attributes.
*
* @covers ::sanitize()
*/
public function test_native_img_tag_has_px_verified_lightbox_attr() {
$source = '<figure class="wp-block-image" data-amp-lightbox="true"><img src="https://placehold.it/100x100" width="100" height="100" data-foo="bar" role="button" tabindex="0" /></a></figure>';
$expected = '<figure class="wp-block-image" data-amp-lightbox="true"><img src="https://placehold.it/100x100" width="100" height="100" data-foo="bar" role="button" tabindex="0" lightbox="" data-px-verified-attrs="lightbox" data-amp-lightbox="" decoding="async" class="amp-wp-enforced-sizes"></figure>';

$dom = AMP_DOM_Utils::get_dom_from_content( $source );
$sanitizer = new AMP_Img_Sanitizer( $dom, [ 'native_img_used' => true ] );
$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 );
}

/**
* Test an Image block wrapped in an <a>, that has right-alignment, links to the media file, and has 'lightbox' selected.
*
Expand Down
92 changes: 92 additions & 0 deletions tests/php/test-class-amp-native-img-attributes-sanitizer.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
<?php
/**
* Test cases for AMP_Native_Img_Attributes_Sanitizer
*
* @package AmpProject\AmpWP
*/

use AmpProject\AmpWP\Tests\TestCase;
use AmpProject\AmpWP\Tests\Helpers\MarkupComparison;

/**
* Class AMP_Native_Img_Attributes_Sanitizer_Test
*
* @since 2.3.1
* @coversDefaultClass AMP_Native_Img_Attributes_Sanitizer
*/
class AMP_Native_Img_Attributes_Sanitizer_Test extends TestCase {

use MarkupComparison;

public function get_data_to_test_sanitize() {
$amp_carousel_source = '<amp-carousel width="600" height="400" type="slides" layout="responsive" lightbox=""><figure class="slide"><img src="http://example.com/img.png" width="600" height="400" layout="fill" object-fit="cover"></figure></amp-carousel>';
$amp_carousel_sanitized = '<amp-carousel width="600" height="400" type="slides" layout="responsive" lightbox=""><figure class="slide"><img src="http://example.com/img.png" width="600" height="400" style="position:absolute;left:0;right:0;top:0;bottom:0;width:100%;height:100%;object-fit:cover"></figure></amp-carousel>';

return [
'disabled' => [
false,
$amp_carousel_source,
null, // Same as above.
],
'carousel_with_img' => [
true,
$amp_carousel_source,
$amp_carousel_sanitized,
],
'amp_img' => [
true,
'<amp-img src="https://example.com/img.png" style="border: solid 1px red;" layout="fill" object-fit="cover"></amp-img>',
null, // Same as above.
],
'img_with_layout_fill' => [
true,
'<img src="https://example.com/img.png" style="border: solid 1px red" layout="fill">',
'<img src="https://example.com/img.png" style="border: solid 1px red;position:absolute;left:0;right:0;top:0;bottom:0;width:100%;height:100%">',
],
'img_with_layout_nodisplay' => [
true,
'<img src="https://example.com/img.png" style="border: solid 1px red;" layout="nodisplay">',
null, // Same as above.
],
'img_with_object_fit_cover' => [
true,
'<img src="https://example.com/img.png" style="border: solid 1px red;" object-fit="cover">',
'<img src="https://example.com/img.png" style="border: solid 1px red;object-fit:cover">',
],
'img_with_object_fit_contain' => [
true,
'<img src="https://example.com/img.png" style="border: solid 1px red;" object-fit="contain">',
'<img src="https://example.com/img.png" style="border: solid 1px red;object-fit:contain">',
],
'img_with_object_position' => [
true,
'<img src="https://example.com/img.png" style="border: solid 1px red;" object-position="top">',
'<img src="https://example.com/img.png" style="border: solid 1px red;object-position:top">',
],
];
}

/**
* Test an native img tag has not layout or object-fit attributes.
*
* `layout` and `object-fit` will be replaced with a style attribute.
*
* @dataProvider get_data_to_test_sanitize
* @covers ::sanitize()
*/
public function test_sanitize( $native_img_used, $source, $expected ) {
if ( null === $expected ) {
$expected = $source;
}

$dom = AMP_DOM_Utils::get_dom_from_content( $source );
$sanitizer = new AMP_Native_Img_Attributes_Sanitizer(
$dom,
compact( 'native_img_used' )
);
$sanitizer->sanitize();

$content = AMP_DOM_Utils::get_content_from_dom( $dom );
$this->assertEqualMarkup( $expected, $content );
}
}
3 changes: 3 additions & 0 deletions tests/php/test-class-amp-tiktok-embed-handler.php
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,9 @@ public function test_conversion( $source, $expected = null ) {
// Remove new data attribute added recently.
$actual = str_replace( ' data-embed-from="oembed"', '', $actual );

// Remove refer param from URL.
$actual = str_replace( '?refer=embed', '', $actual );

$this->assertSimilarMarkup( $expected, $actual );
}
}
4 changes: 4 additions & 0 deletions tests/php/test-class-amp-youtube-embed-handler.php
Original file line number Diff line number Diff line change
Expand Up @@ -348,6 +348,10 @@ public function test__conversion( $source, $expected, $fallback_for_expected = n
version_compare( strtok( get_bloginfo( 'version' ), '-' ), '5.1', '<' )
&& null !== $fallback_for_expected
) {
// Remove the title or alt attribute from $filtered_content if it exists.
$filtered_content = preg_replace( '/ title="[^"]*"/', '', $filtered_content );
$filtered_content = preg_replace( '/ alt="[^"]*"/', '', $filtered_content );

$this->assertEqualMarkup( $fallback_for_expected, $filtered_content );
} else {
$this->assertEqualMarkup( $expected, $filtered_content );
Expand Down