[4.0] Disable lazyloading on images that have no dimensions or added on the first page view

[zero-24]

Pull Request for Issue #29250

Summary of Changes

Disable lazyloading on images that have no dimensions or get added on the first page view

• intro images and full images should be at 99% of the time one of the first things we have on that page and that we don't have dimensions on that we should not try to lazyload them.
• the opt-out stuff via the plugin and the HTMLHelper methods has been switched to only add that tag when height and width attributes are present.

Testing Instructions

• add inline images to an article that dont have dimensions setup in the image tag
• add inline images with the dimensions setup correctly
• check the frontend and notice that they are lazyloaded (meaning the lazyload tag is added there)
• apply this patch
• notice that this is no longer the case

Actual result BEFORE applying this Pull Request

• all images also without height and width attributes has been lazyloaded

Expected result AFTER applying this Pull Request

• per default only images with height and width attributes are now lazyloaded

Documentation Changes Required

none

cc @dgrammatiko given that you wanted to work on that too.

[dgrammatiko]

DO NOT DISABLE LAZYLOAD!!!

The message is a notification not an error, that simply translates to: the browser didn't found any width/height so it has to calculate those at run time. It's totally fine and much better than no lazy loading. For best performance the width and height need to be set per image.

[zero-24]

It is not about the message it is about the spec for lazyloading that is strongly suggesting using width/height attributes. Without your side can be shifted arround.

[dgrammatiko]

Without your site can be shifted arround.

You're wrong, even a simple image without width and height will also push things. So you're disabling a very good implementation without gaining anything. This PR is totally wrong, please close it

PS If the project still wants me to fix this then you have to roll back the plugin for the lazy loaded images because I can't and won't fix that, basically because it's wrong!!!

[wilsonge]

This is based on a direct recommendation from google having discussed the height and width attributes with them.

[dgrammatiko]

direct recommendation

That's a recommendation...

[brianteeman]

I can see the logic about the sizes (not that I agree)
The comment about these images being 99% the first things that are being loaded is just plain wrong especially in a blog page

[dgrammatiko]

The comment about these images being 99% the first things that are being loaded is just plain wrong especially in a blog page

People are complaining about shifting layouts here when Joomla has several seconds of blank screen and then everything pop up out of nowhere. This is acceptable but images pushing content is unacceptable 🤔

[zero-24]

The comment about these images being 99% the first things that are being loaded is just plain wrong especially in a blog page

This should be true for the full image right? I have taken off the intro image already.

[brianteeman]

This pr also ignores that the use of aspect-ratio (https://caniuse.com/#search=aspect-ratio) also prevents the reflow problem.

You can never assume that an article has its image at the top of the page. That might be the case with the default layout (you can change it) and without a big header or modules above the article.

If you really want to deal with this then the only correct way would be to add support for specifying the image dimensions for both intro and full images

[dgrammatiko]

The proper solution is around 10 LOC but requires removal of the plugin...

[zero-24]

You can never assume that an article has its image at the top of the page. That might be the case with the default layout (you can change it) and without a big header or modules above the article.

yes agree on that this is the reasone we just changed the default layout ;)

The proper solution is around 10 LOC but requires removal of the plugin...

Feel free to do that 10 LOC and remove the plugin when it works better i'm more than happy.

[brianteeman]

we just changed the default layout ;)

???

[zero-24]

???

in this PR i have updated (concerning the full image stuff) just the default layout shipped with the core.

[dgrammatiko]

and remove the plugin

That's not on my todo list, I didn't commit the rest of the code when George decided to merge that plugin although I was strongly against it. I won't play this blame game...

[zero-24]

That's not on my todo list, I didn't commit the rest of the code when George decided to merge that plugin although I was strongly against it. I won't play this blame game...

Well I don't know the 10 LOC that you proposed. The plugin approache was the first step to move in the right direction. When there are better ways to acomplish the task I'm happy to take a look into that.

[brianteeman]

???

in this PR i have updated (concerning the full image stuff) just the default layout shipped with the core.

You misunderstood. full_image layout can be included anywhere in the article. By default
<?php echo LayoutHelper::render('joomla.content.full_image', $this->item); ?>
is near the top of the article but there is no reason for it not to be moved to the bottom

[dgrammatiko]

The plugin approache was the first step to move in the right direction

The plugin was never the right direction as many people told you in that PR

[zero-24]

The plugin was never the right direction as many people told you in that PR

As mention I'm happy to review other solutions. Right before that PR and plugin we had nothing about lazyloading (even shutdown an issue) so it was some kind of first step to have that attribute supported at all. I'm happy to move forward and take the better solutions when we have them.

You misunderstood. full_image layout can be included anywhere in the article.

hmm agree will add back that attribute for now not having that other attributes can be solved outside of this PR.

[dgrammatiko]

I'm happy to move forward and take the better solutions when we have them.

Sorry but that won't be by me I'm out

[zero-24]

Sorry but that won't be by me I'm out

Ok, i just mention you as you proposed to work on that in the issue that brian opend.

[SharkyKZ]

+1 for removing the plugin. It's neither here nor there. If anything, media manager should be improved to allow attributes when inserting images.

[dgrammatiko]

If anything, media manager should be improved to allow attributes when inserting images.

There is nothing that needs to be improved in the media manager!!!

All it takes is to translate my comment #28838 (comment) to code like:

from:

joomla-cms/build/media_source/system/js/fields/joomla-field-media.w-c.es6.js

Line 39 in c91326c

Joomla.editors.instances[editor].replaceSelection(`<img loading="lazy" src="${Joomla.selectedFile.url}" alt=""/>`);

to:

          Joomla.editors.instances[editor].replaceSelection(`<img loading="lazy" src="${Joomla.selectedFile.url}"  width="${Joomla.selectedFile.width}" height="${Joomla.selectedFile.height}" alt=""/>`);

This PR totally hideous as the lazyload plugin, but keep on doing what you're doing here...

[brianteeman]

Can you please update the test instructions

[dgrammatiko]

You need to put px at the end

and can you please remove the plugin, it's not doing anything useful anymore?

[zero-24]

Can you please update the test instructions

Done.

All it takes is to translate my comment #28838 (comment) to code like:

Awesome just commited the change I have also updated the line 41 with a similiar change.

You need to put px at the end

Where do you want me to add that?

[dgrammatiko]

width="${Joomla.selectedFile.width}" height="${Joomla.selectedFile.height}"

to

width="${Joomla.selectedFile.width}px" height="${Joomla.selectedFile.height}px"

[dgrammatiko]

@zero-24 I provided the proper solution (it's half there, it's not covering all the cases) based on the fact that you will also remove the stupid plugin. Endorsing my code but not removing the plugin is really dishonourable. It's another way of saying "f#$ off" to someone that was very clear many times in this PR that the correct code exists but requires the removal of the useless plugin.

So long, and thanks for the fish....

[zero-24]

Endorsing my code but not removing the plugin is really dishonourable. It's another way of saying "f#$ off" to someone that was very clear many times in this PR that the correct code exists but requires the removal of the useless plugin.

I'm sorry it seems you edited the original commet in this thread here? I only got the mention of the px stuff in the mail notification and when i wrote my answear on github.

@zero-24 I provided the proper solution (it's half there, it's not covering all the cases) based on the fact that you will also remove the stupid plugin.

Will take a look into the plugin removal in the comming days, thanks. What exactly is not covered yet (with the plugin removed) that we have to work on to get it right?

[dgrammatiko]

@wilsonge what's the impact of this plugin after these 2 conditionals (width, height) given that the media selector doesn't even provide entry points for these attributes?
Sorry to be an @$$ all the time but this is targeting something less than 1% of the current 3.x sites, so it shouldn't be part of the core. My offer for both https://github.com/ttc-freebies/joomla-3.x-images-lazy-loading and https://github.com/ttc-freebies/com_addlazyloading is still there, fork it or do whatever you want with those...

[zero-24]

New PR wich includes the changes from here as well as dops the core plugin as suggested can be found here: #30748