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

Filter the attachment image attributes for the Twenty Seventeen theme. #1321

Merged
merged 2 commits into from Aug 7, 2018

Conversation

3 participants
@hellofromtonya
Copy link
Collaborator

hellofromtonya commented Aug 6, 2018

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:

before-stretched-150x150

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

before-stretched-1200w

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.

after-stretched-150x150

after-stretched-1200w

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.

Filter the attachment image attributes for the Twenty Seventeen attac…
…hment page.

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

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

AMP uses these attributes to render the <amp-img>.

@hellofromtonya hellofromtonya added this to the v1.0 milestone Aug 6, 2018

@hellofromtonya hellofromtonya requested review from westonruter and kienstra Aug 6, 2018

@westonruter

This comment has been minimized.

Copy link
Member

westonruter commented Aug 6, 2018

@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

This comment has been minimized.

Copy link
Collaborator

hellofromtonya commented Aug 6, 2018

@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
Copy link
Collaborator

kienstra left a comment

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:

no-regression-image-display

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

$sizes = wp_get_attachment_image_sizes( $attachment->ID, $size );
if ( false !== $sizes ) {
$attr['layout'] = 'responsive';

This comment has been minimized.

@kienstra

kienstra Aug 6, 2018

Collaborator

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.

This comment has been minimized.

@hellofromtonya

hellofromtonya Aug 7, 2018

Collaborator

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

This comment has been minimized.

@kienstra

kienstra Aug 7, 2018

Collaborator

Thanks, @hellofromtonya!

This comment has been minimized.

@hellofromtonya

hellofromtonya Aug 7, 2018

Collaborator

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

This comment has been minimized.

@kienstra

kienstra Aug 7, 2018

Collaborator

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

This comment has been minimized.

@hellofromtonya

hellofromtonya Aug 7, 2018

Collaborator

@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. 🥇

@postphotos postphotos added this to Definition in v1.0 Aug 7, 2018

@hellofromtonya hellofromtonya force-pushed the fix/attachment-page-stretched-images branch from fdfe411 to 74dc97a Aug 7, 2018

@hellofromtonya hellofromtonya merged commit 0809190 into develop Aug 7, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@hellofromtonya hellofromtonya moved this from Definition to In progress in v1.0 Aug 9, 2018

@hellofromtonya hellofromtonya moved this from In progress to Ready for review in v1.0 Aug 9, 2018

@hellofromtonya hellofromtonya moved this from Ready for review to Ready for QA in v1.0 Aug 9, 2018

@kienstra

This comment has been minimized.

Copy link
Collaborator

kienstra commented Sep 18, 2018

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.

@kienstra kienstra moved this from Ready for QA to Ready for Merging in v1.0 Sep 18, 2018

@kienstra kienstra moved this from Ready for Merging to In Production in v1.0 Dec 11, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment