Problem/Motivation

In #1912608: Update pagination markup for new CSS standards and improved accessibility, the pager was refactored and new markup was implemented. The twig template contains a hard-coded HMTL ID:

    <h4 id="pagination-heading" class="visually-hidden">{{ 'Pagination'|t }}</h4>

This leads to invalid HTML when multiple pagers are present on the same page.

Proposed resolution

  1. In template_preprocess_pager(), build a unique HTML ID dynamically: $variables['pagination_heading_id'] = Html::getUniqueId('pagination-heading');
  2. In all (* see "remaining tasks" below) instances of page.html.twig, replace id="pagination-heading" with id="{{ pagination_heading_id }}"

Remaining tasks

  • Decide if the Stable theme needs this update too. I'm not sure what the policy is. On one hand Stable should never change, on the other hand it's a bugfix rather than a change.
  • Write code, roll patch, test, etc.

Credits

I nominate my colleague @marloes_bosch for commit credits because she found the issue and figured out how to solve it.

CommentFileSizeAuthor
#15 interdiff.txt1.79 KBlauriii
#9 hard-coded-pagination-heading-id-fix-3011722-9.patch3.79 KBpasan.gamage
#2 hard-coded-pagination-heading-id-fix-3011722-2.patch3.87 KBilya.no
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

marcvangend created an issue. See original summary.

ilya.no’s picture

ilya.no CreditAttribution: ilya.no as a volunteer and at Skilld for Skilld commented
Status: Active » Needs review
FileSize
hard-coded-pagination-heading-id-fix-3011722-2.patch3.87 KB

Hi! Attaching initial patch.

Raman Starshykh’s picture

Raman Starshykh CreditAttribution: Raman Starshykh at EPAM Systems commented
Assigned: Unassigned » Raman Starshykh
Raman Starshykh’s picture

Raman Starshykh CreditAttribution: Raman Starshykh at EPAM Systems commented
Status: Needs review » Reviewed & tested by the community

Patch serves the purpose as suggested.

I created several views blocks with paginations -
Result html code -

<nav class="pager" role="navigation" aria-labelledby="pagination-heading--2">
    <h4 id="pagination-heading--2" class="visually-hidden">Pagination</h4>
    <ul class="pager__items js-pager__items">

<nav class="pager" role="navigation" aria-labelledby="pagination-heading--3">
    <h4 id="pagination-heading--3" class="visually-hidden">Pagination</h4>
    <ul class="pager__items js-pager__items">
Raman Starshykh’s picture

Raman Starshykh CreditAttribution: Raman Starshykh commented
Assigned: Raman Starshykh » Unassigned
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce commented

Markup is not part of the API as per core's BC policy so this change is fine to make. Only going to make it in 8.7.x just in case people have CSS targeting this. It'll give them a chance to target something like .pager h4. Also as a bug this should be fixed in stable.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs change record 
+++ b/core/includes/pager.inc
@@ -278,6 +279,7 @@ function template_preprocess_pager(&$variables) {
+  $variables['pagination_heading_id'] = Html::getUniqueId('pagination-heading');

Is there specific reason why this has been named specifically as pagination_heading_id? How about renaming it to heading_id?

We could also open a change record for the change so that people who have overridden the template, know they should apply this change to the overriding template.

marcvangend’s picture

marcvangend
he/him
Primary language Dutch
Location Amsterdam
CreditAttribution: marcvangend as a volunteer and at LimoenGroen commented

@ilya.no, thanks for the patch.

Re #7

Is there specific reason why this has been named specifically as pagination_heading_id? How about renaming it to heading_id

No, not really. I guess I came up with it because it replaces the id that used to be "pagination-header". But by all means, change it to heading_id if that's more consistent with the rest of our code.

The change record sounds like a good idea.

pasan.gamage’s picture

pasan.gamage CreditAttribution: pasan.gamage at PreviousNext commented
FileSize
hard-coded-pagination-heading-id-fix-3011722-9.patch3.79 KB

Hi,

Please see attached patch for reverting pagination_heading_id to heading_id

Change request on https://www.drupal.org/node/3018097

Thanks

pasan.gamage’s picture

pasan.gamage CreditAttribution: pasan.gamage at PreviousNext commented
Status: Needs work » Needs review
Eric115’s picture

Eric115 CreditAttribution: Eric115 at PreviousNext commented
Status: Needs review » Needs work

Hi Pasan, the change record will need to be about changing the css ID from "pagination-heading" to a unique ID, not about the variable name in this case.

Thanks for your work on this!

pasan.gamage’s picture

pasan.gamage CreditAttribution: pasan.gamage at PreviousNext commented

Thank you @Eric115 for mentoring for this issue.

Eric115’s picture

Eric115 CreditAttribution: Eric115 at PreviousNext commented
Status: Needs work » Reviewed & tested by the community
Issue tags: +DrupalSouth 2018

Thanks for updating the change record. The code looks good and is working as per #4.

Marking as RTBC!

  • lauriii committed 846024a on 8.7.x 
    Issue #3011722 by pasan.gamage, ilya.no, Eric115, alexpott, lauriii:...
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Status: Reviewed & tested by the community » Fixed
Issue tags: -Needs change record
FileSize
interdiff.txt1.79 KB

Added a full stop after the variable documentation description (interdiff attached). Other than that this looks good. Thank you for creating the change record as well!

Committed 846024a and pushed to 8.7.x. Thanks!

Status: Fixed » Closed (fixed)

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

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Issue tags: +8.7.0 release notes

Since this fix required a change to our stable base themes, we'll list it in the release notes since it is a small BC break for themes.