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]Make module assignment accessible - step 1 #28726

Merged
merged 10 commits into from May 2, 2020

Conversation

chmst
Copy link
Contributor

@chmst chmst commented Apr 18, 2020

Pull Request for Issue #23911, step 1 .

Summary of Changes

In the issue above are described several a11y issues in the tab menu association of modules. There were already some PRs, as #25128, #28250, which have been closed.

This PR continues this work.
To make things easier I will make several PRs, step by step, to avoid confusion in the discussions.
This first one will make the Global Selection part of the screen menu assignment screen in modules accessible.

module-assignment_before

I have chaged the links to buttons and added sr-only texts to all elements, so that there is a better explanation.
module-assignment

Testing Instructions

Inspect code, test with screenreader, use keyboard

Expected result

Every element can be accessed via keyboard, Every element is announced properly by the screenreader and the meaning ot the buttons is clear.

Actual result

As described in the issue, points 1, 2. and 6.

Documentation Changes Required

no

@joomla-cms-bot joomla-cms-bot added Language Change This is for Translators PR-4.0-dev labels Apr 18, 2020
@brianteeman
Copy link
Contributor

<span class="aria-hidden"><?php echo Text::_('JALL'); ?></span> <span class="width-20">|</span> <span class="sr-only"><?php echo Text::_('COM_MODULES_GLOBAL_SELECT_ALL'); ?></span>

This results in a screen reader announcing

AllExpand the whole Menu Tree.

etc

@chmst
Copy link
Contributor Author

chmst commented Apr 19, 2020

Thank you @brianteeman . Seems It was too late yesterday.

@chmst chmst marked this pull request as draft April 19, 2020 16:56
@chmst chmst marked this pull request as ready for review April 20, 2020 20:39
@chmst
Copy link
Contributor Author

chmst commented Apr 20, 2020

Would be nice to get test here, only for step 1:

module-assignment-step1

The button groups and the search input should be announced by the screen reader.

@ChristineWk
Copy link

I have tested this item ✅ successfully on ee3e1cc

For step 1


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

@ChristineWk
Copy link

I have tested this item ✅ successfully on ba3f721


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

1 similar comment
@jwaisner
Copy link
Member

I have tested this item ✅ successfully on ba3f721


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

@brianteeman
Copy link
Contributor

Can you please remove RTC until the Accessibility Team have reviewed this. At least one part of it is the same as my previous PR that they rejected

@jwaisner
Copy link
Member

The RTC status is going to remain. @chmst is on the accessibility team and is good with the change.

@chmst
Copy link
Contributor Author

chmst commented Apr 21, 2020

Thank you all. @brianteeman as I wrote, I continued your work and added the role="group" so that screen readers announce it all. And I made Buttons, because the original links were not accessible for colour blind. Hopefully I can do the next steps soon.


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

@brianteeman
Copy link
Contributor

But the accessibility team objected to the use of buttons?

@angieradtke
Copy link
Contributor

angieradtke commented Apr 22, 2020

@rdeutz
I think buttons are much better than links.
But I ask myself why not using radio-buttons (switcher)
It is less code with the same effect ?
by Angie

@chmst
Copy link
Contributor Author

chmst commented Apr 22, 2020

@angieradtke Switchers or radio buttons are input fields and have two states. So suppose the screen is build and there are two or three menuitems selected. The the state of a swither is not defined. It is neither all nor none. So what to show?
These buttons don't show a state but they start an action. For me this is not the same.

@richard67
Copy link
Member

So suppose the screen is build and there are two or three menuitems selected. The the state of a swither is not defined.

Makes sense to me.

@angieradtke
Copy link
Contributor

The same issue we have with the state of the buttons. So if the situation is not clear the switscher is still gray - like it is if it has no selected value

@angieradtke
Copy link
Contributor

and I forget the radio can look like button ( not like a switcher) .-)

@chmst
Copy link
Contributor Author

chmst commented Apr 22, 2020

I hesitate to use input fields as buttons. For me it is semantically not correct. After selecting for example all - the swither changes to "all". As soon as the user deselects a menuItem, it is not longer "all" and the switcher must be rest to "undefined". With every select/deselect, the swicher has to be justified. Then it would be ok.

@chmst
Copy link
Contributor Author

chmst commented Apr 22, 2020

Alternative: Make it simple and remove the buttons "menu assignment" from the screen

When the user comes to this screen, he has already decided that he does not want to assign all or none menu items

@angieradtke
Copy link
Contributor

I think it is always good to use the HTML elements that are available to us to build accessible websites. The more aria we use, the more confusing and difficult it becomes to test and maintain. If you take the button version with the aria, the user will not know anything about the state of the selection. I think it's always a trade-off.
I built a very simple test-case .
code1

Code / Live:
https://www.der-auftritt.de/kunden/joomla/radio.php
Whenever the user makes a choice, the selectbox can be reset to custom.
My thoughts .-)

@zwiastunsw
Copy link
Contributor

I also suggested radio-buttons or switches. I think that would be best. But in the discussion @chmst pointed out to me that the option buttons (input type="radio") define the state. And here we have at least 3 states: all selected, nothing selected, some selected. When a user changes the state of at least one element, the state triggered by the buttons will change. And logically, it should be shown in these buttons". But that doesn't make much sense. Because the user wants to either mark everything, or deselect everything.
I've consulted with my friend the programmer. It was easier for me to explain in Polish. He confirmed @chmst's opinion that using option buttons would not be semantically correct.
That's why the solution proposes button groups. The user of the screen reader will hear the information he needs, but he must understand how this screen works. It's difficult.
If you could use radio-button/switcher, this would be the best solution for reasons of accessibility. But it has to be decided by the programmers.


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

@chmst
Copy link
Contributor Author

chmst commented Apr 22, 2020

Thank you all, I think that decisions must be made by disabled / blind users, surely not by programmers. We only can imagine to be blind or I can use a keyboard.

Thank you @angieradtke for the word customer, this is much better word than undefined :).
Now we could do this: If he selects all or none - we have a js which makes all checkboxes readonly. If the user clicks custom, the checkboxes are writable. Then the state always is indicated correct.

@brianteeman
Copy link
Contributor

Obviously I think it is correct to have a button as that's what I proposed originally. It is a button because it is performing an action. I do not believe it is there to display a state. The consistent way to convey the state would be as we do with tables by using a sr-only text. As the action is performed without a page load then it should also be inside an aria-live block.

@chmst
Copy link
Contributor Author

chmst commented Apr 22, 2020

The buttons are there because it is helpful to change the whole selection with ohne click (otherwise the buttons are superfluos here as the user has already selected the custom mode). This is calling an action so like @brianteeman I think that buttons are correct.

But it might be a usefull information, also for sighted users, to know which button has been clicked (to know if all or none items are selected). This could be made with my idea of setting the checkboxes writable or not.

I think we must choose the implementation which is better understandable with sreenreaders, and which is better UX.

@brianteeman
Copy link
Contributor

But it might be a usefull information, also for sighted users, to know which button has been clicked (to know if all or none items are selected).

Then it would not be a button but a display of state. It is not normal (or semantic) to use a button in this way.

The sighted user can see what has changed but if you think they wont notice all the checkboxes then take my comment about an aria-live region and make it visible to all instead of sr-only.

I repeat - these are buttons to perform an action - they are not to display a state

@zwiastunsw
Copy link
Contributor

zwiastunsw commented Apr 22, 2020

I apologize for the long lecture and the obvious statements.
What's this control for?
User have previously selected one of the options from the list:

  • only on the pages selected
  • only all except those selected

Now we let him have it, if he wants it:

  • Group "Select:"
    • option1: all items selected
    • option2: no item is selected
  • Expand group:
    • option1: all expanded items
    • option2: no item is expanded

These options prepare a list for him to continue working. At this moment, it does not matter to the user whether some items are selected or not. It does not matter whether some items are expanded or not.
The user can choose any option.
After the user makes a choice, the script executes the action: selects all items, or the opposite. It expands all items, or the opposite.

The user performs the next steps.

In summary: This control is not intended to record the current status. This control is intended to allow the user to select possible options.

In my opinion, nothing stands in the way of using the radio-buttons (switches). But I'm not a programmer. I listen to what programmers say. And that's why I wrote that it is the programmers who should decide whether to use the radio buttons. Dear Christine, a blind user will not decide that. He can decide if something is accessible to him and not if the coding is correct.

When I use the radio buttons, I don't have to explain anything to you. You know you have two options, and you can choose one. Both the user who sees and the blind user. The screen reader will tell the blind user which option is selected.

When I use typically buttons, the blind user must understand that using the second button changes the setting made with the first button. However, they do not get the message that this state has been changed. Of course, you can complicate the code and add aria-live. But, why?

So if you can use radio buttons or a switch, it's the best solution. But I won't decide that. A blind person won't decide that either. The programmers have to decide.

If I have to evaluate as an accessibility expert the solution that this PR proposes, I can say that this solution ensures accessibility. But another better solution can be used (eg. a solution with radio buttons).

@brianteeman
Copy link
Contributor

So let me summarise what you are saying. This is perfectly accessible but you dont like it. I repeat what I always say. "It only has to be better than it was yesterday"

@angieradtke
Copy link
Contributor

I think Stefan explained it perfectly.
Accessibility is always about levels of accessibility. Not for nothing we have A, AA, and AAA. And Stefan's explanation makes that quite clear. Thanks Stefan

@brianteeman
Copy link
Contributor

My english is obviously failing

@wilsonge wilsonge merged commit 1b94936 into joomla:4.0-dev May 2, 2020
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label May 2, 2020
@wilsonge
Copy link
Contributor

wilsonge commented May 2, 2020

This makes things accessible as a first pass - that's better than not accessible :)

@brianteeman
Copy link
Contributor

Unfortunately this PR was merged without looking at the code and only reading the issue.

that's better than not accessible :)

It is still not accessible

This pr correctly changed some links to buttons but it still has links that should not be links

Does it go somewhere? It's a link.
Does it do something? It's a button.

<a class="dropdown-item checkall" href="javascript://"><span class="icon-check-square" aria-hidden="true"></span> <?php echo Text::_('JSELECT'); ?></a>
<a class="dropdown-item uncheckall" href="javascript://"><span class="icon-square" aria-hidden="true"></span> <?php echo Text::_('COM_MODULES_DESELECT'); ?></a>
<div class="treeselect-menu-expand">
<div class="dropdown-divider"></div>
<a class="dropdown-item expandall" href="javascript://"><span class="icon-plus" aria-hidden="true"></span> <?php echo Text::_('COM_MODULES_EXPAND'); ?></a>
<a class="dropdown-item collapseall" href="javascript://"><span class="icon-minus" aria-hidden="true"></span> <?php echo Text::_('COM_MODULES_COLLAPSE'); ?></a>
</div>

image
Secondly for some reason that I really can't understand and can only assume was made in error by @chmst and no one spotted it the header has been changed from h5 to h1 and then styled with css to be 0.8rem

As well as looking odd to have a heading smaller than the items it is a heading for, it is an h1 which I thought should only be present once on a page

Finally there is a z-index problem
image

cc @wilsonge @carcam tagging you as requested

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet