Fix i-amphtml-sizer responsive issue (#1319)

ediamin · web-flow · commit 1abaa6b3f72e · 2022-04-19T21:56:56.000+02:00
* Remove inline padding-top from responsive sizer

* Add paddint-top only when heights attr is missing

* Add comment and issue link

* Use hasAttribute to detect the heights attr
diff --git a/packages/optimizer/lib/transformers/ApplyLayout.js b/packages/optimizer/lib/transformers/ApplyLayout.js
@@ -111,7 +111,7 @@ function maybeAddSizerInto(node, layout, width, height) {
   }
   let sizer = null;
   if (layout === 'responsive') {
-    sizer = createResponsiveSizer(width, height);
+    sizer = createResponsiveSizer(node, width, height);
   } else if (layout === 'intrinsic') {
     sizer = createIntrinsicSizer(width, height);
   }
@@ -121,11 +121,18 @@ function maybeAddSizerInto(node, layout, width, height) {
   }
 }
 
-function createResponsiveSizer(width, height) {
+function createResponsiveSizer(node, width, height) {
   const padding = (height.numeral / width.numeral) * 100;
+
+  // Skip adding padding-top when heights attr is present. HeightsTransformer will take care of it.
+  // https://github.com/ampproject/amp-toolbox/issues/1314
+  const style = !hasAttribute(node, 'heights')
+    ? `;padding-top:${parseFloat(padding.toFixed(4))}%`
+    : '';
+
   const sizer = createElement('i-amphtml-sizer', {
     slot: 'i-amphtml-svc',
-    style: `display:block;padding-top:${parseFloat(padding.toFixed(4))}%`,
+    style: `display:block${style}`,
   });
   return sizer;
 }
diff --git a/packages/optimizer/spec/transformers/valid/ServerSideRendering/converts_heights_attribute_to_css/expected_output.html b/packages/optimizer/spec/transformers/valid/ServerSideRendering/converts_heights_attribute_to_css/expected_output.html
@@ -9,7 +9,7 @@
 <body>
   <!-- converts heights to css -->
   <amp-img height="256" layout="responsive" src="https://acme.org/image1.png" width="320" class="i-amphtml-layout-responsive i-amphtml-layout-size-defined" i-amphtml-layout="responsive" id="i-amp-0">
-    <i-amphtml-sizer slot="i-amphtml-svc" style="display:block;padding-top:80%"></i-amphtml-sizer>
+    <i-amphtml-sizer slot="i-amphtml-svc" style="display:block"></i-amphtml-sizer>
   </amp-img>
 </body>
 </html>
diff --git a/packages/optimizer/spec/transformers/valid/ServerSideRendering/does_not_convert_invalid_heights_attribute/expected_output.html b/packages/optimizer/spec/transformers/valid/ServerSideRendering/does_not_convert_invalid_heights_attribute/expected_output.html
@@ -8,7 +8,7 @@
 </head>
 <body>
   <amp-img height="300" layout="responsive" heights="(min-width: 320px), 100vw" src="https://acme.org/image1.png" width="400" 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:75%"></i-amphtml-sizer>
+    <i-amphtml-sizer slot="i-amphtml-svc" style="display:block"></i-amphtml-sizer>
   </amp-img>
 </body>
 </html>
diff --git a/packages/optimizer/spec/transformers/valid/ServerSideRendering/transforms_layout_responsive/expected_output.html b/packages/optimizer/spec/transformers/valid/ServerSideRendering/transforms_layout_responsive/expected_output.html
@@ -12,7 +12,7 @@
   </amp-ad>
   <!-- Auto adds responsive attribute if height, width and heights -->
   <amp-ad class="i-amphtml-layout-responsive i-amphtml-layout-size-defined" type="taboola" data-block-on-consent="_till_responded" width="300" height="250" data-publisher="..." data-mode="thumbnails-a-amp" data-placement="Mobile Below Article Thumbnails AMP" data-article="auto" data-target_type="mix" layout="responsive" i-amphtml-layout="responsive" id="i-amp-0">
-    <i-amphtml-sizer slot="i-amphtml-svc" style="display:block;padding-top:83.3333%"></i-amphtml-sizer>
+    <i-amphtml-sizer slot="i-amphtml-svc" style="display:block"></i-amphtml-sizer>
   </amp-ad>
 </body>
 </html>

Original file line number	Diff line number	Diff line change
`@@ -111,7 +111,7 @@ function maybeAddSizerInto(node, layout, width, height) {`
`111`	`111`	`}`
`112`	`112`	`let sizer = null;`
`113`	`113`	`if (layout === 'responsive') {`
`114`		`- sizer = createResponsiveSizer(width, height);`
	`114`	`+ sizer = createResponsiveSizer(node, width, height);`
`115`	`115`	`} else if (layout === 'intrinsic') {`
`116`	`116`	`sizer = createIntrinsicSizer(width, height);`
`117`	`117`	`}`
`@@ -121,11 +121,18 @@ function maybeAddSizerInto(node, layout, width, height) {`
`121`	`121`	`}`
`122`	`122`	`}`
`123`	`123`
`124`		`-function createResponsiveSizer(width, height) {`
	`124`	`+function createResponsiveSizer(node, width, height) {`
`125`	`125`	`const padding = (height.numeral / width.numeral) * 100;`
	`126`	`+`
	`127`	`+ // Skip adding padding-top when heights attr is present. HeightsTransformer will take care of it.`
	`128`	`+ // https://github.com/ampproject/amp-toolbox/issues/1314`
	`129`	`+ const style = !hasAttribute(node, 'heights')`
	`130`	+ ? `;padding-top:${parseFloat(padding.toFixed(4))}%`
	`131`	`+ : '';`
	`132`	`+`
`126`	`133`	`const sizer = createElement('i-amphtml-sizer', {`
`127`	`134`	`slot: 'i-amphtml-svc',`
`128`		- style: `display:block;padding-top:${parseFloat(padding.toFixed(4))}%`,
	`135`	+ style: `display:block${style}`,
`129`	`136`	`});`
`130`	`137`	`return sizer;`
`131`	`138`	`}`