Adapt heights, sizes and media attributes for SSR

[schlessera]

Summary

This PR adds logic to the ServerSideRendering transformer in ampproject/optimizer to adapt the heights, sizes and media attributes on AMP elements and turn them into CSS. This is done so that they don't block server-side layout application.

The PR also moves the AMP_DOM_Utils::get_element_id() to AmpProject\Dom\Document::getElementId() and deprecates the previous method.

Fixes #4439

Checklist

• My pull request is addressing an open issue (please create one otherwise).
• 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).

[westonruter]

Here's my plan for getting this merged:

• 1. Update the AMP spec to the latest via Update allowed tags/attributes from spec in amphtml 2004142326360 #4548.
• 2. Create a new PR which eliminates the removal of the sizes attribute in favor of adding the new disable-inline-width attribute. See Discontinue removing sizes attribute when converting img to amp-img #4606.
• 3. Merge the develop branch into this branch to test those changes with the SSR changes. At the moment the sizes attribute is stripped from all images generated by WordPress, but this is no longer done then the changes in this PR to adapt sizes for SSR will be hugely more important. Also, I suspect that amp-img[sizes][disable-inline-width] will actually be exempt from requiring the boilerplate, so that will be a new condition to check for in the Optimizer. cc @sebastianbenz

[sebastianbenz]

Good find! Really interesting! I'm wondering whether it'd make sense to make this the default behavior for sizes (for transformed AMP) or whether there are cases where the behavior differs.

[westonruter]

This won't work as expected, as the :first-child is a pseudo-selector qualifying what it is attached to. So this would match elements that have ID of __ID__ which are the first-child position. So it needs rather to be:

Suggested change

	'#__ID__:first-child{height:%s}',
	'@media %s{#__ID__:first-child{height:%s}}'
	'#__ID__>:first-child{height:%s}',
	'@media %s{#__ID__>:first-child{height:%s}}'

So it selects the first child of the element that has the ID.

[schlessera]

Ah, you are right. Will correct.

[schlessera]

... I just realized that it is not the height that needs to be adapted for the sizer, I think, but the padding-top.

[schlessera]

... which can't be set as it is inlined by JS already. 🤔

[schlessera]

So, as the Optimizer is the one that inlined the padding-top, I could skip that if I detect that I already had a heights attribute that turned into CSS.

But at what point are we just plain "hacking" here?

[schlessera]

Oh, I could actually store the CSS and properly inline it onto the sizer. That way, I can omit adding it to the size-limited <style amp-custom> as well.

[schlessera]

I'm running in circles here. Inline styles don't allow for media queries. I will just skip adding the padding-top in that case for now.

[schlessera]

Ok, I've got a working version now. Not the most elegant stuff, but it seems to work as expected.

[westonruter]

I'm noticing differences in the rendering of the elements in with and without SSR here.

Given this post_content:

<!-- wp:paragraph -->
<p>Heights:</p>
<!-- /wp:paragraph -->

<!-- wp:html -->
<amp-img alt="AMP" src="https://amp.dev/static/inline-examples/images/amp.jpg" width="320" height="256" heights="(min-width:500px) 200px, 80%">
</amp-img>
<!-- /wp:html -->

<!-- wp:paragraph -->
<p>Sizes:</p>
<!-- /wp:paragraph -->

<!-- wp:html -->
<amp-img alt="Hummingbird" src="https://amp.dev/static/inline-examples/images/hummingbird-wide.jpg" width="640" height="457" srcset="https://amp.dev/static/inline-examples/images/hummingbird-wide.jpg 640w,
            https://amp.dev/static/inline-examples/images/hummingbird-narrow.jpg 320w" sizes="(min-width: 650px) 50vw, 100vw">
</amp-img>
<!-- /wp:html -->

<!-- wp:paragraph -->
<p>Media:</p>
<!-- /wp:paragraph -->

<!-- wp:html -->
<amp-img media="(min-width: 650px)" src="https://amp.dev/static/inline-examples/images/hummingbird-wide.jpg" width="640" height="457" layout="responsive">
</amp-img>
<amp-img media="(max-width: 649px)" src="https://amp.dev/static/inline-examples/images/hummingbird-narrow.jpg" width="320" height="229" layout="responsive">
</amp-img>
<!-- /wp:html -->

The sizes and heights don't seem to be working right:

Width	Without SSR	With SSR
600px
700px

The media example is working, however.

This is using the Twenty Twenty theme.

[schlessera]

I'm currently investigating the visual regression with SSR. What I can already see is that the SSR is somehow changing the images from layout=responsive into layout=fixed. I suspect that something I'm doing is subsequently misinterpreted by the AMP runtime.

[schlessera]

The differences were due to the fact that the sizes, heights and media attributes were remove too early and were missing for the layout calculations, which yielded in wrong layout attributes to be assigned.

I resolved this by deferring the attribute removal via 8767882

With your post content, on my system it looks identical now. Please double-check on your end.

[westonruter]

It works!

[schlessera]

Based on the above issues I also created a new issue #4671, as I think we're at a point where that could drastically reduce peer review time without impacting general coding too much.

[westonruter]

Based on the above issues I also created a new issue #4671, as I think we're at a point where that could drastically reduce peer review time without impacting general coding too much.

Yes, great idea. That will be very useful.