I noticed today that the views mini pager template file has a aria-labelledby attribute that references an id of pagination-heading which doesn't exist anywhere.

<nav role="navigation" aria-labelledby="pagination-heading">
    <h4 class="visually-hidden">{{ 'Pagination'|t }}</h4>
    <ul class="js-pager__items">
      {% if items.previous %}
        <li>
          <a href="{{ items.previous.href }}" title="{{ 'Go to previous page'|t }}" rel="prev"{{ items.previous.attributes|without('href', 'title', 'rel') }}>
            <span class="visually-hidden">{{ 'Previous page'|t }}</span>
            <span aria-hidden="true">{{ items.previous.text|default('‹‹'|t) }}</span>
          </a> 

I'd expect the code to be more like:

<nav role="navigation" aria-labelledby="pagination-heading">
    <h4 id="pagination-heading"  class="visually-hidden">{{ 'Pagination'|t }}</h4>
    <ul class="js-pager__items">
      {% if items.previous %}
        <li>
          <a href="{{ items.previous.href }}" title="{{ 'Go to previous page'|t }}" rel="prev"{{ items.previous.attributes|without('href', 'title', 'rel') }}>
            <span class="visually-hidden">{{ 'Previous page'|t }}</span>
            <span aria-hidden="true">{{ items.previous.text|default('‹‹'|t) }}</span>
          </a> 

Seems to be in both the module template file as well as in the Stable and Classy themes.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

PCate created an issue. See original summary.

PCate’s picture

Issue summary: View changes
keithnull’s picture

I'm new to Drupal and thought this would be something simple and a way for me to start getting into contributing. I've attached a simple patch for review below that makes the changes you suggested in the files mentioned.

mgifford’s picture

Status: Active » Needs review

This makes sense to me. I haven't tested it yet, but it does seem pretty clear that there is a missing reference from aria-labelledby="pagination-heading"

Lendude’s picture

+++ b/core/themes/classy/templates/views/views-mini-pager.html.twig
--- a/core/themes/stable/templates/views/views-mini-pager.html.twig
+++ b/core/themes/stable/templates/views/views-mini-pager.html.twig

I don't think we are allowed to change anything in stable? No sure what the exact policy is.

andrewmacpherson’s picture

I wonder if there used to be an ID on the heading? There was an effort to remove ID selectors from CSS, and some clean-up issues got over-zealous and removed ID attributes from HTML that were used in IDREFs.

See #2723961: Broken aria-labelledby ID reference in menu block and breadcrumb templates. If we look back in the git log for the mini-pager template, we might find some similar ID removal.

This will qualify for Stable. We've allowed a11y fixes in Stable already, specifically to restore the missing ID in that issue.

andrewmacpherson’s picture

Status: Needs review » Needs work

Looking back through the git log, I don't think the aria-labelledby reference has ever worked for the mini-pager. Commit c997eee in #2318341: Views mini pager markup originally introduced the <nav> wrapper to the mini-pager, along with the aria-labelledby="pagination-heading" attribute. However it did not introduce the corresponding ID.

The patch here adds a hard-coded ID to directly to the templates. We shouldn't do that, because if there is more than one view on a page with a mini-pager, they won't have unique IDs.

Instead, we should generate unique IDs like for the pager heading, in template_preprocess_views_mini_pager() (in core/modules/views/view.theme.inc). This would follow the approach used by the full pager (see pager.html.twig template and template_preprocess_pager()).

idebr’s picture

andrewmacpherson’s picture

In the duplicate issue, @KelvinWong provided a patch with a quick fix:

This problem can be fixed by replacing aria-labelledby with aria-label.

This will indeed fix the problem. Note that switching from aria-labelledby to aria-label introduces a second UI string. There's nothing wrong with that as far as accessibility goes, however it can be brittle from a maintenance point of view. If someone edits the heading, they ought to edit the aria-label too for consistency. Since we already have a heading, making the aria-labelledby relationship work properly is somewhat preferable.

Transfering an issue credit for @KelvinWong for the patch on #3063017: Broken ARIA reference in the views mini pager.

KelvinWong’s picture

Follow how the full pager implements the pagination heading id. This patch adds the same heading id in the mini pager.

idebr’s picture

huzooka’s picture

Status: Needs review » Needs work

We have to fix this for Stable theme as well.

huzooka’s picture

idebr’s picture

#13 Added the change to the Stable theme

lauriii’s picture

Status: Needs review » Reviewed & tested by the community

This looks good! This is acceptable non-disruptive change to Stable and Classy since this only adds a new template variable and sets id property to the heading. Thank you @idebr and everyone!

  • catch committed 58abc5c on 8.8.x
    Issue #3059232 by idebr, KelvinWong, keithnull, PCate, andrewmacpherson...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed 58abc5c and pushed to 8.8.x. Thanks!

Status: Fixed » Closed (fixed)

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