Merge pull request #521 from ampproject/fix/511-i-amphtml-sizer-responsiveness-issue

Fix SSR sizer responsiveness issue

Author: ediamin

src/Optimizer/Transformer/ServerSideRendering.php

@@ -590,9 +590,15 @@ private function maybeAddSizerInto(
         if ($layout === Layout::RESPONSIVE) {
             $elementId = $element->getAttribute(Attribute::ID);
             if (!empty($elementId) && array_key_exists($elementId, $this->customSizerStyles)) {
-                $sizer = $this->createResponsiveSizer($document, $width, $height, $this->customSizerStyles[$elementId]);
+                $sizer = $this->createResponsiveSizer(
+                    $document,
+                    $element,
+                    $width,
+                    $height,
+                    $this->customSizerStyles[$elementId]
+                );
             } else {
-                $sizer = $this->createResponsiveSizer($document, $width, $height);
+                $sizer = $this->createResponsiveSizer($document, $element, $width, $height);
             }
         } elseif ($layout === Layout::INTRINSIC) {
             $sizer = $this->createIntrinsicSizer($document, $width, $height);
@@ -607,25 +613,30 @@ private function maybeAddSizerInto(
      * Create a sizer element for a responsive layout.
      *
      * @param Document  $document DOM document to create the sizer for.
+     * @param Element   $element  Element to add a sizer to.
      * @param CssLength $width    Calculated width of the element.
      * @param CssLength $height   Calculated height of the element.
      * @param string    $style    Style to use for the sizer. Defaults to padding-top in percentage.
      * @return Element
      */
     private function createResponsiveSizer(
         Document $document,
+        Element $element,
         CssLength $width,
         CssLength $height,
-        $style = 'padding-top:%s%%'
+        $style = ''
     ) {
         $padding       = $height->getNumeral() / $width->getNumeral() * 100;
         $paddingString = rtrim(rtrim(sprintf('%.4F', round($padding, 4)), '0'), '.');
+        $paddingStyle  = ! $element->hasAttribute(Attribute::HEIGHTS)
+            ? sprintf('padding-top:%s%%', $paddingString)
+            : '';
 
-        $style = empty($style) ? 'display:block' : "display:block;{$style}";
+        $style = "display:block;{$paddingStyle};{$style}";
 
         $sizer = $document->createElement(Amp::SIZER_ELEMENT);
         $sizer->setAttribute(Attribute::SLOT, Amp::SERVICE_SLOT);
-        $sizer->addInlineStyle(sprintf($style, $paddingString));
+        $sizer->addInlineStyle($style);
 
         return $sizer;
     }

tests/Optimizer/Transformer/ServerSideRenderingTest.php

@@ -253,7 +253,7 @@ public function dataTransform()
             'heights attribute without amp-custom' => [
                 $input('<amp-img height="256" heights="(min-width: 500px) 200px, 80%" layout="responsive" width="320"></amp-img>'),
                 $expectWithoutBoilerplate(
-                    '<amp-img height="256" layout="responsive" width="320" 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:80%"></i-amphtml-sizer></amp-img>',
+                    '<amp-img height="256" layout="responsive" width="320" 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"></i-amphtml-sizer></amp-img>',
                     '<style amp-custom>#i-amp-0>:first-child{padding-top:80%}@media (min-width: 500px){#i-amp-0>:first-child{padding-top:200px}}</style>'
                 ),
                 [],
@@ -265,7 +265,7 @@ public function dataTransform()
                     '<style amp-custom>body h1{color:red;}</style>'
                 ),
                 $expectWithoutBoilerplate(
-                    '<amp-img height="256" layout="responsive" width="320" 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:80%"></i-amphtml-sizer></amp-img>',
+                    '<amp-img height="256" layout="responsive" width="320" 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"></i-amphtml-sizer></amp-img>',
                     '<style amp-custom>body h1{color:red;}#i-amp-0>:first-child{padding-top:80%}@media (min-width: 500px){#i-amp-0>:first-child{padding-top:200px}}</style>'
                 ),
                 [],
@@ -274,7 +274,7 @@ public function dataTransform()
             'bad heights attribute' => [
                 $input('<amp-img height="256" heights=",,," layout="responsive" width="320"></amp-img>'),
                 // This adds an ID as it stores the CSS to inline before the actual error is detected.
-                $expectWithBoilerplate('<amp-img class="i-amphtml-layout-responsive i-amphtml-layout-size-defined" height="256" heights=",,," i-amphtml-layout="responsive" layout="responsive" width="320"><i-amphtml-sizer slot="i-amphtml-svc" style="display:block;padding-top:80%"></i-amphtml-sizer></amp-img>'),
+                $expectWithBoilerplate('<amp-img class="i-amphtml-layout-responsive i-amphtml-layout-size-defined" height="256" heights=",,," i-amphtml-layout="responsive" layout="responsive" width="320"><i-amphtml-sizer slot="i-amphtml-svc" style="display:block"></i-amphtml-sizer></amp-img>'),
                 [
                     Error\CannotRemoveBoilerplate::fromAttributeThrowingException(
                         InvalidHtmlAttribute::fromAttribute(