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

Add additional delay JS exclusions #3975

Merged
merged 8 commits into from
Jun 2, 2021

Conversation

Tabrisrp
Copy link
Contributor

Description

Add additional delay JS exclusions to solve known issues.

Fixes #3941
Fixes #3950
Fixes #3959

Type of change

  • Enhancement (non-breaking change which adds functionality)

How Has This Been Tested?

  • Automated tests added for the patterns

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • 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

@Tabrisrp Tabrisrp added type: enhancement Improvements that slightly enhance existing functionality and are fast to implement 3rd party compatibility Issues related to 3rd party compatibility like theme, plugin or hosting labels May 31, 2021
@Tabrisrp Tabrisrp requested review from a team May 31, 2021 20:56
@Tabrisrp Tabrisrp self-assigned this May 31, 2021
@Mai-Saad Mai-Saad self-requested a review June 1, 2021 13:31
@Tabrisrp Tabrisrp added this to the 3.9.0.1 milestone Jun 1, 2021
@Mai-Saad
Copy link

Mai-Saad commented Jun 1, 2021

@Tabrisrp

  • For 3941 , 3959 => fixed
  • For 3950 => script smush-lazy-load.min.js?ver=3.8.5 is not delayed but still image is delayed till user make an action, can you please recheck this @piotrbak

@piotrbak
Copy link
Contributor

piotrbak commented Jun 1, 2021

3950 works fine for me too! 🤔

@piotrbak
Copy link
Contributor

piotrbak commented Jun 1, 2021

@Tabrisrp This PR fixes also #3932 right?

This one can be closed then?
#3972

@Tabrisrp
Copy link
Contributor Author

Tabrisrp commented Jun 1, 2021

This PR includes the code change yes, but I don't know if it was QA'ed for this part?

@Mai-Saad
Copy link

Mai-Saad commented Jun 2, 2021

Here is a video for 3950
https://user-images.githubusercontent.com/76941962/120434702-170d2d80-c37d-11eb-9b56-0ce574ae8df9.mp4

So apparently auto exclude smush script only is not enough in this case, here are the rest of the scripts that were delayed and it's affecting the image
smush

@GeekPress
Copy link
Contributor

@Mai-Saad This seems to be another case and maybe something else is lazyloaded the image. If you try on a clean install with only Smush, the images must be displayed properly.

@Mai-Saad
Copy link

Mai-Saad commented Jun 2, 2021

@GeekPress I tried the following:

  • Using the same demo site, deactivated the rest of the plugins (mainly Elementor and Virtue/Ascend/Pinnacle Toolkit) then created a new page with some images, and still images are delayed.

  • Created a new test site, using the default theme, created a new page with images, activated smush only with WPR and still, images are delayed.

Screencast.2021-06-02.09.11.13.mp4
  • Tried on live test site while theme was newsportal (deactivated any other plugins than WPR +smush) => was working fine here but when used Twenty Twenty-one theme, it didn't work (even after excluding all js that were delayed) ... Please feel free to check it here https://new.rocketlabsqa.ovh/images22

@GeekPress
Copy link
Contributor

@Mai-Saad You aren't using the latest Delay JS script version. You are using the one which has the exclusion issue.

@piotrbak
Copy link
Contributor

piotrbak commented Jun 2, 2021

@GeekPress When using additional animations in the Smush for lazy load, they're adding the no-js CSS which prevents images from being displayed:

<style>
	.no-js img.lazyload { display: none; }
	figure.wp-block-image img.lazyloading { min-width: 150px; }
	.lazyload, .lazyloading { opacity: 0; }
	.lazyloaded {
		opacity: 1;
		transition: opacity 400ms;
		transition-delay: 0ms;
}
</style>

The no-js class is removed with this script:

<script type="rocketlazyloadscript">document.body.classList.remove("no-js");</script>

It should not be delayed also. I think it's pretty safe to exclude it, even if any other solution is sharing the same code, we should remove any no-js classes as soon as possible.

@GeekPress
Copy link
Contributor

GeekPress commented Jun 2, 2021

@piotrbak I'm a little afraid to add a generic exclusion like no-js. We don't know the impact it's going to have.

What about this? We know it should concerns only Smush:
document.body.classList.remove("no-js")

@piotrbak
Copy link
Contributor

piotrbak commented Jun 2, 2021

@GeekPress Yes, I exactly meant that. This script is removing the no-js classes and it can be safely excluded, even if someone else is sharing it.

@Mai-Saad Mai-Saad removed the needs: qa label Jun 2, 2021
@Tabrisrp
Copy link
Contributor Author

Tabrisrp commented Jun 2, 2021

document.body.classList.remove("no-js") is now auto-excluded

@piotrbak
Copy link
Contributor

piotrbak commented Jun 2, 2021

@Tabrisrp @GeekPress When using Twenty One theme their script for handling the no-js is different. We need to exclude this one too 😞
document.documentElement.className = document.documentElement.className.replace( 'no-js', 'js' );

@GeekPress
Copy link
Contributor

@piotrbak @Tabrisrp We can exclude this to be shorter for Twenty One theme:

document.documentElement.className.replace( 'no-js', 'js' )

@Tabrisrp
Copy link
Contributor Author

Tabrisrp commented Jun 2, 2021

Those are added by the themes, not by Smush, are we going to add an exclusion for each theme using this kind of thing?

@piotrbak
Copy link
Contributor

piotrbak commented Jun 2, 2021

@Tabrisrp To be more specific, the same script is added twice. Once by Smush and once by TwentyOne:
image

There's also similar script in TwentySeventeen for no-svg and svg 😓

I think that the number of possible scripts that are removing the no-js is very limited though...

@Tabrisrp
Copy link
Contributor Author

Tabrisrp commented Jun 2, 2021

We will have to think about doing things differently, because we are only at 10% and already seeing the exclusion list growing. We know this kind of list is problematic to maintain on the long run, and it's only going to keep needing more and more.

@piotrbak
Copy link
Contributor

piotrbak commented Jun 2, 2021

Yes, the list here will grow rapidly...

@GeekPress
Copy link
Contributor

For now, we don't have other choices to add this exclusion to totally improve the compatibility with Smush.

@GeekPress
Copy link
Contributor

GeekPress commented Jun 2, 2021

To be more specific, the same script is added twice. Once by Smush and once by TwentyOne:

@piotrbak Does it mean that Smush is adding

document.documentElement.className.replace( 'no-js', 'js' )

and

document.body.classList.remove("no-js")

? 🙄

@piotrbak
Copy link
Contributor

piotrbak commented Jun 2, 2021

@GeekPress For TwentyTwenty One:

  1. Smush is adding both of them
  2. Twenty Twenty One theme is not adding it
    In this case removing only the document.body.classList.remove("no-js") will do the job, as we'll completely remove the class. However, we won't have the js class then in the source then, as it the excluded script removed it.

For TwentyTwenty:

  1. Smush is adding only the document.documentElement.className.replace( 'no-js', 'js' )
  2. TwentyTwenty is adding the same script
    Since we don't have the script that's removing the no-js we need to exclude this one, so it changes the no-js to js

From my point of view, we need to exclude both of them.

@GeekPress
Copy link
Contributor

@Tabrisrp After having a call with Piotr, we decided to have these exclusions:

document.body.classList.remove("no-js")
document.documentElement.className.replace( 'no-js', 'js' )

@Tabrisrp
Copy link
Contributor Author

Tabrisrp commented Jun 2, 2021

Additional pattern added to the exclusions

Copy link

@Mai-Saad Mai-Saad left a comment

Choose a reason for hiding this comment

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

The latest changes fixed smush. Tested on 2020, 2021, 2019, university, Astra and Virtue

@Tabrisrp Tabrisrp merged commit 1a4c05c into develop Jun 2, 2021
@Tabrisrp Tabrisrp deleted the enhancement/delay-js-compatibility branch June 2, 2021 15:21
crystinutzaa pushed a commit that referenced this pull request Jun 3, 2021
* Fixes #3478 Enable installation through composer v2 (PR #3928)

* update woocommerce from wpackagist
* update woocommerce paths
* use composer v2 in GH actions

* Re-add change to remove disable preload fonts when CPCSS is enabled (PR #3927)

* remove callback disabling preload fonts
* remove associated tests

* Stop preload while prewarmup is running (PR #3925)

* start automatic preload just after prewarmup finishes its work
* Block manual preload till finishing prewarmup
* check the RUCSS option enabled along with allow_optimization
* fix old tests
* [WIP] stop preload with enabling RUCSS
* migrate maybe_cancel_preload to RUCSS admin subscriber
* fix old tests

* remove charset=UTF-8 (PR #3946)

* remove charset=UTF-8
* remove charset=UTF-8 from delete

* Add unit tests code coverage (PR #3922)

* add code coverage command
* update incorrect annotations
* add cove coverage action
* enable xdebug for coverage

* Fix failing tests related to adding prewarmup functionality (PR #3949)

* Guard our code by checking if table is exists before updating prewarmup
* With switch_theme only start scanner when RUCSS is enabled

* change trigger event to push

* Fixes RUCSS tests: update time to prevent tests failures (PR #3973)

* Fixes #3600 add wpstage to staging servers list (PR #3948)

* add wpstage to staging servers

* remove image optimization when WP_ROCKET_WHITE_LABEL_ACCOUNT (PR #3956)

* remove image optimization menu when WP_ROCKET_WHITE_LABEL_ACCOUNT is used

* Add Hubspot iframe inline JS exclusion from combine JS (PR #3943)

* Fixes #3945 Display warning when WP Meteor is enabled with WP Rocket delay JS (PR #3974)

* add new add_plugins_incompatibility() method
* add options class to settings class dependencies
* updating existing tests
* add add_plugins_incompatibility() test file
* add unit & integration tests

* Fixes #3924 Remove unnecessary more info link in RUCSS pre-warmup copy (PR #3981)

* Compatibility with Impreza theme (PR #3972)

* Add Impreza inline script pattern to exclude
* Add Fixtures

Co-authored-by: Rémy Perona <remperona@gmail.com>

* Update delay JS script (PR #3971)

* update delay JS script
* update delay JS fixture
* add touchchancel and touchforcechange events
* update fixture with new events

* Fixes #2876 Remove beacon button when white label is enabled (PR #3965)

* remove beacon btn if white label is true
* remove ask support button too

* Add additional delay JS exclusions (PR #3975)

* Add additional exclusions from delay JS
* Update fixtures

* update plugin version to 3.9.0.1

* update operator in version_compare

Co-authored-by: Ahmed Saed <eng.ahmeds3ed@gmail.com>
Co-authored-by: mostafa-hisham <mostafa.hisham.mahmoud@gmail.com>
Co-authored-by: Natalia Drause <60236665+NataliaDrause@users.noreply.github.com>
Co-authored-by: Jonathan Buttigieg <jonathan@wp-media.me>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3rd party compatibility Issues related to 3rd party compatibility like theme, plugin or hosting type: enhancement Improvements that slightly enhance existing functionality and are fast to implement
Projects
None yet
5 participants