Skip to:
Content

BuddyPress.org

Opened 2 months ago

Last modified 15 hours ago

#8340 assigned enhancement

Support native browser image lazy-loading

Reported by: r-a-y Owned by: r-a-y
Milestone: 7.0.0 Priority: normal
Severity: normal Version:
Component: Core Keywords: has-patch commit
Cc:

Description

Let's add <img loading="lazy"> support for any place we output the <img>` tag.

This is mainly for avatars, but could potentially include injecting the loading attribute for images in activity stream content.

The loading attribute is supported in most modern browsers:
https://caniuse.com/#feat=loading-lazy-attr

Safari also supports it, but has to be enabled via an experimental flag.

Should be quite easy to put a patch together. I'll work on this for 7.0.

Attachments (5)

8340.01.patch (4.9 KB) - added by r-a-y 5 days ago.
8340.02.patch (5.0 KB) - added by r-a-y 5 days ago.
Adds filter for private message content, removes filter for 'bp_get_signup_avatar'
8340.noiframe-fixunittest.patch (2.4 KB) - added by imath 5 days ago.
8340.test-activity-oembed-iframe.patch (2.3 KB) - added by r-a-y 5 days ago.
8340.03.patch (5.3 KB) - added by r-a-y 37 hours ago.

Download all attachments as: .zip

Change History (15)

#1 @espellcaste
2 months ago

I think that's a nice addition for 7.0!

#2 @imath
6 days ago

Hi @r-a-y

Did you have the opportunity to progress on this?

#3 @r-a-y
6 days ago

I've introduced a new generic function, bp_core_add_loading_lazy_attribute(), so we can inject the loading="lazy" attribute into the various available filters we have. I should note that it doesn't check for the existence of an existing loading="lazy" attribute. If we want to do some checks before adding the attribute, let me know and I'll do it, but I think we should be good.

In places where there are no filters, I've manually added the loading="lazy" attribute.

I've also chosen to do this for places where we add <iframe> content. This is mostly for oEmbed content. Only Chrome supports <iframe loading="lazy"> at the moment, but Firefox will probably add support later on.

@r-a-y
5 days ago

@r-a-y
5 days ago

Adds filter for private message content, removes filter for 'bp_get_signup_avatar'

#4 @imath
5 days ago

  • Keywords has-patch added

Hi @r-a-y

Thanks a lot for your work on this. It works as expected in many places except when a post has been embed into an activity in BP Nouveau. Maybe it's best to only add lazy loading to images ?

I also noticed a test was failing.

8340.noiframe-fixunittest.patch addresses these 2 cases.

#5 @r-a-y
5 days ago

It works as expected in many places except when a post has been embed into an activity in BP Nouveau. Maybe it's best to only add lazy loading to images ?

Hi imath, can you tell me what you mean here? If I post an activity update from the Sitewide Activity page with a YouTube link, the <iframe> includes the loading="lazy" attribute with the patch applied. I've attached a unit test. Be sure to uncomment the line in the patch to test the addition of the loading attribute.

I noticed that oEmbed isn't working on activity replies after the initial AJAX response in Nouveau. It works after the page is loaded refreshed. However, it works in bp-legacy as expected. Is that what you mean? If so, I'll work on fixing that.

Last edited 5 days ago by r-a-y (previous) (diff)

#6 @imath
4 days ago

The issue happens where you’ve said and yes the refresh of the page makes the embed post displays. But if you go on the permalink of this activity it doesn’t display even if you refresh the page.

#7 @r-a-y
2 days ago

I noticed that oEmbed isn't working on activity replies after the initial AJAX response in Nouveau.

I've fixed this for activity comments. See #8370.

I'm still keen on adding loading="lazy" to iframes, so let me know if there are any other issues with oEmbed with Nouveau.

#8 @imath
41 hours ago

  • Keywords needs-patch added; has-patch removed

Hi @r-a-y

As I've said earlier lazy loading iframe is not working in Nouveau. I've tried with the patch you added on #8370 and it doesn't change anything.

Here are screenshots of what we currently get without lazy loading iframe :

Activity directory:

https://cldup.com/zIEIaZrffe.png

Activity permalink:

https://cldup.com/s91tqO7o82.png

Here are screenshots of what we get if we apply your patch :

Activity directory:
https://cldup.com/ZrW_JBMY2z.png

Activity permalink:
https://cldup.com/zklEN4xJkV.png

So to me lazy loading iframe is a no go unless it's actually working in BP Nouveau, our default template pack.

#9 @r-a-y
37 hours ago

imath, thanks for the screenshots! I was only testing with YouTube.

The problem with WordPress post embedding is it adds "position: absolute; clip: rect(1px, 1px, 1px, 1px);" to the iframe.

Currently, iframe lazyloading in Chrome only works if it is visible, so 03.patch fixes this by removing the absolute positioning. With the patch applied, I tested this with Firefox and iframes still work as expected so we should be safe. Let me know what you think.

@r-a-y
37 hours ago

#10 @imath
15 hours ago

  • Keywords has-patch commit added; needs-patch removed

Thanks a lot for your work on this ticket @r-a-y I confirm your last patch is fixing the WP Embed issue.

Let's have it in 👍

Note: See TracTickets for help on using tickets.