Make WordPress Core

Opened 4 months ago

Closed 2 months ago

#41787 closed defect (bug) (fixed)

Media: JS error when removing a video/audio sourced

Reported by: afercia Owned by: joemcgill
Milestone: 4.9 Priority: normal
Severity: normal Version: 4.9
Component: Media Keywords: has-patch
Focuses: javascript Cc:


Noticed while investigating on #38759.

To reproduce:

  • add a video or audio in a post
  • click on the video/audio player to make the edit details button appear and click it
  • a modal opens with the audio/video details
  • click on "remove video source" (or audio)
  • check your browser console:

Uncaught TypeError: Cannot read property 'remove' of undefined

Happens also when adding and then removing additional, alternate video/audio sources.

Couldn't reproduce on stable 4.8.1 so seems a regression in trunk to me, probably related to recent MediaElement.js update (seems to me the whole $media and $node properties are missing).

Attachments (2)

41787.diff (4.1 KB) - added by joemcgill 3 months ago.
41787.2.diff (1.6 KB) - added by joemcgill 2 months ago.

Download all attachments as: .zip

Change History (17)

#1 @joemcgill
3 months ago

Confirmed. Looks like MediaElement.js removed the $node and $media JS selectors from mejs.MediaElementPlayer() in version 4.0.0, which we're planning on upgrading to in 4.9.

See #39686.

@rafa8626, what is the expected compatibility process here?

#2 @rafa8626
3 months ago

  • Summary changed from Media: JS error when removing a video/audio source to Media: JS error when removing a video/audio sourced

@joemcgill I'll take a look at it as soon as I can. If it's removed those elements should be removed. I didn't know the player was removed at any point. Can you list steps on how to reproduce it to see what type of work will be needed, please?

#3 @rafa8626
3 months ago

Also the selectors referred to jQuery which was completely stripped out in prior versions. With some user cases, I'd be able to determine what to do with those variables

#4 @rafa8626
3 months ago

Oh sorry for some reason the page didn't load completely and I didn't see the steps listed above. I'll create a PR to solve this issue as soon as I can. Thanks

#5 @rafa8626
3 months ago

  • Keywords needs-testing added; needs-patch removed

@joemcgill and @afercia Please test https://github.com/xwp/wordpress-develop/pull/252; I had to update the way WP removes the player for the sake of matching how MEJS do it now and it seems that the error is fixed. Please let me know if everything is OK. I verified that those variables won't exist anywhere else in WP code

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

3 months ago

#7 @joemcgill
3 months ago

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

#8 @westonruter
3 months ago

@joemcgill Patch has been refreshed with merge from master: https://github.com/xwp/wordpress-develop/pull/252

The Travis build should pass. Previously it was failing because trunk itself was failing.

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

3 months ago

3 months ago

#10 @joemcgill
3 months ago

Thanks @westonruter. That PR had gone stale again, and includes a bunch of extra commit noise at this point. 41787.diff is a unified diff of the original changes. I think I have everything, and it seems to fix the issue, but would like @rafa8626 to confirm I haven't missed anything in the move.

#11 @joemcgill
3 months ago

  • Keywords has-patch added

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

2 months ago

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

2 months ago

2 months ago

#14 @joemcgill
2 months ago

  • Keywords needs-testing removed

Noticed that the previous patch included some non-related changes. Additionally, media-audiovideo.js is generated from wp-includes/js/media/audiovideo.manifest.js so I've moved the changes there in 41787.2.diff.

#15 @joemcgill
2 months ago

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

In 41781:

Media: Fix MEJS error when removing a media player.

This fixes a bug introduced by the upgrade to MediaElement.js, where code
calling wp.media.mixin.removePlayer() would result in a JS error.

Props rafa8626, afercia.
Fixes #41787.

Note: See TracTickets for help on using tickets.