Problem/Motivation

The pager by default uses an h4, this creates the `Heading levels should only increase by one` error in many use cases.

This has been addressed for all non stable9 core themes in #3333401: Pager h4 causes accessibility flag on many pages
Since it is an accessibility fix stable9 should also be updated.
@laurii and @xjm confirmed as long as it gets a release note.

Steps to reproduce

Create a view with a mini pager or full pager that has no other content on the page.
Run axe devtools

Proposed resolution

@andrewmacpherson's objection in comment 10 is related to a previous approach that was resolved in the parent issue.
This fix implements an approved solution for all other core themes in stable 9.
Follow accessibility review guidelines from #3333401: Pager h4 causes accessibility flag on many pages
Sidenote: the initial approach discussed on the above issue of moving the heading text into an aria-label attribute on the parent tag, and thereby removing the Heading element altogether will indeed help pass many accessibility tests, but is detrimental to actual users. See comment #30 from @andrewmacpherson for full details.
The final resolution for an accessible pager heading was to add a new configuration option in views pagers.

This issue updates stable9 templates to use the new pagination_heading_level variable.
Update tests to include stable9

Remaining tasks

Apply only to 10.3 since that introduces the new pagination_heading_level variable.

User interface changes

N/A

API changes

N/A

Data model changes

N/A

Release notes snippet

  • Stable9 views mini pager template updated to use new pagination_heading_level variable (Defaults to h4)

  • Stable9 pager template updated to use new pagination_heading_level variable (Defaults to h4)

Issue fork drupal-3333418

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

nicxvan created an issue. See original summary.

nicxvan’s picture

Issue summary: View changes
nicxvan’s picture

nicxvan’s picture

Since this is an accessibility fix I think it should be made in Stable9. The non stable9 themes are updated in an RTBC Patch: https://www.drupal.org/project/drupal/issues/3333401

nicxvan’s picture

Issue summary: View changes
lauriii’s picture

Status: Active » Reviewed & tested by the community
Issue tags: -Needs accessibility review

Seems like this is a pretty low risk change so I'd be fine with introducing this change in 10.1.x. Removing needs accessibility review tag since the approach was already reviewed in #3333401: Pager h4 causes accessibility flag on many pages.

xjm’s picture

Issue tags: +Needs release note

I think so long as we have the stable base themes, changes to them in minor releases should get release notes

nicxvan’s picture

Please let me know if you need additional information in the release note, this is my first time attempting one on core.

In order to prevent heading out of order accessibility errors on views pagers the h4 element was removed from the pager.html.twig and views-mini-pater.html.twig templates in Stable9. This should not affect most sites since the element was hidden by default. The aria attributes were moved to the preexisting nav element.
andrewmacpherson’s picture

Status: Reviewed & tested by the community » Postponed
Issue tags: +Accessibility

Postponing this issue. It was filed as a follow-up to #3333401: Pager h4 causes accessibility flag on many pages, which was committed and later reverted.

Since then, I've made a very strong objection to the proposal in #3333401: Pager h4 causes accessibility flag on many pages. I don't think we should remove the heading.

So, this issue isn't currently needed, and may well turn out to be a wont-fix.

nicxvan’s picture

I think we can keep this open, I'm going to attempt making it configurable which would still be a benefit here.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

nicxvan’s picture

Issue summary: View changes
nicxvan’s picture

I think the parent issue is getting close to be resolved. I think since this is an accessibility fix and we've found an acceptable way to update the other core themes, it makes sense to make the change to Stable9 once it lands and utilize the variable for heading level that views is providing. Otherwise the setting in views will have no affect.

I still need to wait for the parent issue to land cause the tests are all there.

I'll create a new MR once that is merged.

nicxvan changed the visibility of the branch 3333418-stable9-accessibility-update to active.

nicxvan’s picture

Status: Postponed » Active
nicxvan’s picture

Assigned: Unassigned » nicxvan
nicxvan’s picture

Issue summary: View changes

nicxvan’s picture

Status: Active » Needs review

This can only be added in 10.3+ since it relies on https://www.drupal.org/project/drupal/issues/3333401 which is coming out in 10.3

Tests are passing so this is ready for review.

nicxvan’s picture

Issue summary: View changes
nicxvan’s picture

Issue summary: View changes
smustgrave’s picture

Status: Needs review » Needs work

I believe with changes to stable9 we will need a CR for the new/edited templates. As themes built with starterkit will have to update their code right?

nicxvan’s picture

Status: Needs work » Needs review

Starterkit was updated in the previous issue, I just tested generating a new theme on HEAD and the new theme uses the pagination heading variables even though stable9 hasn't been updated. Both templates that were edited are in the generated theme so they wouldn't fall back to stable9.

Anyone who created a theme with the generator before 10.3 will need to update their templates to use the new variable which is mentioned in the parent change record.

I'll create a draft CR just in case.

nicxvan’s picture

Issue summary: View changes
nicxvan’s picture

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

Change LGTM thanks for adding CR.

nicxvan’s picture

Assigned: nicxvan » Unassigned
quietone’s picture

Issue summary: View changes
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs change record updates

I'm triaging RTBC issues. I read the IS and the comments.

The issue summary states that there are "accessibility review guidelines" in #3333401: Pager h4 causes accessibility flag on many pages but I can not find them. Is that the correct link?

In #10 @andrewmacpherson suggested this is a won't fix but then I read that the related issue was committed and nicxvan restarted the work here. I then read the MR and I have no suggestions. My reading of the MR is not a review.

I read the change record and it reads as if the new configuration was added in this issue. But, according to this change notice that was added in another issue. That needs to be clarified, so that this change record is only for the change being made here. Also, "Introduced in branch: 11.x" should be '10.3.x' because 11.x is not a branch, it is really 'main' but we have limitations on d.o such that we can't use 'main'.

I am setting to needs work for the change record updates. That shouldn't take too much time for anyone.

quietone’s picture

Issue summary: View changes

Fix links in the issue summary

quietone’s picture

Title: Stable9 accessibility update: Pager h4 causes accessibility flag on many pages » Fix pager h4 for accessibility on Stable9

Try for a better title

nicxvan’s picture

Issue summary: View changes
Status: Needs work » Needs review

I have updated the change record to be clearer that the configuration option was added in another issue and that this issue is just to take advantage of the new configuration.

If it is not clear @andrewmacpherson's objection was to the originally suggested approach and not the final approach.
His comment in 10 is related to that approach.
We were able to get a more robust solution that resolves his concerns and the initial issue in #3333401: Pager h4 causes accessibility flag on many pages and this issue is updating Stable9 to take advantage of the accessibility fix and update tests to cover stable.

I updated the summary to include the initial rejected approach and the final resolution for the related issue for clarity.

nicxvan’s picture

Issue summary: View changes
smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

CR updates look good to me. Link points to the first CR where the setting was added and CR seems to correctly mention this is a stable9 updat.e

  • nod_ committed ecac2516 on 11.x
    Issue #3333418 by nicxvan, smustgrave, quietone: Fix pager h4 for...

  • nod_ committed ff36302a on 10.3.x
    Issue #3333418 by nicxvan, smustgrave, quietone: Fix pager h4 for...

nod_’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: -Needs change record updates

Committed ecac251 and pushed to 11.x. Thanks!

nod_’s picture

Version: 11.x-dev » 10.3.x-dev

Status: Fixed » Closed (fixed)

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