Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[4.0] [a11y] [NO CACHE] Make media manager browse view work with keyboard navigation #23950

Merged
merged 3 commits into from
Feb 27, 2019

Conversation

wilsonge
Copy link
Contributor

@wilsonge wilsonge commented Feb 19, 2019

This fixes the browse view aspect of what's discussed in #23932 (please do not close that issue as the list view is still to be worked on)

Fixes various a11y issue keyboard navigation issues in the media manager browse view

@joomla-cms-bot joomla-cms-bot added Language Change This is for Translators NPM Resource Changed This Pull Request can't be tested by Patchtester PR-4.0-dev labels Feb 19, 2019
@wilsonge wilsonge changed the title WIP at making media manager work with keyboard navigation WIP at making media manager browse view work with keyboard navigation Feb 19, 2019
@wilsonge wilsonge changed the title WIP at making media manager browse view work with keyboard navigation [4.0] [a11y] Make media manager browse view work with keyboard navigation Feb 19, 2019
@zwiastunsw
Copy link
Contributor

Link "Toggle select all" should be a checkbox. Anachor is an incorrect tag at this point. It is not possible to communicate the status of the switch to the users of the screen reader.
mm_toolbar_toggle png_

    <div class="media-view-icons">
      <a href="#" aria-label="Toggle select all" class="media-toolbar-icon media-toolbar-select-all">
        <span aria-hidden="true" class="fa fa-square-o"></span>
      </a>
    </div> 

@zwiastunsw
Copy link
Contributor

The part of the toolbar is the breadcrumbs. It should be included in the nav tag with aria-label="You are here" (e.g)
mm_breadcrumbs

    <ol class="media-breadcrumb">
      <li class="media-breadcrumb-item">
        <a href="#">images</a>
      </li>
      <li class="media-breadcrumb-item">
        <a href="#">banners</a>
      </li>
    </ol> 

@zwiastunsw
Copy link
Contributor

The toolbar should have the attribute role="toolbar" and the attribute aria-label="Media manager" (e.g).

mm_toolbar

  <div class="media-toolbar" role="toolbar" aria-label="COM_MEDIA_TOOLBAR">
    <!----> 
  </div>

@zwiastunsw
Copy link
Contributor

zwiastunsw commented Feb 20, 2019

In the toolbar we have a set of buttons. All of them are incorrectly tagged as links.
They should indicate to the user the status - a tag a cannot be used to indicate a state.

In addition: Grid switches (increase, decrease) should be hidden from screen readers.

mm_toolbar_buttons

@wilsonge
Copy link
Contributor Author

wilsonge commented Feb 20, 2019

Ok the toolbar role/label change is easy to do. What’s the recommended markup for the breadcrumbs? Should be easy enough

I’m leaving the checkboxes for another PR. Implementing a form element is going to be complicated and is not something I want to try doing as part of this PR

@brianteeman
Copy link
Contributor

Look at the breadcrumbs module o think that's correct

@zwiastunsw
Copy link
Contributor

zwiastunsw commented Feb 20, 2019

<nav role="navigation" aria-label="COM_MEDIA_BREADCRUMBS">
       <ol class="media-breadcrumb">
         <li class="media-breadcrumb-item">
           <a href="#">images</a>
         </li>
       </ol>
   </nav>

@wilsonge wilsonge force-pushed the media-manager-ally branch 3 times, most recently from 9980e29 to d5e62bc Compare February 21, 2019 17:57
@wilsonge
Copy link
Contributor Author

wilsonge commented Feb 21, 2019

OK Fixes to toolbar - it now has a role=toolbar, aria-label, and all the buttons are now <button> elements instead of <a> tags. Breadcrumbs fixes also pushed up. I deliberately didn't add the role=nav in the example @zwiastunsw gave because https://www.w3.org/TR/wai-aria-practices/examples/breadcrumb/index.html was missing it and added in the aria-current mentioned there. But happy to flip back if needed.

@wilsonge
Copy link
Contributor Author

I think that's everything covered aside from the checkboxes you guys have asked for

@wilsonge wilsonge force-pushed the media-manager-ally branch 2 times, most recently from cff49fc to dc32c57 Compare February 21, 2019 18:51
@wilsonge
Copy link
Contributor Author

wilsonge commented Feb 21, 2019

@brianteeman Would you be ok to the changes of an action list to type button - it is just as simple as changing the markup where you commented - but the styling is more complicated and I don't really have a huge amount of time to invest into trying to make it all work nicely

@wilsonge
Copy link
Contributor Author

wilsonge commented Feb 21, 2019

The button change makes the system tests fail - I think joomla/test-system#52 should fix that. Once you guys are happy with this PR i'll merge that up front

Back to the stage of I think it's only the checkboxes left

@brianteeman
Copy link
Contributor

For the checkboxes see #23970

@zwiastunsw
Copy link
Contributor

zwiastunsw commented Feb 21, 2019

@wilsonge : read about links vs. button: Links vs. Buttons in Modern Web Applications

@wilsonge
Copy link
Contributor Author

So can we run a last test on this and ensure that only the checkbox issue is left to solve in a separate place?

@zwiastunsw
Copy link
Contributor

@wilsonge : Which files have you changed?

@brianteeman
Copy link
Contributor

I will try to find some time tomorrow to take a look. because of the npm it takes a long time to build on windows (as you saw)

@zwiastunsw
Copy link
Contributor

  1. When the screen reader is on, the Enter and Space keys do not activate the action buttons (Download, Rename item...)
  2. "Open item actions" button should be a menu button
  3. The action buttons should be grouped by using a list elements (ul, li).
  4. Use the arrow keys to move the focus between action items.

@wilsonge
Copy link
Contributor Author

Done 1 and 3. I've done 2 and 4 on the image thing only. Unfortunately I need to head out but will try and find some time to do them on directory, file and video areas later today

@wilsonge
Copy link
Contributor Author

wilsonge commented Feb 23, 2019

Pushed - @zwiastunsw this should cover all the things you mentioned

@wilsonge
Copy link
Contributor Author

@zwiastunsw @brianteeman Can you guys give this a final test - I understand there's more to do - but I'm being dragged back onto management responsibilities - handover to Harold and merging in j3 etc. So would like to get this in asap as a step in the right direction and then we can build up a list of next steps/you guys can try this yourselves (i promise it's not that bad!)

@brianteeman
Copy link
Contributor

@wilsonge I will try to find some time tonight - I am working on some stuff for the marketing team thats taking my available time

@zwiastunsw
Copy link
Contributor

@wilsonge: We've made an appointment today for a Skype session

@brianteeman
Copy link
Contributor

Is it perfect - no
Is it better than it was yesterday - yes

If the non graphical view was also fixed then I believe that would be a reasonable accommodation

wilsonge added a commit to joomla/test-system that referenced this pull request Feb 27, 2019
@wilsonge wilsonge changed the title [4.0] [a11y] Make media manager browse view work with keyboard navigation [4.0] [a11y] [NO CACHE] Make media manager browse view work with keyboard navigation Feb 27, 2019
@wilsonge wilsonge merged commit 544c9c6 into joomla:4.0-dev Feb 27, 2019
@wilsonge wilsonge deleted the media-manager-ally branch February 27, 2019 11:00
@wilsonge
Copy link
Contributor Author

I'm going to do something separate for the list view - I was having issues with the icons showing - I can push what I had tonight - because I don't think it was a JS issue - it seemed something CSS - that I'm sure i was just being stupid with

@wilsonge wilsonge added this to the Joomla 4.0 milestone Feb 27, 2019
brianteeman added a commit to brianteeman/joomla-cms that referenced this pull request May 1, 2019
The increase/decrease grid size buttons in the media manager have aria-hidden=true

This change was made in joomla#23950 at the request of @zwiastunsw  joomla#23950 (comment)

I dont understand why that request was made but at a minimum its implementation is wrong

A focusable element with aria-hidden="true" is ignored as part of the reading order, but still part of the focus order, making it’s state of visible or hidden unclear.
wilsonge pushed a commit that referenced this pull request May 14, 2019
The increase/decrease grid size buttons in the media manager have aria-hidden=true

This change was made in #23950 at the request of @zwiastunsw  #23950 (comment)

I dont understand why that request was made but at a minimum its implementation is wrong

A focusable element with aria-hidden="true" is ignored as part of the reading order, but still part of the focus order, making it’s state of visible or hidden unclear.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Language Change This is for Translators NPM Resource Changed This Pull Request can't be tested by Patchtester
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants