WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 45 hours ago

#43640 closed feature request (fixed)

Add an audio and video player to the media manager modal

Reported by: johnbillion Owned by: johnbillion
Milestone: 5.6 Priority: normal
Severity: normal Version:
Component: Media Keywords: has-patch has-screenshots needs-testing commit
Focuses: Cc:

Description

When browsing for audio or video files from the Add Media button, there is no means of playing the media in order to preview or verify that it's the file you expect.

When viewing a media item in the media grid, or editing a media item, a player for both audio and video files is shown.

It would be good to have a player embedded in the sidebar of the media manager (above Attachment Details) when viewing a media item.

Attachments (13)

43640.1.patch (4.4 KB) - added by Mista-Flo 3 weeks ago.
43640.2.patch (4.4 KB) - added by Mista-Flo 3 weeks ago.
Fix indentation
Screenshot 2020-09-26 at 20.14.40.png (2.5 MB) - added by Mista-Flo 3 weeks ago.
Screenshot 2020-09-26 at 20.15.03.png (480.2 KB) - added by Mista-Flo 3 weeks ago.
Screenshot 2020-10-15 at 22.36.59.png (50.8 KB) - added by johnbillion 5 days ago.
Screenshot 2020-10-15 at 22.39.17.png (117.1 KB) - added by johnbillion 5 days ago.
43640.3.diff (4.1 KB) - added by garrett-eclipse 5 days ago.
Minor conditional logic refresh.
49b9c1e30cbb53530f6cdd5c20ac6b16.gif (1.2 MB) - added by garrett-eclipse 5 days ago.
Test Results: Upload video shows uploading and displays the embed upon upload.
a0368cdc586837016b0db86632444919.gif (1.1 MB) - added by garrett-eclipse 5 days ago.
Test Result: Audio shows the uploader but upon upload the audio player isn't presented instead there's an empty space until you switch to a different item and back.
58705f0fdbe4d833b26eb0ca0d5387b4.gif (582.8 KB) - added by garrett-eclipse 5 days ago.
Test Result: Deleting a media element changes select focus to the next remaining item but the side panel doesn't load the item.
43640.4.diff (4.5 KB) - added by garrett-eclipse 2 days ago.
Apply wp-audio class to the audio wp-media-wrapper and use it to apply a 12px top margin to provide space for the timer tooltip.
Screen Shot 2020-10-18 at 1.29.08 AM.png (24.8 KB) - added by garrett-eclipse 2 days ago.
Test of the latest patch to show timer tooltip fully in view.
Screenshot 2020-10-18 at 18.49.20.png (27.3 KB) - added by johnbillion 45 hours ago.

Change History (42)

#1 @joemcgill
18 months ago

  • Milestone changed from Awaiting Review to Future Release

Hi @johnbillion, welcome to Trac! I think this is a great suggestion.

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


4 weeks ago

#3 @johnbillion
4 weeks ago

A thought just came to mind, we'll need the ability to disable or replace this player (eg via a filter) because sites with heavily customised media management may need to play the media through a custom player.,

#4 @antpb
4 weeks ago

  • Milestone changed from Future Release to 5.6

I would also like to see this. I'm going to poke around the Media Library to see what this would take. It seems pretty straightforward to me. Worth calling out, if the solution is in the Core frames, we need to make sure to mirror changes in the Gutenberg repository.

@Mista-Flo
3 weeks ago

@Mista-Flo
3 weeks ago

Fix indentation

#5 @Mista-Flo
3 weeks ago

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

Hello,

I have uploaded a patch for this feature request that would be nice for the user experience.

You can also check the two screenshots, one for the audio player and the other for the video player.

It seems good to me, but it's better to double check that there is no regression across media modals.

I decided to not display the default video/audio icon along the details since there is no need for that with the player.

Last edited 3 weeks ago by Mista-Flo (previous) (diff)

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


3 weeks ago

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


12 days ago

#8 @hellofromTonya
12 days ago

  • Keywords needs-design-feedback added

Per Media scrub today, @Mista-Flo noted this ticket will benefit for design feedback. @karmatosed is on it! Capturing here to keep it on our radar.

#9 @hellofromTonya
12 days ago

Capturing testing conversation from the Media scrub today, ie so we don't forget:

@antpb will doing testing on it.

I owe testing on this one.
I'd really love to see this in before Beta.
Let's see if I can get some testing in today.

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


11 days ago

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


6 days ago

#12 @antpb
6 days ago

When we get Design thumbs up or suggestions, we can get this merged before Beta 1 next Tuesday. I'll be on standby for the thumbs up!

#13 @paaljoachim
6 days ago

Looks great to me!
I also pinged @mapk and @nrqsnchz about this ticket.

Last edited 6 days ago by paaljoachim (previous) (diff)

#14 @mapk
6 days ago

  • Keywords needs-design-feedback removed

Thanks for the ping, @paaljoachim. I agree, the design and location of the media player looks great! Thumbs up on design. Let's get this shipped.

#15 @hellofromTonya
5 days ago

  • Keywords commit added

Per conversation with @antpb, this one is ready.

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


5 days ago

#17 @nrqsnchz
5 days ago

Agree with @mapk, this looks good and ready to ship 👍

#18 follow-up: @johnbillion
5 days ago

  • Keywords needs-refresh added; commit removed

This is working nicely and is a very useful addition. I'm just seeing two display issues with the tracker. Screenshots above.

  1. On an audio file, the tooltip for the time gets cut off at the top.
  2. On an audio or video file, there is a square white artefact around the position marker.

Tested with Firefox and Chrome on macOS .

@garrett-eclipse
5 days ago

Minor conditional logic refresh.

#19 @johnbillion
5 days ago

@garrett-eclipse It looks like with your patch the media-progress-bar doesn't get shown for audio and video files that are still uploading. Can you check and confirm?

#20 @garrett-eclipse
5 days ago

From an initial code review I've uploaded 43640.3.diff that combines the <# if ( 'video' !== data.type && 'audio' !== data.type ) { #> conditional into the previous statement as an else since both these conditions were checked in that chain.

Note: This doesn't address @johnbillion's flagged items, just wanted to improve that conditional after my code review.

#21 @garrett-eclipse
5 days ago

Hmm I'll definitely look, that combining of conditionals shouldn't have changed the logic... but then again I do live/work around 2 little kids so I very well could have flubbed.

@garrett-eclipse
5 days ago

Test Results: Upload video shows uploading and displays the embed upon upload.

@garrett-eclipse
5 days ago

Test Result: Audio shows the uploader but upon upload the audio player isn't presented instead there's an empty space until you switch to a different item and back.

@garrett-eclipse
5 days ago

Test Result: Deleting a media element changes select focus to the next remaining item but the side panel doesn't load the item.

#22 follow-ups: @garrett-eclipse
5 days ago

Got a chance to test @johnbillion and I did see the media uploader for both audio and video. Could you check if a larger file to see if it's just not uploading too fast to see the loader?

I did note that;

  1. "On an audio file, the tooltip for the time gets cut off at the top." Confirmed
  2. "On an audio or video file, there is a square white artefact around the position marker." Can't Reproduce (Mac Chrome)
  3. While the video player becomes available upon upload the audio one doesn't. You need to switch to another item and return before it appears.
  4. When deleting an item it's focus moves to the next available item, but the side panel is blank. This may be expected but would assume when focus is on an item it's details would be displayed.

Screengrabs of the above are attached.

@garrett-eclipse
2 days ago

Apply wp-audio class to the audio wp-media-wrapper and use it to apply a 12px top margin to provide space for the timer tooltip.

@garrett-eclipse
2 days ago

Test of the latest patch to show timer tooltip fully in view.

#23 in reply to: ↑ 22 @garrett-eclipse
2 days ago

  1. "On an audio file, the tooltip for the time gets cut off at the top." Confirmed

With 43640.4.diff I've applied a 12px top margin to the audio media wrapper in order to give it the space so the timer tooltip wasn't cut off. The alternative I was looking at was just removing the overflow hidden on the .attachment-info but I was worried that might introduce a back-compact issue with plugins using the forced overflow as their bounds.

#24 in reply to: ↑ 18 @garrett-eclipse
2 days ago

Replying to johnbillion:

  1. On an audio or video file, there is a square white artefact around the position marker.

Tested with Firefox and Chrome on macOS .

Hi @johnbillion can you do some inspecting on this issue, I'm unable to reproduce on mac Chrome/Firefox/Safari/Opera. Possibly a browser extension? Hoping an inspect can at least tell us the artifact class or id or markup.

#25 in reply to: ↑ 22 @garrett-eclipse
2 days ago

Replying to garrett-eclipse:

  1. When deleting an item it's focus moves to the next available item, but the side panel is blank. This may be expected but would assume when focus is on an item it's details would be displayed.

I can confirm now that this is expected, if you open the modal clean and tab through focus will land on the media elements but the sidepanel doesn't load until enter/click action is taken. So when a deletion happens and the focus goes to the next item it is expected a click or enter action is needed for the attachment details to display.

#26 in reply to: ↑ 22 @garrett-eclipse
2 days ago

  • Keywords needs-refresh removed

Replying to garrett-eclipse:

  1. While the video player becomes available upon upload the audio one doesn't. You need to switch to another item and return before it appears.

I can no longer reproduce this behaviour after multiple attempts so can disregard.

The latest patch in my opinion is good to go beyond a retest and investigation from @johnbillion on that artifact.

@antpb / @Mista-Flo would you like to do some more testing and confirm things are looking good here.

#27 @johnbillion
2 days ago

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

#28 @johnbillion
45 hours ago

  • Keywords commit added

The square white artefact around the position marker is from the mejs-time-handle-content element and its corresponding style is here: https://core.trac.wordpress.org/browser/tags/5.5.1/src/js/_enqueues/vendor/mediaelement/mediaelementplayer-legacy.css?marks=417#L403 . Screenshot above.

I've been giving this some testing today and it's all looking good apart from the above.

I'm going to get this committed and we can investigate further and iterate if necessary.

#29 @johnbillion
45 hours ago

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

In 49195:

Media: Add an audio and video player to the media manager modal.

This introduces a means of playing existing audio and video files while browsing them prior to selecting them for use.

Props antpb, Mista-Flo, garrett-eclipse, mapk

Fixes #43640

Note: See TracTickets for help on using tickets.