Don't remove sizes for hero images (#1142)

* Don't remove sizes for hero images Workaround for ampproject/amphtml#32644. * generate default sizes attribute for hero images
ampproject · Feb 15, 2021 · 19fbdba · 19fbdba
1 parent 972aa9a
commit 19fbdba
Show file tree

Hide file tree

Showing 8 changed files with 62 additions and 4 deletions.
diff --git a/packages/optimizer/lib/transformers/ApplyCommonAttributes.js b/packages/optimizer/lib/transformers/ApplyCommonAttributes.js
@@ -249,6 +249,15 @@ class ApplyCommonAttributes {
     customStyles.children[0].data += styles;
     for (const node of this.transformedNodes) {
       for (const attribute of Object.keys(this.attributeTransformations)) {
+        // Don't remove sizes from amp-img
+        // workaround for https://github.com/ampproject/amphtml/issues/32644
+        if (
+          node.tagName === 'amp-img' &&
+          attribute === 'sizes' &&
+          hasAttribute(node, 'data-hero')
+        ) {
+          continue;
+        }
         delete node.attribs[attribute];
       }
     }

diff --git a/packages/optimizer/lib/transformers/OptimizeHeroImages.js b/packages/optimizer/lib/transformers/OptimizeHeroImages.js
@@ -316,6 +316,11 @@ class OptimizeHeroImage {
     if (!node) {
       return;
     }
+    // Generate a default sizes element to avoid that the runtime generate it
+    // See https://github.com/ampproject/amphtml/issues/32644
+    if (hasAttribute(node, 'srcset') && !hasAttribute(node, 'sizes')) {
+      node.attribs.sizes = '100vw';
+    }
     node.attribs['i-amphtml-ssr'] = '';
     node.attribs['data-hero'] = '';
     // Create img node

diff --git a/...s/optimizer/spec/transformers/valid/OptimizeHeroImages/amp-img-sizes/expected_output.html b/...s/optimizer/spec/transformers/valid/OptimizeHeroImages/amp-img-sizes/expected_output.html
@@ -0,0 +1,9 @@
+<html>
+<head>
+  <script async src="https://cdn.ampproject.org/v0.js"></script>
+</head>
+<body>
+  <amp-img data-hero width="500" height="400" src="/foo1.png" srcset="elva-fairy-480w.jpg 480w, elva-fairy-800w.jpg 800w" sizes="(max-width: 600px) 480px, 800px" i-amphtml-ssr><img class="i-amphtml-fill-content i-amphtml-replaced-content" decoding="async" src="/foo1.png" srcset="elva-fairy-480w.jpg 480w, elva-fairy-800w.jpg 800w" sizes="(max-width: 600px) 480px, 800px"></amp-img>
+  <amp-img data-hero width="500" height="400" src="/foo2.png" srcset="elva-fairy-480w.jpg 480w, elva-fairy-800w.jpg 800w" sizes="100vw" i-amphtml-ssr><img class="i-amphtml-fill-content i-amphtml-replaced-content" decoding="async" src="/foo2.png" srcset="elva-fairy-480w.jpg 480w, elva-fairy-800w.jpg 800w" sizes="100vw"></amp-img>
+</body>
+</html>
diff --git a/packages/optimizer/spec/transformers/valid/OptimizeHeroImages/amp-img-sizes/input.html b/packages/optimizer/spec/transformers/valid/OptimizeHeroImages/amp-img-sizes/input.html
@@ -0,0 +1,9 @@
+<html>
+<head>
+  <script async src="https://cdn.ampproject.org/v0.js"></script>
+</head>
+<body>
+  <amp-img data-hero width="500" height="400" src="/foo1.png" srcset="elva-fairy-480w.jpg 480w, elva-fairy-800w.jpg 800w" sizes="(max-width: 600px) 480px, 800px"></amp-img> 
+  <amp-img data-hero width="500" height="400" src="/foo2.png" srcset="elva-fairy-480w.jpg 480w, elva-fairy-800w.jpg 800w"></amp-img> 
+</body>
+</html>
diff --git a/...pec/transformers/valid/OptimizeHeroImages/srcset_cannot_be_preloaded/expected_output.html b/...pec/transformers/valid/OptimizeHeroImages/srcset_cannot_be_preloaded/expected_output.html
@@ -2,7 +2,7 @@
 <head></head>
 <body>
   <!-- srcset is not supported by all browsers -->
-  <amp-img width="500" height="400" src="https://example-com.cdn.ampproject.org/foo.png" srcset="test 100w test2 3dpr" i-amphtml-ssr data-hero><img class="i-amphtml-fill-content i-amphtml-replaced-content" decoding="async" src="https://example-com.cdn.ampproject.org/foo.png" srcset="test 100w test2 3dpr"></amp-img>
+  <amp-img width="500" height="400" src="https://example-com.cdn.ampproject.org/foo.png" srcset="test 100w test2 3dpr" sizes="100vw" i-amphtml-ssr data-hero><img class="i-amphtml-fill-content i-amphtml-replaced-content" decoding="async" src="https://example-com.cdn.ampproject.org/foo.png" srcset="test 100w test2 3dpr" sizes="100vw"></amp-img>
   <!-- not preloaded as it's not a hero image -->
   <amp-img width="500" height="400" src="https://example-com.cdn.ampproject.org/foo.png"></amp-img>
 </body>

diff --git a/...c/transformers/valid/OptimizeHeroImages/srcset_validity_empty_srcset/expected_output.html b/...c/transformers/valid/OptimizeHeroImages/srcset_validity_empty_srcset/expected_output.html
@@ -1,8 +1,8 @@
 <html>
 <head></head>
 <body>
-  <amp-img width="500" height="400" src="https://example-com.cdn.ampproject.org/foo.png" srcset i-amphtml-ssr data-hero>
-    <img class="i-amphtml-fill-content i-amphtml-replaced-content" decoding="async" src="https://example-com.cdn.ampproject.org/foo.png" srcset>
+  <amp-img width="500" height="400" src="https://example-com.cdn.ampproject.org/foo.png" srcset sizes="100vw" i-amphtml-ssr data-hero>
+    <img class="i-amphtml-fill-content i-amphtml-replaced-content" decoding="async" src="https://example-com.cdn.ampproject.org/foo.png" srcset sizes="100vw">
   </amp-img>
 </body>
 </html>
diff --git a/...ansformers/valid/ServerSideRendering/converts_sizes_attribute_to_css/expected_output.html b/...ansformers/valid/ServerSideRendering/converts_sizes_attribute_to_css/expected_output.html
@@ -5,16 +5,24 @@
 
 
   <link href="https://example.com/favicon.ico" rel="icon">
-  <style amp-custom>#i-amp-0{width:100vw}@media (min-width: 320px){#i-amp-0{width:320px}}</style>
+  <style amp-custom>#i-amp-0{width:100vw}@media (min-width: 320px){#i-amp-0{width:320px}}#i-amp-1{width:100vw}@media (min-width: 320px){#i-amp-1{width:320px}}</style>
 </head>
 <body>
   <!-- requires srcset attribute -->
   <amp-img height="300" layout="responsive" 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 style="display:block;padding-top:75%"></i-amphtml-sizer>
   </amp-img>
   <!-- sizes -->
+  <amp-video height="300" layout="responsive" src="https://acme.org/videot" .mp4 width="400" class="i-amphtml-layout-responsive i-amphtml-layout-size-defined" i-amphtml-layout="responsive">
+    <i-amphtml-sizer style="display:block;padding-top:75%"></i-amphtml-sizer>
+  </amp-video>
+  <!-- sizes in amp-img-->
   <amp-img height="300" layout="responsive" srcset="img-480w.jpg 480w,img-800w.jpg 800w" src="https://acme.org/image1.png" width="400" class="i-amphtml-layout-responsive i-amphtml-layout-size-defined" i-amphtml-layout="responsive" id="i-amp-0">
     <i-amphtml-sizer style="display:block;padding-top:75%"></i-amphtml-sizer>
   </amp-img>
+  <!-- sizes in hero img is not removed -->
+  <amp-img data-hero height="300" layout="responsive" sizes="(min-width: 320px) 320px, 100vw" srcset="img-480w.jpg 480w,img-800w.jpg 800w" src="https://acme.org/image1.png" width="400" class="i-amphtml-layout-responsive i-amphtml-layout-size-defined" i-amphtml-layout="responsive" id="i-amp-1">
+    <i-amphtml-sizer style="display:block;padding-top:75%"></i-amphtml-sizer>
+  </amp-img>
 </body>
 </html>
diff --git a/...er/spec/transformers/valid/ServerSideRendering/converts_sizes_attribute_to_css/input.html b/...er/spec/transformers/valid/ServerSideRendering/converts_sizes_attribute_to_css/input.html
@@ -17,7 +17,25 @@
       width=400>
     </amp-img>
     <!-- sizes -->
+    <amp-video
+      height=300
+      layout=responsive
+      sizes="(min-width: 320px) 320px, 100vw"
+      src=https://acme.org/videot .mp4
+      width=400>
+    </amp-video>
+    <!-- sizes in amp-img-->
+    <amp-img
+      height=300
+      layout=responsive
+      sizes="(min-width: 320px) 320px, 100vw"
+      srcset="img-480w.jpg 480w,img-800w.jpg 800w"
+      src=https://acme.org/image1.png
+      width=400>
+    </amp-img>
+    <!-- sizes in hero img is not removed -->
     <amp-img
+      data-hero
       height=300
       layout=responsive
       sizes="(min-width: 320px) 320px, 100vw"