WordPress.org

Make WordPress Core

Opened 16 months ago

Closed 2 weeks ago

#44484 closed defect (bug) (fixed)

Mediaelement scripts forced loaded in header

Reported by: Themezly Owned by: joemcgill
Milestone: 5.3 Priority: normal
Severity: major Version: 4.9.6
Component: Media Keywords: has-patch
Focuses: performance Cc:
PR Number:

Description

Before this https://make.wordpress.org/core/2017/10/30/mediaelement-upgrades-in-wordpress-4-9/

media element scripts were loading where called

example:

wp_enqueue_script( 'wp-mediaelement' ); // loads above site-scripts in footer with dependencies 

/*
mediaelement-and-player.min.js
wp-mediaelement.min.js
*/

wp_enqueue_script( 'site-scripts'); // loads in footer as needed

now

mediaelement.min.js
mediaelement-core.min.js

load in head and wp-mediaelement.min.js loads in footer

If I call them one by one without calling wp-mediaelement the order is kept

wp_enqueue_script( 'mediaelement-migrate' );// loads in footer
wp_enqueue_script( 'mediaelement-core' );// loads in footer
wp_enqueue_script( 'site-scripts'); // loads in footer

if you look here it is intended for all of them to be in footer

https://github.com/WordPress/WordPress/blob/6a9a5e123c49862babf664367c26c89499fed4e0/wp-includes/script-loader.php#L377-L378

https://github.com/WordPress/WordPress/blob/6a9a5e123c49862babf664367c26c89499fed4e0/wp-includes/script-loader.php#L474

but wp_enqueue_script( 'wp-mediaelement' );

depends on wp_enqueue_script( 'mediaelement' ); http://prntscr.com/k15288

https://github.com/WordPress/WordPress/blob/6a9a5e123c49862babf664367c26c89499fed4e0/wp-includes/script-loader.php#L376

which loads in header only

so we can never move the scripts to footer where they should be

If I set the mediaelement to load in footer everything works as intended.

Attachments (1)

44484.diff (900 bytes) - added by joemcgill 2 weeks ago.

Download all attachments as: .zip

Change History (10)

#1 @SergeyBiryukov
16 months ago

  • Component changed from General to Media

#2 @joemcgill
6 months ago

  • Focuses performance added
  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 5.3

Hi @Themezly,

Thanks for the report. This does seem to be an unintentional change from [41877]. I'm going to set this to the next major release, but we might be able to put it out in a 5.2.x release, if it's ready.

This ticket was mentioned in Slack in #core-media by mike. View the logs.


2 months ago

#4 @mikeschroder
2 months ago

  • Owner set to joemcgill
  • Status changed from new to assigned

#5 @mikeschroder
2 weeks ago

@joemcgill I'm noticing this one doesn't have a patch yet, and it's almost beta 3.

Would you like to do this for 5.3 still, or should we move it to 5.4 or Future Release?

This ticket was mentioned in Slack in #core-media by mike. View the logs.


2 weeks ago

@joemcgill
2 weeks ago

#7 @joemcgill
2 weeks ago

  • Keywords needs-testing has-patch added; needs-patch removed

44484.diff ensures mediaelement is loaded in the footer. Before this change, all mediaelement related scripts are loading in the header on the post edit screen. After this change, they are loaded in the footer.

#8 @desrosj
2 weeks ago

  • Keywords needs-testing removed

@joemcgill 44484.diff looks good. I tested in Classic and block editors and didn't see any issues.

#9 @joemcgill
2 weeks ago

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

In 46379:

Media: Ensure medialement scripts are loaded in the footer.

This fixes a regression in [41877] which caused mediaelement scripts to load in the header.

Props Themezly.
Fixes #44484.

Note: See TracTickets for help on using tickets.