WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 11 months 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 (15)

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

Change History (52)

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


12 months ago

#3 @johnbillion
12 months 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
12 months 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
12 months ago

@Mista-Flo
12 months ago

Fix indentation

#5 @Mista-Flo
12 months 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.

Version 0, edited 12 months ago by Mista-Flo (next)

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


12 months ago

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


12 months ago

#8 @hellofromTonya
12 months 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 months 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.


12 months ago

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


11 months ago

#12 @antpb
11 months 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
11 months ago

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

Last edited 11 months ago by paaljoachim (previous) (diff)

#14 @mapk
11 months 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
11 months 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.


11 months ago

#17 @nrqsnchz
11 months ago

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

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

Minor conditional logic refresh.

#19 @johnbillion
11 months 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
11 months 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
11 months 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
11 months ago

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

@garrett-eclipse
11 months 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
11 months 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
11 months 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
11 months 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
11 months ago

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

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

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

#28 @johnbillion
11 months 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
11 months 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
11 months 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.


11 months ago

@helen
11 months ago

#32 @helen
11 months 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.


11 months ago

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


11 months ago

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


11 months ago

@helen
11 months ago

#36 @helen
11 months 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
11 months 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.