Add lazy loading to images #457

bergice · 2021-06-25T03:31:23Z

Resolves #450

emteknetnz

Approach looks good

emteknetnz · 2021-06-28T05:13:45Z

src/ImageManipulation.php

+     *
+     * @var bool
+     */
+    protected $lazyLoad = true;


Suggested change

protected $lazyLoad = true;

private $lazyLoad = true;

emteknetnz · 2021-06-28T05:14:17Z

src/ImageManipulation.php

+     *
+     * @return bool
+     */
+    public function IsLazyLoaded()


Suggested change

public function IsLazyLoaded()

public function getIsLazyLoaded(): bool

emteknetnz · 2021-06-28T05:15:09Z

src/ImageManipulation.php

+     */
+    public function IsLazyLoaded()
+    {
+        if (!Config::inst()->get('SilverStripe\\Assets\\Image', 'lazy_loading_enabled')) {


Suggested change

if (!Config::inst()->get('SilverStripe\\Assets\\Image', 'lazy_loading_enabled')) {

if (!Image::getLazyLoadingEnabled()) {

emteknetnz · 2021-06-28T05:16:46Z

src/ImageManipulation.php

+     * @param bool $lazyLoad
+     * @return $this
+     */
+    public function LazyLoad($lazyLoad)


Suggested change

public function LazyLoad($lazyLoad)

public function setLazyLoad(bool $lazyLoad)

you'll also need a corredsponding function getLazyLoad(): bool

emteknetnz · 2021-06-28T05:17:46Z

src/ImageManipulation.php

+     */
+    public function LazyLoad($lazyLoad)
+    {
+        $this->lazyLoad = $lazyLoad;


Need to check that width and height exist on the image

If not, return false

Will need to unit test this

emteknetnz · 2021-06-28T05:22:24Z

templates/DBFile_image.ss

@@ -1 +1 @@
-<img src="$URL.ATT" alt="$Title.ATT" />
+<img src="$URL.ATT" alt="$Title.ATT"<% if $IsLazyLoaded %> loading="lazy" width="$Width.ATT" height="$Height.ATT"<% end_if %> />


We should probably add width + height regardless whether $IsLazyLoaded is true or not

bergice · 2021-06-29T00:28:06Z

@emteknetnz I've addressed your feedback.
No idea what's going on with failing build.

maxime-rainville

ImageManipulation is applied to File and DBFile as well as Image. I would expect the implementation and tests accounts for this (e.g. What's the expected results if you call getIsLazyLoaded on a PDF File? Where's the test for this scenario.)

maxime-rainville · 2021-06-30T00:00:03Z

src/ImageManipulation.php

+     * @param bool $lazyLoad
+     * @return $this
+     */
+    public function setLazyLoad($lazyLoad)


I think this works.

Suggested change

public function setLazyLoad($lazyLoad)

public function setLazyLoad($lazyLoad): self

bool $lazyLoad

maxime-rainville · 2021-06-30T00:01:33Z

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" />


Still think we should use getAttributes/getAttributeHTML here te be consistent with other parts of the CMS.

Sure, but that is out of scope of this issue.

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.

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.

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.

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.

maxime-rainville · 2021-06-30T00:04:33Z

tests/php/ImageTest.php

+        $this->assertFalse($image->getIsLazyLoaded(), 'Images can be eager loaded on a per-image basis');
+
+        $image->setLazyLoad(true);
+        Config::modify()->set(Image::class, 'lazy_loading_enabled', false);


You could use the new withConfig API here

maxime-rainville · 2021-07-01T23:52:44Z

src/ImageManipulation.php

+     * @param bool $lazyLoad
+     * @return self $this
+     */
+    public function setLazyLoad(bool $lazyLoad): self


If you have this in your template

<h2>Not Lazy Loaded</h2> $ScaleWidth(100).setLazyLoad(0) $setLazyLoad(0) $setLazyLoad(1).$setLazyLoad(0) <h2>Lazy Loaded</h2> $ScaleWidth(100) $Me $setLazyLoad(1)

All the rendered images will not have a loading attribute because only the first rendered image is honoured. If you reverse the two blocks all the Image will have the loading attribute.

setLazyLoad needs to return a copy of itself like the other ImageManipulation function.

This method probably should be start with an upper case letter to match the other image manipulation methods.

We need to remove the explicit bool type so people can provide the value as 'false' or '0' because of a limitation in our SS rendering engine.

This method probably should be start with an upper case letter to match the other image manipulation methods.

Agree, I figured we should do that as well.

We need to remove the explicit bool type so people can provide the value as 'false' or '0' because of a limitation in our SS rendering engine.

That's not gonna work because it will just set the variable to false as a string.
We should really just expect a 0/1 value like every other template API already does.

All the rendered images will not have a loading attribute because only the first rendered image is honoured. If you reverse the two blocks all the Image will have the loading attribute.

setLazyLoad needs to return a copy of itself like the other ImageManipulation function.

I'm struggling to see how this would matter in a real world scenario. What would be the point of lazy loading and not lazy load the same image? After all, it's pointing to the same URL.

The only use case I see is if you manipulate the image, and if you do that it will create a copy and toggling the lazy load for the same image with a different variant will work as expected.

I'm reluctant to create a copy of the image, purely for the sake of doing something that has no real world value.

As a developer, I shouldn't have to worry about how SetLazyLoad(false) will be cast internally. It should just work. So in the method, we should just convert a 'false' string to an actual boolean.

I'm struggling to see how this would matter in a real world scenario. What would be the point of lazy loading and not lazy load the same image? After all, it's pointing to the same URL.

SSViewer tries to cache things as much as possible as it goes along. You could end up with weird scenario where you have an include that gets call in different context. We'll just end up with weird bugs and have inconsistent result if we do it this way.

That's why ImageManipulation has all this convoluted logic and jumps through all these hoops to create duplicate object of Image.

As a developer, I shouldn't have to worry about how SetLazyLoad(false) will be cast internally. It should just work.

Yes, it should. However this is something that should be solved for templates in general, not just hardcoded into individual methods.

It is clear that this is something that will keep being an issue unless we fix it properly, and I don't think this "duct-taping" fix approach is the right way to solve it. silverstripe/silverstripe-cms@ed0680a

SSViewer tries to cache things as much as possible as it goes along. You could end up with weird scenario where you have an include that gets call in different context. We'll just end up with weird bugs and have inconsistent result if we do it this way.

That's why ImageManipulation has all this convoluted logic and jumps through all these hoops to create duplicate object of Image.

I'm not sure about that yet. Do you have an example? I'm under the impression it duplicates the image after altering it / when the variant changes.

As a developer, I shouldn't have to worry about how SetLazyLoad(false) will be cast internally. It should just work.

Yes, it should. However this is something that should be solved for templates in general, not just hardcoded into individual methods.

It is clear that this is something that will keep being an issue unless we fix it properly, and I don't think this "duct-taping" fix approach is the right way to solve it.

FYI this has been resolved in 5.x/dev-master with silverstripe/silverstripe-framework#8152 (and silverstripe/silverstripe-framework#8456 for null values, which was a marginally more invasive change). I treated it as an API change at the time, but I suppose we could look at whether it’d be possible to cherry-pick those changes into 4.x?

I wouldn’t be opposed to “duct-taping” something like this for now. It’s one line of zero-maintenance code to reduce a bunch of confusion/extra documentation for developers using this.

Yeah, I was suggest cherry-picking as well. Might be fine as long as we consider it a bug.

It duplicates the PHP object, but it doesn't dupilcate the actual File database entry or the physical file.

The idea is to stack up all the image manipulation calls, but wait until you actually render the image at the end to execute them. Otherwise, you would be running a bunch of expensive (and potentially unneeded) image manipulation operations immediately when calrling a method like CropHeight.

My guess is you probably just need to do something like this to duplicate you current object.

$this->manipulate('', function($callback, $store, $filename, $hash, $variant) { return [ 'Filename' => $filename, 'Hash' => $hash, 'Variant' => $this->getVariant() ] });

Then you set a lazy loading flag on you duplicate.

You need to make sure that your lazy loading flag gets copied across to any follow up calls to ImageManipulation::manipulate().

maxime-rainville · 2021-07-01T23:58:52Z

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" />


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.

emteknetnz · 2021-07-06T00:32:44Z

Unfortunately this approach does not appear to do what it needs to work as intended. While the unit tests pass, adding an image in the template as $MyImage.LazyLoad(false) doesn't work because:

a) the argument is passed to PHP as a string "false" - which is fixiable within the getLazyLoaded function, however
b) the value in the private field $lazyLoad is not retained, I believe because ImageManipulation is a bit of an abstraction over DBFile

I'm closing this in favour of the alternative approach here #458

Add lazy loading to images

cea2cab

This was referenced Jun 25, 2021

SSViewer image renders loading attribute #450

Closed

Add docs for lazy loading silverstripe/silverstripe-framework#9995

Merged

Fix breaking other tests

0d503ce

emteknetnz requested changes Jun 28, 2021

View reviewed changes

bergice force-pushed the pulls/1/lazy-load-images branch from 14ee61e to df9373c Compare June 28, 2021 23:35

maxime-rainville requested changes Jun 30, 2021

View reviewed changes

bergice force-pushed the pulls/1/lazy-load-images branch 2 times, most recently from c98540b to afe2525 Compare June 30, 2021 04:57

maxime-rainville requested changes Jul 1, 2021

View reviewed changes

Update tests to work with default lazy loading enabled.

4aaf33a

bergice force-pushed the pulls/1/lazy-load-images branch from afe2525 to 4aaf33a Compare July 4, 2021 22:53

bergice mentioned this pull request Jul 5, 2021

DOC Document image lazy-loading silverstripe/silverstripe-framework#9990

Closed

emteknetnz closed this Jul 6, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add lazy loading to images #457

Add lazy loading to images #457

bergice commented Jun 25, 2021

emteknetnz left a comment

emteknetnz Jun 28, 2021

emteknetnz Jun 28, 2021

emteknetnz Jun 28, 2021

emteknetnz Jun 28, 2021

emteknetnz Jun 28, 2021

emteknetnz Jun 28, 2021

bergice commented Jun 29, 2021

maxime-rainville left a comment

maxime-rainville Jun 30, 2021

emteknetnz Jun 30, 2021

maxime-rainville Jun 30, 2021

bergice Jun 30, 2021

maxime-rainville Jul 1, 2021 •

edited

Loading

bergice Jul 1, 2021 •

edited

Loading

maxime-rainville Jul 1, 2021

bergice Jul 2, 2021

maxime-rainville Jun 30, 2021

maxime-rainville Jul 1, 2021

maxime-rainville Jul 1, 2021

maxime-rainville Jul 1, 2021

bergice Jul 2, 2021

bergice Jul 2, 2021 •

edited

Loading

maxime-rainville Jul 2, 2021

bergice Jul 2, 2021

kinglozzer Jul 2, 2021

bergice Jul 2, 2021

maxime-rainville Jul 4, 2021

maxime-rainville Jul 1, 2021

emteknetnz commented Jul 6, 2021

	public function IsLazyLoaded()
	public function getIsLazyLoaded(): bool

	if (!Config::inst()->get('SilverStripe\\Assets\\Image', 'lazy_loading_enabled')) {
	if (!Image::getLazyLoadingEnabled()) {

	public function LazyLoad($lazyLoad)
	public function setLazyLoad(bool $lazyLoad)

		@@ -1 +1 @@
		<img src="$URL.ATT" alt="$Title.ATT" />
		<img src="$URL.ATT" alt="$Title.ATT"<% if $IsLazyLoaded %> loading="lazy" width="$Width.ATT" height="$Height.ATT"<% end_if %> />

	public function setLazyLoad($lazyLoad)
	public function setLazyLoad($lazyLoad): self

Add lazy loading to images #457

Add lazy loading to images #457

Conversation

bergice commented Jun 25, 2021

emteknetnz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bergice commented Jun 29, 2021

maxime-rainville left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxime-rainville Jul 1, 2021 • edited Loading

Choose a reason for hiding this comment

bergice Jul 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bergice Jul 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emteknetnz commented Jul 6, 2021

maxime-rainville Jul 1, 2021 •

edited

Loading

bergice Jul 1, 2021 •

edited

Loading

bergice Jul 2, 2021 •

edited

Loading