WordPress.org

Make WordPress Core

Opened 5 weeks ago

Closed 3 weeks ago

Last modified 3 weeks ago

#50933 closed defect (bug) (fixed)

Lazy loading in 5.5 causes flashing of custom logo in Firefox

Reported by: demetris Owned by: desrosj
Milestone: 5.5.1 Priority: normal
Severity: normal Version: 5.5
Component: Media Keywords: good-first-bug has-patch commit
Focuses: Cc:

Description

That is, when navigating from post to post or from page to page using, for example, the site menu links, the logo flashes sometimes — not always but more often than not.

Happens with Twenty Twenty as well as a custom theme I am working on.

Happens with Firefox 80 (Beta channel) and Firefox 81 (Nightly channel) on Windows 10.

Does not happen with Chrome 84 on Windows 10.

Does not happen if I disable lazy loading using wp_lazy_loading_enabled.

Do other people see the same behaviour?

If yes, I imagine this is something that could or will be fixed in Firefox, but maybe we could do something in the meantime.

For example, we could disable lazy loading for the logo image when calling wp_get_attachment_image() from get_custom_logo() — unless we have reasons to use lazy loading for the custom logo.

Cheers!

Attachments (3)

50933.diff (554 bytes) - added by rebasaurus 4 weeks ago.
Disable lazy loading as default
general-template-50933.patch (646 bytes) - added by demetris 4 weeks ago.
Disable lazy loading for custom logo, with explanatory comment
50933.1.diff (986 bytes) - added by audrasjb 4 weeks ago.
Media: Use false value for loading attribute to disable lazy loading by default on get_custom_logo()

Download all attachments as: .zip

Change History (29)

#1 @demetris
5 weeks ago

  • Version set to 5.5

#2 @johnbillion
5 weeks ago

  • Component changed from General to Media
  • Keywords needs-testing added
  • Milestone changed from Awaiting Review to 5.5.1

Moving to 5.5.1 for investigation.

#3 @johnbillion
5 weeks ago

  • Keywords 2nd-opinion added; needs-testing removed

#4 @khag7
4 weeks ago

I'm having trouble reproducing. WP 5.5, Twenty Twenty, no plugins. Maybe I'm not sure exactly what I'm looking for.

@demetris can you reproduce it on a clean install? Can you show a screen recording of what you're seeing? Or anyone else able to reproduce this?

#5 @demetris
4 weeks ago

Thanks for having a look, Kevin.

Here is a screencast of what I am seeing:

https://vimeo.com/449575076

Using Firefox 80 (Beta channel) I click 17 times to navigate from page to page. The flashing happens 3 times. Usually, it happens more often.

The layout shift along with the flashing is, I think, a by-product of the issue I am seeing. It does not happen if the logo dimensions are specified in the stylesheet.

Here is the test website:

https://0.kikizas.net/

It is on WordPress 5.5 and has two active plugins: GitHub Updater and Two Factor.

And here is a plugin I just made (can be installed with GitHub Updater) to quickly disable native lazy loading in WordPress 5.5:

https://github.com/demetris/disable-native-lazy-loading

#6 @demetris
4 weeks ago

Forgot to add mention: @khag7

#7 follow-up: @khag7
4 weeks ago

I guess I noticed that and thought it was just the page redrawing.

It seems like the layout is shifting from before the image is loaded to after.

Ticket #50367, titled "Avoid layout shifting due to images not having dimension attributes" seems to describe a similar situation. In that ticket, they describe the necessity for lazy loaded images to have a height and a width defined. Our site logo has that (at least on Twenty Twenty) but it also has CSS styles which affect the height and the width. So if an image is larger than what the CSS allows for, maybe there's some issue?

Before I go too far down that path, though, should we just consider not lazy loading the site logo? If the logo is at the top, one of first thing shown in the vast majority of themes, it's not going to be a performance impact to just load it normally. If it fixes this bug, great. Is there a downside?

EDITS:

  1. Another thing I notice. Your site logo on your test site is 576px square. The one I was testing with was 120x90, and I am not having any issue. When I try a larger image on my site, I am having the issue. If you look at the HTML on your test site, the height and width attributes of the image are showing 576 for each, but the CSS prevents it from showing that large. Not sure why this would affect anything involving lazy loading, but I'm observing that the flashes occur if lazy loading is enabled for the site logo and if height and width are at that size.
  1. Everything works fine if I disable lazy loading in firefox (about:config and then find the flag called dom.image-lazy-loading.enabled and disable it). Or you can disable it in wordpress with add_filter('wp_lazy_loading_enabled', '__return_false'); -- either way, disabling lazy loading from server or client fixes the problem.
  1. As far as disabling lazy loading for just the logo, this works:
    add_filter( 'get_custom_logo_image_attributes', function( $attributes ){
    	$attributes['loading'] = 'eager';
    	return $attributes;
    });
    
  1. Related: #50425 - where we were warned against lazy-loading images that are likely to be in the initial viewport.
Last edited 4 weeks ago by khag7 (previous) (diff)

#8 in reply to: ↑ 7 @demetris
4 weeks ago

Replying to khag7:

Thanks for investigating and for confirming!

Before I go too far down that path, though, should we just consider not lazy loading the site logo? If the logo is at the top, one of first thing shown in the vast majority of themes, it's not going to be a performance impact to just load it normally. If it fixes this bug, great. Is there a downside?

I think the same.

It seems to me the best thing we can do is to just stop adding the loading attribute to the custom logo.

We can do that by adding 'loading' => false to the array of attributes we pass to wp_get_attachment_image() in get_custom_logo().

As it is now, we do something that is not needed and that we get no benefit from doing. That is, we ask the browser to decide whether to lazy load an image that:

  1. We know should be in the initial viewport
  1. We know should also be in the browser cache for subsequent visits and subsequent views

The only thing we get in return is that we trigger a jarring UX in Firefox.

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


4 weeks ago

#10 @johnbillion
4 weeks ago

  • Keywords needs-patch added; 2nd-opinion removed

#11 @johnbillion
4 weeks ago

  • Keywords good-first-bug added

@rebasaurus
4 weeks ago

Disable lazy loading as default

#12 @rebasaurus
4 weeks ago

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

@demetris
4 weeks ago

Disable lazy loading for custom logo, with explanatory comment

#13 @demetris
4 weeks ago

Thanks for the quick patching, @rebasaurus

The only thing I would do differently is include a comment, so that people in the future know the reason for the special treatment.

I am attaching a patch with explanatory comment as a second option.

#14 @audrasjb
4 weeks ago

  • Keywords needs-testing removed

Just noting we’re not really/only fixing the issue for Firefox, but rather because custom logos are probably more often located at the top of the screen and most of the time don't need to be lazy loaded.

For theme authors who want to re-add the lazy load, they can use the get_custom_logo_image_attributes filter.

#15 @audrasjb
4 weeks ago

  • Owner set to audrasjb
  • Status changed from new to accepted

Otherwise, the patch is functional on my side, thanks

@audrasjb
4 weeks ago

Media: Use false value for loading attribute to disable lazy loading by default on get_custom_logo()

#16 @audrasjb
4 weeks ago

In 50933.1.diff, let's use an @since comment to explain the change in the function’s changelog.

This ticket was mentioned in Slack in #core by desrosj. View the logs.


3 weeks ago

#18 @desrosj
3 weeks ago

  • Owner changed from audrasjb to desrosj
  • Status changed from accepted to assigned

#19 @desrosj
3 weeks ago

  • Keywords commit added

#20 @desrosj
3 weeks ago

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

In 48870:

Media: Disable lazy-loading for custom logos by default.

Custom site logos are most commonly displayed above the fold, so lazy-loading is unnecessary.

Props demetris, khag7, johnbillion, rebasaurus, audrasjb.
Fixes #50933.

#21 @desrosj
3 weeks ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for backport.

#22 @desrosj
3 weeks ago

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

In 48871:

Media: Disable lazy-loading for custom logos by default.

Custom site logos are most commonly displayed above the fold, so lazy-loading is unnecessary.

Props demetris, khag7, johnbillion, rebasaurus, audrasjb.
Merges [48870] to the 5.5 branch.
Fixes #50933.

#23 @SergeyBiryukov
3 weeks ago

In 48874:

Tests: Update unit tests to account for lazy-loading being disabled for custom logos by default.

Follow-up to [48870].

See #50933.

#24 @SergeyBiryukov
3 weeks ago

In 48875:

Tests: Update unit tests to account for lazy-loading being disabled for custom logos by default.

Follow-up to [48870].

Merges [48874] to the 5.5 branch.
See #50933.

#25 @SergeyBiryukov
3 weeks ago

In 48878:

Tests: Update one more test to account for lazy-loading being disabled for custom logos by default.

Follow-up to [48870], [48874].

Props desrosj.
See #50933.

#26 @SergeyBiryukov
3 weeks ago

In 48879:

Tests: Update one more test to account for lazy-loading being disabled for custom logos by default.

Follow-up to [48870], [48874].

Props desrosj.
Merges [48878] to the 5.5 branch.
See #50933.

Note: See TracTickets for help on using tickets.