Use aria-current=page in pagination links.

Starting to work on this at the Mentored Sprint.

Working on this, as part of Nashville2018 Sprint

Here's a patch that resolves this issue.

I left the visually hidden span in place to cover people using devices that don't support aria-current.

We are reviewing the patch from jmickela now.

Reviewing 2952488-13.patch at Nashville2018 Sprint

We reviewed patch 2952488-13 and found that it worked correctly. See attached screenshots showing the before and after.

Here is a patch that includes the changes from the patch by @jmickela and also removes the span tag in core/includes/pager.inc

Moving back to 'needs work' after failed unit test

Edit: Fixed issues with my previous patch

Here is a patch that includes the changes from the patch by @jmickela and also removes the span tag in core/includes/pager.inc

Fix corrupt patch in previous comment.

Added back in hidden 'Page' text per comment #26.

I also reverted a change that seems to have happened in patch #25 where the attribute was being set in a different part of the code that was checking if there should be an ellipsis.

A question I have: I understand that the scope of this patch is specifically to apply the aria attribute to pagination links. Pagination generated by the views-mini-pager.twig (e.g. used on the Recent Log Messages page) isn't getting this update. Do we want to extend it to that twig file?

i triggered retesting for patch in #28 and it seems ok
s changing the issue status

@kalyansamanta their is no difference for the patch #28 and #30.. They are identical.. why are you posting an identical patch?

I came across similar thing done by you in different ticket - https://www.drupal.org/project/drupal/issues/2920395 #24

I've removed issue credit for @kalyansamanta - they've contacted me and appear to be aware of their mistake.

Unrelated test failure. Back to Needs review.

Tagging for upcoming contribution days.

Fixing tags.

Redoing patch for latest Drupal version.

Updating dir in patch

Looks good to me. I tested it on the home page (and the admin pages). Looks fine. Here are some screenshots for fun.

So the re-rolled patch works. Any reason not to mark this RTBC?

I tested this patch on a Chomebook with ChromeVox and on MacOS's Chrome with VoiceOver. They both work as expected and announce when a page is the current page.

I've updated the TODOs marking those two tests as done.

Updated patch after #3067584: Pager template renders attributes that do not exist was committed:

1. Removed duplicate use statement
2. Removed duplicate new Attribute() line

Tagging for amsterdam 2019.

I have some expertise in a11y and will test this on windows with narrator.

Had to reroll the patch from #51 against 8.9.x.

Why does this issue consider only pagination links? Can we extend it to support menus and tabs?

@Chi: You are right but I think active menu links should be handled in a separate issue.

Anyway, I've tested the pagination on Windows 10 Pro (10.0.17134) with Narrator. The software reads

click info "current page"

This output is based on the value of the title attribute and not on the aria-current attribute. I've tested this by removing/changing the title attribute. So unfortunately the proposed solution does not work with Edge and Narrator. BUT, as long we have the correct title according to the aria-current attribute, it's fine. The title attribute serves as a fallback. Does anyone disagree?

Fixed link to wrong comment in issue summary.

@pminf can you possibly add an interdiff? https://www.drupal.org/documentation/git/interdiff

I also agree that menu links should be addressed in another issue, perhaps you'd like to create that @Chi ?

@realityloop
I've just slightly changed the scope of #3089689: Use aria-current="page" to indicate active link in menus and reopened it.

@realityloop As I learned today, it is not possible to make an interdiff on a reroll. But here is a screenshot of the file compare between patch #51 and #55.

@pminf well, I leant something.. thanks.

1. +++ b/core/includes/pager.inc
   @@ -250,6 +251,8 @@ function template_preprocess_pager(&$variables) {
   +         $items['pages'][$i]['attributes'] = new Attribute();
   

   I'm not sure we should be making this an attributes object since this could be a BC break.

2. +++ b/core/includes/pager.inc
   --- a/core/themes/classy/templates/navigation/pager.html.twig
   +++ b/core/themes/classy/templates/navigation/pager.html.twig
   
   +++ b/core/themes/classy/templates/navigation/pager.html.twig
   @@ -66,7 +66,7 @@
   -              {{ current == key ? 'Current page'|t : 'Page'|t }}
   +              {{ 'Page'|t }}
   

   This change should be done also in core, Stable and Claro.

Hello @laurii, Here's a fix for #63

1. The aria-current attribute is set in the TWIG template.

         <a aria-current="{{ current == key ? 'page' }}">
       

   W3C Recommendation:

   The aria-current attribute is an enumerated type. [...] If the attribute is not present or its value is an empty string or undefined, the default value of false applies and the aria-current state MUST NOT be exposed by user agents or assistive technologies.

2. Change are now also in core, Stable and Claro.

Another question about core/themes/claro/templates/views/views-mini-pager.html.twig file. Should we add aria-current="page" to the current page li wrapper ?

  {% if items.current %}
    <li class="pager__item pager__item--mini pager__item--current">
      <span class="visually-hidden">
        {{ 'Current page'|t }}
      </span>
      {{ items.current }}
    </li>
  {% endif %}

Failed because of a ci error. Let's try again.

FWIW, the patch works great on core 8.8.8, for my "full pager" on a "page" view display!

Thank you so much!

Removing Europe2020 tag, since this issue was not worked on at Drupalcon Europe 2020.

Applied patch #64 on 9.2.x and it works fine. Attaching screenshots below.

Content section:

Admin section:

On 8.9.13:

Patch has been working great for us since August 2020 -- I just added it to another site last week, it worked great!

I realize more work might be needed to get this committed, BUT:
⭐ Does anyone have a feel for whether maintainers are on board with the concept? (i.e. would I be correct to say to colleagues that this thing will likely be committed eventually, or?)

Last tests failed. Working on this during ContributionWeekend2022.

1) Changed twig so that aria-current is not set at all if page is not the current one, because the attributes with no value is useless (both apply the aria-current default value false).
2) Changed file hash to pass test if pager.html.twig files of related themes are identical.

Corrected some wrong indentations in patch #80.

Applied patch #81 successfully. Works fine, sharing screenshots.

Patch #81 applied successfully
Before patch

After patch

However, after checking the before and after with an actual screen reader, i believe this is not the expected behavior for pager links. (excuse the cell phone video but i could not get the screen reader and screen recorder together).

Before the patching the screen reader indicates that this is the current page:
https://www.drupal.org/files/issues/2022-02-14/pagination-afterpatch.mp4

And after the patch it doesn't anymore.
https://www.drupal.org/files/issues/2022-02-14/pagination-before-patch.mp4

The WAI-ARIA documentation https://www.w3.org/TR/wai-aria-1.1/#aria-currentstates that "aria-current="page" can be used in a navigation tree to indicate which page is currently displayed, while aria-selected="true" indicates which page will be displayed if the user activates the treeitem."

Isn't this the case where to use aria-current=true instead of aria-current=page?

More info at: https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Attribut...

The patch works great, the aria-current=page added in pagination links.

Before Patch,

After Patch

Applied patch #81 applied successfully in drupal-9.5.x-dev. We can move this ticket to RTBC.
Thanks for the patch
For ref sharing screenshots...

The changes mentioned in #81 patch is applied successfully on branch 9.5.x-dev.
Sharing screenshot for reference:
Before patch apply:

After patch apply:

Can move this issue in RTBC

Testing patch #81
Verified issue without patch
Verified issue is fixed with patch

Starting at the Issue Summary I see there are still tasks to do in the Issue Summary. The Issue Summary states this needs signoff from an accessibility maintainer and I do not see that in the comments. Setting back to NW. Ah, yes it is tagged for 'needs accessibility review'. That needs to happen before a committer looks at this.

Comment #83 seems rather thorough and concludes the patch is not working as expected. There has been no response addressing the points in that comment. Can someone respond to that?

I appears that #86 and #87 are creating screenshot on the same patch which already has screenshots, therefor removing credit.

This patch being tests is in #81 and from 9.4.x, this is a feature request, so let's get a patch that applies to 9.5 and 10. Also, that patch is failing.

A reminder to only upload new screenshots if they are none on the latest patch or you screenshots are indeed adding new information.

I am in favour of adding this patch. This does very much seem to be a best practice we should be adopting.

https://www.accessibility-developer-guide.com/examples/sensible-aria-usa...
https://design-system.w3.org/components/pagination.html
https://tink.uk/using-the-aria-current-attribute/
https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Attribut...
https://getbootstrap.com/docs/4.0/components/breadcrumb/
https://a11y-guidelines.orange.com/en/articles/using-aria-current-attrib...

Removing the accessibility maintainer review.

Oh man horrible review on my part.

Rerolled for D10 addressing the other themes that were left out.

I don't think the olivero one is needed but wanted some feedback.

Also if this is the correct approach are we going to need tests per theme?

#63 asked this processing to be removed from template_preprocess_pager() as it would be a BC break. BC Layer has been removed in #3087517: Remove BC layer for Pager service so I suppose we could now just make this in this function and that would apply on all themes that didn't remove the item.attributes output from their own templates (including contrib themes).

What do you think about it?

I think that would be the best solution.

Also thing we should add simple test assertion that the attribute appears.

No interdiff as this is a new approach

Applied patch #96 applied successfully in drupal-10.1.x-dev.
We can move this ticket to RTBC.
Thanks for the patch.
Attached screenshot.

Looking good, one obvervation and a change required for the test changes in my book

1. +++ b/core/includes/theme.inc
   @@ -1828,6 +1828,7 @@ function template_preprocess_pager(&$variables) {
          $items['pages'][$i]['attributes'] = new Attribute();
          if ($i == $pager_current) {
            $variables['current'] = $i;
   +        $items['pages'][$i]['attributes']['aria-current'] = t('Current page');
   

   At this point $item['pages'][$i]['attributes'] is an Attribute object, so in theory we could use ->setAttribute here instead of ArrayAccess, but not sure it matters that much

2. +++ b/core/modules/system/tests/src/Unit/Pager/PreprocessPagerTest.php
   @@ -39,6 +39,7 @@ protected function setUp(): void {
   @@ -51,16 +52,17 @@ public function testQuantityNotSet() {
   
   @@ -51,16 +52,17 @@ public function testQuantityNotSet() {
        require_once $this->root . '/core/includes/theme.inc';
        $variables = [
          'pager' => [
   -        '#element' => '',
   +        '#element' => '2',
            '#parameters' => [],
   -        '#quantity' => '',
   +        '#quantity' => '2',
   

   The test method is named testQuantityNotSet but now the quantity is set.

   Is there another test method we can use here instead of modifying the intent of this test? Or should we just add another method? This is a unit test so it should be cheap

Tagging for https://www.w3.org/WAI/WCAG21/Understanding/name-role-value

Addressed points in #99

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

+++ b/core/includes/theme.inc
@@ -1827,6 +1827,7 @@ function template_preprocess_pager(&$variables) {
+      $items['pages'][$i]['attributes']->setAttribute('aria-current', t('Current page'));

it could use new TranslatableMarkup instead of t()

+++ b/core/modules/system/tests/src/Unit/Pager/PreprocessPagerTest.php
@@ -39,6 +39,7 @@ protected function setUp(): void {
+    $container->set('string_translation', $this->getStringTranslationStub());

@@ -63,4 +64,26 @@ public function testQuantityNotSet() {
+    $this->assertEquals('Current page', $variables['items']['pages']['2']['attributes']->offsetGet('aria-current')->__toString());

Please use \Drupal\Core\StringTranslation\TranslatableMarkup::getUntranslatedString() instead of testing string translation

@andypost not sure I follow #106?

I mean that)

Bit better wording

Ah thanks! I'll have to note that.

Since this was a small patch and the change touched each line think I'm good to mark it also.

Backported to 10.1.x as an accessibility bug fix.

Committed and pushed 38e908395d to 11.x and 2f060c96e5 to 10.1.x. Thanks!

Fixed on commit:

--- a/core/modules/system/tests/src/Unit/Pager/PreprocessPagerTest.php
+++ b/core/modules/system/tests/src/Unit/Pager/PreprocessPagerTest.php
@@ -40,7 +40,7 @@ protected function setUp(): void {
     $container = new ContainerBuilder();
     $container->set('pager.manager', $pager_manager);
     $container->set('url_generator', $url_generator);
-    // The template_preprocess_pager() rendering translatable attribute values.
+    // template_preprocess_pager() renders translatable attribute values.
     $container->set('string_translation', $this->getStringTranslationStub());
     \Drupal::setContainer($container);
   }

Looks like the value used for this attribute is incorrect. Filed a follow up

https://www.drupal.org/project/drupal/issues/3384679