Make WordPress Core

Opened 2 years ago

Last modified 7 weeks ago

#56320 accepted defect (bug)

Update mediaelement.js to the latest version

Reported by: desrosj's profile desrosj Owned by: adamsilverstein's profile adamsilverstein
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: External Libraries Keywords: good-first-bug has-patch needs-testing 2nd-opinion
Focuses: accessibility, performance Cc:

Description

Follow up to #56319 (updating to the latest 4.x release).

The latest version of mediaelement, as of today, is 5.0.5.

Version 5.0 made the player WCAG 2.1 compliant, but some of the changes required are breaking. It's possible this update can be merged into WordPress without issue, but this update needs to be fully tested, and there may be a developer note required to communicate action that needs to be taken by plugin and theme developers.

Change History (41)

#1 @joedolson
2 years ago

  • Focuses accessibility added

#2 @desrosj
2 years ago

  • Focuses accessibility removed

The project's Migration Guide has specific changes to make and things to check when updating to version 5.x: https://github.com/mediaelement/mediaelement/blob/master/MIGRATION.md#migrating-from-4x-to-5x-version.

#3 @desrosj
2 years ago

  • Focuses accessibility added

Unintentionally removed the accessibility focus. Thanks for adding that, @joedolson!

This ticket was mentioned in PR #3139 on WordPress/wordpress-develop by CrochetFeve0251.


2 years ago
#4

  • Keywords has-patch added; needs-patch removed

Updated mediaelement.js to 5.0.5
Trac ticket: 56320

This ticket was mentioned in Slack in #core-performance by westonruter. View the logs.


7 months ago

#6 follow-up: @westonruter
7 months ago

  • Focuses performance added

The latest version is now v7.0.4. Note this release includes a performance fix.

#7 @adamsilverstein
7 months ago

  • Milestone changed from Future Release to 6.6
  • Owner set to adamsilverstein
  • Status changed from new to assigned

Lets try to land this in 6.6, ideally we should commit this change soon so it gets as wide spread testing as possible.

#8 in reply to: ↑ 6 @desrosj
6 months ago

  • Keywords needs-refresh added

Replying to westonruter:

The latest version is now v7.0.4. Note this release includes a performance fix.

@coquardcyr I realize it's been nearly 2 years since you created the PR attached here, but would you be interested in refreshing it to include the current latest version for testing?

#9 @coquardcyr
6 months ago

Hey @desrosj,
Just to confirm we are talking about version 7.0.5 for the last version?

I updated the branch with that version.

#10 @desrosj
6 months ago

  • Keywords needs-testing added

The PR is looking good. I'd like to see some manual testing to confirm there's no strangeness.

This ticket was mentioned in Slack in #core-performance by mukeshpanchal27. View the logs.


6 months ago

#12 @adamsilverstein
6 months ago

Testing Instructions

mediaelement.js is used to deliver a uniform video and audio playing experience to users. originally designed for the [video] and [audio] shortcodes (and maybe playlists?), I believe it is still used when inserting video or audio from the media library in the block editor.

  1. create a new post and upload a video and audio file for testing.
  2. embed the audio and video files.
  3. view the post and test that the audio and video controls work correctly and that no errors are visible in the debugger console.
  4. verify that mediaelement.js loaded with current version

Note: mediaelement was previously updated in https://core.trac.wordpress.org/ticket/39686 which might be useful to review for possible issues to look for.

#13 @mukesh27
6 months ago

@annezazu (Test Lead for 6.6) this ticket needs some manual testing. Could you please take a look when you have moment. Thanks!

This ticket was mentioned in Slack in #core-test by annezazu. View the logs.


6 months ago

#15 @annezazu
6 months ago

Flagged it for the core-test crew for now and will test early next week. I've been out much of this week but didn't want it to sit!

#16 @adamsilverstein
5 months ago

@coquardcyr - thanks for working on this.

I gave it a test and noticed some initial issues we need to resolve.

To test this I would up uploading an mp4 video file and an mp3 audio file to my page, and embedding them using shortcodes (instead of the block editor which inserts native video and audio html elements). I did this in the code editor view in Gutenberg (screenshot incoming).

The audio player loaded and worked as expected. The video player worked and loaded, however it seemed to missing part of its UI (see screenshot). One file seems to have failed to load "mejs-controls.svg" that is likely the issue.

#17 @adamsilverstein
5 months ago

I spent a little time trying to fix the loading issue and it will need some more work. The svg is now loaded in JS so it needs some changes to make that work correctly.

Likely we will need to punt this if we can't get the issues fixed by beta 1.

#18 @adamsilverstein
5 months ago

It should be possible to pass the correct path to the Javascript using a configuration variable, see this comment

This ticket was mentioned in PR #6714 on WordPress/wordpress-develop by @adamsilverstein.


5 months ago
#19

  • Keywords needs-refresh removed

Trac ticket:

#20 @adamsilverstein
5 months ago

I was able to fix the svg loading by addign the iconPath to the passed settings, see this commit

After this change, the icons for the video player loaded, however the main play button that is overlaid over the video was in the corner instead of the center of the video. In addition, I noted an error in the console indicating something went wrong. I will upload some screenshots.

Version 0, edited 5 months ago by adamsilverstein (next)

This ticket was mentioned in Slack in #core-performance by adamsilverstein. View the logs.


5 months ago

#22 @adamsilverstein
5 months ago

  • Milestone changed from 6.6 to 6.7

Note that in my PR I also tried upgrading to the latest latest version - 7.0.5.

Since I'm still see unresolved display issues with the player, I'm going to punt this to 6.7 for now.

This ticket was mentioned in Slack in #core-performance by mukeshpanchal27. View the logs.


5 months ago

#24 @adamsilverstein
5 months ago

  • Keywords 2nd-opinion added

After discussing this during a performance bug scrub, we should consider another approach.

  • Upgrading mediaelement is turning out to be a little trickier than just updating the library
  • This is *only* used for legacy [audio] and [video] shortcodes, and really only as a shim for browsers that don't support video or audio natively
  • All WordPress supported browsers now support native video and audio tags

In conclusion, I'd like to propose deprecating support for mediaelement.js and updating the shortcodes to use native HTML elements instead. Feedback welcome!

#25 @joedolson
5 months ago

We should conduct some up to date comparison testing between the accessibility of the native elements and MediaElement.js; last documentation I saw on native video accessibility was not favorable to the native video player, but that was five years ago, so it's probably not totally accurate anymore.

Updating shortcodes to use native HTML elements will probably visually change a lot of sites, since the HTML wrappers for mediaelement.js are common targets for style hooks. I'm not sure that's an easy path to take. Just a casual search on the .mejs-container wrapper shows significant use in themes.

This ticket was mentioned in Slack in #core-performance by mukeshpanchal27. View the logs.


5 months ago

#27 @pbearne
5 months ago

I like the idea of using HTML elements, but style changes might/will be an issue
we ship CSS to make the native look like the old MediaElement.js

#28 @joemcgill
5 months ago

Personally, I think it's ok to retain the MEJS library for shortcodes, but I don't think we need to prioritize upgrading the library, unless there is some real user benefit. We can also switch the shortcodes to using native with a documented way for sites that are relying on the MEJS library for styling purposes to retain its use.

If there are a11y issues with using native HTML video components, then it seems we would need to address the same issues for non-shortcode uses as well, and using the same implementation for shortcodes that we use for blocks would make this less complex to solve.

This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.


5 months ago

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


4 months ago

This ticket was mentioned in Slack in #core-performance by mukeshpanchal27. View the logs.


3 months ago

#32 @adamsilverstein
3 months ago

  • Milestone changed from 6.7 to Future Release
  • Status changed from assigned to accepted

deprioritizing this work due to the challenge of upgrading and the plan to move to more modern solutions.

#33 @adamsilverstein
3 months ago

  • Owner adamsilverstein deleted
  • Status changed from accepted to assigned

#34 @adamsilverstein
3 months ago

  • Owner set to adamsilverstein
  • Status changed from assigned to accepted

This ticket was mentioned in Slack in #core by stoyangeorgiev. View the logs.


2 months ago

This ticket was mentioned in Slack in #core by stoyangeorgiev. View the logs.


7 weeks ago

Note: See TracTickets for help on using tickets.