Using aria-hidden = true on the icons is not enough to satisfy accessibility.
For example with the current code what you are doing is completely removing all the meaning from the icons function

As an example none of the icons in the media-view-icons div have any text so with any assistive technology they are invisible

@brianteeman we are aware that the current state lacks on accessibility. The plan, I guess, is to merge it in core and then have the accessibility team do the evaluation and come up with the required markup changes so we can implement them

The icons used for filetype in the list view appear to be hardcoded against a very limited list of extensions. As a result they miss a lot of common filetypes.

For example there is an icon for *.doc but there is not for *.docx

You can find a more complete list here https://gschoppe.com/uncategorized/add-auto-sensing-file-type-icons-to-lists-of-downloads-with-fontawesome-and-css/

[UPDATE]
Actually the icon usage is inconsistent - in the list view a docx does not have the correct icon but it does have in the thumbnail view

@dgt41 regarding a11y - it doesnt need an a11y review to know that everytime you have an icon with aria-hidden=true you need to have some meaningful text for assistive technology.

It is all documented here http://fontawesome.io/accessibility/ so it is a waste of the a11y time to comment on this when it is easy to do and a known issue with a known fix.

Let the a11y team concentrate on reviewing the issues that we dont know about

@laoneo @dgt41 for accessibility I opened an issue here joomla-projects/media-manager-improvement#404

Create a new folder - i expect the create box to disappear on success - it doesnt

The success message and more importantly the fail message is hard to see as it is hidden under the .media-modal-backdrop

Note that the same is not true for a similar action such as renaming

Double click required to enter a folder or preview an image when in thumbnail view. This is not intuitive for a web application which normally works with a single click

cannot preview an image when in list view. double click etc does nothing

When previewing an image the previous image is displayed before the new one is loaded - see movie

Wrong path displayed in the info display
for example my site is installed in a subdirectory called cms4
the path displayed in the info is for example
local-0:/sampledata/parks
not sure what the local-0 means but at the very least the path should be
local-0:/images/sampledata/parks

Wow! 👍 thx for all the feedback. lets recap for a sec.

Create a new folder - i expect the create box to disappear on success - it doesnt

works for me. At least for success. For failure we need to improve it => Needs to be fixed.

Double click required to enter a folder or preview an image when in thumbnail view. This is not intuitive for a web application which normally works with a single click

that was a UX decision. for me personally that is a natural way to do it. other media managers are doing it that way. single-click navigation would be a little weird.
We should probably add another icon for that: joomla-projects/media-manager-improvement#217 ? Do you have any other suggestions?

cannot preview an image when in list view. double click etc does nothing

know enhancement. will be fixed with joomla-projects/media-manager-improvement#344

When previewing an image the previous image is displayed before the new one is loaded - see movie

State issue. Should be fixed.

Wrong path displayed in the info display

Good catch. Should be fixed.

Quality text breaks outside of its container

Ratio buttons in the crop view dont do anything

Resize plugin - the slider is a nice touch but it is impossible to set an exact size easily - i would expect at least a text box where i can input the required dimension

Rotate - the text for the angle doesnt change always when rotated - the text also doesnt reset when the reset button on the toolbar is selected. Also the slider is a nice touch but it is impossible to set an exact angle easily - i would expect at least a text box where i can input the required angle

See movie

Thumbnail and preview of an image do not update after successfully cropping/rotating/resizing
See movie

New folder issues - see movie

Image upload.
The success message is immediate. There appears to be a progress bar so i wouldnt expect the message to appear until after the progress bar is 100%
Even then the image thumbnail is not displayed until a page reload.

see movie

Dropzone
There is nothing to indicate that drag and drop upload is possible - i only new to try it because i new you were planning to support it

If you disable all the media action plugins the pencil icon is still present. If you click on it then you get a screen with just a toolbar and none of the buttons on the toolbar work so you cannot get out of that screen

Thumbnail size icons
Personally I had no idea what the + - icons would do. Through trial and error I see that they are to increase the thumbnail size. Perhaps the more traditional zoom icons such as http://fontawesome.io/icon/search-plus/ woulld be better?

Re my earlier comments about the media action plugin sliders being inaccurate etc they are also a massive accessibility fail

To move forward. I suggest that we create issues on the media manager repo for all your feedback and will deliver them in a second pr after this one gets merged. Like that we can get stuff done quicker when we are in our repo.

Whatever you think is best BUT if it is merged as is with these obvious errors and bugs then others will go ahead and report them again - thus wasting time. Especially if they are present when @wilsonge releases an alpha

Open issues in the MM repo obviously is fine. But I tend to agree with Brian that it doesn't make much sense to merge this PR when it has so many issues already open. Lets do the regular way and adjust the PR till at least most issues are solved and then merge it.

Dont get confused by the number of comments.

Please lets keep in mind that

• media manager is under heavy development,
• most of the issues are UX, UI and/or a11y related / only a few of them are really high prio issues and
• we want to merge to a dev !== production branch especially to get this kind of feedback early and in short cycles (we tried to motivate people to test our repo, but that didnt work out).

My suggestion is: lets create and prioritize the issues in nmm repo. Fix the urgent ones and merge this thing asap.

I'm worried about the tinymce changes here at the moment. Especially as we're removing legacy code I'm not 100% sure we actually can remove yet. Plus on how the whole interaction between editor xtd and tinymce works at the moment. That's more than just a media manager change and slightly nerves me

This is something me and Dimitris had a chat about last Sunday but I haven't had a proper chance to review yet. But I do want to do a proper review of it before I 'just merge' it

I dont' really have a problem with the media manager itself codebase

@wilsonge do you want me to do a pr with only these changes, so it's easier for you to go through?

When trying to test this, I get this:

This is on XAMPP on Windows.
The error log shows this:

[28-Sep-2017 13:00:36 Europe/Berlin] PHP Notice:  Undefined offset: 1 in D:\xampp\htdocs\joomla4\administrator\components\com_media\Controller\ApiController.php on line 456
[28-Sep-2017 13:00:36 Europe/Berlin] PHP Stack trace:
[28-Sep-2017 13:00:36 Europe/Berlin] PHP   1. {main}() D:\xampp\htdocs\joomla4\administrator\index.php:0
[28-Sep-2017 13:00:36 Europe/Berlin] PHP   2. require_once() D:\xampp\htdocs\joomla4\administrator\index.php:30
[28-Sep-2017 13:00:36 Europe/Berlin] PHP   3. Joomla\CMS\Application\AdministratorApplication->execute() D:\xampp\htdocs\joomla4\administrator\includes\app.php:35
[28-Sep-2017 13:00:36 Europe/Berlin] PHP   4. Joomla\CMS\Application\AdministratorApplication->doExecute() D:\xampp\htdocs\joomla4\libraries\src\Application\CMSApplication.php:340
[28-Sep-2017 13:00:36 Europe/Berlin] PHP   5. Joomla\CMS\Application\AdministratorApplication->dispatch() D:\xampp\htdocs\joomla4\libraries\src\Application\AdministratorApplication.php:149
[28-Sep-2017 13:00:36 Europe/Berlin] PHP   6. Joomla\CMS\Component\ComponentHelper::renderComponent() D:\xampp\htdocs\joomla4\libraries\src\Application\AdministratorApplication.php:106
[28-Sep-2017 13:00:36 Europe/Berlin] PHP   7. Joomla\CMS\Component\ComponentHelper::dispatchComponent() D:\xampp\htdocs\joomla4\libraries\src\Component\ComponentHelper.php:375
[28-Sep-2017 13:00:36 Europe/Berlin] PHP   8. MediaDispatcher->dispatch() D:\xampp\htdocs\joomla4\libraries\src\Component\ComponentHelper.php:434
[28-Sep-2017 13:00:36 Europe/Berlin] PHP   9. Joomla\Component\Media\Administrator\Controller\ApiController->execute() D:\xampp\htdocs\joomla4\libraries\src\Dispatcher\Dispatcher.php:167
[28-Sep-2017 13:00:36 Europe/Berlin] PHP  10. Joomla\Component\Media\Administrator\Controller\ApiController->get_files() D:\xampp\htdocs\joomla4\administrator\components\com_media\Controller\ApiController.php:66
[28-Sep-2017 13:00:36 Europe/Berlin] PHP  11. Joomla\Component\Media\Administrator\Controller\ApiController->getPath() D:\xampp\htdocs\joomla4\administrator\components\com_media\Controller\ApiController.php:141

I tried to apply this PR by merging it and also by just checking out the full repo. Both ways I get that same error.

edit: was a browser cache issue.

@Bakual can you please open your issue in NMM repo ? with Console output

@kasvith Just tried with a fresh J4 installation. I can reproduce it as well, but only if I visit the "old" MM first. Clearing cache solves it. Not sure why this happens but it's now solved for me.

final comment
I seem to recall that there is an issue with having a dash - in the plugin group name and we have had to do something special to support xtd-editor so as this is a new plugin group lets remove the dash -

Double click required to enter a folder or preview an image when in thumbnail view. This is not intuitive for a web application which normally works with a single click

that was a UX decision. for me personally that is a natural way to do it. other media managers are doing it that way. single-click navigation would be a little weird.

Personally, I find the double click a bit weird as well like Brian. Especially since the first click only ticks the box, but the box doesn't have any use as far as I see except for me I want to delete multiple media. But to select multiple items I need to click into the checkbox anyway. So imho there is no use in the single click here.
What was the reasoning given by the UX team?

Another UX question. If I press "Shift-Click", I can select multiple items as well, but that's unintuive at least on Windows machines. We usually use "Ctrl-Click" to select multiple items and "Shift-Click" to select a full line (eg all between first selected and the current one).

Yes it should be ctrl click when you want to select multiple items individually and shift click on the first and last when you want to select the group

@brianteeman we fixed the dash issue already in the 4.0 branch, so it should be fine.

@wilsonge if the tinymce changes are the biggest issue, then it would probably make sense that we revert them and discus in it's own pr. Would that be ok for you?

@laoneo if we revert them then the media field/xtd button will not work, so basically the media manager cannot be used for anything meaningful...

Errors found by Drone / PHPCS

FILE: ...la-cms/pull/18145/administrator/components/com_media/Model/ApiModel.php
--------------------------------------------------------------------------------
FOUND 6 ERROR(S) AFFECTING 6 LINE(S)
--------------------------------------------------------------------------------
 160 | ERROR | Doc comment for var override does not match actual variable name
     |       | $override at position 4
 174 | ERROR | Expected "catch (...)\n...{\n"; found "catch (...)\n...{"
 175 | ERROR | Closing brace must be on a line by itself
 194 | ERROR | Doc comment for var override does not match actual variable name
     |       | $override at position 5
 208 | ERROR | Expected "catch (...)\n...{\n"; found "catch (...)\n...{"
 209 | ERROR | Closing brace must be on a line by itself
--------------------------------------------------------------------------------
UPGRADE TO PHP_CODESNIFFER 2.0 TO FIX ERRORS AUTOMATICALLY
--------------------------------------------------------------------------------

FILE: ...ub.com/joomla/joomla-cms/pull/18145/plugins/editors/tinymce/tinymce.php
--------------------------------------------------------------------------------
FOUND 6 ERROR(S) AFFECTING 6 LINE(S)
--------------------------------------------------------------------------------
 553 | ERROR | Please put a space between the // and the start of comment text;
     |       | found "//var_dump($externalPlugins);"
 554 | ERROR | Please put a space between the // and the start of comment text;
     |       | found "//var_dump($toolbar2);"
 555 | ERROR | Please put a space between the // and the start of comment text;
     |       | found "//die;"
 605 | ERROR | Please put a space between the // and the start of comment text;
     |       | found "//			'dndPath'    => JUri::root() .
     |       | 'media/editors/tinymce/js/plugins/dragdrop/plugin.min.js',"
 606 | ERROR | Please put a space between the // and the start of comment text;
     |       | found "//			'Image'      => JUri::root().
     |       | 'media/editors/tinymce/js/plugins/media/media.js',"
 724 | ERROR | Expected "switch (...)\n...{\n"; found "switch (...) ...{\n"
--------------------------------------------------------------------------------
UPGRADE TO PHP_CODESNIFFER 2.0 TO FIX ERRORS AUTOMATICALLY
--------------------------------------------------------------------------------