Definitively fix issues with images in AMP not visually matching non-AMP version #2036

westonruter · 2019-03-29T01:19:15Z

Build for testing: amp.zip (v1.1-alpha-20190405T220345Z-9246809c)

This PR seeks to finally lay to rest the multitude of issues we've faced regarding amp-img not behaving like the corresponding img. A key piece to this is that the sizes attribute can now be removed from amp-img and the AMP runtime will (soon) compute it automatically; see ampproject/amphtml#19513.

Fixes issues including:

Featured image in Twenty Nineteen appears stretched in desktop.
Centered image is too side.
Align-wide images are not wide enough.
Align-full images are not wide enough.
Small inline image is not displayed tiny as required.
Image in Media/Text overflows right viewport (both normal and wide alignments).
Resized image in Media/Text is not the right dimensions.
Centered image in Classic block is not centered.
Inline image in Classic block appears on its own line.

See screenshots below to compare.

Fixes #1747. Fixes #1305.
See #1656, #1104, #1237, #1086.
Closes #1939.

The post content I've been using to test is as follows. I tried to consider every scenario for where an image can appear:

<!-- wp:image {"id":53,"align":"center"} -->
<div class="wp-block-image"><figure class="aligncenter"><img src="https://wordpressdev.lndo.site/content/uploads/2019/02/1200px-American_bison_k5680-1-1024x668.jpg" alt="" class="wp-image-53"/><figcaption>Bison: centered and not resized<br></figcaption></figure></div>
<!-- /wp:image -->

<!-- wp:paragraph -->
<p>And here is an image without a caption and yet it is centered:</p>
<!-- /wp:paragraph -->

<!-- wp:image {"id":53,"align":"center","width":247,"height":161} -->
<div class="wp-block-image"><figure class="aligncenter is-resized"><img src="https://wordpressdev.lndo.site/content/uploads/2019/02/1200px-American_bison_k5680-1-1024x668.jpg" alt="" class="wp-image-53" width="247" height="161"/></figure></div>
<!-- /wp:image -->

<!-- wp:paragraph -->
<p>And here is a centered image with a caption:</p>
<!-- /wp:paragraph -->

<!-- wp:image {"id":53,"align":"center","width":252,"height":163} -->
<div class="wp-block-image"><figure class="aligncenter is-resized"><img src="https://wordpressdev.lndo.site/content/uploads/2019/02/1200px-American_bison_k5680-1-1024x668.jpg" alt="" class="wp-image-53" width="252" height="163"/><figcaption>Resized and centered</figcaption></figure></div>
<!-- /wp:image -->

<!-- wp:image {"id":53,"align":"left","width":252,"height":163} -->
<div class="wp-block-image"><figure class="alignleft is-resized"><img src="https://wordpressdev.lndo.site/content/uploads/2019/02/1200px-American_bison_k5680-1-1024x668.jpg" alt="" class="wp-image-53" width="252" height="163"/><figcaption>Resized and left-floated</figcaption></figure></div>
<!-- /wp:image -->

<!-- wp:image {"id":53,"align":"right","width":252,"height":163} -->
<div class="wp-block-image"><figure class="alignright is-resized"><img src="https://wordpressdev.lndo.site/content/uploads/2019/02/1200px-American_bison_k5680-1-1024x668.jpg" alt="" class="wp-image-53" width="252" height="163"/><figcaption>Resized and right-floated</figcaption></figure></div>
<!-- /wp:image -->

<!-- wp:paragraph -->
<p>This text is in between two beasts! This text is in between two beasts! This text is in between two beasts! This text is in between two beasts! This text is in between two beasts! This text is in between two beasts! This text is in between two beasts! This text is in between two beasts! This text is in between two beasts! This text is in between two beasts! </p>
<!-- /wp:paragraph -->

<!-- wp:image {"id":53,"align":"wide"} -->
<figure class="wp-block-image alignwide"><img src="https://wordpressdev.lndo.site/content/uploads/2019/02/1200px-American_bison_k5680-1-1024x668.jpg" alt="" class="wp-image-53"/><figcaption>Wide</figcaption></figure>
<!-- /wp:image -->

<!-- wp:image {"id":53,"align":"full"} -->
<figure class="wp-block-image alignfull"><img src="https://wordpressdev.lndo.site/content/uploads/2019/02/1200px-American_bison_k5680-1-1024x668.jpg" alt="" class="wp-image-53"/><figcaption>Full bleed<br></figcaption></figure>
<!-- /wp:image -->

<!-- wp:separator -->
<hr class="wp-block-separator"/>
<!-- /wp:separator -->

<!-- wp:paragraph -->
<p>Here is some text with an inline image: <img class="wp-image-53" style="width: 48px;" src="https://wordpressdev.lndo.site/content/uploads/2019/02/1200px-American_bison_k5680-1.jpg" alt=""> See it is small?</p>
<!-- /wp:paragraph -->

<!-- wp:separator -->
<hr class="wp-block-separator"/>
<!-- /wp:separator -->

<!-- wp:media-text {"mediaPosition":"right","mediaId":53,"mediaType":"image","isStackedOnMobile":true} -->
<div class="wp-block-media-text alignwide has-media-on-the-right is-stacked-on-mobile"><figure class="wp-block-media-text__media"><img src="https://wordpressdev.lndo.site/content/uploads/2019/02/1200px-American_bison_k5680-1-1024x668.jpg" alt="" class="wp-image-53"/></figure><div class="wp-block-media-text__content"><!-- wp:paragraph {"placeholder":"Content…","fontSize":"large"} -->
<p class="has-large-font-size">Bison</p>
<!-- /wp:paragraph -->

<!-- wp:paragraph -->
<p>Image appears to the left. It is not resized. Stacking on mobile.</p>
<!-- /wp:paragraph --></div></div>
<!-- /wp:media-text -->

<!-- wp:media-text {"align":"full","mediaPosition":"right","mediaId":53,"mediaType":"image","isStackedOnMobile":true} -->
<div class="wp-block-media-text alignfull has-media-on-the-right is-stacked-on-mobile"><figure class="wp-block-media-text__media"><img src="https://wordpressdev.lndo.site/content/uploads/2019/02/1200px-American_bison_k5680-1-1024x668.jpg" alt="" class="wp-image-53"/></figure><div class="wp-block-media-text__content"><!-- wp:paragraph {"placeholder":"Content…","fontSize":"large"} -->
<p class="has-large-font-size">Bison and full width</p>
<!-- /wp:paragraph -->

<!-- wp:paragraph -->
<p>Image appears to the left. It is not resized. Stacking on mobile.</p>
<!-- /wp:paragraph --></div></div>
<!-- /wp:media-text -->

<!-- wp:media-text {"mediaId":53,"mediaType":"image","mediaWidth":19} -->
<div class="wp-block-media-text alignwide" style="grid-template-columns:19% auto"><figure class="wp-block-media-text__media"><img src="https://wordpressdev.lndo.site/content/uploads/2019/02/1200px-American_bison_k5680-1-1024x668.jpg" alt="" class="wp-image-53"/></figure><div class="wp-block-media-text__content"><!-- wp:paragraph {"placeholder":"Content…","fontSize":"large"} -->
<p class="has-large-font-size">Bison</p>
<!-- /wp:paragraph -->

<!-- wp:paragraph -->
<p>Image appears to the left. It <em>is</em> resized. Stacking off mobile.</p>
<!-- /wp:paragraph --></div></div>
<!-- /wp:media-text -->

<!-- wp:separator -->
<hr class="wp-block-separator"/>
<!-- /wp:separator -->

<p><img class="alignleft size-thumbnail wp-image-53" src="https://wordpressdev.lndo.site/content/uploads/2019/02/1200px-American_bison_k5680-1-150x150.jpg" alt="" width="150" height="150">This is a <strong>classic block</strong> that contains an image floated to the left. And there is one floated to the right here too:</p>
<p>[caption id="attachment_53" align="alignright" width="150"]<img class="size-thumbnail wp-image-53" src="https://wordpressdev.lndo.site/content/uploads/2019/02/1200px-American_bison_k5680-1-150x150.jpg" alt="" width="150" height="150"> Bison in field[/caption]</p>
<p>This is a classic block that contains an image floated to the left. This is a classic block that contains an image floated to the left. This is a classic block that contains an image floated to the left. This is a classic block that contains an image floated to the left. This is a classic block that contains an image floated to the left. This is a classic block that contains an image floated to the left. But the next image is centered:</p>
<p><img class="aligncenter wp-image-53 size-medium" src="https://wordpressdev.lndo.site/content/uploads/2019/02/1200px-American_bison_k5680-1-300x196.jpg" alt="" width="300" height="196"></p>
<p>And this centered image has a caption:</p>
<p>[caption id="attachment_53" align="aligncenter" width="300"]<img class="wp-image-53 size-medium" src="https://wordpressdev.lndo.site/content/uploads/2019/02/1200px-American_bison_k5680-1-300x196.jpg" alt="" width="300" height="196"> Bison in the field.[/caption]</p>
<p>Next line with tiny image inline:&nbsp;<img class="alignnone wp-image-53 " src="https://wordpressdev.lndo.site/content/uploads/2019/02/1200px-American_bison_k5680-1-150x150.jpg" alt="" width="16" height="16">. See how small it is?</p>
<p>Next line.</p>

Desktop

Non-AMP (control)	✅ AMP with fix	🚫 AMP without fix

Mobile

Non-AMP (control)	✅ AMP with fix	🚫 AMP without fix

Classic Templates

Please note that the Classic templates generally lack styles for blocks in general, so there are some issues that aren't related to images here.

🚫 Before changes	✅ After changes

westonruter · 2019-03-29T04:44:52Z

For some reason the alignfull isn't rendering properly in the Gutenberg demo post: https://2019-theme.amp-wp.org/welcome-to-the-gutenberg-editor/?amp

westonruter · 2019-03-29T05:05:49Z

For some reason the alignfull isn't rendering properly in the Gutenberg demo post: https://2019-theme.amp-wp.org/welcome-to-the-gutenberg-editor/?amp

OK, that seems fixed now by 6677c3b.

westonruter · 2019-03-29T19:59:37Z

As noted in ampproject/amphtml#21371 (comment) I added the Bison image test post to the environment:

✅ Non-AMP: https://2019-theme.amp-wp.org/bison-image-test/
✅ AMP with fix: https://2019-theme.amp-wp.org/bison-image-test/?amp

@cathyxz

…inc also get object-fit:contain This guards against image stretching. See <ampproject/amphtml#21371 (comment)>. Props @cathyxz

…yout

…t is essentially alignwide

AMP will generate the sizes attribute for us. Removing the sizes attribute also fixes many issues with images getting unexpected width being set.

kienstra · 2019-04-03T14:22:18Z

includes/sanitizers/class-amp-core-theme-sanitizer.php

-	 * @todo: remove when this is resolved: https://github.com/ampproject/amphtml/issues/17053
-	 * @since 1.0
-	 */
-	public static function remove_twentynineteen_thumbnail_image_sizes() {


It's great to be able to remove these workarounds!

kienstra · 2019-04-03T15:20:29Z

Brief Testing

Hi @westonruter,
This looks good overall. There doesn't look to be distortion of images, and it's really nice seeing these workarounds removed.

It looks like sometimes large images are a different size in AMP and non-AMP. Though the biggest problem before was distortion, and that doesn't look to be an issue.

Steps to reproduce

Activate Twenty Nineteen and checkout this PR's branch
Create an Image block, without changing anything like the layout or caption
Download a 1024 x 628 image
In the Image block, click 'Upload' and select that image.
Expected: it appears the same size in non-AMP and AMP
Actual: it appears smaller in AMP

Non-AMP

AMP

westonruter · 2019-04-03T19:01:11Z

@kienstra I think this is just an issue with your local environment. Note the image has the class name amp-wp-unknown-size. Compare that with the image at https://2019-theme.amp-wp.org/bison-image-test/

It appears as expected.

westonruter · 2019-04-03T19:03:59Z

In other words, your image is failing because the plugin wasn't able to perform the HTTP request to determine the dimensions.

kienstra · 2019-04-04T00:30:01Z

Thanks, @westonruter. That makes sense.

I'll review this PR.

kienstra · 2019-04-04T08:14:26Z

Approved

Hi @westonruter,
Wow, this is the most thorough explanation with screenshots I've ever seen. Thanks for the great detail.

It's really nice that these workarounds aren't needed anymore.

(Stealing your great table formatting above)

Non-AMP	AMP with this PR

The only images that look different in AMP have amp-wp-unknown-size and amp-wp-unknown-width. That looks to be a similar issue to the one you commented about, possibly just the local environment.

As expected, there's no reference PR #17053 anymore in the plugin, like here.

kienstra · 2019-04-04T07:17:18Z

templates/style.php

-	/** Force the image into a box of fixed dimensions and use object-fit to scale. **/
-	object-fit: contain;
-}
+<?php echo file_get_contents( AMP__DIR__ . '/assets/css/amp-default.css' ); // phpcs:ignore WordPress.WP.AlternativeFunctions ?>


Good idea to echo amp-default.css here.

westonruter · 2019-04-05T23:47:26Z

@kienstra I'm a bit concerned to see stretching here:

This should not be happening even when the sizes are unknown due to object-fit: contain.

westonruter added this to the v1.1 milestone Mar 29, 2019

googlebot added the cla: yes Signed the Google CLA label Mar 29, 2019

westonruter requested a review from swissspidy March 29, 2019 04:01

westonruter marked this pull request as ready for review March 29, 2019 04:01

westonruter force-pushed the fix/image-stretching branch from 7e6230a to 034ac82 Compare March 29, 2019 20:36

westonruter added 12 commits April 1, 2019 21:10

Eliminate handle_centering for images since apparently unnecessary

7371ee5

Eliminate duplicated CSS by including amp-default.css in classic styles

e914f21

Ensure amp-default style is enqueued before theme's stylesheet

835e1da

Ensure <img> elements converted to <amp-img> which have layout=intris…

e1296f7

…inc also get object-fit:contain This guards against image stretching. See <ampproject/amphtml#21371 (comment)>. Props @cathyxz

Give responsive layout to alignwide/alignfull instead of intrinsic la…

fc4ab0a

…yout

Ensure featured image gets responsive layout in Twenty Nineteen, as i…

035131c

…t is essentially alignwide

Eliminate sizes from amp-img converted from img

45bf5a3

AMP will generate the sizes attribute for us. Removing the sizes attribute also fixes many issues with images getting unexpected width being set.

Rename method to adjust_twentynineteen_images

e7d4247

Improve aligncenter, alignwide, and alignfull in classic templates

93ce9e8

Add test for sizes attribute being removed from converted img

a82a87f

Remove obsolete add_auto_width_to_figure from #1086

65f1c0c

Opt-in to amp-img-auto-sizes experiment when there are converted images

0180357

westonruter force-pushed the fix/image-stretching branch from 034ac82 to 0180357 Compare April 2, 2019 04:10

kienstra reviewed Apr 3, 2019

View reviewed changes

westonruter requested a review from kienstra April 3, 2019 19:31

kienstra approved these changes Apr 4, 2019

View reviewed changes

westonruter merged commit 77b0b54 into develop Apr 4, 2019

westonruter deleted the fix/image-stretching branch April 4, 2019 13:40

westonruter added the changelogged label Apr 5, 2019

westonruter mentioned this pull request Sep 3, 2019

Remove launched experiment amp-img-auto-sizes #3176

Merged

westonruter mentioned this pull request Apr 20, 2020

Discontinue removing sizes attribute when converting img to amp-img #4606

Closed

pierlon mentioned this pull request Apr 23, 2020

Prevent distortion of images with a 'sizes' attribute #4622

Merged

3 tasks

westonruter mentioned this pull request Apr 24, 2020

🐛 Restrict display:block/position:relative styles to implicit responsive layout ampproject/amphtml#28020

Merged

westonruter mentioned this pull request Aug 10, 2021

Experiment: Add native img opt-in to prevent conversion to amp-img/amp-anim #6518

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Definitively fix issues with images in AMP not visually matching non-AMP version #2036

Definitively fix issues with images in AMP not visually matching non-AMP version #2036

westonruter commented Mar 29, 2019 •

edited

westonruter commented Mar 29, 2019

westonruter commented Mar 29, 2019

westonruter commented Mar 29, 2019

kienstra Apr 3, 2019

kienstra commented Apr 3, 2019 •

edited

westonruter commented Apr 3, 2019

westonruter commented Apr 3, 2019

kienstra commented Apr 4, 2019

kienstra commented Apr 4, 2019 •

edited

kienstra Apr 4, 2019

westonruter commented Apr 5, 2019

Definitively fix issues with images in AMP not visually matching non-AMP version #2036

Definitively fix issues with images in AMP not visually matching non-AMP version #2036

Conversation

westonruter commented Mar 29, 2019 • edited

Desktop

Mobile

Classic Templates

westonruter commented Mar 29, 2019

westonruter commented Mar 29, 2019

westonruter commented Mar 29, 2019

kienstra Apr 3, 2019

Choose a reason for hiding this comment

kienstra commented Apr 3, 2019 • edited

Non-AMP

AMP

westonruter commented Apr 3, 2019

westonruter commented Apr 3, 2019

kienstra commented Apr 4, 2019

kienstra commented Apr 4, 2019 • edited

kienstra Apr 4, 2019

Choose a reason for hiding this comment

westonruter commented Apr 5, 2019

westonruter commented Mar 29, 2019 •

edited

kienstra commented Apr 3, 2019 •

edited

kienstra commented Apr 4, 2019 •

edited