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

Intent-to-Deprecate: amp-img #30442

Closed
rbeckthomas opened this issue Sep 29, 2020 · 55 comments
Closed

Intent-to-Deprecate: amp-img #30442

rbeckthomas opened this issue Sep 29, 2020 · 55 comments
Labels
INTENT TO DEPRECATE Proposes deprecating an existing AMP feature. Stale Inactive for one year or more

Comments

@rbeckthomas
Copy link
Contributor

rbeckthomas commented Sep 29, 2020

Summary

We would like to allow developers who write AMP to be able to use the native html img tag, use transforms to change existing amp-img tags to native img tags and after sufficient time remove amp-img from v0 and make it an optional component.

Motivation

The amp-img component was primarily created in order to allow for lazy-loading of images in amp pages in order to improve page performance. At the time the native html img tag did not allow for this which necessitated the creation of a custom extension.

Today however, the native img tag does allow for lazy loading through the use of the loading attribute. As a result the amp-img extension is no longer needed to achieve the performance improvements for which it was originally created.
Because of this the amp-img extension will need to be constantly updated in order to maintain feature parity with the native html img tag or risk obsolescence.

In the long term we would like to turn down the use of amp-img in new amp documents, instead allowing developers to use the native html img tag with the loading attribute set to lazy in order to achieve these results. This would remove the burden on amp contributors to constantly maintain feature parity and will additionally allow developers to create simple documents without needing to learn the usage of an extra amp extension.

Impact on existing users

This change will allow users to use native img in future development.

Additionally existing AMP documents which are run through transformers will be modified to replace all existing amp-img tags with img tags.

This will cause the following changes to the amp-img features:
Loading placeholder will no longer exist
The blurry image placeholder will need to be implemented with a background image.
The sizes attribute will no longer be calculated automatically.
height and width attributes will be required
The loading attribute will be required and set to lazy. (not supported on IE)

In order to achieve 1:1 visual parity the native images will also require display:block and margin:0 which are both automatically applied by amp-img css.

Examples:
Sizes/Aspect-ratio - https://glitch.com/edit/#!/amp-img-to-img
Blurry Image placeholder - https://glitch.com/edit/#!/amp-img-blurry-placeholder?path=amp.html%3A77%3A21

Alternative implementation suggestion for developers using AMP

Developers will now be able to use native img instead of amp-img.
We will offer developers the following guidance in order to achieve similar functionality that is offered to amp-img
Use loading= “lazy”
The height and width attributes will respect the actual size of the image, this will cause CLS if they are incorrect therefore for the best experience make sure you use the correct width and height.
Use display: block and margin: 0 which amp automatically applies.
When using srcset you must use sizes in addition in order to verify that the appropriately sized image is selected.

Deprecation Plan

Step 1: Allow developers to use native img in valid amp pages.
Step 2: Use transformers to change amp-img to native img
Step 3: Wait
Step 4: Remove amp-img from the module runtime, because all documents in this runtime have passed through a transformer we are guaranteed that they will only have native img
Step 5: Either remove amp-img entirely, or remove amp-img from v0 and convert it into a regular AMP extension.

/cc @ampproject/wg-approvers

@rbeckthomas rbeckthomas added the INTENT TO DEPRECATE Proposes deprecating an existing AMP feature. label Sep 29, 2020
@pbakaus
Copy link
Contributor

pbakaus commented Oct 1, 2020

First off, amazing!

What about other replacement tags, like amp-audio and amp-video? I know things are somewhat different there, so curious if that is in scope.

@westonruter
Copy link
Member

Isn't there an existing issue for this? See #29786

@kristoferbaxter
Copy link
Contributor

kristoferbaxter commented Oct 1, 2020

There is an existing issue @westonruter.

The other issue was closed and moved here as the original proposal has changed.

Edit: I was mistaken, but have closed the other issue now as this updated proposal is taking feedback from the last conversation into account.

@kristoferbaxter
Copy link
Contributor

@pbakaus This is the first attempt at moving away from custom elements for features that have been implemented natively at the element level.

Those items you mention are worth looking into as well, but there is more custom behavior present that might delay migration and/or deprecation.

@cramforce
Copy link
Member

I think the main thing that makes amp-img special is that images are cached and hence the privacy aspects of the AMP custom elements aren't needed here. This wouldn't be true (generally) for other elements that loads resources like video

@gmajoulet
Copy link
Contributor

We will need to update some code in amp-story and amp-story-360 to support this.

amp-story:

  • amp-story is not scroll based and relies on the AMP lazy loading strategy for amp-img. It positions amp-img it does not want loaded X viewports away, and brings them closer from the visible viewport to trigger their load.
  • amp-story relies on the amp-img tag to monitor the image load and trigger events or even render itself, we should update these
  • amp-story needs to update its validator rules
  • Tracked here: [Story] Support native img tag with loading=lazy attribute #30474

amp-story-360:

cc @ampproject/wg-stories

@zhouyx
Copy link
Contributor

zhouyx commented Oct 2, 2020

To echo @gmajoulet point. I believe we also need to update some code in AMP components that handles layout of its children (e.g. amp-carousel)

Based on what I found image lazy load only applies to element below viewport.
https://web.dev/browser-level-image-lazy-loading/#how-does-the-loading-attribute-work-with-images-that-are-in-the-viewport-but-not-immediately-visible-(for-example:-behind-a-carousel-or-hidden-by-css-for-certain-screen-sizes)

@Gregable
Copy link
Member

@rebeccanthomas

The height and width attributes will respect the actual size of the image, this will cause CLS if they are incorrect therefore for the best experience make sure you use the correct width and height.

If I understand this correctly, this represents a significant departure from amp-img. My understanding here is that the height & width in img only inform initial layout, not enforce final layout. With amp-img, the initial dimension layout is fixed, regardless of what file-specified dimensions the image eventually has.

Is there anything available here that would allow us to enforce this layout? Adding image dimensions has been a burden for many publishers, and I can see them being happy to just set <img width=1 height=1 ...> to satisfy validation, but the image still causing CLS issues when it eventually loads.

If we are comfortable about losing this enforcement, I recommend we do not require width/height in validation.

@dvoytenko
Copy link
Contributor

IMHO we should still require width and height for validation. They are intrinsic data about the image and any recommendation about the layout would be based on these values. The final layout confirmation will have to be left to CLS.

@Gregable
Copy link
Member

@dvoytenko Do you see anything preventing this rule from just turning into teams setting width=1 height=1 or whatever constants until the file loads? If the validator isn't preventing the poor loading behavior, I would not feel comfortable enforcing the property presence. Let Web Vitals do that.

@kristoferbaxter
Copy link
Contributor

https://playground.amp.dev/#share=PCFkb2N0eXBlIGh0bWw+CjxodG1sIOKaoT4KPGhlYWQ+CiAgPG1ldGEgY2hhcnNldD0idXRmLTgiPgogIDx0aXRsZT5NeSBBTVAgUGFnZTwvdGl0bGU+CiAgPGxpbmsgcmVsPSJjYW5vbmljYWwiIGhyZWY9InNlbGYuaHRtbCIgLz4KICA8bWV0YSBuYW1lPSJ2aWV3cG9ydCIgY29udGVudD0id2lkdGg9ZGV2aWNlLXdpZHRoLG1pbmltdW0tc2NhbGU9MSxpbml0aWFsLXNjYWxlPTEiPgogIDxzdHlsZSBhbXAtYm9pbGVycGxhdGU+Ym9keXstd2Via2l0LWFuaW1hdGlvbjotYW1wLXN0YXJ0IDhzIHN0ZXBzKDEsZW5kKSAwcyAxIG5vcm1hbCBib3RoOy1tb3otYW5pbWF0aW9uOi1hbXAtc3RhcnQgOHMgc3RlcHMoMSxlbmQpIDBzIDEgbm9ybWFsIGJvdGg7LW1zLWFuaW1hdGlvbjotYW1wLXN0YXJ0IDhzIHN0ZXBzKDEsZW5kKSAwcyAxIG5vcm1hbCBib3RoO2FuaW1hdGlvbjotYW1wLXN0YXJ0IDhzIHN0ZXBzKDEsZW5kKSAwcyAxIG5vcm1hbCBib3RofUAtd2Via2l0LWtleWZyYW1lcyAtYW1wLXN0YXJ0e2Zyb217dmlzaWJpbGl0eTpoaWRkZW59dG97dmlzaWJpbGl0eTp2aXNpYmxlfX1ALW1vei1rZXlmcmFtZXMgLWFtcC1zdGFydHtmcm9te3Zpc2liaWxpdHk6aGlkZGVufXRve3Zpc2liaWxpdHk6dmlzaWJsZX19QC1tcy1rZXlmcmFtZXMgLWFtcC1zdGFydHtmcm9te3Zpc2liaWxpdHk6aGlkZGVufXRve3Zpc2liaWxpdHk6dmlzaWJsZX19QC1vLWtleWZyYW1lcyAtYW1wLXN0YXJ0e2Zyb217dmlzaWJpbGl0eTpoaWRkZW59dG97dmlzaWJpbGl0eTp2aXNpYmxlfX1Aa2V5ZnJhbWVzIC1hbXAtc3RhcnR7ZnJvbXt2aXNpYmlsaXR5OmhpZGRlbn10b3t2aXNpYmlsaXR5OnZpc2libGV9fTwvc3R5bGU+PG5vc2NyaXB0PjxzdHlsZSBhbXAtYm9pbGVycGxhdGU+Ym9keXstd2Via2l0LWFuaW1hdGlvbjpub25lOy1tb3otYW5pbWF0aW9uOm5vbmU7LW1zLWFuaW1hdGlvbjpub25lO2FuaW1hdGlvbjpub25lfTwvc3R5bGU+PC9ub3NjcmlwdD4KICA8c2NyaXB0IGFzeW5jIHNyYz0iaHR0cHM6Ly9jZG4uYW1wcHJvamVjdC5vcmcvdjAuanMiPjwvc2NyaXB0PgogIDxzdHlsZSBhbXAtY3VzdG9tPgogICAgaDEgewogICAgICBtYXJnaW46IDFyZW07CiAgICB9CiAgPC9zdHlsZT4KPC9oZWFkPgo8Ym9keT4KICA8aDE+SGVsbG8gQU1QSFRNTCBXb3JsZCE8L2gxPgogIDxhbXAtaW1nIHdpZHRoPSIxIiBoZWlnaHQ9IjEiIHNyYz0iaHR0cHM6Ly9hbXAuZGV2L3N0YXRpYy9pbWcvY2FzZS1iYW5kLWltYWdlLTEucG5nIj48L2FtcC1pbWc+CjwvYm9keT4KPC9odG1sPgo=

This is true today, one can use any value for width and height to get the validator to pass.

Is the concern that for web developers they will not see an issue besides the impact to Web Vitals? If this is the case, does it make sense to recommend using Pixi more within the Validator?

@dvoytenko
Copy link
Contributor

It's possible we change the workflow to Pixi. But looking at the current rules that we have. A native <img> can be used when:

  1. It specifies loading=lazy
  2. It has an intrinsic size (width and height) or explicit size (in styles).

I'd just stress that (1) without the (2) is even worse for CLS than wrong values.

@cramforce
Copy link
Member

We could force AMP's current behavior with img {aspect-ratio: attr(width) / attr(height)!important} in browsers that support aspect ratio. That might be enough to disincentivize abuse.

@westonruter
Copy link
Member

westonruter commented Oct 21, 2020

My understanding here is that the height & width in img only inform initial layout, not enforce final layout. With amp-img, the initial dimension layout is fixed, regardless of what file-specified dimensions the image eventually has.

I don't think this is the case? Both of these result in 1x1 elements on the page after it is loaded (without any layout shifting):

<amp-img width="1" height="1" src="https://amp.dev/static/img/case-band-image-1.png"></amp-img>
<img width="1" height="1" loading="lazy" decoding="async" src="https://amp.dev/static/img/case-band-image-1.png">

As I understand, if an author provides an incorrect width & height, there is no CLS concern here at all. The only concern is for oversized-images, no? (And this is a potential issue with amp-img today as well.)

@cramforce
Copy link
Member

cramforce commented Oct 21, 2020 via email

@Gregable
Copy link
Member

Right. @cramforce I think the simple abuse looks something similar to:

<img loading=lazy width=1 height=1 style="width:auto;height:auto" src=foo.png>

@cramforce
Copy link
Member

My proposal would mitigate that.

@Gregable
Copy link
Member

@cramforce Where does that CSS live? Is that part of the AMP CSS, boilerplate, or something we expect publishers to add?

@cramforce
Copy link
Member

AMP CSS

@westonruter
Copy link
Member

westonruter commented Oct 27, 2020

Will deprecation of amp-img entail that the picture element should be allowed in addition to img? cf. #21912 (comment)

@dvoytenko
Copy link
Contributor

@westonruter I believe, yes, the hope is to eventually allow picture element. But it's currently blocked on implicit aspect ratio support for picture elements (see whatwg/html#5894 and crbug/1137794).

@westonruter
Copy link
Member

Deprecation of amp-img also entails deprecation of amp-anim, correct?

@fthead9
Copy link

fthead9 commented Dec 2, 2021

It's been well over a year now since this issue was raised, any progress? The amp-img tag causes many issues with WordPress responsiveness. It's been a constant battle of CSS bandaids trying to get images to look good across devices. Getting rid of the amp-img tag would make AMP WordPress dev much more effective. The fact this has been an open issue for so long makes me question whether developing for AMP is worth it anymore.

@kristoferbaxter
Copy link
Contributor

Apologies for the delay, at this point the only item remaining is in validation.

The caching working group is looking to address as they have availability.

@fthead9
Copy link

fthead9 commented Dec 3, 2021

@kristoferbaxter Thanks for the quick reply. Is there any timeline for the caching working group, i.e. finding time in the next couple of weeks, months, etc.?

@kristoferbaxter
Copy link
Contributor

@Gregable Thoughts on timeline for validator changes?

@Gregable
Copy link
Member

Gregable commented Dec 3, 2021

@banaag who has taken over for this.

Cache work is done pending release, and validator work is active. I suspect we'll have it working by end of month, but with holidays it could possibly go a little further.

@kristoferbaxter
Copy link
Contributor

Thanks for the update Greg!

@westonruter
Copy link
Member

Apologies for the delay, at this point the only item remaining is in validation.

@kristoferbaxter What about #30958? Have extensions been updated to support native img as a descendant where currently amp-img is expected?

@kristoferbaxter
Copy link
Contributor

@westonruter – I believe this is finished.

@westonruter
Copy link
Member

I've opened ampproject/amp-wp#6805 to use native img by default in AMP plugin v2.3, which we plan to release in January.

@westonruter
Copy link
Member

@fthead9 You can try native images in the AMP plugin for WordPress by installing the latest 2.2.x production pre-release build, and enabling the opt-in flag with this WordPress plugin PHP code:

add_filter( 'amp_native_img_used', '__return_true' );

The result is a valid AMP page that uses img instead of amp-img.

@westonruter
Copy link
Member

So can this issue be closed now?

@stale
Copy link

stale bot commented Mar 18, 2023

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the Stale Inactive for one year or more label Mar 18, 2023
@ychsieh ychsieh closed this as completed Sep 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
INTENT TO DEPRECATE Proposes deprecating an existing AMP feature. Stale Inactive for one year or more
Projects
None yet
Development

No branches or pull requests