? Language Change a11y NPM Resource Changed ? Pending

User tests: Successful: Unsuccessful:

avatar crystalenka
crystalenka
15 Sep 2022

The current media quick actions, while useful, are confusing as currently displayed:
Screen Shot of media quick action icons

Depending on the media item, the icons are hard to see, and it's not always clear what each icon means (especially with rename). Additionally, when the mouse leaves the relatively small focus area, the buttons disappear and you have to click into it again. This leads to frustration.

Summary of Changes

I have reworked the display of the actions to include text for sighted users and make it more clear what each icon does. Additionally, I changed the icon for "rename" to one that's more accurate (as the arrows before signified changing width or size).

They now display like so:
Screen Shot of revised media quick action icons

It's not a perfect solution - they still disappear when the mouse leaves the focus area - but it should be easier and clearer for people to understand the available functions.

Testing Instructions

As this PR changes build files for JS and CSS, you will have to download the build package from this PR. The patch in the patch tester will NOT work.

Navigate to the media manager. Click on the ellipses icon on an item.

Actual result BEFORE applying this Pull Request

Icon-only actions displayed.
Screen Shot of media quick action icons

Expected result AFTER applying this Pull Request

A more comprehensive menu displays.
Screen Shot of revised media quick action icons

Documentation Changes Required

None that I'm aware of.

Additional Notes

Open to a more elegant solution. Vue.js is not something I'm familiar with so it's possible there's a better way to do this. Regarding accessibility, it should function for keyboard/screenreader users exactly as before.

avatar crystalenka crystalenka - open - 15 Sep 2022
avatar crystalenka crystalenka - change - 15 Sep 2022
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 15 Sep 2022
Category Administration com_media NPM Change Repository
avatar coolcat-creations
coolcat-creations - comment - 15 Sep 2022

+1000 Very great improvement!

avatar crystalenka crystalenka - change - 15 Sep 2022
Labels Added: NPM Resource Changed ?
avatar brianteeman
brianteeman - comment - 15 Sep 2022

Does it need the word "item"? Isnt that a redundant word.

avatar crystalenka
crystalenka - comment - 15 Sep 2022

It’s very much redundant; that’s what the language strings were to begin with. Was considering removing it alsoOn Sep 15, 2022, at 4:35 PM, Brian Teeman @.***> wrote:
Does it need the word "item"? Isnt that a redundant word.

—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you authored the thread.Message ID: @.***>

avatar crystalenka
crystalenka - comment - 15 Sep 2022

Does it need the word "item"? Isnt that a redundant word.

Removed :)

avatar joomla-cms-bot joomla-cms-bot - change - 15 Sep 2022
Category Administration com_media NPM Change Repository Administration com_media NPM Change Language & Strings Repository
avatar crystalenka
crystalenka - comment - 15 Sep 2022

Note to self - need to check what happens with super long language strings. I have a feeling it will break if the translation is too long so I need to anticipate that.

avatar brianteeman
brianteeman - comment - 15 Sep 2022

image

Doesn't seem correct but I can't view the source to check it ;(

avatar Kostelano
Kostelano - comment - 15 Sep 2022

Removing redundant 'item' from media action language strings

@crystalenka remove also for frontend please.

avatar crystalenka crystalenka - change - 20 Sep 2022
Labels Added: Language Change
avatar crystalenka crystalenka - change - 20 Sep 2022
The description was changed
avatar crystalenka crystalenka - edited - 20 Sep 2022
avatar crystalenka
crystalenka - comment - 20 Sep 2022

Removing redundant 'item' from media action language strings

@crystalenka remove also for frontend please.

Done, thank you!

avatar crystalenka
crystalenka - comment - 20 Sep 2022

Converting to draft, as it's not even close to accessible for keyboard users.

avatar crystalenka
crystalenka - comment - 20 Sep 2022

This is ready for review and testing.

There are KNOWN ISSUES for accessibility in the media manager (keyboard access, focus styles, etc) however fixing those would require considerable changes to the markup of the media manager and are outside the scope of this PR. This PR does not introduce additional barriers to access and hopefully mitigates some prior a11y issues regarding understanding of the actions, etc.

@chmst can we discuss in JAT the best way to approach the accessibility here? I am sure it will require minor breaking changes given how everything is currently generated with vue.js

avatar coolcat-creations
coolcat-creations - comment - 20 Sep 2022

There is a z-index issue, sorry!
grafik

avatar crystalenka
crystalenka - comment - 20 Sep 2022

There is a z-index issue, sorry!
grafik

I fixed it in the last commit, caught it probably just after you downloaded the build!

avatar coolcat-creations
coolcat-creations - comment - 20 Sep 2022

I have tested this item successfully on befefcd

Thank you the new menu makes much more sense. Tested successfully.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/38771.

avatar coolcat-creations coolcat-creations - test_item - 20 Sep 2022 - Tested successfully
avatar brianteeman
brianteeman - comment - 20 Sep 2022

I have tested this item successfully on befefcd

Nice improvements


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/38771.

avatar brianteeman brianteeman - test_item - 20 Sep 2022 - Tested successfully
avatar crystalenka
crystalenka - comment - 20 Sep 2022

Converting this back to draft. Mobile is a mess. I'm frustrated. ? Will take another look at this another time. Others are welcome to give it a try also.

avatar brianteeman
brianteeman - comment - 20 Sep 2022

Leave mobile to another pr

avatar chmst
chmst - comment - 20 Sep 2022

@chmst can we discuss in JAT the best way to approach the accessibility here? I am sure it will require minor breaking changes given how everything is currently generated with vue.js

Thanks so much, this is a nice improvement .JAT noted a11y issues since the very begining, some have been resolved, some still are open. Implementing is a challenge, as vue.js is requried, as you say.

avatar chmst
chmst - comment - 20 Sep 2022

It would be so great if the name of the item could be added to the task instead of "item" - but need help in vue.js. @angieradtke suggests "media item" instead of "item" which can be everything

avatar laoneo
laoneo - comment - 20 Sep 2022

Can you not just hide the item text on small screens and then have another look on mobile in a follow up pr?

avatar brianteeman
brianteeman - comment - 20 Sep 2022

It would be really helpful if JAT created issues for any accessibility issues so that developers are able to work on them. If they're not on github no one knows

avatar brianteeman
brianteeman - comment - 20 Sep 2022

It would be so great if the name of the item could be added to the task instead of "item" - but need help in vue.js. @angieradtke suggests "media item" instead of "item" which can be everything

in what whay does adding the word item or the words media item add to the meaning of this option?

avatar crystalenka
crystalenka - comment - 20 Sep 2022

Can you not just hide the item text on small screens and then have another look on mobile in a follow up pr?

The mobile functionality is unrelated to the language string discussion. It doesn't work.

Leave mobile to another pr

It's broken enough that I don't feel comfortable leaving it as-is.

avatar crystalenka crystalenka - change - 20 Sep 2022
Labels Added: a11y
avatar crystalenka
crystalenka - comment - 20 Sep 2022

I don't know HOW I managed it but I figured out the Vue enough to get the item name and put it in an sr-only span as a description of the button.

Hard to debug, so the rendered HTML for each button is:

<button type="button" class="action-rename" aria-describedby="rename-desc">
     <span class="image-browser-action fa fa-i-cursor" aria-hidden="true"></span>
     <span class="action-text">Rename</span>
     <span class="sr-only" id="rename-desc">Rename item: joomla_black.png</span>
</button>

I imagine this will work better than generic 'rename item' etc. :)

Now to get it working better in mobile so I can open this again for testing...

avatar brianteeman
brianteeman - comment - 20 Sep 2022

Doesn't it now become

Button, rename joomla.png, rename

?

avatar coolcat-creations
coolcat-creations - comment - 20 Sep 2022

Yes I think its now: Rename Rename item: joomla_black.png

avatar brianteeman
brianteeman - comment - 20 Sep 2022

Tested with Narrator

Rename item: joomla_black.png

Tested with NVDA

Rename item: joomla_black.png
button Rename item: joomla_black.png

And that of course is not taking into account that there are six buttons which will all be announced with the potentially unpronouncable filename.

This is trying to fix a problem that does not exist - which I am sure @angieradtke will realise when she gets back from holiday and is in front of a computer

avatar joomla-cms-bot joomla-cms-bot - change - 20 Sep 2022
Category Administration com_media NPM Change Repository Language & Strings Administration com_media NPM Change JavaScript Language & Strings Repository
avatar crystalenka
crystalenka - comment - 20 Sep 2022

Tested with Narrator

Rename item: joomla_black.png

Tested with NVDA

Rename item: joomla_black.png
button Rename item: joomla_black.png

And that of course is not taking into account that there are six buttons which will all be announced with the potentially unpronouncable filename.

This is trying to fix a problem that does not exist - which I am sure @angieradtke will realise when she gets back from holiday and is in front of a computer

I was hoping that by using aria-describedby it would be an optional readout instead of reading out everything. Looking into it...

avatar crystalenka
crystalenka - comment - 20 Sep 2022

Perhaps aria-details would be better? I don't believe that is read automatically.

avatar brianteeman
brianteeman - comment - 20 Sep 2022

Perhaps aria-details would be better? I don't believe that is read automatically.

even better than that it is rarely read at all as most screen readers do not support it

avatar crystalenka
crystalenka - comment - 20 Sep 2022

?

avatar crystalenka
crystalenka - comment - 21 Sep 2022

I am going to leave this PR as-is; it can always be better, but this is still a significant improvement to the current pattern.

In a future PR I will rework the html/js here so that it's a fully accessible disclosure widget; as it's structured now, anything further I could do would just be covering up the symptom instead of addressing the root of the problem.

But for now, it's better.

Ready for re-testing and review.

avatar crystalenka crystalenka - change - 21 Sep 2022
The description was changed
avatar crystalenka crystalenka - edited - 21 Sep 2022
avatar brianteeman
brianteeman - comment - 21 Sep 2022

I have not tested this item.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/38771.

avatar brianteeman brianteeman - test_item - 21 Sep 2022 - Not tested
avatar brianteeman
brianteeman - comment - 21 Sep 2022

I have tested this item successfully on ec9d65c

sorry pressedwrong radio on my phone.

This is better than it was yesterday


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/38771.

avatar brianteeman brianteeman - test_item - 21 Sep 2022 - Tested successfully
avatar nikosdion
nikosdion - comment - 21 Sep 2022

I have tested this item successfully on ec9d65c

Tested on desktop (Safari, Edge) and mobile (Safari on iPhone and iPad). It's not perfect (nor can it be without trashing it and starting afresh) but it's now far more usable than the Media Manager before this PR was applied on both desktop and mobile.

On mobile I had to increase the preview size — which I do anyway as I don't have Superman's telephoto vision — and then it's working fine for me. It's far, FAR better than it was before. I'll take it! I mark this successful because, in the words of Steve Jobs, “Real artists ship”.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/38771.

avatar nikosdion nikosdion - test_item - 21 Sep 2022 - Tested successfully
avatar crystalenka
crystalenka - comment - 24 Sep 2022

@richard67 I updated the latest changes from the branch but it previously had 2 successful tests (whoops), no real changes after that. Is there anything else that we need to do here before it can be reviewed or merged?

avatar richard67 richard67 - alter_testresult - 24 Sep 2022 - brianteeman: Tested successfully
avatar richard67 richard67 - alter_testresult - 24 Sep 2022 - nikosdion: Tested successfully
avatar richard67
richard67 - comment - 24 Sep 2022

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/38771.

avatar richard67 richard67 - change - 24 Sep 2022
Status Pending Ready to Commit
avatar richard67
richard67 - comment - 24 Sep 2022

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/38771.

avatar richard67
richard67 - comment - 24 Sep 2022

@crystalenka Human test results get reset in the test counter of the issue tracker after any commit. Therefore I recommend to update the branch only if necessary when a PR has some tests because searching for PRs with 2 tests will work in the issue tracker, which simplifies it a lot for me.

I have restored the test results now and set RTC.

avatar crystalenka
crystalenka - comment - 24 Sep 2022

Good to know, thank you!

avatar stevefaulkner
stevefaulkner - comment - 25 Sep 2022

suggest:

<button type="button" class="action-rename" aria-describedby="rename-desc">
     <span class="image-browser-action fa fa-i-cursor" aria-hidden="true"></span>
     <span class="action-text">Rename</span>
     <span class="sr-only" id="rename-desc">item: joomla_black.png</span>
</button>

This will be announced by screen readers as "Rename, item: joomla_black.png"

The use of aria-details in this context is not appropriate as the additional information clearly falls into the category of an accessible description. aria-details is also not as robustly implemented/supported as aria-describedby

The aria-details property is for referencing elements that provide more detailed information than would normally be provided via aria-describedby.

avatar angieradtke
angieradtke - comment - 26 Sep 2022

@stevefaulkner Many thanks for your advice, it is so helpful

2 last questions for better understanding, futher implementation and our documentation:

aria-describedby -> I read it is possible to reference an element even if that element is hidden.
Does it mean that it can be used for elements that are set to display:none als well or only for elments using visibility:hidden ?
Or for elements set out of the viewport?
In our case the <span class="sr-only" id="rename-desc">item: joomla_black.png</span> is set out of the viewport.

<button type="button" class="action-rename" >
Is the result without using the attribute aria-describedby not the same, because the span is an part of the button contents?

I ask because if it is like that - we can save lots of code.

Thank you

avatar stevefaulkner
stevefaulkner - comment - 26 Sep 2022

@stevefaulkner Many thanks for your advice, it is so helpful

2 last questions for better understanding, further implementation and our documentation:

aria-describedby -> I read it is possible to reference an element even if that element is hidden. Does it mean that it can be used for elements that are set to display:none als well or only for elments using visibility:hidden ? Or for elements set out of the viewport? In our case the <span class="sr-only" id="rename-desc">item: joomla_black.png</span> is set out of the viewport.
Yes, in fact for the pattern i described to work as intended the description must be hidden using display:none or visibility:hidden otherwise the desc text will be included in the accessible name as well.
This may be helpful: Hiding Content Has No Effect on Accessible Name or Description Calculation

codepen example

<button type="button" class="action-rename" > Is the result without using the attribute aria-describedby not the same, because the span is an part of the button contents?

I ask because if it is like that - we can save lots of code.

in this case the accessible name for the button is "Rename" if the sr-only text is hidden using CSS

<button type="button" class="action-rename">
     <span class="image-browser-action fa fa-i-cursor" aria-hidden="true"></span>
     <span class="action-text">Rename</span>
     <span class="sr-only" id="rename-desc">item: joomla_black.png</span>
</button>

some alternatives, with less code:

<button type="button" class="action-rename" aria-description="item: joomla_black.png">
     <span class="image-browser-action fa fa-i-cursor" aria-hidden="true"></span>
     <span class="action-text">Rename</span>
</button>
<button type="button" class="action-rename" aria-label="Rename item: joomla_black.png">
     <span class="image-browser-action fa fa-i-cursor" aria-hidden="true"></span>
     <span class="action-text">Rename</span>
</button>
avatar brianteeman
brianteeman - comment - 26 Sep 2022

The code in this pr produces a list made of buttons

<div class="media-browser-actions-list">
  <ul>
    <li aria-hidden="true" class="media-browser-actions-item-name">
      <strong>joomla_black.png</strong>
    </li>
    <li>
      <button type="button" class="action-preview" aria-details="preview-desc">
        <span
          class="image-browser-action icon-search-plus"
          aria-hidden="true"
        ></span
        ><span class="action-text">Preview</span></button
      ><span class="sr-only" id="preview-desc"
        >Preview item: joomla_black.png</span
      >
    </li>
    <li>
      <button
        type="button"
        class="action-download"
        aria-details="download-desc"
      >
        <span
          class="image-browser-action icon-download"
          aria-hidden="true"
        ></span
        ><span class="action-text">Download</span></button
      ><span class="sr-only" id="download-desc"
        >Download item: joomla_black.png</span
      >
    </li>
    <li>
      <button type="button" class="action-rename" aria-details="rename-desc">
        <span
          class="image-browser-action fa fa-i-cursor"
          aria-hidden="true"
        ></span
        ><span class="action-text">Rename</span></button
      ><span class="sr-only" id="rename-desc"
        >Rename item: joomla_black.png</span
      >
    </li>
    <li>
      <button type="button" class="action-edit" aria-details="edit-desc">
        <span
          class="image-browser-action icon-pencil-alt"
          aria-hidden="true"
        ></span
        ><span class="action-text">Edit</span></button
      ><span class="sr-only" id="edit-desc">Edit item: joomla_black.png</span>
    </li>
    <li>
      <button type="button" class="action-url" aria-details="share-desc">
        <span class="image-browser-action icon-link" aria-hidden="true"></span
        ><span class="action-text">Get link</span></button
      ><span class="sr-only" id="share-desc"
        >Get a direct link to joomla_black.png</span
      >
    </li>
    <li>
      <button type="button" class="action-delete" aria-details="delete-desc">
        <span class="image-browser-action icon-trash" aria-hidden="true"></span
        ><span class="action-text">Delete</span></button
      ><span class="sr-only" id="delete-desc"
        >Delete item: joomla_black.png</span
      >
    </li>
  </ul>
</div>

Which (without the fancy css and js) is rendered as
image

Here we can see the following issues

  1. The first list item is really a title of the list and not a first item
  2. each item in the list repeats the verb in the button and the description
  3. each item in the list repeats the filename. this is not necessary as the entire list relates to the filename
  4. each item in the list repeats the unnecessary word "item"

If we look at the suggestion from @stevefaulkner it has the same/similar issues. (probably because steve was not aware of the full context.

Important

please remember that NO list is ever in the dom until the action button is selected. So if assistive tech is used to display a list of links on the page there will not be multiple delete buttons etc.

Answer to Angie

aria-describedby -> I read it is possible to reference an element even if that element is hidden.
If it is in the dom then yes. the class sr-only, as used in joomla only visually hides,
image

avatar brianteeman
brianteeman - comment - 26 Sep 2022

personaly I wonder if this really should be a menu and not a list as it appears to perfectly satisfy the description of a menu (not to be confused with a navigation menu)

https://www.w3.org/WAI/ARIA/apg/patterns/menu/

avatar stevefaulkner
stevefaulkner - comment - 26 Sep 2022

@brianteeman in the case you describe I would suggest:

<button type="button" class="action-delete" aria-labelledby="delete-desc">
<span class="image-browser-action icon-trash" aria-hidden="true"></span>
<span class="action-text">Delete</span>
</button>
<span class="sr-only" id="delete-desc">Delete item: joomla_black.png</span>

or

<button type="button" class="action-rename" aria-label="Delete item: joomla_black.png">
     <span class="image-browser-action fa fa-i-cursor" aria-hidden="true"></span>
     <span class="action-text">Delete</span>
</button>
avatar angieradtke
angieradtke - comment - 26 Sep 2022

Yes, in fact for the pattern i described to work as intended the description must be hidden using display:none or visibility:hidden otherwise the desc text will be included in the accessible name as well.

Our .sr-only class has NO display:none and NO visibility: hidden
`{
position: absolute !important;
width: 1px !important;
height: 1px !important;
padding: 0 !important;
margin: -1px !important;
overflow: hidden !important;
clip: rect(0, 0, 0, 0) !important;
white-space: nowrap !important;
border: 0 !important;

} `

so that the content is part of the accessible name

<button type="button" class="action-rename">
     <span class="image-browser-action fa fa-i-cursor" aria-hidden="true"></span>
     <span class="action-text">Rename</span>
     <span class="sr-only" id="rename-desc">item: joomla_black.png</span>
</button>

Expected Result: Rename item: joomla_black.png

This is shows a good alternative no CSS is needed and aria-label is well supported:

<button type="button" class="action-rename" aria-label="Rename item: joomla_black.png">
 <span class="image-browser-action fa fa-i-cursor" aria-hidden="true"></span>
  <span class="action-text">Rename</span>
</button>

aria-label wins over the button content

Expected Result: Rename item: joomla_black.png

Questions:

  • What is faster?
  • Using only HTML is more familiar to me, but it is more code.
avatar angieradtke
angieradtke - comment - 26 Sep 2022

In all it can look like:

<ul>
		<li>
			<button type="button" class="action-rename" aria-label="Preview item: joomla_black.png">
				<span class="image-browser-action fa fa-i-cursor" aria-hidden="true"></span>
				<span class="action-text">Preview</span>
			</button>
		</li>
		<li>
			<button type="button" class="action-rename" aria-label="Download item: joomla_black.png">
				<span class="image-browser-action fa fa-i-cursor" aria-hidden="true"></span>
				<span class="action-text">Download</span>
			</button>
		</li>
		<li>
			<button type="button" class="action-rename" aria-label="Rename item: joomla_black.png">
				<span class="image-browser-action fa fa-i-cursor" aria-hidden="true"></span>
				<span class="action-text">Rename</span>
			</button>
		</li>
		<li>
		......
	</ul>
avatar brianteeman
brianteeman - comment - 26 Sep 2022

I still say item is a useless word

avatar crystalenka
crystalenka - comment - 26 Sep 2022

Thank you everyone for the ongoing discussion here. :) Pleasantly surprised to see @stevefaulkner in our little corner of Github! Thank you for your time and feedback.

One more thing that has not been noted but may be important for context:
This list of actions only shows up when triggered by an action from the user on the following button:

<button
    type="button"
    class="action-toggle"
    tabindex="0"
    aria-label="Manage item: joomla_black.png"
    title="Manage item: joomla_black.png"
  >
    <span class="image-browser-action icon-ellipsis-h" aria-hidden="true"></span>
  </button>

(now that I'm looking at it, I should probably also remove that tabindex attribute...)

@brianteeman is probably right that this should be a menu and not a list of buttons, but I'm focusing on incremental changes since there's some heavy tech debt here with how it's built.

So I can see it's pretty clear that that aria-details was the wrong attribute to use on the button, and that we can use either aria-label or aria-labelledby to get the desired behavior.

One topic of ongoing discussion was whether or not each "action button" needed to repeat the filename or "item" part of the string, since the button that triggered this list to display already included the filename. Is it better to include the filename regardless via the methods you suggested on each item, or is the context provided on the button that triggered the list enough?

One of the reasons this has been a topic of discussion is both because of the verbosity and repetitiveness of each button, but also because the filename might be something long and nonsensical, like a generated alphanumeric string or something.

Thank you again for your time and input! We will put it to good use both here as well as in our developer documentation for accessible patterns :)

avatar stevefaulkner
stevefaulkner - comment - 26 Sep 2022

One of the reasons this has been a topic of discussion is both because of the verbosity and repetitiveness of each button

if there are a group (more specifically in this case it could be a toolbar of controls that all act upon a common object this could have a group label rather than repeating the object name in each description.

<div role="toolbar" aria-label="item: joomla_black.png">
<button type="button" class="action-rename">
<span class="image-browser-action fa fa-i-cursor" aria-hidden="true"></span>
<span class="action-text">Download</span>
</button>
<button type="button" class="action-delete">
<span class="image-browser-action fa fa-i-cursor" aria-hidden="true"></span>
<span class="action-text">Delete</span>
</button>
...
</div>

avatar brianteeman
brianteeman - comment - 26 Sep 2022

Exactly.

avatar crystalenka
crystalenka - comment - 26 Sep 2022

Super helpful. Thank you!!

Converting back to draft; It seems I have some more to do here...

avatar crystalenka crystalenka - change - 26 Sep 2022
Labels Added: ?
avatar richard67 richard67 - change - 26 Sep 2022
Status Ready to Commit Pending
avatar richard67
richard67 - comment - 26 Sep 2022

Back to pending


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/38771.

avatar crystalenka crystalenka - change - 26 Sep 2022
Labels Removed: ?
avatar crystalenka
crystalenka - comment - 26 Sep 2022

Okay, so here is the new structure with the toolbar active:

<div class="media-browser-actions">
    <button type="button" class="action-toggle" aria-label="Manage item: joomla_black.png" title="Manage item: joomla_black.png">
        <span class="image-browser-action icon-ellipsis-h" aria-hidden="true"></span>
    </button>
    <div class="media-browser-actions-list" role="toolbar" aria-label="Item: joomla_black.png">
        <span aria-hidden="true" class="media-browser-actions-item-name"><strong>joomla_black.png</strong></span>
        <button type="button" class="action-preview">
            <span class="image-browser-action icon-search-plus" aria-hidden="true"></span>
            <span class="action-text">Preview</span>
        </button>
        <button type="button" class="action-download">
            <span class="image-browser-action icon-download" aria-hidden="true"></span>
            <span class="action-text">Download</span>
        </button>
        [etc...]
    </div>
</div>

And here are screenshots of how it's rendered before and after clicking/triggering the toggle button:
Screen Shot of a media item on hover
Screen Shot of the visible actions toolbar

Before I mark this as ready for testing/review, does anyone have further feedback on aria attributes, elements, etc?

I got rid of the ul altogether since it was not serving a functional purpose. I'm not sure a span is the best element for the item name, but wanted to post it as such regardless for feedback.

avatar brianteeman
brianteeman - comment - 26 Sep 2022

You can add a role menu

avatar crystalenka
crystalenka - comment - 26 Sep 2022

You can add a role menu

Doesn't that imply/rely on other aria roles in the menu items or on the toolbar that don't currently exist? I am hesitant to add too many aria roles here since the javascript is a bit of a black box for me and I don't want to imply some kind of interaction for screen reader users that does not actually exist or work. Remember that even hover and focus on these items are being managed by javascript until I figure out how it works and can strip it down to more native html interactivity.

avatar stevefaulkner
stevefaulkner - comment - 26 Sep 2022

You can add a role menu

Doesn't that imply/rely on other aria roles in the menu items or on the toolbar that don't currently exist? I am hesitant to add too many aria roles here since the javascript is a bit of a black box for me and I don't want to imply some kind of interaction for screen reader users that does not actually exist or work. Remember that even hover and focus on these items are being managed by javascript until I figure out how it works and can strip it down to more native html interactivity.

if a role="menu" is used the UI will need to follow the interaction requirements of an ARIA menu.

avatar crystalenka
crystalenka - comment - 26 Sep 2022

Okay. Since messing with the underlying Vue is beyond what I can do right now I am going to leave it as-is with the improved a11y commits and put this back to "ready for review".

Thank you everyone for your help!

avatar brianteeman
brianteeman - comment - 26 Sep 2022

the keyboard up/down arrow navigation does not work - see animation of just using the down arrow

keynpard navigation

avatar brianteeman
brianteeman - comment - 26 Sep 2022

Was it intenional to change the size of the previews?

Before

image

After

image

avatar crystalenka
crystalenka - comment - 27 Sep 2022

Was it intenional to change the size of the previews?

Not so drastically. I reworked the CSS to use grid and grid-gap instead of hardcoded percentages (which left empty space to the right a lot); but I left my testing variables in there (to make sure it worked) and they slipped in. New variables committed; should be closer to before now but it depends on the screen width as the columns will auto-fit.

the keyboard up/down arrow navigation does not work - see animation of just using the down arrow

Did it work before? I definitely didn't touch that, but will take a look and see if I can figure it out.

avatar brianteeman
brianteeman - comment - 27 Sep 2022

Possibly not. But then there wasn't a role or ariavertical telling me that i should use the arrows

avatar crystalenka
crystalenka - comment - 27 Sep 2022

There is aria-orientation="vertical" on the element with role="toolbar" as of this commit: 24821f2

I'm trying to fix the javascript now.

avatar brianteeman
brianteeman - comment - 27 Sep 2022

Yes that's what I mean. With that change I now expected the arrows to work.

avatar crystalenka
crystalenka - comment - 27 Sep 2022

Yeah, the arrows are not working because the javascript is binding to the keys instead of native interactivity. I'm working on figuring it out...

avatar crystalenka
crystalenka - comment - 27 Sep 2022

Okay, I have done what I can with the javascript there. Interactions should now follow documented toolbar behavior:

  • tab and shift+tab should move the focus in and out of the toolbar
  • home key now moves the focus to the first item of the toolbar
  • end key now moves the focus to the last item of the toolbar
  • up and down arrows should be functioning as expected
  • tabindex is now behaving as expected within the toolbar

all of this is based on the MDN docs: https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/roles/toolbar_role

Please let me know if it works.

avatar brianteeman
brianteeman - comment - 27 Sep 2022

arrows keys work
thumbnail size - improved

Thanks

Still a lot of a11y issues to address but beyond the scope of this PR

avatar crystalenka
crystalenka - comment - 27 Sep 2022

Still a lot of a11y issues to address but beyond the scope of this PR

Don't I know it ? one thing at a time...

After this latest commit I think it's ready for human tests again. I won't be tweaking CSS anymore at this point.

avatar crystalenka
crystalenka - comment - 27 Sep 2022

@nikosdion @coolcat-creations could you please retest it after the latest a11y improvements?

avatar crystalenka
crystalenka - comment - 27 Sep 2022

nvm hold off marking tests until I fix the code style stuff.

avatar crystalenka
crystalenka - comment - 27 Sep 2022

Okay, NOW we're ready for marking tests and stuff. :) thanks again everyone for your help, this is a huge improvement to the current UI.

avatar coolcat-creations
coolcat-creations - comment - 27 Sep 2022

I am very sorry to report this, but I have an issue on a very small screen, the popup would need to open to the right then:

grafik

avatar crystalenka
crystalenka - comment - 27 Sep 2022

Is that the latest build @coolcat-creations ? The language string is definitely not the most recent one and I think I had mitigated this issue already

avatar coolcat-creations
coolcat-creations - comment - 27 Sep 2022

Yes, I installed the prebuilt package update from 4.2.x to 4.2.3 with your patch. Maybe that's not the right way to do. I will investigate further.

avatar crystalenka
crystalenka - comment - 27 Sep 2022

Strange, I'll take a look. Maybe I missed something

avatar coolcat-creations
coolcat-creations - comment - 27 Sep 2022

I reinstalled the full update package and fixed the database and cleared the cache, now it looks like that:
grafik

avatar crystalenka
crystalenka - comment - 27 Sep 2022

I know. It's terrible. If you make the thumbnails bigger it's a little better and the actions are scrollable.

The current UI on mobile is not very usable either. (Not that that's an excuse!) Just have hit a wall in making it behave properly in mobile without completely refactoring the structure and how it functions.

If you want to fiddle with the CSS there to make it better you're welcome to. Major issues I was facing were overflow on the left, right, or bottom of the screen.

avatar crystalenka
crystalenka - comment - 27 Sep 2022

Once this PR is merged I will start a new one just to focus on the mobile experience because there are a few things in the media manager that could be improved.

I will be the first to admit that the current mobile experience is horrendous, but I'd like to move forward anyway in the spirit of continuous improvement (rather than delayed perfection). I think the PR goes a long way to making things better as it is, and while it doesn't make mobile better, I don't think it makes it totally worse, either. (Right now in mobile there's not much indication what each icon will do and the icons display inconsistently in my experience.)

avatar coolcat-creations
coolcat-creations - comment - 27 Sep 2022

It's a setting on screens < 576px that limits the dropdown to the size of the thumbnail, which is strange because the size could be actually full screen in theory... Its hard for me to debug because the list dissapears when I try to go into the inspector

avatar crystalenka
crystalenka - comment - 27 Sep 2022

Yes, limiting the width in small screens is intentional because otherwise it will go beyond the viewport for items to the far left or right.

It can't be full screen because the parent element has position relative set, so there's no way to reliably position the menu relative to the browser.

Removing position relative from the parent element when the menu is open causes unexpected behavior with the checkbox etc since they're positioned absolutely to the element.

It would require reworking the html and css for the entire item, basically, which may affect the vue scripts. That's why I want to do it in a subsequent PR.

avatar coolcat-creations
coolcat-creations - comment - 27 Sep 2022

Fine by me! Often PRs get stuck because of such issues. I think it would be good to move a step into the good direction and use this.

avatar angieradtke
angieradtke - comment - 27 Sep 2022

Puh Crystal , you had a lot of work. I just looked at this and even testing it is complex . I think the whole thing needs to be reviewed . I'm not sure, but maybe we should just swap out the icons for now, leaving the CSS and markup untouched. I think it needs a bigger project out of it. What do you think?

avatar crystalenka
crystalenka - comment - 27 Sep 2022

Angie, the new markup here was reviewed at length by yourself as well as Steve and Brian.

In doing so we uncovered additional issues with the JavaScript events that exist in the actions that make it completely inaccessible by keyboard or screen reader, so this is not just a visual improvement anymore but a huge accessibility improvement for this little part of the CMS.

What additional review do you feel is necessary?

avatar brianteeman
brianteeman - comment - 27 Sep 2022

@angieradtke please try to be positive. This pr has already been derailed once by a false argument - please let us not repeat that again. If you want to start from scratch then by all means do it but please dont derail this PR for something that may never happen (or not for a long time) As @coolcat-creations already said - too many pull requests stall when someone suggests a new unrelated issue and then disappears.

To quote Leonie Watson - it doesnt have to be perfect just better than it was yesterday

image

avatar coolcat-creations
coolcat-creations - comment - 27 Sep 2022

I have tested this item successfully on b2dcca2


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/38771.

avatar coolcat-creations coolcat-creations - test_item - 27 Sep 2022 - Tested successfully
avatar brianteeman
brianteeman - comment - 27 Sep 2022

I have tested this item successfully on b2dcca2

Big improvement in usability and accessibility. thank you for battling through vue to make this happen. I am sure it will be greatly appreciated!!


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/38771.

avatar brianteeman brianteeman - test_item - 27 Sep 2022 - Tested successfully
avatar angieradtke
angieradtke - comment - 27 Sep 2022

This is really a good work, I just thought it would be better to give us more time. Crystal and Brian , I think you know best how complex this is.

The proposed changes are important and right. But they seem to come with side effects. Brian, you are super fit with the backend. I just kept getting lost when navigating with the keyboard. Suddenly there were things on top of each other that didn't belong on top of each other.

Even small changes in the right direction are great.
I don't want to be a blocker here.

avatar angieradtke
angieradtke - comment - 27 Sep 2022

I have tested this item successfully

avatar crystalenka
crystalenka - comment - 27 Sep 2022

I totally understand @angieradtke , and yes there are definitely other issues and problems with accessibility in the media manager. Just taking it a little at a time because iterative PRs are easier to test and merge than one massive complete overhaul. :)

Thank you for all of your input and suggestions.

avatar richard67
richard67 - comment - 27 Sep 2022

@angieradtke The right way to mark a test result is to go to the issue tracker to the PR https://issues.joomla.org/tracker/joomla-cms/38771 and use the blue "Test this" button at the top left corner. Just leaving a comment with a green check mark is not sufficient. But as we have 2 other tests I can set RTC anyway.

avatar brianteeman
brianteeman - comment - 27 Sep 2022

But they seem to come with side effects.

No it doesnt. You just hadnt even tried to use it before. If you had then you would have commented much much earlier about the issues on mobile. And please remember to test everything in context and not just a snippet of code as that led to a lot of wasted time here on this pr

avatar richard67 richard67 - change - 27 Sep 2022
Status Pending Ready to Commit
avatar richard67
richard67 - comment - 27 Sep 2022

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/38771.

avatar angieradtke
angieradtke - comment - 27 Sep 2022

I have tested this item successfully on b2dcca2


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/38771.

avatar angieradtke angieradtke - test_item - 27 Sep 2022 - Tested successfully
avatar reilldesign
reilldesign - comment - 3 Oct 2022

I am not sure if this is the right place and time, but if you make the effort it would be useful to think about the icon. there are only images editable and here maybe the would be better. but this is just a thought.

avatar roland-d
roland-d - comment - 7 Oct 2022

Removing RTC
@crystalenka Could you please check the latest comments? Once they are resolved this can be merged.

avatar crystalenka crystalenka - change - 10 Oct 2022
Labels Added: ?
avatar crystalenka
crystalenka - comment - 10 Oct 2022

@HLeithner can you please explain your comment about it changing the meaning of the string? The string changed but it is basically the same thing. "Open item actions" implies "...to manage this item". "Manage item: (filename)" is more direct but at least in English, the same thing. What is the benefit of adding a new string when the old one won't be used anywhere else?

avatar HLeithner
HLeithner - comment - 10 Oct 2022

@HLeithner can you please explain your comment about it changing the meaning of the string? The string changed but it is basically the same thing. "Open item actions" implies "...to manage this item". "Manage item: (filename)" is more direct but at least in English, the same thing. What is the benefit of adding a new string when the old one won't be used anywhere else?

Since @brianteeman can only say "no it is not" and doesn't explain why it's not, I try to explain why it is.

When the count of wildcards don't match you generate a warning in the translation function.

avatar crystalenka
crystalenka - comment - 10 Oct 2022

I see, thanks for explaining. But in order to add a new string instead the old string will be removed, so it will just be a broken language string if it's not added. (Right?) Is that better than an old string and a PHP warning?

As I'm not a translator, I want to understand the best practices here for future reference.

avatar HLeithner
HLeithner - comment - 10 Oct 2022

I see, thanks for explaining. But in order to add a new string instead the old string will be removed, so it will just be a broken language string if it's not added. (Right?) Is that better than an old string and a PHP warning?

As I'm not a translator, I want to understand the best practices here for future reference.

We simple don't remove language strings in minor/patch levels, or change the meaning of the string.

https://developer.joomla.org/news/586-joomla-development-strategy.html#6.1.6%20Language%20keys:~:text=them%20is%20not.-,6.1.6%20Language%20keys,-Changing%20or%20deleting

6.1.6 Language keys
Changing or deleting a language key is considered a backwards compatibility break. Adding new ones is not. Substantially changing the meaning associated with a language key is a compatibility break. Rephrasing something for a more accurate description or proper en-GB grammar is not.

I know some people don't like this policy but since they don't push an update for the policy forward it is still valid.

avatar crystalenka
crystalenka - comment - 10 Oct 2022

I would argue that it's rephrasing for a more accurate description in this case. "Open item actions" is unclear/borderline useless in the context it's used (as title or aria label for screen readers).

avatar HLeithner
HLeithner - comment - 10 Oct 2022

you can do that but then you can't add %s to it.

avatar crystalenka
crystalenka - comment - 10 Oct 2022

Well, that's why it's borderline useless for screenreaders—there's no accessible indication of which item you're managing. :)

But if the addition of the string variable (I dunno the proper term for it) is a breaking change I guess I will add a new string so that this can be merged. I just hope that it won't make it less accessible for users in using it in other languages if it doesn't get translated in time.

avatar HLeithner
HLeithner - comment - 10 Oct 2022

But if the addition of the string variable (I dunno the proper term for it) is a breaking change I guess I will add a new string so that this can be merged. I just hope that it won't make it less accessible for users in using it in other languages if it doesn't get translated in time.

if the language translators don't update the language then the language is dead and we should find new translators for the language or remove it at some point. So don't worry about new language strings. Only add a notice to the old string that it is deprecated something like:
// Deprecated will be removed with 5.0

thanks

avatar brianteeman
brianteeman - comment - 10 Oct 2022

When the count of wildcards don't match you generate a warning in the translation function.

That might be true but its not applicable here and no warning is issued.

avatar brianteeman
brianteeman - comment - 10 Oct 2022

The only time I can see that the chnge here could ever produce an error would be if you used a new language file with an old version of joomla but surely we dont support that anyway. Using an old language file with a new joomla version does not produce an error in this specific case.

avatar crystalenka
crystalenka - comment - 11 Oct 2022

@HLeithner I've added a new language string; not sure how to mark the old one as deprecated since it shows up both in the .ini files and in the default_texts.php file that passes the language strings to vue.

avatar crystalenka
crystalenka - comment - 12 Oct 2022

@HLeithner I've added deprecation notices in the .ini files.

The old language string is also referenced here:

Text::script('COM_MEDIA_OPEN_ITEM_ACTIONS', true);

I'm not sure how to deprecate that. If it's even necessary? I would really really really love to be done with this pull request, 128 comments later... ?

avatar HLeithner
HLeithner - comment - 12 Oct 2022

@HLeithner I've added deprecation notices in the .ini files.

The old language string is also referenced here:

Text::script('COM_MEDIA_OPEN_ITEM_ACTIONS', true);

I'm not sure how to deprecate that. If it's even necessary? I would really really really love to be done with this pull request, 128 comments later... ?

thanks, no it's not needed to deprecated in the defaults_texts.php, but you could write an PR against 5.0 which removes both ;-)

avatar crystalenka
crystalenka - comment - 12 Oct 2022

@HLeithner I've added deprecation notices in the .ini files.
The old language string is also referenced here:

Text::script('COM_MEDIA_OPEN_ITEM_ACTIONS', true);

I'm not sure how to deprecate that. If it's even necessary? I would really really really love to be done with this pull request, 128 comments later... ?

thanks, no it's not needed to deprecated in the defaults_texts.php, but you could write an PR against 5.0 which removes both ;-)

Ahhhhhh!!! And how many comments will that one get? ? I'll wait, in case there are other changes to media manager in the meantime...

avatar HLeithner
HLeithner - comment - 12 Oct 2022

Ahhhhhh!!! And how many comments will that one get? ? I'll wait, in case there are other changes to media manager in the meantime...

You are right don't write a PR because it has to be keep till 6.0, maybe moved into the legacy plugin

avatar crystalenka
crystalenka - comment - 12 Oct 2022

Are there any other changes you'd like me to make @HLeithner? I still see the change requested flag. :)

avatar roland-d roland-d - change - 15 Oct 2022
Status Ready to Commit Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2022-10-15 16:47:50
Closed_By roland-d
avatar roland-d roland-d - close - 15 Oct 2022
avatar roland-d roland-d - merge - 15 Oct 2022
avatar roland-d
roland-d - comment - 15 Oct 2022

Thank you.

Add a Comment

Login with GitHub to post a comment