Validator: Allow SSR for amp-img's img #29025

jridgewell · 2020-06-24T18:50:38Z

This adds validator support for server-side rendering an <amp-img>.

The one weird part of this is the requirement that the SSR'd <amp-img> include a i-amphtml-ssr attribute. I think this was necessary because we couldn't properly support intrinsic and blurry-image placeholder SSR at the same time. But with recent bug fixes, this shouldn't be a blocker anymore. So, we could remove the i-amphtml-ssr requirement, which will remove the duplicate tag spec here.

Fixes #28672.

amp-owners-bot · 2020-06-24T18:50:47Z

Hey @ampproject/wg-caching! These files were changed:

validator/testdata/transformed_feature_tests/amp-img.html
validator/testdata/transformed_feature_tests/amp-img.out
validator/validator-main.protoascii

sebastianbenz · 2020-06-24T19:32:20Z

validator/validator-main.protoascii

+  # requires an i-amphtml-ssr attribute on the amp-img. We could do away with
+  # that, though.
+  mandatory_parent: "amp-img (transformed)"
+  attrs: { name: "alt" }


Do we need to require class="i-amphtml-fill-content i-amphtml-replaced-content" which get added by the runtime?

We could, but:

This would require merging Validator: require i-amphtml-intrinsic-sizer class in transformed intrinsic output #29021 first (it fixes issues with specifying a mandatory class attr)

It wouldn't allow the reverse order, which might be weird?

It wouldn't allow the reverse order, which might be weird?

Agree, not ideal. Could be OK in the case of transformed AMP.

Follow up next week after cl/318548991 (via Validator: require i-amphtml-intrinsic-sizer class in transformed intrinsic output #29021) lands here?

Would value_regex help implement an order-independent check?

sebastianbenz

Looks good! Thanks for the quick turnaround.

banaag · 2020-06-29T19:35:31Z

@Gregable is out this week, adding @twifkak

twifkak · 2020-06-29T22:36:56Z

validator/testdata/transformed_feature_tests/amp-img.html

+<body>
+  <!-- Valid: amp-img > img -->
+  <amp-img src="/img/canoe_900x600.jpg" alt="An image about canoeing" layout="responsive" width="900" height="600" i-amphtml-ssr>
+    <img src="/img/canoe_900x600.jpg" alt="An image about canoeing" decoding="async" loading="lazy">


In advance of landing this PR, it would be helpful to add a Google-internal test that the cache-side transforms, when applied to such a file, convert both of these URLs to cache URLs. Reasons:

Privacy preservation on browsers that don't support loading=lazy (plus maybe they'll add more interventions over time besides scripting disabled?).

Ensure cache-side image transforms for UX/sec.

Filed b/160184148.

twifkak · 2020-06-29T23:01:14Z

validator/validator-main.protoascii

+  # requires an i-amphtml-ssr attribute on the amp-img. We could do away with
+  # that, though.
+  mandatory_parent: "amp-img (transformed)"
+  attrs: { name: "alt" }


Follow up next week after cl/318548991 (via Validator: require i-amphtml-intrinsic-sizer class in transformed intrinsic output #29021) lands here?

Would value_regex help implement an order-independent check?

jridgewell · 2020-07-11T00:47:58Z

Closing for cl/320708127.

cramforce · 2020-07-11T03:17:08Z

validator/validator-main.protoascii

+  }
+  attrs: {
+    name: "loading"
+    value: "lazy"


Did we test whether loading=lazy still triggers preload scanner to fetch the hero image pre-layout?

Oh, this is a good point. Testing with https://output.jsbin.com/husuxij/quiet:

Browser Loading Pre-scan?

Chrome 83 Lazy ❌

^ Eager ✅

Safari TP 109 Lazy ❌

^ Eager ✅

Firefox 80 Nightly Lazy ❌

^ Eager ❌

@sebastianbenz: I'm assuming this means we need to use non-lazy loading for SSR images?

We should probably only allow 1-2 non-lazy images per doc. I'm not sure how much sense it makes to add the lazy images at all. I think the simplest would be to make these non-lazy and only allow a small amount.

preload scanner to fetch the hero image pre-layout

for my own edification: Is this relevant in the case of SSR without boilerplate?

Yep, the impact is actually roughly the same in both cases as the boilerplate does not block layout (it just blocks visibility of content). You still get a win without the prescanner, as you can get downloads before JS loads, however, you'd expect a big win with the prescanner because

layout can be expensive

AMP's JS may run before layout when it is cached

Outside of Chrome, this would only work for images without srcset due to lack of support for imagesrcset.

There's currently a imagesrcset transformer that rewrite them to multiple <link rel=preload media="…"> with mutually exclusive media queries. We need to expand this to support imagesizes, but I think we can use the same general idea for better support.

I've given feedback before that while that might seem to work it is a very dangerous game. E.g. a browser in data-saver mode might choose to pick a smaller srcset value than the media query would yield leading to double download.

Why can't we generate <img srcset="…" sizes="…" style="display:none"> in place of <link rel=preload as=image>?

Re: @jridgewell's comment from Jul 14, if I set the script to async and set body visibility to hidden, the lazy image is fetched immediately. See https://output.jsbin.com/potuwanuco.

Are the pre-visibility: visible AMP layouts a useful approximation of which images will be above the fold post-full-layout? If so, there could be value in setting <img loading=lazy> on all images. Then we let the browser decide which images to fetch/decode before AMP layout has finished.

Could be combined with hero image detection for a more explicit <link rel=preload> (or <img style="display:none"> per @cramforce's last comment) where we're confident.

On cache side, it would need to be removed for privacy, but still useful on-origin and on-SXG.

correction: Wouldn't need to be removed for privacy, since they'd be rewritten URLs. Might want to remove to reduce data usage, for when navigation doesn't happen.

…re applied to all <img> tags, not just those inside <noscript>. This is relevant as of ampproject/amphtml#29025. PiperOrigin-RevId: 318935030

jridgewell requested a review from sebastianbenz June 24, 2020 18:50

amp-owners-bot bot requested a review from amaltas June 24, 2020 18:50

googlebot added the cla: yes label Jun 24, 2020

sebastianbenz reviewed Jun 24, 2020

View reviewed changes

jridgewell requested review from Gregable and removed request for amaltas June 24, 2020 21:13

sebastianbenz approved these changes Jun 24, 2020

View reviewed changes

jridgewell force-pushed the amp-img-ssr branch 2 times, most recently from 6ddf602 to 8bf129d Compare June 26, 2020 02:24

banaag requested a review from twifkak June 29, 2020 19:35

twifkak added the WG: caching label Jun 29, 2020

twifkak requested changes Jun 29, 2020

View reviewed changes

jridgewell added 5 commits July 10, 2020 18:18

Validator: Allow img inside transformed amp-img

2067c79

Require loading attr

5940454

Fix test output

173df6f

Allow referrerpolicy on transformed amp-img

8041136

Require class attribute

0ec6746

jridgewell force-pushed the amp-img-ssr branch from 8bf129d to 0ec6746 Compare July 10, 2020 22:59

Fix protoascii regex

f762754

jridgewell closed this Jul 11, 2020

cramforce reviewed Jul 11, 2020

View reviewed changes

sebastianbenz mentioned this pull request Jul 16, 2020

Optimize hero images ampproject/amp-wp#5055

Closed

twifkak mentioned this pull request Aug 20, 2020

SSR all amp-img tags to img (not just the hero images) ampproject/amppackager#465

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validator: Allow SSR for amp-img's img #29025

Validator: Allow SSR for amp-img's img #29025

jridgewell commented Jun 24, 2020

amp-owners-bot bot commented Jun 24, 2020 •

edited by ampprojectbot

Loading

sebastianbenz Jun 24, 2020 •

edited

Loading

jridgewell Jun 24, 2020

sebastianbenz Jun 24, 2020

twifkak Jun 29, 2020

sebastianbenz left a comment

banaag commented Jun 29, 2020

twifkak Jun 29, 2020

twifkak Jun 29, 2020

jridgewell commented Jul 11, 2020

cramforce Jul 11, 2020

jridgewell Jul 15, 2020

cramforce Jul 15, 2020

sebastianbenz Jul 15, 2020

cramforce Jul 15, 2020

sebastianbenz Aug 17, 2020

jridgewell Aug 17, 2020

cramforce Aug 17, 2020

twifkak Aug 18, 2020 •

edited

Loading

twifkak Aug 18, 2020

Browser	Loading	Pre-scan?
Chrome 83	Lazy	❌
^	Eager	✅
Safari TP 109	Lazy	❌
^	Eager	✅
Firefox 80 Nightly	Lazy	❌
^	Eager	❌

Validator: Allow SSR for amp-img's img #29025

Validator: Allow SSR for amp-img's img #29025

Conversation

jridgewell commented Jun 24, 2020

amp-owners-bot bot commented Jun 24, 2020 • edited by ampprojectbot Loading

sebastianbenz Jun 24, 2020 • 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

sebastianbenz left a comment

Choose a reason for hiding this comment

banaag commented Jun 29, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jridgewell commented Jul 11, 2020

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

twifkak Aug 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amp-owners-bot bot commented Jun 24, 2020 •

edited by ampprojectbot

Loading

sebastianbenz Jun 24, 2020 •

edited

Loading

twifkak Aug 18, 2020 •

edited

Loading