Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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.
Comment | File | Size | Author |
---|---|---|---|
#15 | 3059232-15.patch | 3.35 KB | idebr |
#15 | interdiff-11-15.txt | 921 bytes | idebr |
Comments
Comment #2
PCate CreditAttribution: PCate as a volunteer commentedComment #3
keithnull CreditAttribution: keithnull commentedI'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.
Comment #4
mgiffordThis 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"
Comment #5
LendudeI don't think we are allowed to change anything in stable? No sure what the exact policy is.
Comment #6
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer commentedI 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.
Comment #7
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer commentedLooking 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 thearia-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()
(incore/modules/views/view.theme.inc
). This would follow the approach used by the full pager (seepager.html.twig
template andtemplate_preprocess_pager()
).Comment #8
idebr CreditAttribution: idebr at ezCompany commentedClosed #3063017: Broken ARIA reference in the views mini pager as a duplicate issue.
Comment #10
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer commentedIn the duplicate issue, @KelvinWong provided a patch with a quick fix:
This will indeed fix the problem. Note that switching from
aria-labelledby
toaria-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 thearia-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.
Comment #11
KelvinWong CreditAttribution: KelvinWong commentedFollow how the full pager implements the pagination heading id. This patch adds the same heading id in the mini pager.
Comment #12
idebr CreditAttribution: idebr at ezCompany commentedClosed #3076006: Views mini pager is missing an ID on its top-level h4 element as a duplicate issue.
Comment #13
huzookaWe have to fix this for Stable theme as well.
Comment #14
huzookaComment #15
idebr CreditAttribution: idebr at ezCompany commented#13 Added the change to the Stable theme
Comment #16
lauriiiThis 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!
Comment #18
catchCommitted 58abc5c and pushed to 8.8.x. Thanks!