AdvancedCustomFields / acf

Advanced Custom Fields
http://advancedcustomfields.com/
835 stars 170 forks source link

ACF 5.9.3 Unnecessary Javacsript Loading (Frontend) #412

Open jmkucerak opened 3 years ago

jmkucerak commented 3 years ago

Intro While auditing site performance I noticed unnecessary javascript files being loaded on certain pages. This applied only to page with a comment form that contained an ACF text field. Some of the javascript files are iris.min.js, color-picker.min.js and datepicker.js. All of the files total to a few hundred kilobytes and does have a noticeable affect on mobile performance.

Problem When the location of a field group is set to comment, ACF will load javascript files on the frontend for field types regardless of if they are present in the field group when the comment form is present.

This seems to be related to how the input_admin_enqueue_scripts method in each field type file is called. Now, as the method name suggests, it would seem this method should only be called in the admin but that is not the case. Lets start with how that particular method is called. So we start in includes\fields\class-acf-field.php on line 65 inside the constructor. $this->add_action("acf/input/admin_enqueue_scripts", array($this, 'input_admin_enqueue_scripts'), 10, 0);

This then takes use to includes\assets.php line 374 inside the enqueue_scripts method. do_action( 'acf/input/admin_enqueue_scripts' );

This is where the problem begins to arise. The enqueue_scripts method is seemingly attached to the admin_enqueue_scripts on line 177 in assets.php using the add_action method... $this->add_action( 'admin_enqueue_scripts', 'enqueue_scripts' , 20 );

but, if you look at the add_action method you will notice the $replacements variable which contains alternative hooks to use based on what hook is given in the arguments and the current context. This is an issue because admin_enqueue_scripts should, in theory, only be run on the admin but is converted to wp_enqueue_scripts in the frontend wp context. This appears to be by design if you look at the input_admin_enqueue_scripts method on line 47 in _includes\fields\class-acf-field-colorpicker.php


function input_admin_enqueue_scripts() {

    // Register scripts for non-admin.
    // Applies logic from wp_default_scripts() function defined in "wp-includes/script-loader.php".
    if( !is_admin() ) {
        $suffix = defined('SCRIPT_DEBUG') && SCRIPT_DEBUG ? '' : '.min';
        $scripts = wp_scripts();
        $scripts->add( 'iris', '/wp-admin/js/iris.min.js', array( 'jquery-ui-draggable', 'jquery-ui-slider', 'jquery-touch-punch' ), '1.0.7', 1 );
        $scripts->add( 'wp-color-picker', "/wp-admin/js/color-picker$suffix.js", array( 'iris' ), false, 1 );

        // Handle localisation across multiple WP versions. 
        // WP 5.0+
        if( method_exists($scripts, 'set_translations') ) {
            $scripts->set_translations( 'wp-color-picker' );
        // WP 4.9
        } else {
            $scripts->localize( 'wp-color-picker', 'wpColorPickerL10n', array(
                'clear'            => __( 'Clear' ),
                'clearAriaLabel'   => __( 'Clear color' ),
                'defaultString'    => __( 'Default' ),
                'defaultAriaLabel' => __( 'Select default color' ),
                'pick'             => __( 'Select Color' ),
                'defaultLabel'     => __( 'Color value' ),
            ));
        }

    }

    // Enqueue.
    wp_enqueue_style( 'wp-color-picker' );
    wp_enqueue_script( 'wp-color-picker' );         
}

The if statement clearly shows an expectation that the code is not restricted to admin despite the method name. This leads to code running that should not and results in performance losses that should not happen without any simple workaround.

How to reproduce

Possible Solution Have enqueue happen just before render. This would prevent javascript from loading for field types that will not be rendered.

Extra Notes This may only be isolated to form-comment.php, form-user.php and form-front.php due to acf_enqueue_scripts being called without checking for the screen or testing is_admin.

elliotcondon commented 3 years ago

Hi @jmkucerak

Thanks for the thorough bug report 👍.

You have outlined very well an area of the plugin that could be optimized. Currently, we load all JS and CSS assets for areas where Fields may appear. This is a simple, but not so elegant solution, as you have discovered.

It should definitely be possible to optimize the enqueuing process to only enqueue assets that are needed for the page.