MediaElement.js 4.2 is not backward compatible with 2.2

[bradyvercher]

It looks like a lot of work has gone into modernizing and improving the codebase for the new version of MediaElement.js, but backward compatibility doesn't really seem to have been a major consideration.

Here are a few issues I've run across over the past few days while trying to prepare our themes and plugins:

ME.js has its own API for writing plugins, which is how it builds its own UI, however the API for this changed in 4.x. This is a basic callback for a new feature:

MediaElementPlayer.prototype.buildfeaturename = function( player, controls, layers, media ) {

};

In MediaElement.js 2.x, controls, layers, and media are all jQuery objects, but 4.x dropped the jQuery requirement, so those are now HTML elements. Any code that expects the jQuery objects now breaks. There are also various public properties on the player object that were jQuery objects that have been replaced or removed.

All HTML classes were prefixed with mejs- in 2.x. The latest version requires setting the new classPrefix option to mejs- to maintain backward compatibility. That's fine for the core players, but as soon as users upgrade WordPress before their theme or plugins that relied on the old behavior, they'll have broken media players.

Various global utiliies were also renamed, which breaks compatibility in scripts that were relying on them. For instance, mejs.MediaFeatures was renamed to mejs.Features and mejs.Utility is now mejs.Utils.

The styles and HTML structure for players have also changed.

With all these changes, there really isn't a way to prevent issues for users who don't upgrade their themes and plugins before WordPress.

We're prepared to handle the support burden and I generally approve of all the improvements in the new version of MediaElement.js, but with all the data type and API changes, it's basically a new library, so I thought I'd raise these concerns now in case anyone else is having similar issues.

[westonruter]

Thoughts on this?

[celloexpressions]

I noticed some minor visual back-compat issues with the update, which may cause issues with some themes and plugins that customize the styling. #41844 partially addresses that. The JS compatibility implications brought up here are more concerning, and likely more difficult to resolve, as the implication is broken players as opposed to visual inconsistencies.

[rafa8626]

@bradyvercher For the global variables, they can be added easily. As for the jQuery pieces, you have listed the main one, which is the way features are built. One of the methods can be overridden via MediaElementPlayer.prototype to prepare those arguments to accept jQuery objects if they are not the basic ones, like you described on ​https://github.com/mediaelement/mediaelement/issues/2415. I don't know what are the other properties on the player object that you may need but if you list them I can assess how they can be converted using the same approach described before. That code snippet to override variables or reassign variables can be placed on the wp-mediaelement.js. I can prepare a PR with this possible solution, but let me know

[rafa8626]

@bradyvercher and @celloexpressions Please test ​https://github.com/xwp/wordpress-develop/pull/282 and let me know your thoughts. I don't know all the ins and outs since I started my contributions when 3.x version was being developed, so I don't know what WP was using before. @westonruter FYI

[bradyvercher]

@rafa8626 I haven't had a chance to test your last commit, but I'm not sure proxying the feature build method in wp-mediaelement.js will work because themes and plugins don't use that file when initializing their own players. It would really have to be part of mediaelement-and-player.min.js unless WordPress forced the compatibility layer to load instead of relying on themes and plugins to enqueue it.

I'd have to dig in again, but these are some of the issues I've noticed based on the commits I've made over the past week or so:

New Default Class Prefix

Themes and plugins that initialize their own players won't have the new classPrefix option, so users that upgrade WordPress will have the new HTML class prefix applied to their players instead.

Changed or Missing Properties

The player instance throughout had these properties that were changed or are now missing:

• player.container (was a jQuery collection, now an HTML element)
• player.controls (was a jQuery collection, now an HTML element)
• player.$node (missing)
• player.$media (missing)

Removed Features

We used the same method core was using to add support for WMA files using the Silverlight plugin. Although that's been dropped, users running the older version of a theme or plugin will get a JavaScript error since these properties no longer exist.

mejs.plugins.silverlight[0].types.push( 'audio/x-ms-wma' );

Making it so that doesn't throw an error would be ideal.

Changed Reference

In success and feature callbacks, I believe the media parameter was the actual HTML media element ( <audio> or <video> ), whereas now it's a custom <mediaelement> wrapper, which doesn't have the same attributes and events.

We kinda pushed MediaElement.js to its limits and worked around a bunch of issues by proxying various public methods, so we'll have to live with some of the issues, but all of the above were changes to public data types and APIs that will cause JS errors for users when upgrading WordPress. We also had to make numerous CSS tweaks, but those shouldn't make pages unusable due to errors.

[ocean90]

Replying to bradyvercher:

@rafa8626 I haven't had a chance to test your last commit, but I'm not sure proxying the feature build method in wp-mediaelement.js will work because themes and plugins don't use that file when initializing their own players. It would really have to be part of mediaelement-and-player.min.js unless WordPress forced the compatibility layer to load instead of relying on themes and plugins to enqueue it.

We could try something similar to ​https://github.com/jquery/jquery-migrate. I thought wp-mediaelement.js does this already but it's really just a helper for the video/audio shortcodes.

[rafa8626]

@bradyvercher I see. That's where my knowledge on WP is limited. I agree with @ocean90 idea, and the PR I created contain must of the things you have listed. Last night, I updated MEJS to include a new method to override the issue with the features, so if the plan is to add a new JS just make sure that MEJS matches with the version of that PR. The <mediaelement> wrapper is needed since it's the fake node that allows all the other renderers to mimic the API, so that cannot be changed on my side; to access the original tag, you have to use media.originalNode. The other variable that I cannot change is player.container, given that is being used in lots of places.

Let me know if there's anything else I can help you, other than updating the PR as a baseline for the potential mediaelement-migrate.js file

[rafa8626]

@ocean90 and @bradyvercher please check the PR I listed above; it has been updated with more notes and some of the missing variables. Let me know if there's anything else I can do to assist you

[bradyvercher]

Replying to ocean90:

We could try something similar to ​https://github.com/jquery/jquery-migrate.

A migration shim would be perfect.

Replying to rafa8626:

The <mediaelement> wrapper is needed since it's the fake node that allows all the other renderers to mimic the API, so that cannot be changed on my side; to access the original tag, you have to use media.originalNode. The other variable that I cannot change is player.container, given that is being used in lots of places.

I would have to do some more testing around related to the fake node to know what kind of issues that poses, but player.container was widely used as a jQuery object in previous versions, so if we can't update that, the other changes won't help much.

What about updating MediaElement.js to use a method for retrieving those properties. That way if they've been converted to a jQuery object, they can be dereferenced before being used? I don't know how you'd structure it, but some sort of utility method like this:

MediaElementPlayer.prototype.get = function( el ) {
	return jQuery !== undefined && el instanceof jQuery ? el[0] : el;
};

Then anywhere you reference t.container or t.controls, change them to: t.get(t.container) and t.get(t.controls). Or create specific wrappers:

MediaElementPlayer.prototype.getControls = function() {
	return this.get(this.controls);
}

I think there were a couple of other places where elements might need to be dereferenced, too, so a generic utility method might be useful.

Replying to rafa8626:

please check the PR I listed above; it has been updated with more notes and some of the missing variables. Let me know if there's anything else I can do to assist you

That PR looks like a good start. I'll leave some comments on that regarding a couple of issues I noticed.

[rafa8626]

@bradyvercher I could implement that method you describe in the next few days, so you can override it in the same fashion you describe above. I will implement that change in the PR I created above and let you know when ready. All the code is now placed in wp-mediaelement.js, but I assume then it's gonna be moved to a different file correct?

[rafa8626]

@bradyvercher I have updated the PR; please review it and test. The new method I created is t.getElement(). That should help but let me know your thoughts

[bradyvercher]

@rafa8626 Thanks! Those changes look like they'll help clear up some of the errors. A couple of things I'm still noticing:

1) player.controls and player.container don't look like they're being converted to jQuery collections in the migration script.

2) It looks like the ​visible() method here may be passed a jQuery object at some point, which throws an error. That probably needs to be dereferenced. That's the only one I saw throwing an error, but I'm not sure if some of those other methods will ever get passed a jQuery object, too.

3) I haven't tracked this one down, but every element in the layers div is receiving an inline style with width: 100% on it now.

This is giving me hope that it may actually be possible to make this upgrade backward compatible. There are a couple of other minor things I've noticed, but I think if we can get those few issues worked out, it should prevent most of the JS errors on upgrade.

[rafa8626]

@bradyvercher Thanks for the feedback I'll check the issues you listed above and add whatever is needed in the PR.

[rafa8626]

@bradyvercher I have fixed the first 2 elements in the PR. As for the layers, the players is in responsive mode, so all the layers have 100% so when the user resizes the window everything gets resized accordingly. If that's not desired, we need to set the mode to none so it uses the width and height of the media. Let me know your thoughts

[bradyvercher]

@rafa8626 I added a note on the PR. There are also a few more instances in mediaelement-and-player.js where player.container is referenced without using the player.getElement(player.container) method.

If there's any way I can contribute some of these changes, let me know. At the very least I can work on the migration shim. Thanks for handling everything so quickly thus far!

[rafa8626]

@bradyvercher You are welcome! It's the least I can do, and I don't want to delay the release because of this issue. I have created in the PR the migration file and the reference now. You are contributing on this by reviewing constantly and making sure everything is compliant with WP. If you can continue doing this until you fully approve the changes, I'd appreciate it. Let me know if this last PR addressed your concerns, and if there's anything I need to change let me know. Thanks

[bradyvercher]

@rafa8626 There are still a few references to player.container in some of the features. I noticed them in at least progress, tracks, and volume.

For registering the migration script, I think you can look to how jQuery migrate is handled:

$scripts->add( 'mediaelement', false, array( 'jquery', 'mediaelement-core', 'mediaelement-migrate' ), '4.2.6-44f2e99' );
$scripts->add( 'mediaelement-core', "/wp-includes/js/mediaelement/mediaelement-and-player$suffix.js", array(), '4.2.6-44f2e99', 1 );
$scripts->add( 'mediaelement-migrate', "/wp-includes/js/mediaelement/mediaelement-migrate$suffix.js", array('mediaelement'), false, 1 );

Doing it that way should ensure the migration script is always enqueued after mediaelement-and-player.js. The formatting in mediaelement-migrate.js doesn't match WP's coding standards, so I'm not sure if that's something you want to take on, or leave to someone else.

I also came across another method that's been moved and can be added to the migration script:

mejs.HtmlMediaElementShim = mejs.HtmlMediaElementShim || {
	getTypeFromFile: mejs.Utils.getTypeFromFile
};

I'm still not sure about every element in the layers div receiving an inline width of 100%. I've tried setting stretching: none and they still receive that attribute. I've traced it down to the setDimensions() method, which appears to apply an inline width on layers no matter the stretching mode. Disabling setDimensions seems to help, but I'm not sure if that can be done as a default.

[rafa8626]

@bradyvercher Thanks for the feedback. I thought I had replaced al the calls of player.container completely. I'll double check that and the issue with setDimensions. I'll add that method that you described above in the migration script. As far as the WP's coding standards, where can I find them so I can format it accordingly?

[bradyvercher]

@rafa8626 The ​JS coding standards are here and you can run the file through JSHint using WordPress' config in the project root.

[rafa8626]

@bradyvercher Thanks I'll check it and let you know when PR it's ready to be reviewed again

[rafa8626]

@bradyvercher As for the setDimensions issue, it all depends of this (it's a comment in the player):

// determine the size

/* size priority:
(1) videoWidth (forced),
(2) style="width;height;"
(3) width attribute,
(4) defaultVideoWidth (for unspecified cases)
*/

If the video tag has 100% in CSS stylesheet or inline, it will be taken over the width and height attributes

[rafa8626]

@bradyvercher PR has been updated with your specifications; please review and let me know your thoughts. Thanks

[bradyvercher]

@rafa8626 I'm not seeing any more JS errors. I'll keep testing and report back if I come across anything else. Thanks again!

[rafa8626]

Great news! Thanks for testing this. Let me know if you need anything else from me. @westonruter @joemcgill @afercia FYI

[westonruter]

Just saw the confirmation from @bradyvercher at ​https://github.com/xwp/wordpress-develop/pull/282#issuecomment-336930795

[westonruter]

In 41877:

Media: Add MediaElement.js 2.2 back-compat for MediaElement.js 4.2

• Introduces mediaelement-migrate.js.
• Upgrades ME.js from 4.2.5-74e01a40 to 4.2.6-78496d1.

Props rafa8626, bradyvercher.
See #39686.
Fixes #42189.

[westonruter]

In 41891:

Media: Add missing mediaelement-migrate.js script to get uglified.

Amends [41877].
See #42189, #39686.