looking forward to testing this tomorrow

Tested using your supplied files. Only the pdf uploads. The mp3 and mp4 give a generic cant be uploaded message.
Tested a larger 5mb mp4 and it doesnt appear to do anything. No upload, no error, nothing
Sometimes after one of the failures the session is killed and you have to log in again

The problem was in your test instructions. Better to clear the existing values in the options and then save in order to get the defaults. If it had been customised at all it would fail

Click on a pdf file and then click on the three dots
Check the console

media-manager.min.js?6a2a37958931d4cd9934c0d049a970a8:1 Uncaught (in promise) TypeError: Cannot read property 'focus' of undefined
    at Proxy.<anonymous> (media-manager.min.js?6a2a37958931d4cd9934c0d049a970a8:1)

mp4 files have previews but there is no preview icon (you need to double click on the file to see the preview)

pdf files do not have previews but you can still double click on the file and the previous item is displayed

If you dont check the box "Use native elements" before inserting then nothing added to the article

For files and folder deletion on update with script.php you can leave it to @wilsonge and me. One of us will run the tool to generate the lists of files and folders when this PR has been merged and before the next release after that. So it does not need to be done in this PR. But if you want to do it here because you wanna make it right, I can help later.

For SQL scripts we have to decide how much we want to touch the media manager options on update. Since it will be at least risky and hacky trying to add single values to the json, we might have to completely replace the params so people would lose their settings on update if they have changed some. I can help with making the update SQL but not with
making the decision if we want that or not.

@richard67 whichever is decided on the sql there should be a postinstallation message. either saying the settings have been reset to the defaults or saying that you can update the settings to add support for pdf, etc

@richard67 whichever is decided on the sql there should be a postinstallation message. either saying the settings have been reset to the defaults or saying that you can update the settings to add support for pdf, etc

Yeah, that makes sense. Personally I'd prefer the "We don't touch the settings and let the postinstall message tell people that they can update their settings to add support for PDF and so on" approach.

It can be that we have already an update SQL touching these parameters when updating from 3.10. If this is the case, we could modify that one to set them right. I have to check if there is one.

IIRC we discussed this when I added webp support (or tried to)

I've just checked: It seems there is no update SQL script where we already touch the params of the com_media extension. So there is no need to modify such a script.

If you dont check the box "Use native elements" before inserting then nothing added to the article

Fixed, there are 3 modes: embed, insert asa link with predefined text, insert as a link with selected text from the editor (the 2nd was missing)

mp4 files have previews but there is no preview icon (you need to double click on the file to see the preview)

The preview needs refactoring so it can handle all the file types, also I thought that removing the magnifying glass I removed the preview but I totally forgot the double click FIXED

Click on a pdf file and then click on the three dots
Check the console

Some side effect from copy/pasting, investigating FIXED

The problem was in your test instructions. Better to clear the existing values in the options and then save in order to get the defaults. If it had been customised at all it would fail

Will update the instructions for existing installations

A comment about SQL files and B/C:

• Since J4 supports multiple local adapters the images adapter should be sniffed on update time and the default directory for media files should be media-uploads or something similar. Having audio, video or other files under a directory images is weird but maybe I'm heretic here. My point is that this can be done in a non B/C breaking way with a post install script for updates. Also doing it this way means that we get rid of runtime code that we have right now to do the adjustment, sounds more solid solution to me anyways.
  Anyways I'm ok whatever the decision is here, post install message or not or whatever...

A comment about SQL files and B/C:

* Since J4 supports multiple local adapters the `images` adapter should be sniffed on `update` time and the default directory for media files should be `media-uploads` or something similar. Having audio, video or other files under a directory `images` is weird but maybe I'm heretic here. My point is that this can be done in a non B/C breaking way with a post install script for updates. Also doing it this way means that we get rid of runtime code that we have right now to do the adjustment, sounds more solid solution to me anyways.
  Anyways I'm ok whatever the decision is here, post install message or not or whatever...

@dgrammatiko It would be good to have that post install script or procedure. But that would maybe be better a new routine in the script.php than an update SQL script. If necessary @joomdonation could have a look on it.

Preview of video now working correctly

Preview of pdf - almost - the preview opens but its black. Clicking on rotate makes it appear (maybe that will give a clue)

Audio file will not upload

Audio file will not upload

Make sure the legal mime types are: image/jpeg,image/gif,image/png,image/bmp,audio/mp3,video/mp4,application/msword,application/excel,application/pdf,application/powerpoint,text/plain,application/x-zip

Checked that twice before posting

image/jpeg,image/gif,image/png,image/bmp,audio/mp3,video/mp4,application/msword,application/excel,application/pdf,application/powerpoint,text/plain,application/x-zip

Article intro and full image
This works when selecting an image (as before)
However you can select a non image file - instead of some type of error message saying "this isnt an image etc" it just silently returns

it just silently returns

Yes, I couldn't decide what would be the best scenario for this case so I silently ignored anything non-image. This might need an alert or something similar.

Checked that twice before posting

There's something fishy as the tests are also failing for no obvious reason

@dgrammatiko Here the screenshot from drone from system tests: https://ci.joomla.org/artifacts/joomla/joomla-cms/4.0-dev/34634/system-tests/45401/MediaListCest.showsDefaultFilesAndFolders.mysql.fail.png

It says "syntax error, unexpected ')'".

Here the part of the log with the test where it fails:

MediaListCest: Test that it shows the joomla default media files and folders.
89s
325Signature: MediaListCest:showsDefaultFilesAndFolders
89s
326Test: tests/Codeception/acceptance/administrator/components/com_media/MediaListCest.php:showsDefaultFilesAndFolders
89s
327Scenario --
89s
328 I do administrator login 
89s
329  [Cookies] [{"name":"0cd014c79b960a69d3b33ac5245b0833","value":"2c944nm5pml9uatbrlgsjngk43","path":"/","domain":"localhost","expiry":1624893164,"secure":false,"httpOnly":false}]
89s
330  [Snapshot] Restored "ci-admin" session snapshot
89s
331 I create directory "images/test-dir"
89s
332   I get suite configuration 
89s
333   I get suite configuration 
89s
334 Created /tests/www/test-install/images/test-dir
89s
335 I am on page "administrator/index.php?option=com_media&..."
89s
336  [GET] http://localhost/test-install/administrator/index.php?option=com_media&path=local-images:/
89s
337 I wait for media loaded 
90s
338   I wait for element {"class":"media-loader"},3
90s
339   I wait for element not visible {"class":"media-loader"}
90s
340   I wait 1
90s
341 I see contents ["banners","headers","sampledata","joomla_black.png","powered_by.png"]
91s
342   I see element {"class":"media-browser-item"}
91s
343 I delete directory "images/test-dir"
91s
344   I get suite configuration 
91s
345 Deleted /tests/www/test-install/images/test-dir
91s
346 I execute js "window.sessionStorage.removeItem("joomla...."
91s
347  Screenshot and page source were saved into '/********/src/tests/Codeception/_output/' dir
91s
348 FAIL 

Does that help?

Does that help?

Yup, turns out PHP is far away from becoming a functional programming language...

@dgrammatiko Now only analysis4x is failing in drone, which is some security stuff using RIPS and is not related to your PR.

There was a general problem with that, but that should be solved now. So if you wanna get rid of the red test and see all green, just update your branch to latest 4.0-dev. But it's not absolutely necessary because we know the drone failure here is that thing.

Article intro and full image
This works when selecting an image (as before)
However you can select a non image file - instead of some type of error message saying "this isnt an image etc" it just silently returns

Now the media field will display only images (this is configurable in the code and can be adjusted to fit any combo)

Audio file will not upload

For this, I need some help, maybe @laoneo ?

Looks good to me (except broken upload for audio/video).

I would suggest to add webm, mov for video, and ogg for audio, from beginning. Or later we will get a complains about it.

Looks good to me (except broken upload for audio/video).

Technically this bug is unrelated to this PR, it just happens to become obvious from the changes here

I would suggest to add webm, mov for video, and ogg for audio, from beginning. Or later we will get a complains about it.

Yeah, also webp images and maybe avif (avif is already supported on GD and Imagick but right now you have to compile from source to enable support, but will be available in some next minor release)

There is already an open PR for webp. Its just stuck in an open state because cropper.js doesnt work in firefox for windows.

@brianteeman can you do a simple check: in your apache configuration there should be a file mime.types, could you compare it with this one
mime.types.txt (backup your existing, remove the .txt and place it in the same folder).
I have no clue if php is sniffing the mime types from apache but if it does then there isn't a problem but rather a misconfiguration (I can upload mp3/mp4 without any problems here)

Rebuilding my site at the moment but I think I may have found the problem

mime.types says
audio/mpeg mpga mp2 mp2a mp3 m2a m3a

This PR says
audio\mp3

Checking the spec audio\mpeg

So the problem does appear to be related to the pr and not the server

Good catch!

I can now add mp3

What is the intended behaviour with the mp3 preview. I just get a filename

I find this confusing. I expected the description to be an explanation of the option. The last sentence really describes what to do if you dont want a d/l link.

I would probably have two options
Embed
Use native elements audio, video or object

Download
Use a download link

What is the intended behaviour with the mp3 preview. I just get a filename

Weird, you should get:

and a markup:

Does your apache mime.types has uncommented the mp3 part? (I think you can drop a mimes.type in the root of the joomla folder, eg grab the one from a couple of comments above)

I would probably have two options

You mean something like a switcher or use one of the 2 proposals?

Weird - the preview works now

You mean something like a switcher or use one of the 2 proposals?

switcher/checkbox/radio - dont care but something that means the user is actively selecting embed or download

Nice to finally see this.

switcher/checkbox/radio - dont care but something that means the user is actively selecting embed or download

I went for a radio and it looks like:

Is there a reason you went for a fixed size embed instead of a responsive embed? (https://embedresponsively.com/)

Is there a reason you went for a fixed size embed instead of a responsive embed?

The wide/height are not for sizing but rather (as with images) to inform the browser what's the aspect ratio (plain English to allow the browser to correctly calculate the empty space till the video/object is loaded)

For example the code that linked does this (creates an empty 16/9 parent div and then inserts the iframe into it. This is a hack, we don't need to do this anymore, browsers respect and retrieve the correct aspect ratio from the width/height attributes.

A good resource for width/height from Jen Simmons: https://speakerdeck.com/jensimmons/using-the-html-width-and-height-attributes-to-improve-web-page-loading

Is there a reason you went for a fixed size embed instead of a responsive embed?

and thats the good reason - thanks

I have tested this item ✅ successfully on 08ce817

I have tested this item ✅ successfully on 08ce817

Maybe swap Embed and Download radio buttons since Enable Controls relates to Embed.

Maybe swap Embed and Download radio buttons since Enable Controls relates to Embed.

Done

Edit an article.
Click CMS Content > Media
Click on an MP3 file.
Click Embed radio button.
Click Enable Controls checkbox.
The checkbox should not disappear.

The checkbox should not disappear.

Fixed, thanks

Checked/unchecked Enable Controls produce the same markup.

<p><audio src="images/t-rex-roar.mp3" controls="controls"></audio></p>

Checked/unchecked Enable Controls produce the same markup.

Fixed, thanks

Missing icon in list view.

can you take a look at #34314 @dgrammatiko please

Missing icon in list view.

Fixed

can you take a look at #34314

I've seen it already but I don't think I'm the right guy for feedback there. My feeling is that we might want to control the contents of a folder that is returned (mind there's no permissions as in other components so things might have to be done different, eg utilising the same technic as: https://github.com/joomla/joomla-cms/pull/34634/files#diff-8404917e9476fe232ef9c49920ced95e9ce5825e5efd60f955ecde240a2a090aL533-R583 ) or wait till a db layer is added in com_media (or I'm totally wrong...).

Select an MP3 file.
Click Embed radio button.
Don't click Enable Controls.

Here is the markup, but nothing is displayed.

<audio src="images/t-rex-roar.mp3"></audio>

Here is the markup, but nothing is displayed.
<audio src="images/t-rex-roar.mp3"></audio>

Option to add or not controls removed. To my defend here, if you have a JS player you only need the tag with the source as the rest are often controlled by the JS player. Anyhow, if in the future people asking for this option the code is here...

Embedding an object requires and alt or equivalent see
https://accessibilityinsights.io/info-examples/web/object-alt/

The preview of a video is different to a preview of an image with regards to width and the position of the close button. I am assuming its some css

Embedding an object requires and alt or equivalent see

I will implement this later on today Implemented

The preview of a video is different to a preview of an image with regards to width and the position of the close button. I am assuming its some css

The current implementation is using inline style background for the image, thus the difference

Close icon overlaps filename.

Hello i tested work very good for me, just 2 notices :

• the additionnal windows doesn't close when we close media
• it will be cool to open media popup when we do right click or double click on media easier to change option
  thanks a million @dgrammatiko Now we have a real MEDIA manager !!!

the additionnal windows doesn't close when we close media

I don't understand what you mean, the media manager opens in a modal, not a new window...

it will be cool to open media popup when we do right click or double click on media easier to change option

This is not possible for 2 main reasons:

• there is always an extra step after selecting a file (provide alt text or embed or link for download) so it will never be one-step process
• there are 2 client-side applications: the media manager and a tiny Custom Element that asks for the extra info. These 2 cannot be combined into one as the one runs in an iframe and the other one on the main document. To get everything in the same window requires a huge refactor of few parts (doable but not a couple of days work...)

Ok for point 2
For point sorry for my Bad explain i mean the small zone with option (embed or download) this zone doesnt close when WE close media

i mean the small zone with option (embed or download) this zone doesnt close when WE close media

Can you record a video demonstrating what's not working? Seems fine here

i do small video, that the additionnal option zone
https://www.loom.com/share/b378c2a3c7304547ad577516f65c4742
thnaks

i do small video, that the additionnal option zone

Ok, I see what you mean and already fixed 3 cases that the aditional data should be dismissed but as long as some file is selected this will be visible. You can click on the title Aditional Data though, and that will be minimised. I can't do anything more here

Ok that already better !

@micker can you please mark you test on the issues site?

I have tested this item ✅ successfully on 42d28ef

I have tested this item ✅ successfully on 42d28ef too

@micker Could you please go to the issue tracker here and mark your test result in the right way? Just copying the message from another test is not sufficient. G to https://issues.joomla.org/tracker/joomla-cms/34634 , use the "Test this" button at the top left corner, select the appropriate test result and then submit. Thanks in advance.

I have tested this item ✅ successfully on 42d28ef

Tested and perfect
Sorry Richard a mistake

RTC

This seems to important to not merge right now given it has b/c implications. Although it's not great given our point of RC. Thankyou so much @dgrammatiko for your hard work on this!

woohoo!!!

Thanks

@wilsonge the B/C break is the change of some of the com_media params and to be honest here even what I did here is not perfect. An ideal solution is to have a data structure like

{
  supported: [
    {
      extension: 'png',
      mine: 'image/png',
      preview: true,
      upload: true
    }
  ]
}

That will allow to remove the hardcoded preview extensions in the com_media app (vue) and also if done right (subforms?) it will be easier in the backend (eg have a pool that users enable/disable, instead of typing tings)