Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[4.0] Implement loading=lazy for images #28838

Merged
merged 12 commits into from
May 30, 2020
Merged

Conversation

zero-24
Copy link
Contributor

@zero-24 zero-24 commented Apr 27, 2020

Pull Request for Issue #27761

Summary of Changes

  • Implement a new loading=lazy attribute for images on content via a plugin that can be disabled.
  • Implement a default enabled loading=lazy option that can be overwritten by extension devs useing the options array.

As discussed here: https://volunteers.joomla.org/departments/production/reports/1226-production-dept-meeting-april-21-2020

Allow progressive features
Joomla's current policy is that we only accept changes or features that work for everything that meets our minimum requirement (e.g. PHP versions, browsers, database versions etc..)

The motion goes beyond this by allowing progressive features. This allows us to take advantage of new technology for those that use it as long as there is no adverse effect to those that don't (this should also take into account the support/documentation affects on such conditional features). As an example we could look at "lazy loading" or at "same site cookies". This motion means that a PR or ISSUE should not be closed or rejected on the argument that it IS a progressive feature. The acceptance of such an issue or feature is still subject to the usual and accepted scrutiny and discussion.

We are now even official allowed to introduce featues like this here that don't break existing sites and just improve the overall expiricece for people using modern browsers.

Feature

This PR sets the lazyloading attribute to all images that passes the onContentPrepare Event or HTMLHelper::image to allow modern browsers to lazyload the images.

More information about the loading attribute:

Testing Instructions

  • install 4.0
  • setup one article with images and atd it to the frontend
  • apply this PR
  • discover install the plugin
  • enable the plugin
  • check the frontend
  • see that it not has the lazyloading attribute
  • disable the plugin
  • see that the loading attribute is not set

Expected result

Joomla by its core can set the loading=lazy attribute to images.

Actual result

Joomla can not set that loading=lazy attribute.

Documentation Changes Required

A doc page will be produced on this, I have just added the label and assigned it to me.

Beyond this PR

There is a backport of this feature / plugin for Joomla 3 here: GitHub JED

Special thanks

Thanks @SniperSister and @felixarntz for your support on that PR.

Copy link

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not familiar with Joomla, but the implementation here looks solid, also in terms of covering the common cases I would expect to be covered (images being output via dedicated function or as part of "content blob"). 👍

@viocassel
Copy link
Contributor

But what about loading=lazy for iframe?
For example: <iframe src="video-player.html" loading="lazy"></iframe>

@zero-24
Copy link
Contributor Author

zero-24 commented Apr 27, 2020

But what about loading=lazy for iframe?

Sounds interesting. Will take a closer look into the implementation my main concern would be that nowdays you should not use iframes that much any more right? Do you know wether there are possible side affects by adding that loading to iframes?

@felixarntz
Copy link

@viocassel @zero-24 While I think loading=lazy on iframe tags will come in handy, I think that should be revisited separately. While some browsers already support it, it's not part of any approved specification yet, so I think it would be a bit early. But following up on this in a separate effort definitely sounds like a good idea.

@zero-24
Copy link
Contributor Author

zero-24 commented Apr 27, 2020

Thanks for you input on that @felixarntz extending the plugin is easy when there is a final specification out for sure.

@dgrammatiko
Copy link
Contributor

But what about loading=lazy for iframe?

Please, please don't use experimental features:
https://twitter.com/zcorpan/status/1242408072129187847

@C-Lodder
Copy link
Member

Will this not conflict with https://extensions.joomla.org/extension/photos-a-images/images/imagelazyloading?

E.g if:

A user has a Joomla 3 site
Installs this extension from JED
Updates to J4

@zero-24
Copy link
Contributor Author

zero-24 commented Apr 29, 2020

No it is a different extension name.

@zero-24
Copy link
Contributor Author

zero-24 commented Apr 29, 2020

plg_content_imagelazyloading vs plg_content_imagelazyload ;-)

@luisorozoli
Copy link

I have tested this item ✅ successfully on d36505f

Tested successfully in Launch Joomla in J4. :)


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/28838.

@Quy
Copy link
Contributor

Quy commented Apr 30, 2020

I have tested this item ✅ successfully on d36505f


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/28838.

@Quy
Copy link
Contributor

Quy commented Apr 30, 2020

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/28838.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Apr 30, 2020
@zero-24
Copy link
Contributor Author

zero-24 commented Apr 30, 2020

Thanks for all the feedback and the tests beeing done here 👍

@laoneo
Copy link
Member

laoneo commented May 1, 2020

I'm really in favor to use the loading attribute. But why adding another plugin? We can just change it in the layouts to use the loading attribute and then it can be overridden when required. Every plugin has negative impact on page load so we should choose it wisely. Just my 2 cents here.

@zero-24
Copy link
Contributor Author

zero-24 commented May 1, 2020

By using a plugin everyone can disable that feature and we are not hardcoding a feature in com_content as well as this works for all extensions that fire the onContentPrepare event.

@Bakual
Copy link
Contributor

Bakual commented May 20, 2020

I also don't see why this would be needed. Even for upgrades, the site will work just fine without that lazy loading attribute.
Could it be more performant? Probably. But those who care, can also solve it themself already.

For those who don't care, the lazy loading doesn't matter and the impact it has will be nothing since they likely have far bigger issues to solve.

@laoneo
Copy link
Member

laoneo commented May 20, 2020

So we agree now on the approach. Great

On which approach? To inform the user after Joomla got upgraded to 4? Yes sure, you can add there a whole summary of performance optimization suggestions. I do not care. What I care is that there is no need for a plugin.

@HLeithner
Copy link
Member

This is not sustainable, the whole thinking needs to shift to lighter, less opinionated codebase. Joomla tries to do so much and fails miserably on most of them...

I think that's not true. Anyway...

I'm in favor for a Joomla managed component that does such "optional" things on upgrade, I'm sure there are more things that can be done after upgrading to j4 a more or less migration component (even if we don't need it for j4 upgrade). Which could be installed by a post upgrade message or by pointing to JED.

On the other side we see how good com_weblinks is maintained...

@zero-24
Copy link
Contributor Author

zero-24 commented May 20, 2020

Maybe even disabled by default?

Tobias the argument is that J4 is becoming extremely heavy, iirc it's 30% more plugins compared to J3... So people objecting here probably are more into a lightweight core, with proper API, instead of questionable implementations for any user case. Joomla cannot possibly solve all the possible user cases but still tries to do so. This is not sustainable, the whole thinking needs to shift to lighter, less opinionated codebase. Joomla tries to do so much and fails miserably on most of them...

This has nothing todo with this plugin than right? It is more a general thing that should be discussed in in a different issue i guess.

Btw is that 30% a real number or just a feeled number? Yes we have more plugins but for the site load we have the following right now:

Content 3.x 10; 4.x 11 (with this plugin included)
System 3.x 18 4.x 21

As for the system plugins it is that most of the new of them are not running on all sites or have sesible defaults to skip early on the site nor have a that bad impact.

What other pluigins influence the inital page load?

Sure we added a lot for media manager and webservices but they usually not run on events that Influence the intial page load right?

@dgrammatiko
Copy link
Contributor

This is not sustainable, the whole thinking needs to shift to lighter, less opinionated codebase. Joomla tries to do so much and fails miserably on most of them...
I think that's not true. Anyway...

I wouldn't make such a comment without backing it with some hard data, and yes Joomla is failing hard to deliver competitive sites: https://joomla-needed-fixes.netlify.app/data/ (this is all of the showcase sites with most of them still running on HTTP instead of HTTPS, having a blank page for more than 3 seconds before anything is painted, etc)

Sorry that's the hard truth 😞

Btw is that 30% a real number or just a feeled number

Oh yes, it's very real, count ALL the plugins.

@zero-24
Copy link
Contributor Author

zero-24 commented May 20, 2020

I also don't see why this would be needed. Even for upgrades, the site will work just fine without that lazy loading attribute.
Could it be more performant? Probably. But those who care, can also solve it themself already.

For those who don't care, the lazy loading doesn't matter and the impact it has will be nothing since they likely have far bigger issues to solve.

Yes that is the point what speaks against moving that to the most sites by default? I guess you all saw that post of the motivation behind lazyloading and that images are the things that are consuming the most resouces on pageload as well as on the servers.

So i would like to turn the questions why should we not do that?

As it improves the Performance for all sites with nearly zero costs for us nor the site owner.

The reality is most of them don't care about all that that's the reason i think we should come up with good defaults thag can be disabled by the people that do have issues with it.

So the general people can stop claming 'joomla is bad for performance because they dont do layzloading' like they do for other things we dont do.

It is something that is build in native in the browsers not a JS lib with a gazillion depentecys right?

@dgrammatiko
Copy link
Contributor

are the things that are consuming the most resouces on pageload as well as on the servers

NO, you're wrong here. The problem is the excessive CSS (bootstrap and font awesome) and all the JS that is not deferred. These are RENDER BLOCKING meaning the visitor of the site stairs on a blank screen until these are downloaded, parsed and executed. Images are not render blocking thus the only negative effect is their size, which by the way loading=lazy is not solving

@zero-24
Copy link
Contributor Author

zero-24 commented May 20, 2020

According to HTTPArchive, images are the most requested asset type for most websites and usually take up more bandwidth than any other resource. At the 90th percentile, sites send about 4.7 MB of images on desktop and mobile. That's a lot of cat pictures.

https://web.dev/native-lazy-loading/

This is what i'm talking about. @dgrammatiko

We don't fix all issues with it for sure but we can make a good step in the right direction for all users of Joomla.

@dgrammatiko
Copy link
Contributor

We don't fix all issues with it for sure

The loading=lazy can hardly be assumed as a solution to the size images. For that you need srcset and a plugin like: https://ttc-freebies.github.io/plugin-responsive-images/
Which by the way I wrote it 3 years ago with the hope that the GSOC would have brought it in the core (I didn't do that due to lack of time, I was already at that point involved in way too many sub-projects). What they did instead was to try, again, to fulfil a niche market with adaptive images which is a total failure from workflow perspective (upload the image, then go edit it, then use it, and then nobody will follow such stupid workflow in 2020 😂).
In short, what I stated before: Joomla tries to solve every possible user case and it fails miserably...

@zero-24
Copy link
Contributor Author

zero-24 commented May 20, 2020

We don't fix all issues with it for sure

The loading=lazy can hardly be assumed as a solution to the size images. For that you need srcset and a plugin like: https://ttc-freebies.github.io/plugin-responsive-images/
Which by the way I wrote it 3 years ago with the hope that the GSOC would have brought it in the core (I didn't do that due to lack of time, I was already at that point involved in way too many sub-projects). What they did instead was to try, again, to fulfil a niche market with adaptive images which is a total failure from workflow perspective (upload the image, then go edit it, then use it, and then nobody will follow such stupid workflow in 2020 😂).
In short, what I stated before: Joomla tries to solve every possible user case and it fails miserably...

Agree what about starting an RFC on your proposed pluigin and with your permission we build a core plugin based on yours. You don't have to do the coding when you don't want to. But starting the discussion would help. I for example was not aware of that plugin.

@wilsonge wilsonge merged commit d036927 into joomla:4.0-dev May 30, 2020
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label May 30, 2020
@wilsonge
Copy link
Contributor

I've had a chat with @zero-24 about this offline and have agreed to merge this (it was my idea to only have it enabled for existing installs). I do understand the cons of performance here. And definitely we should be advocating for people to fix this properly in the database. But I don't think it's realistic to believe that the majority of people will do this either.

@wilsonge wilsonge added this to the Joomla 4.0 milestone May 30, 2020
@dgrammatiko
Copy link
Contributor

@wilsonge even the googler devs were against this: GoogleChrome/lighthouse-stack-packs#44 (comment)

Also do you realise that I already have:

  • proper back port for J3 (plugin)
  • proper db update (component)

You want to have them under Joomla because you don't trust me? Fork them, their OSS

@wilsonge
Copy link
Contributor

No it was the google devs suggestion. Tobias was the one interacting with them so I think he knows best what they were suggesting

@dgrammatiko
Copy link
Contributor

@wilsonge probably it's my bad English but even Tobias responds that the plugin will be the first on the search. So it wasn't their suggestion at any point
Screenshot 2020-05-30 at 18 57 33

@Bramfield
Copy link

Sounds interesting. Will take a closer look into the implementation my main concern would be that nowdays you should not use iframes that much any more right? Do you know wether there are possible side affects by adding that loading to iframes?

How can this be implemented for an iframe?


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/28838.

@zero-24
Copy link
Contributor Author

zero-24 commented Aug 10, 2023

That plugin has been reverted out of the core before the final release was made.

Please find here more details about lazy loading iframes:

https://web.dev/iframe-lazy-loading/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet