Skip to:
Content

BuddyPress.org

Opened 4 months ago

Closed 7 weeks ago

Last modified 7 weeks ago

#8340 closed enhancement (fixed)

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 2 months ago.
8340.02.patch (5.0 KB) - added by r-a-y 2 months ago.
Adds filter for private message content, removes filter for 'bp_get_signup_avatar'
8340.noiframe-fixunittest.patch (2.4 KB) - added by imath 2 months ago.
8340.test-activity-oembed-iframe.patch (2.3 KB) - added by r-a-y 2 months ago.
8340.03.patch (5.3 KB) - added by r-a-y 2 months ago.

Download all attachments as: .zip

Change History (20)

#1 @espellcaste
4 months ago

I think that's a nice addition for 7.0!

#2 @imath
2 months ago

Hi @r-a-y

Did you have the opportunity to progress on this?

#3 @r-a-y
2 months 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
2 months ago

@r-a-y
2 months ago

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

#4 @imath
2 months 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
2 months 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 2 months ago by r-a-y (previous) (diff)

#6 @imath
2 months 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 months 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
2 months 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
2 months 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
2 months ago

#10 @imath
2 months 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 👍

#11 @imath
7 weeks ago

Hi @r-a-y I was to package the 7.0.0-beta1 today (but I'm too tired tonight to do so 🥱), and I was wondering if there was something more we needed to do about the patch. Is it ok for you to have it committed? FYI I'll package beta1 tomorrow 👌

#12 @r-a-y
7 weeks ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 12757:

Add lazy loading support for images and iframes.

In order to take advantage of native browser support for lazy loading,
this commit adds the 'loading="lazy"' attribute to all <img> and
<iframe> elements used throughout BuddyPress (activity content,
private messaging content and avatar display).

The generic function, bp_core_add_loading_lazy_attribute(), is
introduced to help with this.

A special case is made for WordPress post embeds. As of this commit,
only Chromium-based browsers (77+) support iframe lazy loading if the
iframe is visible on load. (See https://web.dev/iframe-lazy-loading/#iframe-specific-lazy-loading-behavior.)
Since WordPress post embeds uses inline CSS to hide their iframe on
initial load, we need to remove their inline CSS so lazy loading can
be properly applied.

Props r-a-y, imath.

Fixes #8340.

#13 @r-a-y
7 weeks ago

Thanks for the nudge and feedback for this ticket, imath. Just committed the patch.

Also thanks for all the work you've done not just for this release, but for the past couple of releases as well! Merci beaucoup!

#14 @imath
7 weeks ago

In 12760:

PHP Unit tests: fix avatar filter failing test

This is a follow up of [12757] in order to take in account the lazy loading attribute when testing the avatar filter output.

See #8340

#15 @imath
7 weeks ago

Hi @r-a-y Thanks for committing the patch and for your kind message 😍.

Note: See TracTickets for help on using tickets.