[amp-story-player] Replace poster img CSS variable with img el

[Enriqe]

Context #30512

Replaces usages of the CSS variable with an <img loading="lazy" amp-story-player-poster-img> tag.

In preparation for dev preview of the player, we should clean up all the samples / docs before wide spread comms.

I'm holding off on updating the docs / samples of the AMP version until we address #30512 (comment)

[amp-owners-bot]

Hey @gmajoulet, @newmuis! These files were changed:

extensions/amp-story-player/0.1/amp-story-player.md
extensions/amp-story-player/0.1/test/validator-amp-story-player-error.html
extensions/amp-story-player/0.1/test/validator-amp-story-player-error.out
extensions/amp-story-player/0.1/test/validator-amp-story-player-img.html
extensions/amp-story-player/0.1/test/validator-amp-story-player-img.out
extensions/amp-story-player/0.1/test/validator-amp-story-player.html
extensions/amp-story-player/0.1/test/validator-amp-story-player.out
extensions/amp-story-player/validator-amp-story-player.protoascii

Hey @alanorozco! These files were changed:

extensions/amp-story-player/0.1/amp-story-player.md

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

extensions/amp-story-player/0.1/test/validator-amp-story-player-error.html
extensions/amp-story-player/0.1/test/validator-amp-story-player-error.out
extensions/amp-story-player/0.1/test/validator-amp-story-player-img.html
extensions/amp-story-player/0.1/test/validator-amp-story-player-img.out
extensions/amp-story-player/0.1/test/validator-amp-story-player.html
extensions/amp-story-player/0.1/test/validator-amp-story-player.out
extensions/amp-story-player/validator-amp-story-player.protoascii
validator/validator-main.protoascii

[CLAassistant]

All committers have signed the CLA.

[Enriqe]

PTAL

[Gregable]

We should probably hold off until issues in #30442 reach a decision.

[Gregable]

We should probably hold off until issues in #30442 reach a decision.

[Enriqe]

I think @kristoferbaxter agrees that we could work on making <img> valid for the <amp-story-player> and the rest of #30442 could be addressed separately. (see #30512 (comment))

[Enriqe]

Thanks for the helpful comments, @Gregable!

Could we get some guidance on how to move forward here? The change would only be valid for <img> tags under <amp-story-player>. Which we think can come separately from the deprecation of <amp-img> #30512 (comment).

We'd like to keep both AMP and non-AMP versions of the player with the same HTML API (which would not be possible if we tell publishers to use amp-img on the AMP version).

Since we're about to communicate updates about the player to a wider audience, we would like them to use this new API (using <img>) instead of having them change it later.

WDYT? @gmajoulet @kristoferbaxter @Gregable

[gmajoulet]

@Gregable

More context on why we want to move forward with this PR:

• The amp-story-player is mostly used on non AMP pages, but also available for AMP pages
• That means we can't use amp-img, because of the non AMP pages, but would like to keep the HTML configuration identical on both versions
• That validation change to allow img loading=lazy would be scoped to amp-story-player, and I think it's what we'd want to do regardless of the ongoing amp-img deprecation process

Please lmk if you think this is reasonable.

[Gregable]

WDYT? @gmajoulet @kristoferbaxter @Gregable

How do you feel about blocking on @cramforce 's suggested changes to the AMP CSS to enforce aspect-ratio before rolling this out?

[Enriqe]

How do you feel about blocking on @cramforce 's suggested changes to the AMP CSS to enforce aspect-ratio before rolling this out?

I'm not sure I understand the proposal. It sounds like the idea is to override the image values with the aspect-ratio CSS property for those browsers that support it, but what happens to those that don't support it?

If we are just going to override the values if the browsers support it, it sounds like an incremental improvement and we don't really have to wait on that change?

[Gregable]

it sounds like an incremental improvement and we don't really have to wait on that change?

My understanding is that it could break documents that relied on the non-overridden style. While it wouldn't invalidate them, a document could go from rendering correctly to rendering incorrectly after the aspect-ratio change.

This is why I recommend considering blocking until that change, so that we avoid surprises.

[honeybadgerdontcare]

it sounds like an incremental improvement and we don't really have to wait on that change?

My understanding is that it could break documents that relied on the non-overridden style. While it wouldn't invalidate them, a document could go from rendering correctly to rendering incorrectly after the aspect-ratio change.

This is why I recommend considering blocking until that change, so that we avoid surprises.

The issue @Gregable recommends blocking on is #30442

[Enriqe]

Synced offline with @honeybadgerdontcare and we've decided the general AMP scenario for validating <img> shouldn't block the case for amp-story-player.

I've updated the PR to address all comments, PTAL :)

[Enriqe]

Thanks for the review @honeybadgerdontcare , this is ready for review again.

[Enriqe]

/to @Gregable for blocking approval

[Gregable]

approved to resolve my old minor nit comments. I'm deferring to @honeybadgerdontcare on the larger questions.