SSViewer image renders loading attribute

[brynwhyman]

Overview

Modify DBFile.php and DBFile_image.ss to render the new lazy-loading attribute.

Background: https://forum.silverstripe.org/t/browser-level-lazy-loading-from-silverstripe-cms/4218

Acceptance Criteria

• It is possible for the loading="lazy" attribute to be added when you render an image in the template
• The loading="lazy" attribute will only be added to images where dimensions are defined
• There is a method to opt-out of lazy-loading on a per image basis
• Where an image opts out of lazy-loading, the loading attribute should be skipped entirely
• Where a site globally opts out of lazy-loading, the loading attribute is skipped

Notes

• When it comes to docs, make sure it's clear that as a developer, if you manually write your image tag, this won't apply.

PR's

• Add lazy load to Image/DBFile rendering #458
• Add docs for lazy loading silverstripe-framework#9995

[bergice]

Proposed implementation

1. Add DBFile.getLoadingAttribute API for retrieving the lazy load value (or null)
   Returns null if Image.enable_lazy_load is false
   Otherwise, it returns the value of Image.default_lazy_load
   a. To opt out on a per-image basis, extend this function
2. Modify DBFile_image.ss to render the new lazy-loading attribute if not null
   If the loading attribute is supplied we also supply the width, height attributes

Re AC: The loading="lazy" attribute will only be added to images where dimensions are defined

We should just supply the width and height attributes on our end if lazy loading is enabled.
We've already got this data in the Image class anyways, and users would just expect this feature to work out of the box.

[emteknetnz]

Add DBFile.getLoadingAttribute API

In the intention here to return the whole attribute e.g. loading="lazy" or just lazy? The originally cited web.dev article recommends only either showing the loading="lazy" attribute or completely omitted it (see the warning section below the linked section above)

Because of this I think it would make sense to return the entire loading="lazy" string, or an empty string (i.e. not return null)

To opt out on a per-image basis, extend this function

The original floated idea was to have a callable function from the template e.g. $MyImage.LazyLoad(false) or $MyImage.EagerLoad() (which probably makes more sense given the lazy-load default). Intention is for developers to optinally add this to above the folder images - that seemed like a good approach to override on a per image basis?

Otherwise, it returns the value of Image.default_lazy_load

This value won't be configurable, so it should just return 'loading="lazy"'

We should just supply the width and height attributes on our end if lazy loading is enabled.

I think I agree here. This would imply a bit of a change with the current default behaviour which doesn't specify the width and height on DBFile_image.ss. This might have some set image dimensions via CSS, though this is weird. We should probably always be setting width and height here.

[maxime-rainville]

Might make more sense to have this in ImageManipulation rather than DBFile. This needs needs to be chainable (e.g.: $Logo.SetWidth(123).EagerLoad() and $Logo.EagerLoad().SetWidth(123) should do the same thing).

You also need to consider what should happen if you call this on a regular file.

How are you planning on getting the loading attribute in DBFile_image.ss? The quick way to do it would be to stick a "Loading" method on DBFile/ImageManipulation and access that directly in your template. However, on most other methods, we have a getAttributes() method that returns a list attributes and we render the full list. This would have the nice side benefit of providing an extension points for people who might want to add custom attributes to to their images.

Don't really have an opinion on rather we call it EagerLoad() or LazyLoad(false) or Loading(). If we do have method that accepts a value, it might be nice to give the option to people to provide a boolean or a string.

[emteknetnz]

I'd probably prefer straight EagerLoad() since we're going with a non-configurable default loading value of 'lazy' and it's more straight forward than LazyLoad(false), which seems kind of backwards?

EagerLoad(false) will result in lazy-loading, and is functionally the same as omitting the function call altogether, so seems like it provides no value.

There's currently no signs on the horizon that there will ever be any loading attributes beyond 'lazy' | 'eager' | 'auto' (which is eager).

There's also no sign of this, though it may one day happen that browsers change their default behaviour from eager to lazy. In this scenario, the .EagerLoad() function still makes sense, it's just we'll need to update the backend functions to add loading='eager' to the HTML output rather than just omitting it altogether which is the current recommendation

[bergice]

In the intention here to return the whole attribute e.g. loading="lazy" or just lazy? The originally cited web.dev article recommends only either showing the loading="lazy" attribute or completely omitted it (see the warning section below the linked section above)

Because of this I think it would make sense to return the entire loading="lazy" string, or an empty string (i.e. not return null)

I think we should just use an if condition, then build the html inside the template. Returning an HTML string like that from a PHP function seems weird.

We can just skip rendering the attribute if it's null.
But a decision should be made if we wan't to render it if it's explicitly set as eager.

The original floated idea was to have a callable function from the template e.g. $MyImage.LazyLoad(false) or $MyImage.EagerLoad() (which probably makes more sense given the lazy-load default). Intention is for developers to optinally add this to above the folder images - that seemed like a good approach to override on a per image basis?

Correct, this will be added to ImageManipulation.

How are you planning on getting the loading attribute in DBFile_image.ss? The quick way to do it would be to stick a "Loading" method on DBFile/ImageManipulation and access that directly in your template. However, on most other methods, we have a getAttributes() method that returns a list attributes and we render the full list. This would have the nice side benefit of providing an extension points for people who might want to add custom attributes to to their images.

That's what I meant by adding the API earlier.

But I can add that to getAttributes() instead of calling it directly from the template and then refactor DBFile_image.ss to just render the attributes. getAttributesHTML is a FormField function. DBFile does not seem to extend this.

I'd probably prefer straight EagerLoad() since we're going with a non-configurable default loading value of 'lazy' and it's more straight forward than LazyLoad(false), which seems kind of backwards?

I'm in favor of LazyLoad(bool|string) because that's the first thing developers will be looking for.

There's currently no signs on the horizon that there will ever be any loading attributes beyond 'lazy' | 'eager' | 'auto' (which is eager).

We don't know that for sure.

There's also no sign of this, though it may one day happen that browsers change their default behaviour from eager to lazy. In this scenario, the .EagerLoad() function still makes sense, it's just we'll need to update the backend functions to add loading='eager' to the HTML output rather than just omitting it altogether which is the current recommendation

We don't know that for sure either.
But if it does happen, we can just deprecate the old API and introduce a new one.

---

What WP does is allow string|bool when setting the lazy loading. It seems like this is a more flexible and user friendly approach as it allows the on/off logic, but also custom values.
Note that the specifications mention that the loading attribute has a default fallback for invalid values as well.

[maxime-rainville]

I think LazyLoad(bool|string) is probably a pretty good approach.

We should try our best to replicate behaviours for similar classes. So adding a getAttributesHTML() method that generates an HTML string by calling a getAttributes() method is advisable. Reusing pattern consistently makes it easier for devs to understand how to interact with our classes.

You seem to have this nailed down.

[emteknetnz]

bool|string - like a mixed type? Can we not do that? Mixed types are confusing and just add complexity

e.g. LazyLoad('eager') doesn't really make sense as a way to modifying the loading attribute

Given we've already decided the default value is non-configurable, it seems like the logical options that use a strict type or no argument are:

• LazyLoad(false)
• DoNotLazyLoad() (odd)
• EagerLoad()
• LoadingAttribute('eager') (outcome is no loading attribute attribute? so this is weird)
• LoadingAttribute('') (also weird?)

[maxime-rainville]

I guess if you're going to have a method that accepts a value, calling Loading('someValue) is probably the best so it matches the actual attribute.

If the method doesn't accept a parameter, than it needs a matching method to undo it.

Let's maybe pick 4/5 options throw them in the bespoke channel and see what makes the most sense to folks. If the point of this is to get devs to adopt it, it's probably worth spending some time figuring out what makes the most sense to the average dev.

It shouldn't hold back André from progressing this. It's just changing a method name later on.