Filter the attachment image attributes for the Twenty Seventeen theme.

[hellofromtonya]

Per issue #1237, the Twenty Seventeen theme is adding $attr['sizes'] = '100vw'; which stretches images on the attachment page when in AMP mode.

AMP uses a combination of the layout and sizes attributes to determine how to set the inline style width.

This PR filters 'wp_get_attachment_image_attributes' to change the layout to responsive and sizes to the calculated image sizes per wp_get_attachment_image_sizes().

- Changing the layout to responsive ensures the image renders within the parent container in all viewports.

• Changing the sizes value provides the information AMP needs to set the inline width.

Before

Notice the differences in non-AMP vs. AMP rendering before this PR is applied. The images are stretched due to the sizes and layout attributes.

Here is a 150x150 thumbnail image before this PR is applied:

Here is a 1200w image before this PR is applied:

After

Now let's compare both of the above images after applying this PR. Notice the images render the same in non-AMP and AMP modes without stretching.

Is this a fix or bandaid?

There is an open issue with the AMPHTML project about images being stretched. IMO this PR fixes the layout and sizes for the Twenty Seventeen theme as it is adjusting the sizes to 100vw when on the attachment page. Therefore, I think this PR could be rolled in without worrying about the changes from the open issue.

[westonruter]

@hellofromtonya does this negatively effect the behavior in the featured image?

It seems the purpose of 100vw is to:

Add custom image sizes attribute to enhance responsive image functionality for post thumbnails.

Does that negatively impact such an image?

[hellofromtonya]

@westonruter

does this negatively effect the behavior in the featured image?

No, as it's only targeting the image's attachment page. Therefore, it will not have an effect on the featured image.

[kienstra]

Looks Good, Works As Expected

Hi @hellofromtonya,
Thanks, this looks good. I also saw that images on attachment pages now look the same on AMP and non-AMP.

And like you mentioned, I didn't see any regression in images on non-attachment pages:

There's a minor question below, but not blocker.

[kienstra]

This is a a minor point, but would it be possible to not set the layout here, and let the image sanitizer default to intrinsic (if there's a height and width)?

So far, it looks like the image displays the same with intrinsic and responsive. But you've tested this more 😄

Of course, you have thought about this:

Changing the layout to responsive ensures the image renders within the parent container in all viewports.

[hellofromtonya]

@kienstra with the intrinsic layout, we then get into the issue of wider images overflowing the parent container. But let me do a little more investigation to gather some test data for us.

[kienstra]

Thanks, @hellofromtonya!

[hellofromtonya]

@kienstra Upon review, we can allow the layout to default and be handled in the image sanitizer.

Why? The wide image overflow problem is tied to the Gutenberg image block. That image block adjusts the width on the added parent <figure>. When the layout is set to intrinsic, it sets the width to fit-content. Combining that width with the inline width that AMP sets causes the overflowing problem.

Here we do not have that issue. Therefore, we do not need to force the layout attribute.

I've removed it in commit 74dc97a.

[kienstra]

Thanks, @hellofromtonya! That commit looks good. Sorry to nitpick 😄

[hellofromtonya]

@kienstra Don't be sorry about nitpicking. We want the best solutions and high quality code. It's not nitpicking, but rather striving for excellence. 🥇

[kienstra]

Moving To 'Ready For Merging'

If it's alright, I'm moving this to 'Ready For Merging,' as I don't think it needs functional beyond the testing already done for this PR. Feel free to move it back.