Inherit object-fit and object-position styles so more conversions work out of the box #5955

westonruter · 2021-03-07T05:41:35Z

Summary

This is a~~n experimental~~ follow-up on #5927. We can fix issues with object-fit/object-position for the Cover Block and for any other amp-img/amp-video that gets converted from img/video if we merely inherit the properties.

Reduce specificity for style rule that applies object-fit:contain on images converted by the plugin. This is necessary for images which get a 100% width in content to prevent them from getting stretched or cropped. For example:

<p><img src="https://amp.dev/static/img/icons/icon-72x72.png"></p>

Non-AMP	AMP

(Note the image does appear to be centered in AMP, but this is an existing issue: #4735.)

Make sure that object-fit, object-position, and image-resolution are inherited from an AMP component wrapper to the contained shadow element and noscript fallback element. This allows us to entirely remove the ampify_cover_block logic.
Eliminate obsolete CSS from core theme sanitizer:
- Twenty Seventeen: 175,662 - 173,835 = 1,827 bytes saved (~1% before tree shaking).
- Twenty Nineteen: 308,611 - 307,721 = 890 bytes saved (~1% before tree shaking).
Omit attributes from noscript fallback elements which are likely to cause styling problems. Fixes Ensure noscript fallbacks get expected styling #6028.

QA

Header image appears the same before/after in Twenty Nineteen.
Header image and header video appear the same before/after in Twenty Seventeen.
Cover block with image or video and assigned focal point appears the same as non-AMP.
Image with object-fit, object-position, and image-rendering styles apply on the amp-img > img and amp-img > noscript > img, both with JavaScript enabled and disabled. For example, given a Custom HTML block with this markup:

<img 
    src="https://amp.dev/static/img/icons/icon-72x72.png"
    style="
        width: 300px;
        height: 100px;
        object-fit: cover;
        object-position: 100% 100%;
        image-rendering: pixelated;
    "
>

Verify that noscript fallback elements are styled as expected. For example, a Custom HTML block with:

<div style="outline:solid 1px black;">
<style>
.auto-height {
    object-fit: contain;
    object-position: 50% 50%;
    margin: auto;
    height: auto;
    -webkit-filter: grayscale(50%);
    filter: grayscale(50%); 
}
</style>
<img class="auto-height" src="https://amp.dev/static/img/icons/icon-128x128.png">
</div>

Non-AMP and AMP sans JS w/ fix 👍	AMP Before with JS disabled 👎

Checklist

My code is tested and passes existing tests.
My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

jwold · 2021-04-01T16:44:03Z

WR > May allow us to delete code, the Twenty Seventeen theme has a header image; in order to get header image styling set properly, where header image needs object fit contain. CSS from core theme doesn't work because amp-image has image that's a child, so it doesn't properly set object fit style property.
WR > With this PR we might delete that code and it will just work for other themes that are using object fit on images; and it will inherit (in theory). That should make sure that themes just work more often.
WR > Then again, amp-images will be deprecated soon, then we can delete that code again.

…ndant with object-fit=contain attr

…nts and noscript fallbacks

westonruter · 2021-04-01T19:51:00Z

assets/css/src/amp-default.css

- * Additionally, in side of \AMP_Img_Sanitizer::determine_dimensions() it could $amp_img->setAttribute( 'object-fit', 'contain' )
- * so that the following rules wouldn't be needed.


This was previously implemented in #2793 via 5b024ec.

codecov · 2021-04-01T19:54:51Z

Codecov Report

Merging #5955 (ed1c930) into develop (2d0de18) will increase coverage by 0.12%.
The diff coverage is 100.00%.

@@              Coverage Diff              @@
##             develop    #5955      +/-   ##
=============================================
+ Coverage      75.27%   75.39%   +0.12%     
+ Complexity      5709     5700       -9     
=============================================
  Files            218      218              
  Lines          17283    17255      -28     
=============================================
  Hits           13009    13009              
+ Misses          4274     4246      -28

Flag	Coverage Δ	Complexity Δ
javascript	`80.05% <ø> (ø)`	`0.00 <ø> (ø)`
php	`75.19% <100.00%> (+0.12%)`	`0.00 <0.00> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ	Complexity Δ
includes/embeds/class-amp-core-block-handler.php	`87.38% <ø> (-3.60%)`	`43.00 <0.00> (-8.00)`
...udes/sanitizers/class-amp-core-theme-sanitizer.php	`36.28% <ø> (+0.41%)`	`228.00 <0.00> (-1.00)`	⬆️
src/Transformer/DetermineHeroImages.php	`91.04% <ø> (ø)`	`25.00 <0.00> (ø)`
...ncludes/sanitizers/trait-amp-noscript-fallback.php	`100.00% <100.00%> (+100.00%)`	`10.00 <0.00> (ø)`

github-actions · 2021-04-01T23:46:56Z

Plugin builds for ed1c930 are ready 🛎️!

Download development build
Download production build

westonruter · 2021-04-02T20:30:52Z

assets/css/src/amp-default.css

 */
-amp-img.amp-wp-enforced-sizes[layout="intrinsic"] > img,
-amp-anim.amp-wp-enforced-sizes[layout="intrinsic"] > img {
+.amp-wp-enforced-sizes {


There is a chance that this may have too much specificity and override user styles, although this is very unlikely.

There is a new hack to implement low specificity selector as seen here:

https://github.com/ampproject/amphtml/blob/ec3090a369633ea930ac8d85e211c8bd2451cd53/extensions/amp-accordion/1.0/amp-accordion.css#L41-L53

Example: https://codepen.io/westonruter/pen/JjENYGK

This being the case, this selector could actually be changed to:

Suggested change

.amp-wp-enforced-sizes {

:where(.amp-wp-enforced-sizes) {

And this would ensure that if anyone has a bare img { object-fit:cover } it won't get overridden by the default style here.

Nevertheless, :where is not supported yet by all browsers: https://caniuse.com/mdn-css_selectors_where

It is very unlikely that someone is going to want to style all images with object-fit, and the examples of using object-fit in core all make use of selectors that have much more specificity than 0.1.0:

Selector Specificity

.has-header-image .custom-header-media img 0.2.1

.has-header-video .custom-header-media video 0.2.1

.has-header-image:not(.twentyseventeen-front-page):not(.home) .custom-header-media img 0.4.1

.site-featured-image .post-thumbnail img 0.2.1

.site-header.featured-image .site-featured-image .post-thumbnail img 0.4.1

Although unlikely, I could see someone using something like header > img { object-fit:cover }, which would have a specificity of 0.0.2.

Unfortunately @supports selector is not widely supported by all browsers either so we cannot rely on that.

pierlon · 2021-04-06T10:03:42Z

includes/sanitizers/trait-amp-noscript-fallback.php

+		// Remove attributes which are likely to cause styling conflicts, as the noscript fallback should get treated like it has fill layout.
+		unset(
+			$this->noscript_fallback_allowed_attributes[ Attribute::ID ],
+			$this->noscript_fallback_allowed_attributes[ Attribute::CLASS_ ],


Removing the class attribute has inadvertently caused noscript>img elements of custom logos to be detected as hero images, and because of that the AMP page fails to render due to a PHP fatal error:

[06-Apr-2021 08:46:38 UTC] PHP Warning: Couldn't fetch AmpProject\Dom\Element. Node no longer exists in /app/public/content/plugins/amp/vendor/ampproject/amp-toolbox/src/Dom/ElementDump.php on line 55 [06-Apr-2021 08:46:38 UTC] PHP Notice: Undefined property: AmpProject\Dom\Element::attributes in /app/public/content/plugins/amp/vendor/ampproject/amp-toolbox/src/Dom/Element.php on line 100 [06-Apr-2021 08:46:38 UTC] PHP Fatal error: Method AmpProject\Dom\ElementDump::__toString() must not throw an exception, caught TypeError: Argument 1 passed to iterator_to_array() must implement interface Traversable, null given in /app/public/content/plugins/amp/vendor/ampproject/amp-toolbox/src/Optimizer/Error/CannotPreloadImage.php on line 0 [06-Apr-2021 08:46:38 UTC] PHP Fatal error: Unknown: Cannot use output buffering in output buffering display handlers in Unknown on line 0 [06-Apr-2021 08:49:25 UTC] PHP Warning: Couldn't fetch AmpProject\Dom\Element. Node no longer exists in /app/public/content/plugins/amp/vendor/ampproject/amp-toolbox/src/Dom/ElementDump.php on line 55 [06-Apr-2021 08:49:25 UTC] PHP Notice: Undefined property: AmpProject\Dom\Element::attributes in /app/public/content/plugins/amp/vendor/ampproject/amp-toolbox/src/Dom/Element.php on line 100 [06-Apr-2021 08:49:25 UTC] PHP Fatal error: Method AmpProject\Dom\ElementDump::__toString() must not throw an exception, caught TypeError: Argument 1 passed to iterator_to_array() must implement interface Traversable, null given in /app/public/content/plugins/amp/vendor/ampproject/amp-toolbox/src/Optimizer/Error/CannotPreloadImage.php on line 0 [06-Apr-2021 08:49:25 UTC] PHP Fatal error: Unknown: Cannot use output buffering in output buffering display handlers in Unknown on line 0

Further investigation led to the query AmpProject\AmpWP\Transformer\DetermineHeroImages::CUSTOM_HEADER_XPATH_QUERY incorrectly detecting the noscript>img due to the subquery not( contains( concat( ' ', normalize-space( @class ), ' ' ), ' custom-logo ' ) ) now being true as the custom-logo class is no longer being copied.

Fatal error prevented with fb0928f but I'm still not entirely clear why the error occurred in the first place.

In any case, the change in fb0928f is key because we never want to select a noscript > img for SSR.

Actually, it doesn't even make sense to select any img now that I think of it, because all of the sanitizers will have already run and so there should be no img left in the page that we need to SSR.

Done in c72d487.

Fatal error prevented with fb0928f but I'm still not entirely clear why the error occurred in the first place.

The error occurred because the noscript>img was receiving the data-hero-candidate attribute, and AmpProject\Optimizer\Transformer::detectImageWithAttribute() detects it as a valid candidate because when it retrieves the CSS background image URL for the image (which resolves to an empty string '') and parses it as a URL (which becomes the valid URL path /), it is returned as a hero image for preloading, which should not be the case.

I understand why the error was happening: PreloadHeroImage::findHeroImages() was including the noscript>img for an amp-img that was going to be prerendered. So it included both the amp-img and the amp-img > noscript > img as the hero images. When prerendering occurred, then the latter got removed and so it was no longer in the document. And then the exception ensued.

This should all be sorted now.

pierlon

Really great work done here, LGTM!

delawski · 2021-04-23T17:59:41Z

QA passed.

My environment setup:

"Before" AMP version: 2.0.11
"After" AMP version: dev build: 2.1.0-beta1-20210406T172354Z-aafc656
WordPress version in both cases: 5.7.1
No other plugins installed

Compare various use cases with JS disabled:

Before	After

The last block was created using the snippet provided in the PR description:

<div style="outline:solid 1px black;">
<style>
.auto-height {
    object-fit: contain;
    object-position: 50% 50%;
    margin: auto;
    height: auto;
    -webkit-filter: grayscale(50%);
    filter: grayscale(50%); 
}
</style>
<img class="auto-height" src="...">
</div>

In the "Before" state, notice the cover image block distortion (focal point set to bottom right corner) and the grayscale filter applied twice to the AMP logo (the last block).

Compare the same blocks with JS enabled:

Before	After

In the "Before" state, notice that the middle block (a pixelated AMP banner) doesn't honor the object-fit: cover rule.

The middle block is a Custom HTML block with the markup provided in the PR description:

<img 
    src="https://amp.dev/static/img/icons/icon-72x72.png"
    style="
        width: 300px;
        height: 100px;
        object-fit: cover;
        object-position: 100% 100%;
        image-rendering: pixelated;
    "
>

I can also confirm that the header images are rendered in the same way before and after in Twenty Nineteen and in Twenty Seventeen:

Before	After

delawski · 2021-04-23T18:27:29Z

Since I've been testing on the beta1 and not the latest beta2 build, I've redone the "After" state QA. Everything is still working as expected.

AMP plugin version: 2.1.0-beta2-20210423T172922Z-cf7b2b3

JS disabled	JS enabled

Header image - Twenty Seventeen	Header image - Twenty Nineteen

…#5955

westonruter added the CSS label Mar 7, 2021

westonruter added this to the v2.1 milestone Mar 7, 2021

westonruter self-assigned this Mar 7, 2021

Base automatically changed from fix/cover-block to develop March 8, 2021 07:19

WIP: Inherit object-fit and object-position styles

e579216

westonruter force-pushed the add/object-fit-position-inheritance branch from 2f28231 to e579216 Compare April 1, 2021 18:44

westonruter added 4 commits April 1, 2021 12:06

Restrict object-fit to images which have the .amp-wp-unknown-size class

419ade0

Remove obsolete object-fit:contain for unknown-sized image since redu…

f53bcb0

…ndant with object-fit=contain attr

Inherit replaced-content properties for replaced content shadow eleme…

a5fcafc

…nts and noscript fallbacks

Remove obsolete core sanitizer CSS code from 2019, 2017, and 2014

c280227

westonruter force-pushed the add/object-fit-position-inheritance branch from be2f77d to c280227 Compare April 1, 2021 19:48

westonruter commented Apr 1, 2021

View reviewed changes

westonruter added 4 commits April 1, 2021 13:25

Fix double arrow alignment

8611e57

Fix inheritance of replaced-content properties on noscript fallbacks

c914957

Omit problematic styling attributes from noscript fallback elements

7c6c4e4

Restore object-fit:contain for a reduced-specificity selector

62842d4

westonruter changed the title ~~WIP: Inherit object-fit and object-position styles~~ Inherit object-fit and object-position styles so more conversions work out of the box Apr 1, 2021

westonruter marked this pull request as ready for review April 1, 2021 23:41

westonruter requested a review from pierlon April 1, 2021 23:41

westonruter added 2 commits April 1, 2021 23:17

Fix tests and optimize attribute removal

ab6f2b1

Add covers phpdoc tags

06de9df

westonruter commented Apr 2, 2021

View reviewed changes

pierlon suggested changes Apr 6, 2021

View reviewed changes

westonruter added 2 commits April 6, 2021 08:31

Avoid identifying noscript fallback img as hero image candidates

fb0928f

Prevent DetermineHeroImages transformer from targeting any img element

c72d487

Update DetermineHeroImagesTest to use amp-img instead of img

ed1c930

westonruter requested a review from pierlon April 6, 2021 17:21

pierlon reviewed Apr 7, 2021

View reviewed changes

pierlon approved these changes Apr 7, 2021

View reviewed changes

westonruter merged commit e21a628 into develop Apr 7, 2021

westonruter deleted the add/object-fit-position-inheritance branch April 7, 2021 05:56

delawski assigned delawski and unassigned westonruter Apr 23, 2021

delawski removed their assignment Apr 23, 2021

westonruter added a commit that referenced this pull request Aug 6, 2021

Remove object-fit styling for amp-img>img made obsolete by #5955

5f2eff7

pierlon pushed a commit to pierlon/amp-wp that referenced this pull request Aug 15, 2021

Remove object-fit styling for amp-img>img made obsolete by ampproject…

cf72bdd

…#5955

westonruter mentioned this pull request Nov 30, 2021

PHP error thrown in Customizer when using object-fit="cover" attribute and styles #6748

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Inherit object-fit and object-position styles so more conversions work out of the box #5955

Inherit object-fit and object-position styles so more conversions work out of the box #5955

westonruter commented Mar 7, 2021 •

edited

Loading

jwold commented Apr 1, 2021

westonruter Apr 1, 2021 •

edited

Loading

codecov bot commented Apr 1, 2021 •

edited

Loading

github-actions bot commented Apr 1, 2021 •

edited

Loading

westonruter Apr 2, 2021

pierlon Apr 6, 2021

pierlon Apr 6, 2021

westonruter Apr 6, 2021

westonruter Apr 6, 2021

westonruter Apr 6, 2021

westonruter Apr 6, 2021

pierlon Apr 6, 2021

westonruter Apr 6, 2021

pierlon left a comment

delawski commented Apr 23, 2021 •

edited

Loading

delawski commented Apr 23, 2021 •

edited

Loading

		* Additionally, in side of \AMP_Img_Sanitizer::determine_dimensions() it could $amp_img->setAttribute( 'object-fit', 'contain' )
		* so that the following rules wouldn't be needed.

Selector	Specificity
`.has-header-image .custom-header-media img`	0.2.1
`.has-header-video .custom-header-media video`	0.2.1
`.has-header-image:not(.twentyseventeen-front-page):not(.home) .custom-header-media img`	0.4.1
`.site-featured-image .post-thumbnail img`	0.2.1
`.site-header.featured-image .site-featured-image .post-thumbnail img`	0.4.1

Inherit object-fit and object-position styles so more conversions work out of the box #5955

Inherit object-fit and object-position styles so more conversions work out of the box #5955

Conversation

westonruter commented Mar 7, 2021 • edited Loading

Summary

QA

Checklist

jwold commented Apr 1, 2021

westonruter Apr 1, 2021 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Apr 1, 2021 • edited Loading

Codecov Report

github-actions bot commented Apr 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pierlon left a comment

Choose a reason for hiding this comment

delawski commented Apr 23, 2021 • edited Loading

delawski commented Apr 23, 2021 • edited Loading

westonruter commented Mar 7, 2021 •

edited

Loading

westonruter Apr 1, 2021 •

edited

Loading

codecov bot commented Apr 1, 2021 •

edited

Loading

github-actions bot commented Apr 1, 2021 •

edited

Loading

delawski commented Apr 23, 2021 •

edited

Loading

delawski commented Apr 23, 2021 •

edited

Loading