WordPress.org

Make WordPress Core

Opened 3 months ago

Closed 2 months ago

Last modified 2 months ago

#52098 closed defect (bug) (fixed)

Twenty Twenty-One: IE specific polyfills are loaded in all browsers

Reported by: ismail.elkorchi Owned by: desrosj
Milestone: 5.6.1 Priority: normal
Severity: normal Version: 5.6
Component: Bundled Theme Keywords: has-patch, twenty-twenty-one-1.1
Focuses: javascript, performance Cc:

Description

The file /assets/js/polyfills.js must be enqueued only if the used browser is Internet Explorer, but it shouldn't in other browsers.

In this function, both the navigation and responsive-embeds scripts are registered with /assets/js/polyfills.js dependency for all users. But we can use $is_IE conditional to register the IE polyfills script only when IE is used.

The code will then look something like this :

<?php
function twenty_twenty_one_scripts() {
        // Note, the is_IE global variable is defined by WordPress and is used
        // to detect if the current browser is internet explorer.
        global $is_IE;
        $twenty_twenty_one_scripts_dependencies = array();
        if ( $is_IE ) {
                // If IE 11 or below, use a flattened stylesheet with static values replacing CSS Variables.
                wp_enqueue_style( 'twenty-twenty-one-style', get_template_directory_uri() . '/assets/css/ie.css', array(), wp_get_theme()->get( 'Version' ) );
                
                wp_register_script(
                        'twenty-twenty-one-ie11-polyfills',
                        get_template_directory_uri() . '/assets/js/polyfills.js',
                        array(),
                        wp_get_theme()->get( 'Version' ),
                        true
                );
                
                array_push( $twenty_twenty_one_scripts_dependencies, 'twenty-twenty-one-ie11-polyfills' );
                
        } else {
                // If not IE, use the standard stylesheet.
                wp_enqueue_style( 'twenty-twenty-one-style', get_template_directory_uri() . '/style.css', array(), wp_get_theme()->get( 'Version' ) );
        }

        // RTL styles.
        wp_style_add_data( 'twenty-twenty-one-style', 'rtl', 'replace' );

        // Print styles.
        wp_enqueue_style( 'twenty-twenty-one-print-style', get_template_directory_uri() . '/assets/css/print.css', array(), wp_get_theme()->get( 'Version' ), 'print' );

        // Threaded comment reply styles.
        if ( is_singular() && comments_open() && get_option( 'thread_comments' ) ) {
                wp_enqueue_script( 'comment-reply' );
        }

        // Main navigation scripts.
        if ( has_nav_menu( 'primary' ) ) {
                wp_enqueue_script(
                        'twenty-twenty-one-primary-navigation-script',
                        get_template_directory_uri() . '/assets/js/primary-navigation.js',
                        $twenty_twenty_one_scripts_dependencies,
                        wp_get_theme()->get( 'Version' ),
                        true
                );
        }

        // Responsive embeds script.
        wp_enqueue_script(
                'twenty-twenty-one-responsive-embeds-script',
                get_template_directory_uri() . '/assets/js/responsive-embeds.js',
                $twenty_twenty_one_scripts_dependencies,
                wp_get_theme()->get( 'Version' ),
                true
        );
}

Change History (17)

#1 @peterwilsoncc
3 months ago

  • Keywords needs-patch added
  • Version changed from trunk to 5.6

Thanks for the report @ismailelkorchi

I think you're correct, ideally the polyfill would be included for IE11 only but I'll leave the 2021 team to decide whether to milestone this ticket as it may have been discussed earlier.

Unfortunately the $is_IE variable is unreliable for sites with full page caching but the wp_get_script_polyfill() function can be used when enqueuing the script similar to how WordPress uses it in the dashboard for wp-polyfills.

#2 @ismail.elkorchi
3 months ago

Unfortunately the $is_IE variable is unreliable for sites with full page caching

This should also be a problem for the IE CSS, because the Twenty Twenty-One relies on $is_IE to know which CSS file it should enqueue : https://github.com/WordPress/wordpress-develop/blob/master/src/wp-content/themes/twentytwentyone/functions.php#L397-L406

Version 0, edited 3 months ago by ismail.elkorchi (next)

#3 @poena
3 months ago

The cache problems are known and different solutions were discussed. Using $is_IE was still considered the best solution for the CSS.

We should use the best solution for both the polyfill and the CSS. The best solutions are not necessarily the same for both, but I have never used / am not familiar with https://developer.wordpress.org/reference/functions/wp_get_script_polyfill/

This ticket was mentioned in PR #826 on WordPress/wordpress-develop by peterwilsoncc.


3 months ago

  • Keywords has-patch added; needs-patch removed

#5 @peterwilsoncc
3 months ago

I've created a proof of concept pull request for the script loader:

  • The twenty-twenty-one-ie11-polyfills asset becomes a loader with a null source (similar to how core used to do jQuery)
  • A new script is registered with the actual source linked, twenty-twenty-one-ie11-polyfills-asset
  • I've used the loader pattern to:
    • avoid breaking backward-compatibility with any child themes that are using twenty-twenty-one-ie11-polyfills as a dependency
    • avoid having to double up on the pollyfill script for both the responsive embeds and primary navigation script

Sorry about any confusion around using $is_IE for scripts vs styles.

@poena Would you or the TT1 team mind taking a look at the PR and let me know if you'd prefer a different approach or I've missed a required test?

#6 @prbot
2 months ago

carolinan commented on PR #826:

When I test on IE11 I see the following before primary-navigation.js and responsive-embeds.js>

<script id='twenty-twenty-one-ie11-polyfills-js-after'>
( ( ! Element.prototype.matches ) || ( ! Element.prototype.closest ) || ( window.NodeList && ! NodeList.prototype.forEach ) ) || document.write( '<script src="http://localhost:8889/wp-content/themes/twentytwentyone/assets/js/polyfills.js?ver=1.0"></scr' + 'ipt>' );
</script>

But these JS errors are present

Object doesn't support property or method 'forEach'
responsive-embeds.js (18,2)

Object doesn't support property or method 'forEach'
primary-navigation.js (153,3)

This ticket was mentioned in Slack in #core-js by peterwilsoncc. View the logs.


2 months ago

#8 @prbot
2 months ago

peterwilsoncc commented on PR #826:

Thanks for the reviews folks,

wp_get_script_polyfill() uses document.write() in an attempt to force the polyfill scripts to load synchronously but IE11 still loads them asynchronously using that method.

In 5a7c3a376c18d36b7abdb9d90d6466611a9454dd I tried a different approach but it ended up breaking gutenberg so I reverted it. If anyone is able to take a look at the commit, that would be dandy.

#9 @desrosj
2 months ago

  • Milestone changed from Awaiting Review to 5.7

I'm going to move this to 5.7. If it's completed in time before 5.6.1, we could potentially consider backporting this.

#10 @prbot
2 months ago

peterwilsoncc commented on PR #826:

@carolinan @aristath

I figured it out what I was doing incorrectly. The test should be for the presence of the feature rather than the absence of the feature.

Fixed and ready for another look.

I've tested this in IE11 where the polyfill is loading, and Firefox where it is not.

#11 @prbot
2 months ago

carolinan commented on PR #826:

I have tested the update and it works well, thank you.

This ticket was mentioned in Slack in #core-themes by poena. View the logs.


2 months ago

#13 @desrosj
2 months ago

  • Milestone changed from 5.7 to 5.6.1
  • Owner set to desrosj
  • Status changed from new to reviewing

#14 @desrosj
2 months ago

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

In 49865:

Twenty Twenty-One: Only load IE specific polyfills when actually using Internet Exploreer.

The twenty-twenty-one-ie11-polyfills script now has a null source, and the new twenty-twenty-one-ie11-polyfills-asset (which points to the actual polyfills.js source) will be loaded only if IE is detected by through the use of wp_get_script_polyfill().

Because the original script name remains the same, this change is backwards compatible with any code registering twenty-twenty-one-id11-polyfills as a script dependency.

Props poena, ismail.elkorchi, peterwilsoncc
Fixes #52098.

#15 @desrosj
2 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for backport.

#16 @desrosj
2 months ago

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

In 49868:

Twenty Twenty-One: Only load IE specific polyfills when actually using Internet Exploreer.

The twenty-twenty-one-ie11-polyfills script now has a null source, and the new twenty-twenty-one-ie11-polyfills-asset (which points to the actual polyfills.js source) will be loaded only if IE is detected by through the use of wp_get_script_polyfill().

Because the original script name remains the same, this change is backwards compatible with any code registering twenty-twenty-one-id11-polyfills as a script dependency.

Props poena, ismail.elkorchi, peterwilsoncc.
Merges [49865] to the 5.6 branch.
Fixes #52098.

#17 @desrosj
2 months ago

  • Keywords twenty-twenty-one-1.1 added

Adding a custom keyword for querying tickets included in the Twenty Twenty-One version 1.1 release in the future.

Note: See TracTickets for help on using tickets.