Make WordPress Core

Opened 5 years ago

Closed 4 years ago

Last modified 7 weeks ago

#48562 closed defect (bug) (fixed)

Audio keeps playing on closing media/attachment details popup in WP Admin

Reported by: amolv's profile amolv Owned by: antpb's profile antpb
Milestone: 5.7 Priority: normal
Severity: normal Version:
Component: Media Keywords: has-patch commit
Focuses: Cc:

Description

/wp-admin/upload.php

Click on audio file, it opens audio in Media Popup, play audio, if we click on next attachment/media audio stops playing but if we just close "Attachment Details" popup audio keeps playing in the background.

Attachments (4)

48562.1.patch (733 bytes) - added by Mista-Flo 4 years ago.
48562.2.patch (712 bytes) - added by Mista-Flo 4 years ago.
Improve patch, pause the player instead of refreshing everything
48562.3.diff (469 bytes) - added by Mista-Flo 4 years ago.
Better fix
48562-before-and-after.mp4 (5.1 MB) - added by hellofromTonya 4 years ago.
Video with audio to test before and after with patch 48562.3.diff. Works as expected.

Change History (42)

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


5 years ago

#2 @antpb
5 years ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to Future Release
  • Owner set to antpb
  • Status changed from new to assigned

Sure enough, looks like this is happening for me too. :)

This ticket was brought up in the recent Media component meeting. Just to clarify reproduction steps, be sure to be in grid view of the Media Library when selecting the file. Then push play, close, see error.

@afercia mentioned in the meeting that this also existed in 5.2 so is not a regression from the recent 5.3 release.

Thanks for reporting this @amolv !

#3 @afercia
5 years ago

Looks like this happens for videos as well :)

#4 @Mista-Flo
4 years ago

I can reproduce as well, it's hard to know where is it configured in the code

@Mista-Flo
4 years ago

#5 @Mista-Flo
4 years ago

  • Keywords has-patch added; needs-patch removed

All right, so I finally ended up finding the code.

I have uploaded a patch that works, but I'm not sure about what I do exactly, so it might be a 'bad fix'.

I saw that when switching to the next attachment in the modal, it stops the audio/video. So I looked at the code, and I saw that it fires a refresh event. This refresh event call the rerender function which do some stuff in the background that stops the audio. So I fired that event in the close event as well and it worked.

#6 @justinahinon
4 years ago

48562.1.patch is fine and fixes the issue both for audios and videos files.

Last edited 4 years ago by justinahinon (previous) (diff)

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


4 years ago

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


4 years ago

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


4 years ago

#10 follow-up: @Clorith
4 years ago

Wondering if it might be a better approach to pause the playback, instead of triggering a refresh of the modal that's already been dismissed here (you still won't be able to resume if you open the same file, just like now, but I did notice that refreshing it triggers the update meta AJAX call when no changes have been made, while pausing did not appear to do so).

You should be able to achieve a similar approach to your current trigger, but instead you'll want to either trigger a click event on the mejs-pause button, or loop over any MediaElement instance and call the pause() function on them to do so.

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


4 years ago

#12 @antpb
4 years ago

  • Milestone changed from Future Release to 5.7

This is really close to being solved and would greatly improve the media library experience. As mentioned above, we may want to just pause the playback instead of refreshing things. I like the idea. Adding this to 5.7 so we can track along as we near release. :)

#13 @antpb
4 years ago

worth noting, if the pause approach is difficult or provides new bugs to solve, I think the refresh approach is a-ok.

@Mista-Flo
4 years ago

Improve patch, pause the player instead of refreshing everything

#14 in reply to: ↑ 10 @Mista-Flo
4 years ago

  • Keywords needs-testing added

Replying to Clorith:

Wondering if it might be a better approach to pause the playback, instead of triggering a refresh of the modal that's already been dismissed here (you still won't be able to resume if you open the same file, just like now, but I did notice that refreshing it triggers the update meta AJAX call when no changes have been made, while pausing did not appear to do so).

You should be able to achieve a similar approach to your current trigger, but instead you'll want to either trigger a click event on the mejs-pause button, or loop over any MediaElement instance and call the pause() function on them to do so.

Hi Clorith, thanks for your proposal, I just triggered a mejs-pause button pause, and it works, so I guess it's the simplest way to fix the issue.

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


4 years ago

#16 follow-up: @hellofromTonya
4 years ago

  • Keywords needs-testing-info added

As this ticket has been marked ready for testing, please provide need more information for testers to manually test the patch (including at Test Scrubs):

  • What are the steps to reproduce the reported problem?
  • What are the steps to test?
  • Are there any testing dependencies, such as a plugin or script?
  • What is the expected behavior after applying the patch?

#17 in reply to: ↑ 16 @Mista-Flo
4 years ago

  • Keywords needs-testing-info removed

Replying to hellofromTonya:

As this ticket has been marked ready for testing, please provide need more information for testers to manually test the patch (including at Test Scrubs):

  • What are the steps to reproduce the reported problem?
  • What are the steps to test?
  • Are there any testing dependencies, such as a plugin or script?
  • What is the expected behavior after applying the patch?

To reproduce:

  1. Go to the upload.php page (grid mode)
  2. Search for an audio file (or upload one long enough to test)
  3. Click on the audio file and play
  4. Close the media popup, the audio is still playing

To test:

  1. Apply the patch to your wordpress-develop environment
  2. Run npm run build:dev if you're vhost is set for src folder, otherwise npm run build

Try again the same steps to reproduce the issue, the audio should stop when closing the modal.

That's it.

Last edited 4 years ago by Mista-Flo (previous) (diff)

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


4 years ago

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


4 years ago

#20 @paaljoachim
4 years ago

Testing.
Went to a local site.
In media library grid mode.
Uploaded a long audio file.
Clicked to open the audio file and clicked play.
Closed the modul by clicking the X icon after playing the audio file a few seconds but audio kept playing.
--> Noticed the error.

Testing with localhost:8889
In terminal switched to

cd wordpress-develop
npm run grunt patch:48562

In media library grid mode.
Uploaded a long audio file.
Clicked to open the audio file and clicked play.
Closed the modul by clicking the X icon after playing the audio file a few seconds but audio kept playing.

Running "patch:48562" (patch) task
? Please select a patch to apply 48562.2.patch​ (712 bytes) - added by Mista-Flo
 7 days ago.
patching file src/js/media/views/frame/edit-attachments.js

Done.
paaljoachimromdahl@Paals-MacBook-Pro wordpress-develop %


Not sure what I am doing wrong here...

Version 1, edited 4 years ago by paaljoachim (previous) (next) (diff)

#21 @Mista-Flo
4 years ago

Hi @paaljoachim, thanks for your help, you missed the last step of the testing conditions, building assets.

Run npm run build:dev if you're vhost is set for src folder, otherwise npm run build

#22 @paaljoachim
4 years ago

Retesting.
Exited Terminal and reopened.
Added the following.

paaljoachimromdahl@Paals-MacBook-Pro wordpress-develop % git reset --hard
HEAD is now at 151787f21c Docs: Update documentation for `WP_Application_Passwords::application_name_exists_for_user()` per the documentation standards.
paaljoachimromdahl@Paals-MacBook-Pro wordpress-develop % npm run grunt patch:48562

> WordPress@5.7.0 grunt
> grunt "patch:48562"

package.json has not been modified.
Running "patch:48562" (patch) task
? Please select a patch to apply 48562.2.patch​ (712 bytes) - added by Mista-Flo 
7 days ago.
patching file src/js/media/views/frame/edit-attachments.js

Done.
paaljoachimromdahl@Paals-MacBook-Pro wordpress-develop % npm run build:dev

A every long list of messages showed up when running npm run build:dev

Reopening and logged in at: http://localhost:8889/

Went to the Media library. (Grid mode). Uploaded another long audio file.
Clicked to open Attachment details page.
Clicked to play the file.
Clicked X to close the modul.
Audio file continued playing.

#23 @paaljoachim
4 years ago

I also tested with

npm run build

and this time the patch worked!

Conclusion:
Clicking the audio file so the modul opens.
Then clicking the audio file so it begins playing.
Clicking the X to close the modul, also stop the audio from playing.

Last edited 4 years ago by paaljoachim (previous) (diff)

#24 @sannevndrmeulen
4 years ago

Reproduced before applying the patch.

After applying the patch I've tested:

  • Open and play audio file, click ✖ to close the window, audio file stops playing.
  • Open and play audio file, click outside the window, the window closes and audio file stops playing.
  • Open and play audio file, switch to a different media file, audio file stops playing.

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


4 years ago

#26 @paaljoachim
4 years ago

NB!
One thing that this patch has not included is for the Audio block in Gutenberg.

Open/create a post or page.
Add an Audio block.
Click "Media Library" to add the audio file.
In the almost full screen "Select or Upload Media" modal shows up.
Click the audio file.
See the Attachment Details in the right sidebar.
In the right sidebar click the play button.
Then click the X located top right to close the modal.
It goes back to Gutenberg.
The audio file keeps playing.

Please include a fix also for Gutenberg.
@Mista-Flo

#27 @Clorith
4 years ago

Wouldn't the audio block fall under the Gutenberg repo (with any other block), and not the generic media-uploads page in core? Or am I missing a relationship here?

#28 @paaljoachim
4 years ago

We might need to open a Github issue on the Gutenberg repo for this bug.
The reason for adding it here is that the bug is similar to what was patched for core.

#29 @Mista-Flo
4 years ago

Hum, I'm not sure how Gutenberg works with the audio block, because there is the React code, but if there is a use of the Media library, it's kind of linked with the Core as well, so how to deal with it I'm not sure, it was not raised in the initial ticket's description.

EDIT: Oh actually, I'm the one who pushed the patch to enable an audio/video player in the right sidebar, it was shipped in 5.6, so it might be just a Core issue. [43640]

Last edited 4 years ago by Mista-Flo (previous) (diff)

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


4 years ago

@Mista-Flo
4 years ago

Better fix

#31 @Mista-Flo
4 years ago

Ok so finally I have found a way of fixing both issues, it's directly handling it in the modal close function, instead of just one modal type.

You can test again both scenarios please (it works for me).

#32 @paaljoachim
4 years ago

  • Keywords needs-testing removed

Testing again.

Applied the patch.
Opened a page and added an Audio block.
Clicked Media Library
In the "Select or Upload Media" screen clicked an audio file.
Under Attachment Details. Clicked the play button to start listening to the audio file.
Clicked X to close the modal. Music stopped.

Good work @Mista-Flo

The patch is also working in Gutenberg!

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


4 years ago

@hellofromTonya
4 years ago

Video with audio to test before and after with patch 48562.3.diff. Works as expected.

#34 @hellofromTonya
4 years ago

  • Keywords commit added

Tested with .mp3 audio and .mp4 video and audio. Here's a link to the video showing before and after results.

Results:

Before applying the patch:

  • Audio kept playing after closing the modal.
  • Stopped playing when reopening the modal.

After applying the patch:

  • Audio stopped playing after closing the modal.

No errors in the console. Works as expected.

Marking for commit.

Last edited 4 years ago by hellofromTonya (previous) (diff)

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


4 years ago

#36 @adamsilverstein
4 years ago

Thanks for the detailed review @hellofromTonya, code looks good to me!

#37 @antpb
4 years ago

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

In 50256:

Media: Pause any playing media when closing the the media modal.

Previously, any video or audio playing in the media modal failed to stop playing when the modal was closed. Now we pause the player when the modal is closed.

Props adamsilverstein, hellofromTonya, paaljoachim, Mista-Flo, Clorith, justinahinon, afercia, amolv.
Fixes #48562.

This ticket was mentioned in Slack in #meta by johnbillion. View the logs.


3 years ago

Note: See TracTickets for help on using tickets.