Merge pull request #136 from ampproject/fix/check-hero-image-media-before-srcset

Author: schlessera

src/Optimizer/Transformer/PreloadHeroImage.php

@@ -181,6 +181,7 @@ public function transform(Document $document, ErrorCollection $errors)
         }
 
         for ($index = 0; $index < $heroImageCount; $index++) {
+            $this->removeLazyLoading($heroImages[$index]);
             $this->generatePreload($heroImages[$index], $document, $errors);
             $this->generateImg($heroImages[$index], $document);
         }
@@ -470,22 +471,12 @@ private function getPlaceholderImage(Element $element)
     }
 
     /**
-     * Generate the preload link for a given hero image.
+     * Remove the lazy loading from the hero image.
      *
-     * @param HeroImage       $heroImage Hero image to generate the preload link for.
-     * @param Document        $document  Document to generate the preload link in.
-     * @param ErrorCollection $errors    Collection of errors that are collected during transformation.
+     * @param HeroImage $heroImage Hero image to remove the lazy loading for.
      */
-    private function generatePreload(
-        HeroImage $heroImage,
-        Document $document,
-        ErrorCollection $errors
-    ) {
-        if ($heroImage->getSrcset() && ! $this->supportsSrcset()) {
-            $errors->add(Error\CannotPreloadImage::fromImageWithSrcsetAttribute($heroImage->getAmpImg()));
-            return;
-        }
-
+    private function removeLazyLoading(HeroImage $heroImage)
+    {
         $img = $heroImage->getAmpImg();
 
         if (
@@ -495,13 +486,28 @@ private function generatePreload(
         ) {
             $img->removeAttribute(Attribute::LOADING);
         }
+    }
 
+    /**
+     * Generate the preload link for a given hero image.
+     *
+     * @param HeroImage       $heroImage Hero image to generate the preload link for.
+     * @param Document        $document  Document to generate the preload link in.
+     * @param ErrorCollection $errors    Collection of errors that are collected during transformation.
+     */
+    private function generatePreload(HeroImage $heroImage, Document $document, ErrorCollection $errors)
+    {
         if (empty($heroImage->getMedia())) {
             // We can only safely preload a hero image if there's a media attribute
             // as we can't detect whether it's hidden on certain viewport sizes otherwise.
             return;
         }
 
+        if ($heroImage->getSrcset() && ! $this->supportsSrcset()) {
+            $errors->add(Error\CannotPreloadImage::fromImageWithSrcsetAttribute($heroImage->getAmpImg()));
+            return;
+        }
+
         if ($this->hasExistingImagePreload($document, $heroImage->getSrc())) {
             return;
         }
@@ -517,6 +523,7 @@ private function generatePreload(
         $preload->appendChild($document->createAttribute(Attribute::DATA_HERO));
         if ($heroImage->getSrcset()) {
             $preload->setAttribute(Attribute::IMAGESRCSET, $heroImage->getSrcset());
+            $img = $heroImage->getAmpImg();
             if ($img && $img->hasAttribute(Attribute::SIZES)) {
                 $preload->setAttribute(Attribute::IMAGESIZES, $img->getAttribute(Attribute::SIZES));
             }

tests/Optimizer/Transformer/PreloadHeroImageTest.php

@@ -98,26 +98,43 @@ public function dataTransform()
 
             'throws error when scrset detected on image to be preloaded' => [
                 $input(
-                    '<amp-img data-hero width="500" height="400" src="https://example-com.cdn.ampproject.org/hero1.png" srcset="test 100w test2 3dpr"></amp-img>'
-                    . '<amp-img data-hero width="500" height="400" src="https://example-com.cdn.ampproject.org/hero2.png"></amp-img>'
+                    '<amp-img data-hero media="(min-width: 250px)" width="500" height="400" src="https://example-com.cdn.ampproject.org/hero1.png" srcset="test 100w test2 3dpr"></amp-img>'
+                    . '<amp-img data-hero media="(min-width: 250px)" width="500" height="400" src="https://example-com.cdn.ampproject.org/hero2.png"></amp-img>'
                 ),
                 $output(
-                    '<amp-img data-hero width="500" height="400" i-amphtml-ssr src="https://example-com.cdn.ampproject.org/hero1.png" srcset="test 100w test2 3dpr">'
+                    '<amp-img data-hero media="(min-width: 250px)" width="500" height="400" i-amphtml-ssr src="https://example-com.cdn.ampproject.org/hero1.png" srcset="test 100w test2 3dpr">'
                     . '<img class="i-amphtml-fill-content i-amphtml-replaced-content" decoding="async" src="https://example-com.cdn.ampproject.org/hero1.png" srcset="test 100w test2 3dpr">'
                     . '</amp-img>'
-                    . '<amp-img data-hero width="500" height="400" i-amphtml-ssr src="https://example-com.cdn.ampproject.org/hero2.png">'
+                    . '<amp-img data-hero media="(min-width: 250px)" width="500" height="400" i-amphtml-ssr src="https://example-com.cdn.ampproject.org/hero2.png">'
                     . '<img class="i-amphtml-fill-content i-amphtml-replaced-content" decoding="async" src="https://example-com.cdn.ampproject.org/hero2.png">'
-                    . '</amp-img>'
+                    . '</amp-img>',
+                    '<link as="image" data-hero href="https://example-com.cdn.ampproject.org/hero2.png" media="(min-width: 250px)" rel="preload">'
                 ),
                 [
                     Error\CannotPreloadImage::fromImageWithSrcsetAttribute(
                         Document::fromHtmlFragment(
-                            '<amp-img data-hero width="500" height="400" src="https://example-com.cdn.ampproject.org/hero1.png" srcset="test 100w test2 3dpr"></amp-img>'
+                            '<amp-img data-hero media="(min-width: 250px)" width="500" height="400" src="https://example-com.cdn.ampproject.org/hero1.png" srcset="test 100w test2 3dpr"></amp-img>'
                         )->body->firstChild
                     ),
                 ],
             ],
 
+            'preloads scrset image when configured to do so' => [
+                $input(
+                    '<amp-img data-hero media="(min-width: 250px)" width="500" height="400" src="https://example-com.cdn.ampproject.org/hero.png" srcset="test 100w test2 3dpr" sizes="100vw"></amp-img>'
+                ),
+                $output(
+                    '<amp-img data-hero media="(min-width: 250px)" width="500" height="400" i-amphtml-ssr src="https://example-com.cdn.ampproject.org/hero.png" srcset="test 100w test2 3dpr" sizes="100vw">'
+                    . '<img class="i-amphtml-fill-content i-amphtml-replaced-content" decoding="async" src="https://example-com.cdn.ampproject.org/hero.png" srcset="test 100w test2 3dpr" sizes="100vw">'
+                    . '</amp-img>',
+                    '<link as="image" data-hero href="https://example-com.cdn.ampproject.org/hero.png" imagesizes="100vw" imagesrcset="test 100w test2 3dpr" media="(min-width: 250px)" rel="preload">'
+                ),
+                [],
+                [
+                    PreloadHeroImageConfiguration::PRELOAD_SRCSET => true,
+                ]
+            ],
+
             'fetches placeholders for animations' => [
                 $input(
                     '<amp-anim data-hero width="500" height="400" src="/foo.gif">'