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

Feature/5987: Lazyload bg css #6039

Merged
merged 125 commits into from
Aug 29, 2023
Merged

Conversation

CrochetFeve0251
Copy link
Contributor

@CrochetFeve0251 CrochetFeve0251 commented Jul 11, 2023

Description

This feature allows us to lazy load images in the CSS.

Fixes #5987

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)

Is the solution different from the one proposed during the grooming?

Yes.
Some classes were changed to make testing easier.
The selector have been removed from the name of CSS variables as it wasn't valid.

How Has This Been Tested?

  • Automated Test
  • Manual Test

Checklist:

Please delete the options that are not relevant.

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@CrochetFeve0251 CrochetFeve0251 changed the base branch from feature/lazyload-bg-images to develop August 17, 2023 12:42
@CrochetFeve0251 CrochetFeve0251 changed the base branch from develop to branch-3.15 August 17, 2023 13:03
@CrochetFeve0251 CrochetFeve0251 changed the base branch from branch-3.15 to develop August 17, 2023 13:04
@CrochetFeve0251 CrochetFeve0251 changed the base branch from develop to branch-3.15 August 17, 2023 13:20
@CrochetFeve0251 CrochetFeve0251 changed the base branch from branch-3.15 to develop August 17, 2023 13:26
@CrochetFeve0251 CrochetFeve0251 changed the base branch from develop to branch-3.15 August 17, 2023 13:26
@Mai-Saad
Copy link
Contributor

Mai-Saad commented Aug 18, 2023

  • Compatibility with Autoptimize plugin, Console error is there if autoptimize LL is enabled/disabled (JS and CSS optimization on) along with LL BG image. so far, the case to have no console error is when AO LL/js/css optimizations are off or disable our LL BG image while AO is on => under investigation to decide PO needs here
    Screenshot from 2023-08-18 15-05-28
    if we disable autoptimize js optimization while our LL BG image on, we will have this error (AO CSS is on)
    Screenshot from 2023-08-18 15-13-30 (2)

  • Image with space in the URL is removed from FE while LL BG is enabled
    example:
    .internal-css-background-images-space{ width: 100%; height: 400px; background-image: url('/wp-content/rocket-test-data/images/butterfly%202.avif'); }

  • Console error and BG images are not displayed when Combine JS is enabled (while using Leto theme, Ashe theme, SpicePress ) => note: working fine with other themes like astra , 2020 (Handled from BE exclusions and updating dynamic list)
    Screenshot from 2023-08-22 15-04-34

  • URLs inside NoScript or LL data script are not written to CNAME while CDN is enabled

  • When deleting permanently a post, BG CSS-used files will be deleted from the cache (although it is used in other pages, causing console error on those pages)
    Screenshot from 2023-08-24 09-47-52

  • Depreciation notice when enabling LL BGI while WPML is enabled (directory setup or domain setup)
    [24-Aug-2023 12:06:11 UTC] PHP Deprecated: str_replace(): Passing null to parameter #1 ($search) of type array|string is deprecated in /var/www/rocketlabsqa.ovh/htdocs/wp-content/plugins/wp-rocket/inc/Engine/Optimization/Minify/AbstractMinifySubscriber.php on line 131

  • Console error when switching language while LLBGI is enabled (WPML domain base setup)
    Screenshot from 2023-08-24 15-47-36

  • using alpha1, console warning when enabling LL BG image on a site hosted on WP Engine
    Screenshot from 2023-08-25 12-59-50

  • Console error with eclipse theme when LL BG Image is enabled
    Screenshot from 2023-08-25 10-28-02

  • External BG images give 404 when enabling LL BGI and scrolling in the page template While WPML is on (any setup) slack discussion

  • Css external file with URL containing special char (or space) and background image is not LL (note: even RUCSS / CSS minification are not applied to this type of URL) ex:https://new.rocketlabsqa.ovh/wp-content/rocket-test-data/styles/test%20style/lazyload_css_background%20images.css

  • enable/disable LL BGI will clear used CSS although nothing changed in the used CSS file itself (same occurs on 3.14.4.2 when enabling normal LL)

  • Excluding CSS file from LL BGI while RUCSS is on, won't work once used CSS added to page (as per discussion, It's acceptable for this to be as future enhancement if that's too complicated)

  • Disable LL BG image will clear the BG CSS folder => better just clear the normal cache and not the BG cache folder

  • Edit post with Beaver Builder will clear the background CSS cache folder => as per discussion, fine to be an exception for now

  • Fatal error when the filter set to null, clear cache then enable LL options and access FE (already on 3.14.4.2)

add_filter( 'rocket_lazyload_excluded_src', function( $images ) {
return null;
} );

[23-Aug-2023 12:57:36 UTC] PHP Fatal error: Uncaught TypeError: WP_Rocket\Engine\Media\Lazyload\Subscriber::add_exclusions(): Argument #1 ($exclusions) must be of type array, null given, called in /var/www/new.rocketlabsqa.ovh/htdocs/wp-includes/class-wp-hook.php on line 310 and defined in /var/www/new.rocketlabsqa.ovh/htdocs/wp-content/plugins/wp-rocket/inc/Engine/Media/Lazyload/Subscriber.php:457 Stack trace: #0 /var/www/new.rocketlabsqa.ovh/htdocs/wp-includes/class-wp-hook.php(310): WP_Rocket\Engine\Media\Lazyload\Subscriber->add_exclusions() #1 /var/www/new.rocketlabsqa.ovh/htdocs/wp-includes/plugin.php(205): WP_Hook->apply_filters() #2 /var/www/new.rocketlabsqa.ovh/htdocs/wp-content/plugins/wp-rocket/inc/Dependencies/RocketLazyload/Image.php(444): apply_filters() #3 /var/www/new.rocketlabsqa.ovh/htdocs/wp-content/plugins/wp-rocket/inc/Dependencies/RocketLazyload/Image.php(357): WP_Rocket\Dependencies\RocketLazyload\Image->getExcludedSrc() #4 /var/www/new.rocketlabsqa.ovh/htdocs/wp-content/plugins/wp-rocket/inc/Dependencies/RocketLazyload/Image.php(33): WP_Rocket\Dependencies\RocketLazyload\Image->canLazyload() #5 /var/www/new.rocketlabsqa.ovh/htdocs/wp-content/plugins/wp-rocket/inc/Engine/Media/Lazyload/Subscriber.php(341): WP_Rocket\Dependencies\RocketLazyload\Image->lazyloadImages() #6 /var/www/new.rocketlabsqa.ovh/htdocs/wp-includes/class-wp-hook.php(310): WP_Rocket\Engine\Media\Lazyload\Subscriber->lazyload() #7 /var/www/new.rocketlabsqa.ovh/htdocs/wp-includes/plugin.php(205): WP_Hook->apply_filters() #8 /var/www/new.rocketlabsqa.ovh/htdocs/wp-content/plugins/wp-rocket/inc/Engine/Optimization/Buffer/Optimization.php(100): apply_filters() #9 [internal function]: WP_Rocket\Engine\Optimization\Buffer\Optimization->maybe_process_buffer() #10 /var/www/new.rocketlabsqa.ovh/htdocs/wp-includes/functions.php(5349): ob_end_flush() #11 /var/www/new.rocketlabsqa.ovh/htdocs/wp-includes/class-wp-hook.php(310): wp_ob_end_flush_all() #12 /var/www/new.rocketlabsqa.ovh/htdocs/wp-includes/class-wp-hook.php(334): WP_Hook->apply_filters() #13 /var/www/new.rocketlabsqa.ovh/htdocs/wp-includes/plugin.php(517): WP_Hook->do_action() #14 /var/www/new.rocketlabsqa.ovh/htdocs/wp-includes/load.php(1252): do_action() #15 [internal function]: shutdown_action_hook() #16 {main} thrown in /var/www/new.rocketlabsqa.ovh/htdocs/wp-content/plugins/wp-rocket/inc/Engine/Media/Lazyload/Subscriber.php on line 457

  • (Agreed as future enhancement) Exclusion of a single image in a multi-background markup won't exclude it in the network
    if we have the following markup
.internal-css-background-images{
            width: 100%;
            height: 400px;
            background-image: url('https://new.rocketlabsqa.ovh/wp-content/rocket-test-data/images/600px-Mapang-test.gif'), url( "/wp-content/rocket-test-data/image/file_example_JPG_100kB.jpg" );
            background-color: #cccccc;
        }

and exclude: https://new.rocketlabsqa.ovh/wp-content/rocket-test-data/images/600px-Mapang-test.gif, the image won't appear in network tab till we scroll the page

  • Page excluded from cache in never cache URL won't be LL , however, if disable cache filter used add_filter( 'do_rocket_generate_caching_files', '__return_false' );, we will LL BGI normally

  • When enable RUCSS, we delete the BG CSS cache => as per discussion, can be improved later

  • Decreasing the threshold to 1 won't make any difference in the number of loaded images before scroll (same on trunk), however increasing it to 1000 will work as expected

  • Handling the relative URL inside the CSS file inside noscript and script data while CDN is enabled => discussion if change is needed and why?
    (https://i5f5b9r7.stackpathcdn.com/../../wp-content/plugins/revslider/public/assets/assets/test_internal3.jpg)
    before processing, URL background: url("../../wp-content/plugins/revslider/public/assets/assets/test_internal3.jpg");

  • Clear and preload cache while RUCSS is enabled, won't recreate the CSS/JSON files for the page if used CSS was already created => fine as per further discussion

  • If we excluded in UI the full URL of BGI while it was relative in the source, the image won't be excluded (Note: the same exists for normal LL) => won't fix

  • Processed images by LL BG image are not displayed on IE, we better redirect to nowprocket as we do with DJS (not priority)

  • Background images added with Elementor are not displayed and console errors similar to the following appear for every background image:

DOMException: Failed to execute 'querySelectorAll' on 'Document': ', .elementor-34848 .elementor-element.elementor-element-0ceebb6 > .elementor-motion-effects-container > .elementor-motion-effects-layer' is not a valid selector.

Note: Their background image lazyload feature (Elementor > Settings > Features > LazyLoad Background Images) should be disabled.

  • Background images added with Beaver builder are not displayed and console errors similar to the following appear:
lazyload-css.min.js?ver=3.15-alpha1:1 DOMException: Failed to execute 'querySelectorAll' on 'Document': 'media(max-width: 1200px)' is not a valid selector.

Note: In a row, select settings and use an external URL image:
image

  • Flatsome background images are not loaded. The var() seems to be not defined.

Also, paths like background-image: url(../img/[email protected]); part of /wp-content/themes/flatsome/assets/css/flatsome.css are converted to:

style
: 
":root{--wpr-bg-dee3f782-6db1-4327-aab9-a78013f72949: url('https://wprocketest.test/../../../../../../../../themes/flatsome/assets/img/[email protected]');}"

If that's actually used somewhere it will result in error 404. Note that wp-content is missing.

  • Newpaper background images are not loaded. The following console errors are there:
DOMException: Failed to execute 'querySelectorAll' on 'Document': 'media only screen and (-webkit-min-device-pixel-ratio: 2), only screen and (min--moz-device-pixel-ratio: 2), only screen and (min-device-pixel-ratio: 2)' is not a valid selector.
    at https://wprocketest.test/wp-content/plugins/wp-rocket/assets/js/lazyload-css.min.js?ver=3.15-alpha2:1:953
    at Array.forEach (<anonymous>)
    at https://wprocketest.test/wp-content/plugins/wp-rocket/assets/js/lazyload-css.min.js?ver=3.15-alpha2:1:920
    at 1 (https://wprocketest.test/wp-content/plugins/wp-rocket/assets/js/lazyload-css.min.js?ver=3.15-alpha2:1:1078)
    at i (https://wprocketest.test/wp-content/plugins/wp-rocket/assets/js/lazyload-css.min.js?ver=3.15-alpha2:1:245)
    at o (https://wprocketest.test/wp-content/plugins/wp-rocket/assets/js/lazyload-css.min.js?ver=3.15-alpha2:1:403)
    at https://wprocketest.test/wp-content/plugins/wp-rocket/assets/js/lazyload-css.min.js?ver=3.15-alpha2:1:420

Copy link
Contributor

@engahmeds3ed engahmeds3ed left a comment

Choose a reason for hiding this comment

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

What a huge PR :D I'm still checking it but some comments below.

@CrochetFeve0251 good job, The code is clean, although it came at the expense of complexity :D :D but that's a good trade-off.

inc/Engine/Admin/Settings/Page.php Outdated Show resolved Hide resolved
inc/Engine/Admin/Settings/Page.php Outdated Show resolved Hide resolved
inc/Engine/Common/Cache/FilesystemCache.php Outdated Show resolved Hide resolved
inc/Engine/Common/Context/AbstractContext.php Outdated Show resolved Hide resolved
inc/Engine/Media/Lazyload/CSS/Subscriber.php Show resolved Hide resolved
});
}

rocket_css_lazyload();
Copy link
Contributor

Choose a reason for hiding this comment

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

we are loading the script directly when the file is loaded but page is still loading at this time so what about something like:

(function(callback) {
	if (document.readyState !== "loading") {
		callback();
	} else {
		document.addEventListener("DOMContentLoaded", callback);
	}
})(() => {
	rocket_css_lazyload();
});

@engahmeds3ed engahmeds3ed merged commit 803aa28 into branch-3.15 Aug 29, 2023
6 of 7 checks passed
@engahmeds3ed engahmeds3ed deleted the feature/5987-lazyload-css branch August 29, 2023 11:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: new feature Indicates the issue is a request for new functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

LazyLoad CSS Background Images
5 participants