NPM Resource Changed ? Pending

User tests: Successful: Unsuccessful:

avatar zero-24
zero-24
29 Jul 2020

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.

avatar zero-24 zero-24 - open - 29 Jul 2020
avatar zero-24 zero-24 - change - 29 Jul 2020
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 29 Jul 2020
Category Layout Libraries Front End Plugins
avatar dgrammatiko
dgrammatiko - comment - 29 Jul 2020

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.

avatar zero-24
zero-24 - comment - 29 Jul 2020

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.

avatar dgrammatiko
dgrammatiko - comment - 29 Jul 2020

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!!!

avatar wilsonge
wilsonge - comment - 29 Jul 2020

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

avatar dgrammatiko
dgrammatiko - comment - 29 Jul 2020

direct recommendation

That's a recommendation...

avatar zero-24 zero-24 - change - 29 Jul 2020
Labels Added: ?
avatar brianteeman
brianteeman - comment - 29 Jul 2020

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

avatar dgrammatiko
dgrammatiko - comment - 29 Jul 2020

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 ?

avatar zero-24
zero-24 - comment - 29 Jul 2020

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.

avatar brianteeman
brianteeman - comment - 29 Jul 2020

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

avatar dgrammatiko
dgrammatiko - comment - 29 Jul 2020

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

avatar zero-24
zero-24 - comment - 29 Jul 2020

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.

avatar brianteeman
brianteeman - comment - 29 Jul 2020

we just changed the default layout ;)

???

avatar zero-24
zero-24 - comment - 29 Jul 2020

???

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

avatar dgrammatiko
dgrammatiko - comment - 29 Jul 2020

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...

avatar zero-24
zero-24 - comment - 29 Jul 2020

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.

avatar brianteeman
brianteeman - comment - 29 Jul 2020

???

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

avatar dgrammatiko
dgrammatiko - comment - 29 Jul 2020

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

avatar zero-24
zero-24 - comment - 29 Jul 2020

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.

avatar joomla-cms-bot joomla-cms-bot - change - 29 Jul 2020
Category Layout Libraries Front End Plugins Libraries Front End Plugins
avatar dgrammatiko
dgrammatiko - comment - 29 Jul 2020

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

avatar zero-24
zero-24 - comment - 29 Jul 2020

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.

avatar SharkyKZ
SharkyKZ - comment - 30 Jul 2020

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

avatar dgrammatiko
dgrammatiko - comment - 30 Jul 2020

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.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...

avatar brianteeman
brianteeman - comment - 30 Jul 2020

Can you please update the test instructions

avatar zero-24 zero-24 - change - 30 Jul 2020
The description was changed
avatar zero-24 zero-24 - edited - 30 Jul 2020
avatar joomla-cms-bot joomla-cms-bot - change - 30 Jul 2020
Category Libraries Front End Plugins JavaScript Repository NPM Change Libraries Front End Plugins
avatar dgrammatiko
dgrammatiko - comment - 30 Jul 2020

You need to put px at the end

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

avatar zero-24
zero-24 - comment - 30 Jul 2020

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?

avatar dgrammatiko
dgrammatiko - comment - 30 Jul 2020
width="${Joomla.selectedFile.width}" height="${Joomla.selectedFile.height}"

to

width="${Joomla.selectedFile.width}px" height="${Joomla.selectedFile.height}px"
avatar zero-24 zero-24 - change - 30 Jul 2020
Labels Added: NPM Resource Changed
avatar dgrammatiko
dgrammatiko - comment - 30 Jul 2020

@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....

avatar zero-24
zero-24 - comment - 30 Jul 2020

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.

image

@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?

avatar zero-24
zero-24 - comment - 23 Sep 2020

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

avatar zero-24 zero-24 - close - 23 Sep 2020
avatar zero-24 zero-24 - change - 23 Sep 2020
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2020-09-23 16:11:58
Closed_By zero-24

Add a Comment

Login with GitHub to post a comment