Plugin Issue #864: Remove <amp-img> workaround filters.

[kienstra]

Request For Review

Hi @westonruter,
This pull request removes the workaround filters for <amp-img>. As you know, they were in place because the AMP plugin's sanitizer didn't produce the correct <amp-img>.

The sanitizer now removes the sizes attribute in Plugin PR #937. So this removes the workarounds, as they prevented the sanitizer from working.

Remaining Styling Issue
Steps to reproduce

1. Checkout the plugin branch fix/864-remove-sizes
2. Checkout this branch on the theme
3. On the home page, set a wide image as the post thumbnail of the 2nd or 3rd posts on the page
4. Expected: the thumbnail displays with the proper aspect ratio
5. Actual: the thumbnail has a compressed width:

The workaround filters seemed to be causing an inferred layout of responsive, because the <amp-img> had a width, height, and sizes.

<amp-img width="200" height="200" src="https://local.plug/wp-content/uploads/2018/02/cropped-lorempixel200200.jpeg" class="custom-logo i-amphtml-element i-amphtml-layout-responsive i-amphtml-layout-size-defined i-amphtml-layout" alt="local.plug" itemprop="logo" srcset="https://local.plug/wp-content/uploads/2018/02/cropped-lorempixel200200.jpeg 200w, https://local.plug/wp-content/uploads/2018/02/cropped-lorempixel200200-150x150.jpeg 150w" sizes="(max-width: 200px) 100vw, 200px" style="width: 200px;"><i-amphtml-sizer style="display: block; padding-top: 100%;"></i-amphtml-sizer><img decoding="async" alt="local.plug" class="i-amphtml-fill-content i-amphtml-replaced-content" src="https://local.plug/wp-content/uploads/2018/02/cropped-lorempixel200200.jpeg"></amp-img>

But on removing the filters, the inferred layout is fixed, as this class suggests: i-amphtml-layout-fixed . This can be a problem when used with this style rule in the theme:

amp-img {
	max-width: 100%;
	height: auto;
}

Because its inferred layout is fixed, it has these inline style values:

element.style {
        width: 768px;
        height: 432px;
}

So it uses the max-width: 100%;, but ignores height: auto;, in favor of the inline value of height: 432px

The plugin could force the layout to be responsive. But this could cause images to expand to fill their container, where they hadn't before.

[westonruter]

Deployed latest to https://sizesfix-ampconfdemo.pantheonsite.io/

[westonruter]

@delawski I'm not sure the best way forward but I have a feeling you might.

[delawski]

@westonruter @kienstra I've added a CSS fix based on object-fit: cover along with some position: absolute and padding-based height setting.

[westonruter]

@delawski I'm noticing Elon looks different…

master branch at https://dev-ampconfdemo.pantheonsite.io/?cb:

remove/864-amp-img-filters branch at https://sizesfix-ampconfdemo.pantheonsite.io/?cb:

In both cases the src image is 1600x900. The difference seems to be that the version in master has an <i-amphtml-sizer style="display: block; padding-top: 56.25%;"></i-amphtml-sizer> element present.

I don't know what is right.

[delawski]

@westonruter @kienstra My initial implementation was flawed. This time the images should be scaled and cropped properly since I've followed the AMP by Example instruction.

However, in order for this new implementation to work, a change is needed in the AMP plugin itself. Right now the layout attribute added to the img tag is ignored in the class-amp-img-sanitizer.php.

[westonruter]

See ampproject/amp-wp#937 (comment)