search

ampproject / amp-toolbox

A collection of AMP tools making it easier to publish and host AMP pages.
Apache License 2.0
449 stars 243 forks source link

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

Closed ediamin closed 2 years ago

ediamin commented 2 years ago

Related to https://github.com/ampproject/amp-toolbox-php/issues/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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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