Skip to content
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

Copy object-position & object-fit into style attribute to reduce LCP #6

Closed
schlessera opened this issue Nov 3, 2020 · 1 comment · Fixed by #29
Closed

Copy object-position & object-fit into style attribute to reduce LCP #6

schlessera opened this issue Nov 3, 2020 · 1 comment · Fixed by #29
Labels
Bug Something isn't working P0 High Priority SSR Related to the serverside rendering of the Optimizer Upstream Bug Requires an upstream change from WordPress, Gutenberg, or another dependency

Comments

@schlessera
Copy link
Collaborator

This is moved over from outstanding issues on the PR in the ampproject/amp-wp repository (ampproject/amp-wp#5350 (comment)).

Via @westonruter:

They are valid on amp-img. If supplied on an img, then the AMP_Image_Sanitizer passes them through.

Nevertheless, for the purposes of SSR images, given this non-optimized input:

<amp-img src="https://amp.dev/static/img/icons/icon-512x512.png" width="512" height="250" object-fit="cover" object-position="right top"></amp-img>

The Node.js Optimizer does this for SSR:

<amp-img src="https://amp.dev/static/img/icons/icon-512x512.png" width="512" height="250" object-fit="cover" object-position="right top" i-amphtml-ssr data-hero class="i-amphtml-layout-fixed i-amphtml-layout-size-defined" style="width:512px;height:250px;" i-amphtml-layout="fixed">
	<img class="i-amphtml-fill-content i-amphtml-replaced-content" decoding="async" object-fit="cover" object-position="right top" src="https://amp.dev/static/img/icons/icon-512x512.png">
</amp-img>

So it is copying the object-fit and object-position attributes. Nevertheless, the Node.js Optimizer does not seem to be right here either. I can see that the AMP runtime is also adding an inline style to the img: object-fit: cover; object-position: right top;. This is causing some LCP. So in reality, the object-position and object-fit should instead be copied into a style attribute like so:

<amp-img src="https://amp.dev/static/img/icons/icon-512x512.png" width="512" height="250" object-fit="cover" object-position="right top" i-amphtml-ssr data-hero class="i-amphtml-layout-fixed i-amphtml-layout-size-defined" style="width:512px;height:250px;" i-amphtml-layout="fixed">
	<img class="i-amphtml-fill-content i-amphtml-replaced-content" decoding="async" style="object-fit: cover; object-position: right top;" src="https://amp.dev/static/img/icons/icon-512x512.png">
</amp-img>

This will eliminate any negative LCP once the runtime initializes.

This also needs to be fixed in the Node.js Optimizer.

See AMP Playground example.

@schlessera schlessera added the Bug Something isn't working label Nov 3, 2020
@schlessera schlessera added Upstream Bug Requires an upstream change from WordPress, Gutenberg, or another dependency SSR Related to the serverside rendering of the Optimizer P0 High Priority labels Nov 6, 2020
@schlessera
Copy link
Collaborator Author

Copying the styles over will need to take the CSS byte count limit into account. If we're going over the count, we can just omit the inline style in that case, as it will automatically be corrected by the AMP runtime (although we're forced to take the LCP hit then).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working P0 High Priority SSR Related to the serverside rendering of the Optimizer Upstream Bug Requires an upstream change from WordPress, Gutenberg, or another dependency
Projects
None yet
1 participant