Responsive issue of i-amphtml-sizer created by the ServerSideRendering

[ediamin]

Related to ampproject/amp-toolbox-php#511

Consider an example snippet like this

<amp-embed
  width="780"
  height="100"
  heights="(max-width:645px) 100%, (max-width:845px) 31%, 23%"
  layout="responsive"
  type="zergnet"
  data-zergid="42658"
></amp-embed>

When we use an element with heights attribute the SSR or more specifically the createResponsiveSizer method in ApplyLayout.js adds an inline padding-top in the sizer element. But the SSR also generates amp-custom styles like this,

    <style amp-custom>
      #i-amp-0 > :first-child {
        padding-top: 23%;
      }
      @media (max-width: 845px) {
        #i-amp-0 > :first-child {
          padding-top: 31%;
        }
      }
      @media (max-width: 645px) {
        #i-amp-0 > :first-child {
          padding-top: 100%;
        }
      }
    </style>
    <amp-embed
      width="780"
      height="100"
      layout="responsive"
      type="zergnet"
      data-zergid="42658"
      id="i-amp-0"
      class="i-amphtml-layout-responsive i-amphtml-layout-size-defined"
      i-amphtml-layout="responsive"
      >
       <i-amphtml-sizer slot="i-amphtml-svc" style="display: block; padding-top: 12.8205%"></i-amphtml-sizer>
    </amp-embed>

For this output, the browser will always use the inline padding instead of the padding declared in amp-custom style. Hence, the generated markup is not responsive as expected.

In addition to that, according to the source code of amphtml, when we simply use the non-SSR element, it adds an inline padding-top using the heights attribute value. There is no affect of height/width ratio here. It only works if there is a heights attribute.

So, from my observation, we should remove the inline padding-top and only use the amp-custom style generated by the heights attribute value to get a responsive sizer.

[sebastianbenz]

Thanks for investigating. This is a bug. Not sure though what the best way to fix this is.

1. Adding !important to amp-custom would fix this, but results in invalid AMP
2. We could remove the inline padding when generating the custom css for the heights attribute (as you suggested)
3. We generate inline styles for the heights attribute

I'm leaning towards 3, but there might be side-effects I'm not aware of.

[ediamin]

heights attribute contains padding-top value for multiple media queries to make it responsive. AFAIK, it is not possible to add media query in inline style.

[sebastianbenz]

Good point. Then this leaves only 2. as an option.