Protostar pagination update by brianteeman · Pull Request #18326 · joomla/joomla-cms

brianteeman · 2017-10-13T10:05:45Z

Improve thew accessibility of the pagination - there are no visual changes

Wrap the pagination in a nav and add a role (needed for IE) and an aria-label
Ensure font icons have aria-hidden=true
Add aria-current to the selected page
Add aria-title to the active links eg Go to page 1

Note 1

that the disabled links will not be announced to a screen reader which is the intended behaviour

Note 3

the list is an unordered list which appears to be the standard as otherwise the links would be announced as Link 1 go to page 2 etc which is nonsense

Note 4

I dont know why but protostar uses an override for pagination instead of the layout. For b/c this PR is just for the override but when approved I will do a similar PR for the layout

Thanks to @fuzzbomb (Drupal 8 Core Accessibility Maintainer) for his advice and reviewing this

@fuzzbomb

Improve thew accessibility of the pagination 1. Wrap the pagination in a nav and add a role (needed for IE) and an aria-label 2. Ensure font icons have aria-hidden=true 3. Add aria-current to the selected page 4. Add aria-title to the active links eg Go to page 1 ### Note 1 that the disabled links will not be announced to a screen reader which is the intended behaviour Note that the list is an unordered list which appears to be the standard as otherwise the links would be announced as Link 1 go to page 2 etc which is nonsense ### Note 2 I dont know why but protostar uses an override for pagination instead of the layout. For b/c this PR is just for the override but when approved I will do a similar Pr for the layout Thanks to @fuzzbomb (Drupal 8 Core Accessibility Maintainer) for reviewing this

ciar4n · 2017-10-13T11:23:21Z

templates/protostar/html/pagination.php

 	}

-	$html = '<ul class="pagination-list">';
+	$html  = '<nav role="navigation" aria-label="' . JText::_('JPAGINATION') . '">';


JPAGINATION... is that correct? Can't find this constant anywhere.

Should be JLIB_HTML_PAGINATION

Quy · 2017-10-13T12:35:06Z

templates/protostar/html/pagination.php

 	if ($item->text === JText::_('JNEXT'))
 	{
-		$display = '<span class="icon-next"></span>';
+		$display = '<span class="icon-next" aria-hidden="true" ></span>';


Remove space "true" >

ciar4n · 2017-10-13T13:41:14Z

I have tested this item ✅ successfully on 08d0638

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/18326.}

Quy · 2017-10-13T13:54:01Z

I would prefer it to say Go to next page rather than Go to page Next. The same goes for Start, Prev, and End. Also should this change to use sprintf?

brianteeman · 2017-10-13T20:53:39Z

@Quy I agree - the sprintf for the page number is easy - will need a little code change for the prev,next strings - i will look at it on sunday

brianteeman · 2017-10-15T20:42:18Z

@Quy I have had a go at the sprintf stuff - it works but maybe you can optimise it and make it DRY

Quy · 2017-10-15T21:44:16Z

templates/protostar/html/pagination.php

 	if (isset($item->active) && $item->active)
 	{
-		return '<li class="active hidden-phone"><a>' . $item->text . '</a></li>';
+		return '<li class="active hidden-phone"><a aria-current="true" aria-label="' . JText::_('JLIB_HTML_PAGE') . ' ' . $item->text . '">' . $item->text . '</a></li>';


sprintf here? JText::_('JLIB_HTML_PAGE') . ' ' . $item->text

Quy · 2017-10-15T21:47:14Z

Looks good!

Go to prev page I understand why it is prev, but it would be nice to be previous.

brianteeman · 2017-10-16T07:36:04Z

Changing to previous will have to wait until this is approved and I can work n the pagination layout as well

Quy · 2017-10-16T10:50:08Z

administrator/language/en-GB/en-GB.lib_joomla.ini

 JLIB_HTML_MOVE_UP="Move Up"
 JLIB_HTML_NO_PARAMETERS_FOR_THIS_ITEM="There are no parameters for this item."
 JLIB_HTML_NO_RECORDS_FOUND="No records found."
+JLIB_HTML_PAGE="Page"


No - we dont delete strings ;)

ah whoops it was me that added it

Delete this one too

Quy · 2017-10-16T10:50:21Z

language/en-GB/en-GB.lib_joomla.ini

 JLIB_HTML_MOVE_UP="Move Up"
 JLIB_HTML_NO_PARAMETERS_FOR_THIS_ITEM="There are no parameters for this item."
 JLIB_HTML_NO_RECORDS_FOUND="No records found."
+JLIB_HTML_PAGE="Page"


Quy · 2017-10-16T10:50:51Z

templates/protostar/html/pagination.php

 	if (isset($item->active) && $item->active)
 	{
-		return '<li class="active hidden-phone"><a>' . $item->text . '</a></li>';
+		$aria    = JText::sprintf('JLIB_HTML_PAGE_CURRENT', $item->text);


Remove extra spaces before =

Quy · 2017-10-16T12:56:56Z

I have tested this item ✅ successfully on fa05cc6

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/18326.}

brianteeman · 2017-10-17T10:50:53Z

See #18357 for similar work on the ISIS template

ghost · 2017-10-22T12:23:13Z

@ciar4n can you please test again?

C-Lodder · 2017-10-22T12:30:16Z

I'm seeing a lot of <a> elements without href which I think may be an issue for a11y in regards to tab indexing.

@zwiastunsw ?

Could be wrong

brianteeman · 2017-10-22T12:32:37Z

It s perfectly ok because there is no link in there it is ignored. We checked that before submitting as we were not sure either

brianteeman · 2017-10-22T12:33:56Z

See the test report here https://gist.github.com/brianteeman/46e1cf36b6c57944c4b9c5efcad84c1d#gistcomment-2225785

dgrammatiko · 2017-10-22T12:35:32Z

I'm seeing a lot of elements without href which I think may be an issue for a11y in regards to tab indexing.

Just use href="javascript:void(0)"

brianteeman · 2017-10-22T12:43:56Z

There is no need

zwiastunsw · 2017-10-22T20:34:14Z

@brianteeman: Thanks to this work.
In my opinion, no additional nav tag is needed. Just replace the nav tag div in function pagination_list_footer. (This footer is part of navigation).
If you think you want to stay, as you proposed, let us know. Or correct.
And in one case and the other, I will test it successfully.

@C-Lodder:

I'm seeing a lot of elements without href which I think may be an issue for a11y in regards to tab indexing.

It's OK.

ciar4n · 2017-10-22T20:38:15Z

I have tested this item ✅ successfully on fa05cc6

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/18326.}

brianteeman · 2017-10-22T23:07:00Z

The nav tag is absolutely required.and i will not be removing it. Every accessibility source clearly says that this is a requirement. I am not surprised that the a11y team once again chose to do nothing and instead criticise those that actually contribute to making joomla more accessible

zwiastunsw · 2017-10-22T23:19:47Z

I am sorry, but you misunderstood, of course, the nav tag is required. I have only proposed to include the whole element. Instead of div tag in function pagination_list_footer use nav tag:

function pagination_list_footer($list)
{
	$html = "<nav role=\"navigation\" aria-label=\"' . JText::_('JLIB_HTML_PAGINATION') . '\" class=\"pagination\">\n";
	$html .= $list['pageslinks'];
	$html .= "\n<input type=\"hidden\" name=\"" . $list['prefix'] . "limitstart\" value=\"" . $list['limitstart'] . "\" />";
	$html .= "\n</nav>";

	return $html;
}

It seems to me that directing, especially at my address, and not for the first time, the comments such as: " I am not surprised that the a11y team once again chose to do nothing and instead criticise those that actually contribute to making joomla more accessible" is at least out of place.
As I wrote earlier - if you leave the code unchanged, also I will test it successfully.

brianteeman · 2017-10-22T23:23:50Z

your code will break the templates i will not be changing it

zwiastunsw · 2017-10-22T23:25:39Z

OK.

zwiastunsw · 2017-10-22T23:28:08Z

I have tested this item ✅ successfully on fa05cc6

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/18326.}

ghost · 2017-10-23T05:43:19Z

RTC after 3 successful tests.

brianteeman · 2017-10-24T00:09:30Z

Thanks

joomla-cms-bot added Language Change This is for Translators PR-staging labels Oct 13, 2017

ciar4n reviewed Oct 13, 2017

View reviewed changes

brianteeman added 2 commits October 13, 2017 13:16

oops forgot to commit this line

abed14a

sort

579e774

Quy reviewed Oct 13, 2017

View reviewed changes

space

08d0638

sprintf

d260ad9

more dry

1bd403f

Quy reviewed Oct 15, 2017

View reviewed changes

active sprintf

98f34bb

Quy reviewed Oct 16, 2017

View reviewed changes

brianteeman added 3 commits October 16, 2017 13:24

extra string removed

cac7cb3

keep @Quy happy

d2d2091

I keep forgetting to save changes before commit

fa05cc6

brianteeman mentioned this pull request Oct 17, 2017

Isis Pagination update #18357

Merged

brianteeman closed this Oct 22, 2017

brianteeman reopened this Oct 22, 2017

joomla-cms-bot added RTC This Pull Request is Ready To Commit and removed RTC This Pull Request is Ready To Commit labels Oct 23, 2017

brianteeman deleted the protostar-pagination branch October 24, 2017 00:09

tecpromotion mentioned this pull request Nov 2, 2017

Protostar pagination update joomlagerman/joomla#284

Closed

brianteeman mentioned this pull request Jul 5, 2018

[4.0] [a11y] pagination #20983

Closed

Uh oh!

Conversation

brianteeman commented Oct 13, 2017 • edited by joomla-cms-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Note 1

Note 3

Note 4

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ciar4n commented Oct 13, 2017

Uh oh!

Quy commented Oct 13, 2017

Uh oh!

brianteeman commented Oct 13, 2017

Uh oh!

brianteeman commented Oct 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Quy commented Oct 15, 2017

Uh oh!

brianteeman commented Oct 16, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Quy commented Oct 16, 2017

Uh oh!

brianteeman commented Oct 17, 2017

Uh oh!

ghost commented Oct 22, 2017

Uh oh!

C-Lodder commented Oct 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brianteeman commented Oct 22, 2017

Uh oh!

brianteeman commented Oct 22, 2017

Uh oh!

dgrammatiko commented Oct 22, 2017

Uh oh!

brianteeman commented Oct 22, 2017

Uh oh!

zwiastunsw commented Oct 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ciar4n commented Oct 22, 2017

Uh oh!

brianteeman commented Oct 22, 2017

Uh oh!

zwiastunsw commented Oct 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brianteeman commented Oct 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zwiastunsw commented Oct 22, 2017

Uh oh!

zwiastunsw commented Oct 22, 2017

Uh oh!

ghost commented Oct 23, 2017

Uh oh!

brianteeman commented Oct 13, 2017 •

edited by joomla-cms-bot

Loading

C-Lodder commented Oct 22, 2017 •

edited

Loading

zwiastunsw commented Oct 22, 2017 •

edited

Loading

zwiastunsw commented Oct 22, 2017 •

edited

Loading

brianteeman commented Oct 22, 2017 •

edited

Loading