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

Images on attachment page appear stretched #1237

Closed
westonruter opened this Issue Jul 2, 2018 · 9 comments

Comments

4 participants
@westonruter
Copy link
Member

westonruter commented Jul 2, 2018

In Twenty Seventeen at least, an image on the attachment template looks stretched:

image

<amp-img width="300" height="300" src="https://src.wordpress-develop.test/wp-content/uploads/2018/05/cropped-wordpress-amp-300x300.png" class="attachment-medium size-medium amp-wp-enforced-sizes" alt="" srcset="https://src.wordpress-develop.test/wp-content/uploads/2018/05/cropped-wordpress-amp-300x300.png 300w, https://src.wordpress-develop.test/wp-content/uploads/2018/05/cropped-wordpress-amp-150x150.png 150w, https://src.wordpress-develop.test/wp-content/uploads/2018/05/cropped-wordpress-amp-100x100.png 100w, https://src.wordpress-develop.test/wp-content/uploads/2018/05/cropped-wordpress-amp-270x270.png 270w, https://src.wordpress-develop.test/wp-content/uploads/2018/05/cropped-wordpress-amp-192x192.png 192w, https://src.wordpress-develop.test/wp-content/uploads/2018/05/cropped-wordpress-amp-180x180.png 180w, https://src.wordpress-develop.test/wp-content/uploads/2018/05/cropped-wordpress-amp-32x32.png 32w, https://src.wordpress-develop.test/wp-content/uploads/2018/05/cropped-wordpress-amp.png 512w" sizes="100vw" layout="intrinsic"></amp-img>

Non-AMP version:

image

<img width="300" height="300" src="https://src.wordpress-develop.test/wp-content/uploads/2018/05/cropped-wordpress-amp-300x300.png" class="attachment-medium size-medium" alt="" srcset="https://src.wordpress-develop.test/wp-content/uploads/2018/05/cropped-wordpress-amp-300x300.png 300w, https://src.wordpress-develop.test/wp-content/uploads/2018/05/cropped-wordpress-amp-150x150.png 150w, https://src.wordpress-develop.test/wp-content/uploads/2018/05/cropped-wordpress-amp-100x100.png 100w, https://src.wordpress-develop.test/wp-content/uploads/2018/05/cropped-wordpress-amp-270x270.png 270w, https://src.wordpress-develop.test/wp-content/uploads/2018/05/cropped-wordpress-amp-192x192.png 192w, https://src.wordpress-develop.test/wp-content/uploads/2018/05/cropped-wordpress-amp-180x180.png 180w, https://src.wordpress-develop.test/wp-content/uploads/2018/05/cropped-wordpress-amp-32x32.png 32w, https://src.wordpress-develop.test/wp-content/uploads/2018/05/cropped-wordpress-amp.png 512w" sizes="100vw" />

@westonruter westonruter added this to the v1.0 milestone Jul 2, 2018

@postphotos postphotos added the release label Jul 9, 2018

@postphotos postphotos added this to Definition in v1.0 Jul 9, 2018

@westonruter westonruter moved this from Definition to To do in v1.0 Jul 9, 2018

@kienstra kienstra self-assigned this Jul 23, 2018

@kienstra

This comment has been minimized.

Copy link
Collaborator

kienstra commented Jul 24, 2018

Seems Related To Inline Styling
But That Might Be Expected

This looks to only exist when using the sizes attribute, like in Twenty Seventeen.

It looks like the <amp-img> adds an inline style of the same width as the sizes attribute:

https://paired-ampconfdemo.pantheonsite.io/image-alignment-150x150/?amp
sizes-attribute

<amp-img ... sizes="100vw" layout="intrinsic" style="width: 100vw;">

In the example above, the sizes and style="width: values are both 100vw. When I tried blocking the sizes via the plugin's sanitizer, this issue didn't exist. Though I'm not suggesting that as a solution.

Still, maybe the inline styling of width: 100vw; is expected.

@hellofromtonya

This comment has been minimized.

Copy link
Collaborator

hellofromtonya commented Jul 24, 2018

Bringing @kienstra and my conversation here.

The inline width styling is applied by AMP (not our plugin) from the sizes attribute. WordPress provides both the srcset and sizes while exposing a filter 'wp_calculate_image_sizes' for theme/plugin developers to customize the sizes.

Here's what AMP Docs say about the attributes:

srcset
Same as srcset attribute on the img tag. For browsers that do not support srcset, <amp-img> will default to using src. If only srcset and no src is provided, the first url in the srcset will be selected.
sizes
Same as sizes attribute on the img tag.

Reading deeper into the AMP's docs about sizes:

sizes
You can also use the sizes attribute along with srcset. The sizes attribute describes how to calculate the element size based on any media expression. Based on the element’s calculated size, the user agent selects the most relative source supplied by the srcset attribute.

The sizes attribute defines the element’s width to be 50% the size of the viewport when the viewport is 650px or more. For example, if the viewport is 800px, the element’s width is set to 400px. The browser then selects the srcset resource relative to 400px, assuming the device pixel ratio is 1, which in this instance is hummingbird-narrow.jpg (320px).

When sizes attribute is specified along with width and height, layout defaults to responsive.

The width of the image then is set by the value in the sizes attribute as @kienstra notes above. AMP uses it to add the inline width style.

Compare AMP to non-AMP versions

Let's compare the difference between AMP and non-AMP attachments to see if we can find the solution:

Here is the non-AMP HTML:

<p class="attachment">
    <a href="https://paired-ampconfdemo.pantheonsite.io/wp-content/uploads/2018/07/image-alignment-150x150.jpg">
        <img width="150" height="150" src="https://paired-ampconfdemo.pantheonsite.io/wp-content/uploads/2018/07/image-alignment-150x150.jpg" class="attachment-medium size-medium" alt="Image 150x150" srcset="https://paired-ampconfdemo.pantheonsite.io/wp-content/uploads/2018/07/image-alignment-150x150.jpg 150w, https://paired-ampconfdemo.pantheonsite.io/wp-content/uploads/2018/07/image-alignment-150x150-100x100.jpg 100w" sizes="100vw">
    </a>
</p>

Here is the AMP version of that attachment:

<p class="attachment">
    <a href="https://paired-ampconfdemo.pantheonsite.io/wp-content/uploads/2018/07/image-alignment-150x150.jpg">
        <amp-img width="150" height="150" src="https://paired-ampconfdemo.pantheonsite.io/wp-content/uploads/2018/07/image-alignment-150x150.jpg" class="attachment-medium size-medium amp-wp-enforced-sizes i-amphtml-element i-amphtml-layout-intrinsic i-amphtml-layout-size-defined i-amphtml-layout" alt="Image 150x150" srcset="https://paired-ampconfdemo.pantheonsite.io/wp-content/uploads/2018/07/image-alignment-150x150.jpg 150w, https://paired-ampconfdemo.pantheonsite.io/wp-content/uploads/2018/07/image-alignment-150x150-100x100.jpg 100w" sizes="100vw" layout="intrinsic" style="width: 100vw;">..</amp-img>
    </a>
</p>

The differences include:

  • <amp-img>
  • Extra class attributes that are added for <amp-img>
  • The inline style style="width: 100vw;" that AMP adds

Let's look at the CSS rules added by those extra class attributes:

element.style {
    width: 100vw;
}

.i-amphtml-layout-responsive, [layout=responsive][width][height]:not(.i-amphtml-layout-responsive), [width][height][sizes]:not(.i-amphtml-layout-responsive) {
    display: block;
    position: relative;
}

.i-amphtml-layout-size-defined {
    overflow: hidden!important;
}

.i-amphtml-layout-intrinsic {
    display: inline-block;
    position: relative;
    max-width: 100%;
}

Possible Solutions

@kienstra notes some possible solutions above.

Option A: Remove the inline width style.

Removing the inline width style also requires us to set the display to inline-block. While this can work, we'll get into timing issues between when AMP applies the inline style and then we remove it. It could cause flashes on the screen.

Option B: Target the width of the parent container.

AMP's Tips & Tricks section suggests setting the parent container's max-width. Attachments use p.attachment. We could target that and set a max-width equal to the size of the image, i.e. grab the width attribute of the image and use it.

This possible solution would ensure the image does not stretch.

@westonruter

This comment has been minimized.

Copy link
Member

westonruter commented Jul 24, 2018

@kienstra @hellofromtonya I'm sure you've seen this already, but just to make clear that the source of the problem is Twenty Seventeen's wp_get_attachment_image_attributes filter:

https://github.com/WordPress/wordpress-develop/blob/ddc8f803c6e99118998191fd2ea24124feb53659/src/wp-content/themes/twentyseventeen/functions.php#L534-L554

That being the case, the solution chosen could perhaps be made part of the AMP_Core_Theme_Sanitizer as a specific fix for Twenty Seventeen.

It could be that the issue here is a fundamental incompatibility between img and amp-img. In particular, if you set object-fit:contain on img it works, whereas it does not work with amp-img. This is the same problem we faced with getting the header image to work in Twenty Seventeen (see #1074), and the solution for that was to add CSS that targets the img inside of the amp-img:

https://github.com/Automattic/amp-wp/blob/e3d036c2db60b215636a2b665414b0e599e9ad97/includes/sanitizers/class-amp-core-theme-sanitizer.php#L498-L514

So a similar approach may be needed here. In fact, the existing add_twentyseventeen_masthead_styles method there could be renamed add_twentyseventeen_styles and this attachment-specific style rule could be put there potentially.

@kienstra

This comment has been minimized.

Copy link
Collaborator

kienstra commented Jul 25, 2018

Hi @westonruter,
Thanks, good point that this is coming from the wp_get_attachment_image_attributes filter.

Could we wait a little on this issue, to see what happens with amphtml/17053? There's discussion there about possibly changing the <amp-img> behavior for sizes.

@westonruter

This comment has been minimized.

Copy link
Member

westonruter commented Jul 25, 2018

Excellent. I'm glad you raised that.

@hellofromtonya hellofromtonya moved this from To do to Definition in v1.0 Aug 6, 2018

hellofromtonya added a commit that referenced this issue Aug 6, 2018

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.  The attachment image's width

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 added a commit that referenced this issue Aug 6, 2018

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

This comment has been minimized.

Copy link
Collaborator

hellofromtonya commented Aug 6, 2018

@westonruter and @kienstra Upon further investigation, we can remedy the Twenty Seventeen theme's image stretching issue by filtering the attachment image's attributes and changing the following:

  1. Set the layout attribute to responsive.
  2. Set the sizes attribute to the returned sizes from wp_get_attachment_image_sizes().

IMO these attribute fixes are not a bandaid but rather a fix for how the theme is setting the sizes for attachment pages.

@hellofromtonya

This comment has been minimized.

Copy link
Collaborator

hellofromtonya commented Aug 6, 2018

For reference, PR #1321 fixes this specific issue. However, since we have an open issue with the AMPHTML project, let's keep this issue open until that one is resolved. Once resolved, we can come back and validate this issue to ensure there are no regression problems.

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

@kienstra

This comment has been minimized.

Copy link
Collaborator

kienstra commented Sep 24, 2018

Moving To 'Ready For Merging'

It looks like the Issue for the AMP project probably won't be fixed by this plugin's stable 1.0 release.

So I'm moving this to 'Ready For Merging' if that's alright, as this already has a merged PR that fixes the issue.

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

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

@kienstra

This comment has been minimized.

Copy link
Collaborator

kienstra commented Dec 4, 2018

Moving Out Of Milestone

As this is only tracking ampproject/amphtml#17053, I'm moving this out of the v1.0 milestone. There's no action needed, other than remove this issue's workaround if that issue is ever resolved.

@kienstra kienstra removed this from the v1.0 milestone Dec 4, 2018

@kienstra kienstra closed this Dec 4, 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