Make WordPress Core

Opened 7 years ago

Closed 4 years ago

#43640 closed feature request (fixed)

Add an audio and video player to the media manager modal

Reported by: johnbillion's profile johnbillion Owned by: johnbillion's profile 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 (15)

43640.1.patch (4.4 KB) - added by Mista-Flo 5 years ago.
43640.2.patch (4.4 KB) - added by Mista-Flo 5 years ago.
Fix indentation
Screenshot 2020-09-26 at 20.14.40.png (2.5 MB) - added by Mista-Flo 5 years ago.
Screenshot 2020-09-26 at 20.15.03.png (480.2 KB) - added by Mista-Flo 5 years ago.
Screenshot 2020-10-15 at 22.36.59.png (50.8 KB) - added by johnbillion 5 years ago.
Screenshot 2020-10-15 at 22.39.17.png (117.1 KB) - added by johnbillion 5 years ago.
43640.3.diff (4.1 KB) - added by garrett-eclipse 5 years ago.
Minor conditional logic refresh.
49b9c1e30cbb53530f6cdd5c20ac6b16.gif (1.2 MB) - added by garrett-eclipse 5 years ago.
Test Results: Upload video shows uploading and displays the embed upon upload.
a0368cdc586837016b0db86632444919.gif (1.1 MB) - added by garrett-eclipse 5 years 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 years 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 5 years 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 5 years 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 5 years ago.
43640.diff (396 bytes) - added by helen 4 years ago.
43640.2.diff (431 bytes) - added by helen 4 years ago.

Change History (52)

#1 @joemcgill
6 years 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.


5 years ago

#3 @johnbillion
5 years 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
5 years 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
5 years ago

@Mista-Flo
5 years ago

Fix indentation

#5 @Mista-Flo
5 years 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 5 years ago by Mista-Flo (previous) (diff)

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


5 years ago

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


5 years ago

#8 @hellofromTonya
5 years 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
5 years 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.


5 years ago

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


5 years ago

#12 @antpb
5 years 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
5 years ago

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

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

#14 @mapk
5 years 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 years 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 years ago

#17 @nrqsnchz
5 years ago

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

#18 follow-up: @johnbillion
5 years 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 years ago

Minor conditional logic refresh.

#19 @johnbillion
5 years 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 years 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 years 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 years ago

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

@garrett-eclipse
5 years 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 years 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 years 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
5 years 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
5 years ago

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

#23 in reply to: ↑ 22 @garrett-eclipse
5 years 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
5 years 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
5 years 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
5 years 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
5 years ago

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

#28 @johnbillion
5 years 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
5 years 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

#30 @garrett-eclipse
4 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for further investigation on the remaining white artifact.

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


4 years ago

@helen
4 years ago

#32 @helen
4 years ago

43640.diff fixes the placement issue in the admin context - wp-mediaelement.css seems to need a general review of colors so I'm going to ignore that I don't really love the styling here and leave that for another ticket and time :)

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


4 years ago

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


4 years ago

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


4 years ago

@helen
4 years ago

#36 @helen
4 years ago

Looked at this more deeply, comparing between media modal and front-end display via shortcode ([audio src="blah"]), 43640.2.diff addresses the base issue better, which is that box-sizing was being overridden by another media modal rule.

#37 @helen
4 years ago

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

In 49343:

Media: Fix styling for MediaElement player in media modal.

Fixes #43640.

Note: See TracTickets for help on using tickets.