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 ActionButton Featured accessible #23718

Merged
merged 10 commits into from
Feb 3, 2019

Conversation

chmst
Copy link
Contributor

@chmst chmst commented Jan 30, 2019

Summary of Changes

Action Buttons for featuring articles for example in com_content/articles are Links, not Buttons.
This PR

Testing Instructions

Apply the patch and see that the featured, unfeatured function works as before.

featured-column-articles

NOTE:
When activating one featured button, the corresponding checkbox and the Status Change Dropdown are activated for a moment. #23717

Documentation Changes Required

new screen

@brianteeman
Copy link
Contributor

Almost perfect. woohoo

  1. The td for the featured header should be a th
  2. Needs some sort of onhover visual guide to indicate to a mouse user that this is clickable

@zwiastunsw
Copy link
Contributor

zwiastunsw commented Jan 31, 2019

  1. It is a good idea to create a new column and separate the status and feaured buttons.
  2. I wonder if a simple button is a good choice in this case. Shouldn't it be a toggle button?. Here we have a switchover of the position status. @dgrammatiko - What do you think about it? Maybe the toggle button change should be in a separate PR?
  3. I am not a programmer. It seems to me, however, that the attribute href="javascript://" is not allowed for the button tag. It causes the button to work as a link.
  4. The action should not be followed by a change of context. The focus should remain on the button. When I use a screen reader, the focus jumps to a modal window with a status change message and then returns to the button (it is OK). But when I use only the keyboard (without the screen reader), the focus jumps to the modal window and remains at the top of the page when the window is turned off.
  5. If the button is a toggle button,then it has an aria-pressed=”true/false” state to let the screen reader user know when the button is pressed. Then you don't need to display a modal window with a message and move the focus to the modal window.

@dgrammatiko
Copy link
Contributor

Shouldn't it be a toggle button?. Here we have a switchover of the position status.

In sort I think no. The reason is that the effect is not realtime (you submitting a form each time you're clicking on any of these). If you change the front end logic so the form submission will happen with an Ajax then a switcher will be fine but with the current code the current approach is fine

@zwiastunsw
Copy link
Contributor

zwiastunsw commented Jan 31, 2019

@dgrammatiko : OK, thx.
I understand that you now need to correct the code (remove the href attribute) and solve the focus problem. But I don't have an idea here.

Needs some sort of onhover visual guide to indicate to a mouse user that this is clickable

Add cursor: pointer in CSS?

@dgrammatiko
Copy link
Contributor

@chmst @zwiastunsw here is some (untested) code:

		<?php if (empty($disabled)): ?>
				onclick="return false;"
			<?php endif; ?>

			<?php if(!empty($task) && empty($disabled)): ?>
				onclick="return Joomla.listItemTask('<?php echo $checkboxName . $this->escape($row ?? ''); ?>', '<?php echo $this->escape(isset($task) ? $taskPrefix . $task : ''); ?>')"
                         <?php endif; ?>

@brianteeman
Copy link
Contributor

The action should not be followed by a change of context. The focus should remain on the button. When I use a screen reader, the focus jumps to a modal window with a status change message and then returns to the button (it is OK). But when I use only the keyboard (without the screen reader), the focus jumps to the modal window and remains at the top of the page when the window is turned off.

You are absolutely correct. But its really beyond the scope of this PR to address this as it is a bigger issue that I raised before. The information that the feature has been changed that takes the focus has incorrectly got a role of alert. It shouldnt be an alert as an alert will always "steal" the focus.

Needs some sort of onhover visual guide to indicate to a mouse user that this is clickable

Add cursor: pointer in CSS?

Either that or an outline in CSS

@chmst
Copy link
Contributor Author

chmst commented Jan 31, 2019

Thank you for advices. I have prepared a new commit.

  • No toggle button at the moment
  • @dgrammatiko i've tested you code but "retun false" would block the change from unfeatured to featured if I understand this right.
  • cursor pointer is a good idea I will do some css for cursor and hover later

I have changed:
If a user is not allowed to change the state, only an icon is displayed, it is not a button.

featured-disabled

About the message and the focus - as @brianteeman says, it is out of scope of this PR

@zwiastunsw
Copy link
Contributor

I propose to add to the tag button title attribute calling the current status

@chmst
Copy link
Contributor Author

chmst commented Feb 2, 2019

I added a title instead of sr-only.
I'd like to make another PR for Cursor pointer and other css actions, this should be done in a own PR.


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

@chmst chmst changed the title [4.0][POC]Make ActionButton Featured accessible [4.0] [a11y] Make ActionButton Featured accessible Feb 2, 2019
@zwiastunsw
Copy link
Contributor

zwiastunsw commented Feb 2, 2019

I'm sory. My mistake. I am withdrawing my positive test.
Please restore the label for screen readers.
The title attribute is not announced by screen readers.
Adding the title attribute means that the button also has a readable name for sighted users (e.g. when they point the icon with the mouse). The code should contain and attribute title and label for screen reader users (sr-only)

@chmst
Copy link
Contributor Author

chmst commented Feb 2, 2019

Please retest. Thank you for your patience. If this is all right, I will change other list views in the same way.

@zwiastunsw
Copy link
Contributor

zwiastunsw commented Feb 2, 2019

I suggested using the title attribute, because without this attribute the meaning of the button may not be understood by sighted users. Perhaps there is a better way to do this. I don't know. I remember what he said Karl Groves:

If I found a genie and it gave me one wish, it would be that web developers stopped using the title attribute.

(Karl Groves, December 15, 2017).

Now some screen readers will double the label. However, I think this is more accessible than without the title attribute. If it was a toggle buton, there would be no such problem :).
See: The Trials and Tribulations of the Title Attribute
That is why I am testing successfully.

@zwiastunsw
Copy link
Contributor

I have tested this item ✅ successfully on 390f47b


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

@chmst
Copy link
Contributor Author

chmst commented Feb 2, 2019

Thank you for testing. The title attribute is ugly. I have isolated the column for Featured, hoping that the meaning of the button is evident for sighted administrators. But if it is necessary ...

@zwiastunsw
Copy link
Contributor

zwiastunsw commented Feb 2, 2019

@brianteeman What is your opinion. Because @chmst rightly says that the name of the column should be enough. Maybe I mixed up unnecessarily...

@brianteeman
Copy link
Contributor

I agree 100% with @chmst

@zwiastunsw
Copy link
Contributor

I have tested this item ✅ successfully on c3772f9


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

1 similar comment
@brianteeman
Copy link
Contributor

I have tested this item ✅ successfully on c3772f9


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

@Quy
Copy link
Contributor

Quy commented Feb 2, 2019

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Feb 2, 2019
@wilsonge wilsonge merged commit e2a1c19 into joomla:4.0-dev Feb 3, 2019
@wilsonge
Copy link
Contributor

wilsonge commented Feb 3, 2019

Thanks!

@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Feb 3, 2019
@wilsonge wilsonge added this to the Joomla 4.0 milestone Feb 3, 2019
@Quy Quy mentioned this pull request Jul 2, 2019
@chmst chmst deleted the a11y-action-button branch October 5, 2019 08:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants