silverstripe · bergice · Jun 25, 2021 · Jun 25, 2021 · Jun 28, 2021 · emteknetnz
diff --git a/src/ImageManipulation.php b/src/ImageManipulation.php
@@ -58,6 +58,13 @@ trait ImageManipulation
      */
     protected $allowGeneration = true;
 
+    /**
+     * If image is lazy loaded
+     *
+     * @var bool
+     */
+    private $lazyLoad = true;
+
     /**
      * Set whether image resizes are allowed
      *
@@ -782,6 +789,41 @@ public function getHeight()
         return 0;
     }
 
+    /**
+     * Return whether image will be lazy loaded
+     *
+     * @return bool
+     */
+    public function getIsLazyLoaded() : bool
+    {
+        if (Image::getLazyLoadingEnabled() && $this->getWidth() && $this->getHeight()) {
+            return $this->lazyLoad;
+        }
+        return false;
+    }
+
+    /**
+     * Set whether image will be lazy loaded
+     *
+     * @param bool $lazyLoad
+     * @return self $this
+     */
+    public function LazyLoad(bool $lazyLoad): self
+    {
+        $this->lazyLoad = $lazyLoad;
+        return $this;
+    }
+
+    /**
+     * Get whether image will be lazy loaded
+     *
+     * @return bool
+     */
+    public function getLazyLoad(): bool
+    {
+        return $this->lazyLoad;
+    }
+
     /**
      * Get the orientation of this image.
      *
@@ -951,7 +993,7 @@ public function manipulate($variant, $callback)
                 $tuple = $result;
                 Deprecation::notice(
                     '5.0',
-                    'Closure passed to ImageManipulation::manipulate() should return null or a two-item array 
+                    'Closure passed to ImageManipulation::manipulate() should return null or a two-item array
                         containing a tuple and an image backend, i.e. [$tuple, $result]',
                     Deprecation::SCOPE_GLOBAL
                 );

diff --git a/templates/DBFile_image.ss b/templates/DBFile_image.ss
@@ -1 +1 @@
-<img src="$URL.ATT" alt="$Title.ATT" />
+<img src="$URL.ATT" alt="$Title.ATT"<% if $IsLazyLoaded %> loading="lazy"<% end_if %> width="$Width.ATT" height="$Height.ATT" />
diff --git a/tests/php/FileTest.php b/tests/php/FileTest.php
@@ -172,6 +172,10 @@ public function testInvalidImageManipulations()
         $this->assertFalse($pdf->getIsImage());
         $this->assertTrue($pdf->exists());
         $this->assertNull($pdf->Pad(100, 100));
+        Config::withConfig(function () use ($pdf) {
+            Config::modify()->set(Image::class, 'lazy_loading_enabled', true);
+            $this->assertFalse($pdf->getIsLazyLoaded());
+        });
 
         // Non-existant image
         $image = new Image();

diff --git a/tests/php/ImageTest.php b/tests/php/ImageTest.php
@@ -63,7 +63,7 @@ public function testGetTagWithTitle()
         Config::modify()->set(DBFile::class, 'force_resample', false);
 
         $image = $this->objFromFixture(Image::class, 'imageWithTitle');
-        $expected = '<img src="/assets/ImageTest/folder/test-image.png" alt="This is a image Title" />';
+        $expected = '<img src="/assets/ImageTest/folder/test-image.png" alt="This is a image Title" loading="lazy" width="300" height="300" />';
         $actual = trim($image->getTag());
 
         $this->assertEquals($expected, $actual);
@@ -91,7 +91,7 @@ public function testGetTagWithoutTitle()
         Config::modify()->set(DBFile::class, 'force_resample', false);
 
         $image = $this->objFromFixture(Image::class, 'imageWithoutTitle');
-        $expected = '<img src="/assets/ImageTest/folder/test-image.png" alt="test image" />';
+        $expected = '<img src="/assets/ImageTest/folder/test-image.png" alt="test image" loading="lazy" width="300" height="300" />';
         $actual = trim($image->getTag());
 
         $this->assertEquals($expected, $actual);
@@ -102,7 +102,7 @@ public function testGetTagWithoutTitleContainingDots()
         Config::modify()->set(DBFile::class, 'force_resample', false);
 
         $image = $this->objFromFixture(Image::class, 'imageWithoutTitleContainingDots');
-        $expected = '<img src="/assets/ImageTest/folder/test.image.with.dots.png" alt="test.image.with.dots" />';
+        $expected = '<img src="/assets/ImageTest/folder/test.image.with.dots.png" alt="test.image.with.dots" loading="lazy" width="300" height="300" />';
         $actual = trim($image->getTag());
 
         $this->assertEquals($expected, $actual);
@@ -532,6 +532,36 @@ public function testGetSetImageBackend()
         $this->assertEquals(99999, $image->getWidth());
     }
 
+    public function testLazyLoad()
+    {
+        Config::withConfig(function () {
+            Config::modify()->set(Image::class, 'lazy_loading_enabled', true);
+
+            /** @var Image $image */
+            $image = $this->objFromFixture(Image::class, 'imageWithTitle');
+            $this->assertTrue($image->getIsLazyLoaded(), 'Images lazy load by default');
+
+            $expected = '<img src="/assets/ImageTest/folder/test-image.png" alt="This is a image Title" loading="lazy" width="300" height="300" />';
+            $actual = trim($image->getTag());
+            $this->assertEquals($expected, $actual, 'Lazy load img tag renders correctly');
+
+            $image->LazyLoad(false);
+            $this->assertFalse($image->getIsLazyLoaded(), 'Images can be eager loaded on a per-image basis');
+
+            $image->LazyLoad(true);
+            Config::modify()->set(Image::class, 'lazy_loading_enabled', false);
+            $this->assertFalse($image->getIsLazyLoaded(), 'Lazy loading can be disabled globally');
+
+            Config::modify()->set(Image::class, 'lazy_loading_enabled', true);
+
+            $mockBackend = $this->getMockBuilder(InterventionBackend::class)->getMock();
+            $mockBackend->expects($this->any())->method('getWidth')->will($this->returnValue(0));
+            $image->setImageBackend($mockBackend);
+            $this->assertEquals(0, $image->getWidth(), 'Image does not have dimensions');
+            $this->assertFalse($image->getIsLazyLoaded(), 'Lazy loading is disabled if image does not have dimensions');
+        });
+    }
+
     /**
      * @param $filename
      * @param $hash

diff --git a/tests/php/Storage/DBFileTest.php b/tests/php/Storage/DBFileTest.php
@@ -48,7 +48,7 @@ public function testRender()
         $this->assertFileExists($fish);
         $obj->MyFile->setFromLocalFile($fish, 'awesome-fish.jpg');
         $this->assertEquals(
-            '<img src="/mysite/assets/a870de278b/awesome-fish.jpg" alt="awesome-fish.jpg" />',
+            '<img src="/mysite/assets/a870de278b/awesome-fish.jpg" alt="awesome-fish.jpg" loading="lazy" width="300" height="300" />',
             trim($obj->MyFile->forTemplate())
         );
Original file line number	Diff line number	Diff line change
		@@ -1 +1 @@
		<img src="$URL.ATT" alt="$Title.ATT" />
		<img src="$URL.ATT" alt="$Title.ATT"<% if $IsLazyLoaded %> loading="lazy"<% end_if %> width="$Width.ATT" height="$Height.ATT" />
Copy link Contributor maxime-rainville Jun 30, 2021 Choose a reason for hiding this comment The reason will be displayed to describe this comment to others. Learn more. Still think we should use getAttributes/getAttributeHTML here te be consistent with other parts of the CMS. Copy link Contributor Author bergice Jun 30, 2021 Choose a reason for hiding this comment The reason will be displayed to describe this comment to others. Learn more. Sure, but that is out of scope of this issue. Copy link Contributor maxime-rainville Jul 1, 2021 • edited Loading Choose a reason for hiding this comment The reason will be displayed to describe this comment to others. Learn more. I don't think taking the time to analyse a problem and finding the most sensible way of solving it is a question of "scope". If you don't like the "getAttributes/getAttributeHTML" approach or if you think it's not worth the extra work, explain why ... just don't call it out-of-scope. Copy link Contributor Author bergice Jul 1, 2021 • edited Loading Choose a reason for hiding this comment The reason will be displayed to describe this comment to others. Learn more. The scope of this project is to make images lazy load, not create fancy new API's that will increase the total time we spend on this epic. I like the idea of having an API for rendering attributes that is not exclusive to form fields, but it would require some refactoring and is considered a generic improvement / minor feature. If you really need this done then either do it yourself or create another issue. In addition to that, if any refactoring should be done regarding front-end rendering of images, it should be refactoring to using React. TLDR; it's not worth the extra work. Copy link Contributor maxime-rainville Jul 1, 2021 Choose a reason for hiding this comment The reason will be displayed to describe this comment to others. Learn more. You just need to copy this bit of logic and adjust it for your purposes https://github.com/silverstripe/silverstripe-framework/blob/dcdc25500be729f4340d42f8a4fc797461f862f3/src/Forms/FormField.php#L680-L757 It shouldn't be that long. Plus you'll need refactor the entire thing anyway to address the problem with sequentially rendered images. Copy link Contributor Author bergice Jul 2, 2021 Choose a reason for hiding this comment The reason will be displayed to describe this comment to others. Learn more. You just need to copy this bit of logic and adjust it for your purposes https://github.com/silverstripe/silverstripe-framework/blob/dcdc25500be729f4340d42f8a4fc797461f862f3/src/Forms/FormField.php#L680-L757 "Just". My previous arguments against doing this still stand. In addition to that I'm against copying identical code. If anything, we should be refactoring the other methods as well so they use the same code, which is not a trivial effort. It shouldn't be that long. Plus you'll need refactor the entire thing anyway to address the problem with sequentially rendered images. That is not related.