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.

Sign up for GitHub

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

Use native img by default #6805

Merged
merged 2 commits into from
Mar 14, 2022
Merged

Use native img by default #6805

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e9714ba
Use native img by default
westonruter Mar 11, 2022
cca6c8e
Add amp-anim to dynamic_element_selectors alongside amp-img
westonruter Mar 11, 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 includes/amp-helper-functions.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1469,7 +1469,7 @@ function amp_is_native_img_used() {
*
* @param bool $use_native Whether to use `img`.
*/
return (bool) apply_filters( 'amp_native_img_used', false );
return (bool) apply_filters( 'amp_native_img_used', true );
}

/**
Expand Down
2 changes: 1 addition & 1 deletion includes/sanitizers/class-amp-img-sanitizer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ class AMP_Img_Sanitizer extends AMP_Base_Sanitizer {
*/
protected $DEFAULT_ARGS = [
'add_noscript_fallback' => true,
'native_img_used' => false,
'native_img_used' => true,
'allow_picture' => false,
];

Expand Down
2 changes: 2 additions & 0 deletions includes/sanitizers/class-amp-style-sanitizer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,8 @@ class AMP_Style_Sanitizer extends AMP_Base_Sanitizer {
protected $DEFAULT_ARGS = [
'disable_style_processing' => false,
'dynamic_element_selectors' => [
'amp-img',
westonruter marked this conversation as resolved.
Show resolved Hide resolved
'amp-anim',
'amp-list',
'amp-live-list',
'[submit-error]',
Expand Down
10 changes: 5 additions & 5 deletions tests/php/test-amp-helper-functions.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1834,13 +1834,13 @@ public function test_amp_is_dev_mode() {
* @covers ::amp_is_native_img_used()
*/
public function test_amp_is_native_img_used() {
$this->assertFalse( amp_is_native_img_used(), 'Expected to be disabled by default for now.' );
$this->assertTrue( amp_is_native_img_used(), 'Expected to be disabled by default for now.' );

add_filter( 'amp_native_img_used', '__return_true' );
$this->assertTrue( amp_is_native_img_used() );

add_filter( 'amp_native_img_used', '__return_false', 20 );
add_filter( 'amp_native_img_used', '__return_false' );
$this->assertFalse( amp_is_native_img_used() );

add_filter( 'amp_native_img_used', '__return_true', 20 );
$this->assertTrue( amp_is_native_img_used() );
}

/**
Expand Down
13 changes: 9 additions & 4 deletions tests/php/test-amp-img-sanitizer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ public function test_get_selector_conversion_mapping() {

$with_defaults = new AMP_Img_Sanitizer( $dom );
$this->assertEquals(
[ 'img' => [ 'amp-img', 'amp-anim' ] ],
[],
$with_defaults->get_selector_conversion_mapping()
);

Expand Down Expand Up @@ -615,6 +615,11 @@ public function test_converter( $source, $expected = null, $args = [], $expected

$error_codes = [];

$args = array_merge(
[ 'native_img_used' => false ],
$args
);

$args = array_merge(
[
'use_document_element' => true,
Expand Down Expand Up @@ -678,7 +683,7 @@ public function test_no_gif_image_scripts() {
$expected = [ 'amp-anim' => true ];

$dom = AMP_DOM_Utils::get_dom_from_content( $source );
$sanitizer = new AMP_Img_Sanitizer( $dom );
$sanitizer = new AMP_Img_Sanitizer( $dom, [ 'native_img_used' => false ] );
$sanitizer->sanitize();

$validating_sanitizer = new AMP_Tag_And_Attribute_Sanitizer( $dom );
Expand All @@ -703,7 +708,7 @@ public function test_image_block_link_to_media_file_with_lightbox() {
$expected = '<figure class="wp-block-image" data-amp-lightbox="true"><amp-img src="https://placehold.it/100x100" width="100" height="100" data-foo="bar" role="button" tabindex="0" data-amp-lightbox="" lightbox="" class="amp-wp-enforced-sizes" layout="intrinsic"><noscript><img src="https://placehold.it/100x100" width="100" height="100" role="button" tabindex="0"></noscript></amp-img></figure>';

$dom = AMP_DOM_Utils::get_dom_from_content( $source );
$sanitizer = new AMP_Img_Sanitizer( $dom );
$sanitizer = new AMP_Img_Sanitizer( $dom, [ 'native_img_used' => false ] );
$sanitizer->sanitize();

$sanitizer = new AMP_Tag_And_Attribute_Sanitizer( $dom );
Expand All @@ -724,7 +729,7 @@ public function test_image_block_link_to_media_file_and_alignment_with_lightbox(
$expected = '<div data-amp-lightbox="true" class="wp-block-image"><figure class="alignright size-large"><amp-img src="https://placehold.it/100x100" width="100" height="100" data-foo="bar" role="button" tabindex="0" data-amp-lightbox="" lightbox="" class="amp-wp-enforced-sizes" layout="intrinsic"><noscript><img src="https://placehold.it/100x100" width="100" height="100" role="button" tabindex="0"></noscript></amp-img></figure></div>';

$dom = AMP_DOM_Utils::get_dom_from_content( $source );
$sanitizer = new AMP_Img_Sanitizer( $dom );
$sanitizer = new AMP_Img_Sanitizer( $dom, [ 'native_img_used' => false ] );
$sanitizer->sanitize();

$sanitizer = new AMP_Tag_And_Attribute_Sanitizer( $dom );
Expand Down
23 changes: 21 additions & 2 deletions tests/php/test-amp-style-sanitizer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1298,6 +1298,9 @@ public function get_amp_selector_data() {
sprintf( '<div><img class="logo" src="%s" width="200" height="100"></div>', admin_url( 'images/wordpress-logo.png' ) ),
'div img.logo{border:solid 1px red}',
'div amp-img.logo{border:solid 1px red}', // Note amp-anim is still tree-shaken because it doesn't occur in the DOM.
[
AMP_Img_Sanitizer::class => [ 'native_img_used' => false ],
],
],
'img-missing-class' => [
sprintf( '<div><img class="logo" src="%s" width="200" height="100"></div>', admin_url( 'images/wordpress-logo.png' ) ),
Expand All @@ -1308,16 +1311,25 @@ public function get_amp_selector_data() {
sprintf( '<div><img class="logo" src="%s" width="200" height="100"><img class="spinner" src="%s" width="200" height="100"></div>', admin_url( 'images/wordpress-logo.png' ), admin_url( 'images/spinner-2x.gif' ) ),
'div img{border:solid 1px red}',
'div amp-img,div amp-anim{border:solid 1px red}',
[
AMP_Img_Sanitizer::class => [ 'native_img_used' => false ],
],
],
'amp-img-and-amp-anim' => [
sprintf( '<amp-img class="logo amp-wp-enforced-sizes" src="%s" width="200" height="100" layout="intrinsic"></amp-img><amp-anim class="spinner amp-wp-enforced-sizes" src="%s" width="200" height="100" layout="intrinsic"></amp-anim>', admin_url( 'images/wordpress-logo.png' ), admin_url( 'images/spinner-2x.gif' ) ),
'amp-img.amp-wp-enforced-sizes[layout="intrinsic"] > img,amp-anim.amp-wp-enforced-sizes[layout="intrinsic"] > img{object-fit:contain}',
'amp-img.amp-wp-enforced-sizes[layout="intrinsic"] > img,amp-anim.amp-wp-enforced-sizes[layout="intrinsic"] > img{object-fit:contain}',
[
AMP_Img_Sanitizer::class => [ 'native_img_used' => false ],
],
],
'admin-bar-style-selectors' => [
'<div id="wpadminbar"><a href="https://example.com/"><amp-img src="https://example.com/foo.png" width="100" height="100"></amp-img><amp-anim src="https://example.com/foo.gif" width="100" height="100"></amp-anim></a></div>',
'#wpadminbar a, #wpadminbar a:hover, #wpadminbar a img, #wpadminbar a img:hover { border: none; text-decoration: none; background: none;}',
'#wpadminbar a,#wpadminbar a:hover,#wpadminbar a amp-img,#wpadminbar a amp-anim,#wpadminbar a amp-img:hover,#wpadminbar a amp-anim:hover{border:none;text-decoration:none;background:none}',
[
AMP_Img_Sanitizer::class => [ 'native_img_used' => false ],
],
],
'img_with_amp_img' => [
'<amp-img></amp-img>',
Expand All @@ -1328,6 +1340,9 @@ public function get_amp_selector_data() {
sprintf( '<div><amp-img class="logo" src="%s" width="200" height="100"></amp-img></div>', admin_url( 'images/wordpress-logo.png' ) ),
'div amp-img.logo img{object-fit:cover}',
'div amp-img.logo img{object-fit:cover}',
[
AMP_Img_Sanitizer::class => [ 'native_img_used' => false ],
],
],
'img-tree-shaking' => [
sprintf( '<article><img class="logo" src="%s" width="200" height="100"></article>', admin_url( 'images/wordpress-logo.png' ) ),
Expand Down Expand Up @@ -1649,6 +1664,8 @@ static function ( $selector ) {
)
);

add_filter( 'amp_native_img_used', '__return_false' );

// The toggling of the 'add_noscript_fallback' arg is to catch a bizzare PHP DOM issue whereby if you replace
// an element in a Document, and that replaced element had an ID, the element will still be returned by
// getElementById even though it is no longer inside of the document. When add_noscript_fallback is false,
Expand Down Expand Up @@ -3453,7 +3470,9 @@ function() {
?>
<figure class="wp-block-audio"><figcaption></figcaption></figure>
<div class="wp-block-foo"><figcaption></figcaption></div>
<img src="https://example.com/example.jpg" width="100" height="200">
<div class="amp-wp-default-form-message">
<p>Hi</p>
</div>
<?php
}
);
Expand All @@ -3476,7 +3495,7 @@ function( $original_dom, $original_source, $amphtml_dom, $amphtml_source ) {

$this->assertStringContainsString( '.wp-block-audio figcaption', $amphtml_source, 'Expected block-library/style.css' );
$this->assertStringContainsString( '[class^="wp-block-"]:not(.wp-block-gallery) figcaption', $amphtml_source, 'Expected twentyten/blocks.css' );
$this->assertStringContainsString( 'amp-img img', $amphtml_source, 'Expected amp-default.css' );
$this->assertStringContainsString( '.amp-wp-default-form-message>p', $amphtml_source, 'Expected amp-default.css' );
$this->assertStringContainsString( 'ab-empty-item', $amphtml_source, 'Expected admin-bar.css to still be present.' );
$this->assertStringNotContainsString( 'earlyprintstyle', $amphtml_source, 'Expected early print style to not be present.' );
$this->assertStringContainsString( 'admin-bar', $amphtml_dom->body->getAttribute( 'class' ) );
Expand Down
18 changes: 9 additions & 9 deletions tests/php/test-class-amp-theme-support.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1613,7 +1613,7 @@ public function test_finish_output_buffering() {
$this->assertTrue( AMP_Theme_Support::is_output_buffering() );
$this->assertSame( 3, ob_get_level() );

echo '<html><head></head><body><img src="test.png"><script data-test>document.write(\'Illegal\');</script>';
echo '<html><head></head><body><video src="test.mp4" width="100" height="100"></video><script data-test>document.write(\'Illegal\');</script>';
wp_footer();

// Additional nested output bufferings which aren't getting closed.
Expand All @@ -1637,7 +1637,7 @@ static function( $response ) {
$this->assertStringContainsString( '<html amp', $output );
$this->assertStringContainsString( 'foo', $output );
$this->assertStringContainsString( 'BAR', $output );
$this->assertStringContainsString( '<amp-img src="test.png"', $output );
$this->assertStringContainsString( '<amp-video src="test.mp4"', $output );
$this->assertStringNotContainsString( '<script data-test', $output );

}
Expand All @@ -1654,11 +1654,11 @@ public function test_filter_customize_partial_render() {
AMP_Theme_Support::init();
AMP_Theme_Support::finish_init();

$partial = '<img src="test.png" style="border:solid 1px red;"><script data-head>document.write(\'Illegal\');</script><style>img { background:blue }</style>';
$partial = '<video src="test.mp4" width="100" height="200" style="border:solid 1px red;"></video><script data-head>document.write(\'Illegal\');</script><style>video { background:blue }</style>';
$output = AMP_Theme_Support::filter_customize_partial_render( $partial );
$this->assertStringContainsString( '<amp-img src="test.png"', $output );
$this->assertStringContainsString( '<amp-video src="test.mp4"', $output );
$this->assertStringContainsString( '<style amp-custom-partial="">', $output );
$this->assertStringContainsString( 'amp-img{background:blue}', $output );
$this->assertStringContainsString( 'amp-video{background:blue}', $output );
$this->assertStringContainsString( '.amp-wp-b123f72:not(#_#_#_#_#_){border:solid 1px red}', $output );
$this->assertStringEndsWith( '/*# sourceURL=amp-custom-partial.css */</style>', $output );
$this->assertStringNotContainsString( '<script', $output );
Expand Down Expand Up @@ -1769,8 +1769,9 @@ function ( $dom, $effective_sandboxing_level ) {
$prev_ordered_contain = $ordered_contain;
}

$this->assertStringContainsString( '<noscript><img', $sanitized_html );
$this->assertStringContainsString( '<amp-img', $sanitized_html );
$this->assertStringNotContainsString( '<noscript><img', $sanitized_html );
$this->assertStringContainsString( '<img width="100" height="100" src="https://example.com/hero.png" decoding="async" class="amp-wp-enforced-sizes">', $sanitized_html );
$this->assertStringContainsString( '<img width="100" height="100" src="https://example.com/test.png" loading="lazy" decoding="async" class="amp-wp-enforced-sizes">', $sanitized_html );

$this->assertStringContainsString( '<noscript><audio', $sanitized_html );
$this->assertStringContainsString( '<amp-audio', $sanitized_html );
Expand Down Expand Up @@ -2162,9 +2163,8 @@ static function() {
</head>
<body><!-- </body></html> -->
<div id="dynamic-id-0"></div>
<!-- 2nd image is needed for testing <noscript> as first is SSR'ed -->
<img width="100" height="100" src="https://example.com/hero.png">
<img width="100" height="100" src="https://example.com/test.png">
<img width="100" height="100" src="https://example.com/test.png" loading="lazy">
<audio src="https://example.com/audios/myaudio.mp3"></audio>
<amp-ad type="a9"
width="300"
Expand Down