wp-media / wp-rocket

Performance optimization plugin for WordPress
https://wp-rocket.me
GNU General Public License v2.0
698 stars 216 forks source link

Make Delay JavaScript Execution exclusions case sensitive #4633

Closed DahmaniAdame closed 1 year ago

DahmaniAdame commented 2 years ago

Before submitting an issue please check that you’ve completed the following steps:

Describe the bug The current Delay JavaScript Execution won't consider case sensitive strings. Which will exclude anything that matches the pattern, and consequently exclude items that weren't targeted by the exclusion.

This might also create dependency errors sometimes.

A good example is the Divi suggested exclusions for UX purposes.

/jquery-?[0-9.](.*)(.min|.slim|.slim.min)?.js
/jquery-migrate(.min)?.js
.dipi_preloader_wrapper_outer
/Divi/js/scripts.min.js
/Divi/js/custom.unified.js
DIVI

The DIVI keyword is meant to exclude this specific inline script:

<script type='text/javascript' id='divi-custom-script-js-extra'>
/* <![CDATA[ */
var DIVI = {"item_count":"%d Item","items_count":"%d Items"};
var et_builder_utils_params = {"condition":{"diviTheme":true,"extraTheme":false},"scrollLocations":["app","top"],"builderScrollLocations":{"desktop":"app","tablet":"app","phone":"app"},"onloadScrollLocation":"app","builderType":"fe"};
var et_frontend_scripts = {"builderCssContainerPrefix":"#et-boc","builderCssLayoutPrefix":"#et-boc .et-l"};
var et_pb_custom = {"ajaxurl":"https:\/\/advanz.dk\/wp-admin\/admin-ajax.php","images_uri":"https:\/\/advanz.dk\/wp-content\/themes\/Divi\/images","builder_images_uri":"https:\/\/advanz.dk\/wp-content\/themes\/Divi\/includes\/builder\/images","et_frontend_nonce":"fb654242e4","subscription_failed":"V\u00e6lg venligst felterne nedenfor for at sikre, at du har indtastet de korrekte oplysninger.","et_ab_log_nonce":"542a6e7723","fill_message":"Udfyld venligst f\u00f8lgende felter:","contact_error_message":"Fiks venligst f\u00f8lgende fejl:","invalid":"Ugyldig e-mail","captcha":"Captcha","prev":"Forrige","previous":"Tidligere","next":"N\u00e6ste","wrong_captcha":"Du indtastede det forkerte nummer i captcha.","wrong_checkbox":"Checkbox","ignore_waypoints":"no","is_divi_theme_used":"1","widget_search_selector":".widget_search","ab_tests":[],"is_ab_testing_active":"","page_id":"1438","unique_test_id":"","ab_bounce_rate":"5","is_cache_plugin_active":"yes","is_shortcode_tracking":"off","tinymce_uri":""};
var et_pb_box_shadow_elements = [];
/* ]]> */
</script>

But it ends up excluding anything that has divi on its filename or code.

<script type='text/javascript' src='/wp-content/themes/Divi/js/scripts.min.js?ver=4.14.4' id='divi-custom-script-js' defer></script>
<script data-minify="1" type='text/javascript' src='/wp-content/cache/min/1/wp-content/themes/Divi/js/smoothscroll.js?ver=1641455895' id='smoothscroll-js' defer></script>
<script data-minify="1" type='text/javascript' src='/wp-content/cache/min/1/wp-content/themes/Divi/includes/builder/feature/dynamic-assets/assets/js/hashchange.js?ver=1641455895' id='hashchange-js' defer></script>
<script type='text/javascript' src='/wp-content/plugins/divi-mega-menu/scripts/frontend-bundle.min.js?ver=3.2' id='divi-mega-menu-frontend-bundle-js' defer></script>
<script type='text/javascript' src='/wp-content/plugins/divi-mobile/scripts/frontend-bundle.min.js?ver=1.0.0' id='divi-mobile-frontend-bundle-js' defer></script>
<script data-minify="1" type='text/javascript' src='/wp-content/cache/min/1/wp-content/themes/Divi/core/admin/js/common.js?ver=1641455895' id='et-core-common-js' defer></script>
<script type='text/javascript' src='/wp-content/plugins/divi-mega-menu/scripts/divi-mega-menu.min.js?ver=3.2' id='divi-mega-menu-js-js' defer></script>
<script type="text/javascript">window.addEventListener('DOMContentLoaded', function() {
jQuery(document).ready(function( $ ) {
  if ( $('#main-header').length > 0 && $('.et-l--header').length == 0  ) {
  } else if ( $('.et-l--header').length > 0 && $('#main-header').length == 0 ) {
  } else {
    //$('.divi-mobile-menu').remove();
    //$('body').addClass("no-mobile-menu");
  }
$("body").addClass("dm-bm-pos-right");
$("body").addClass("dm-expand-shape");
$("body").addClass("dm-side-slide");
$("body").addClass("dm-circle-expand");
$("body").addClass("dm-menuside-right");

var get_one_click = false;
$(document).on('touchstart click', ".anchorpoint", function(e){
  var _this = $(e.target);
  if ($("body").hasClass("show-menu")) {
    console.log("has show menu");
    $('#open-button').trigger('click');
  }

  var getLink = _this.find('a').attr('href');
  var currentUrl = window.location.href;
  var linkUrlPart = '', linkAnchorPart = '', currentUrlPart = currentUrl;

  e.preventDefault();

  if(currentUrl.indexOf("#") != -1){
    currentUrlPart = getLink.substring(0, getLink.indexOf("#") );
  }

  if(getLink.indexOf("#") == 0){
    linkUrlPart = currentUrlPart;
    linkAnchorPart = getLink.substring(1);
  }else if (getLink.indexOf("#") != -1 ){
    linkUrlPart = getLink.substring(0, getLink.indexOf("#") );
    linkAnchorPart = getLink.substring(getLink.indexOf("#")+1);
  }

  if ( linkUrlPart == currentUrlPart ){
    // Same Url
    if ( linkAnchorPart != '' ){
      $('html, body').animate({
        scrollTop: $('#' + linkAnchorPart).offset().top
      }, 2000);
    }
  } else {
    window.location.href = getLink;
  }
});

});
});</script>

To Reproduce Steps to reproduce the behavior:

  1. Use the Divi theme
  2. Exclude DIVI
  3. See the false positives

Expected behavior We should be able to target specific scripts by using case sensitive keywords/phrases, with no false positives.

Screenshots N/A

Additional context Behavior noticed in this case - https://secure.helpscout.net/conversation/1747586547/317449/

Backlog Grooming (for WP Media dev team use only)

webtrainingwheels commented 2 years ago

Seems that we should be recommending a more specific keyword to exclude the inline script then? Like var DIVI?

piotrbak commented 2 years ago

Agree with @webtrainingwheels

Let's keep this open to see if we receive any positive feedback about that from our customers.

remyperona commented 2 years ago

Making the exclusions case sensitive also increases the chances of user input errors, if the user mistyped something with/without the correct casing

DahmaniAdame commented 2 years ago

Good catch @webtrainingwheels. I was in the middle of it. I overlooked that detail 😅. But it will only fix the Divi situation cited as an example. Updated the Delay JavaScript Execution Exclusions. We can't control how users will enter their exclusions. A case sensitive exclusions will surely be a plus for the Delay JavaScript Execution.

DahmaniAdame commented 2 years ago

Making the exclusions case sensitive also increases the chances of user input errors, if the user mistyped something with/without the correct casing

It's a risk, yes.

I would argue that a mismatch won't be necessarily be site breaking and easier to debug vs. non-obvious false positives that might break other things that the customer is initially focusing on, and would be harder to make a link to them while debugging.

We don't have enough cases to justify any action, though.

piotrbak commented 1 year ago

For more than one year we have no feedback about that from customers and no real examples where this is necessary. Let's close it and we'll reopen when there's a justified reason for that.