Use aria-current=page in pagination links.

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

The last submitted patch, 96: 2952488-96-tests-only.patch, failed testing. View results →

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 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);
   }

Automatically closed - issue fixed for 2 weeks with no activity.

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

https://www.drupal.org/project/drupal/issues/3384679 🐛 aria-current is giving an invalid value Fixed