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
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:
Comments
Comment #2
nicxvan CreditAttribution: nicxvan commentedComment #3
nicxvan CreditAttribution: nicxvan commentedComment #5
nicxvan CreditAttribution: nicxvan commentedSince 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
Comment #6
nicxvan CreditAttribution: nicxvan commentedComment #7
lauriiiSeems 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.
Comment #8
xjmI think so long as we have the stable base themes, changes to them in minor releases should get release notes
Comment #9
nicxvan CreditAttribution: nicxvan commentedPlease let me know if you need additional information in the release note, this is my first time attempting one on core.
Comment #10
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer commentedPostponing 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.
Comment #11
nicxvan CreditAttribution: nicxvan commentedI think we can keep this open, I'm going to attempt making it configurable which would still be a benefit here.
Comment #13
nicxvan CreditAttribution: nicxvan commentedComment #14
nicxvan CreditAttribution: nicxvan commentedI 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.
Comment #17
nicxvan CreditAttribution: nicxvan commentedComment #18
nicxvan CreditAttribution: nicxvan commentedComment #19
nicxvan CreditAttribution: nicxvan commentedComment #21
nicxvan CreditAttribution: nicxvan commentedThis 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.
Comment #22
nicxvan CreditAttribution: nicxvan commentedComment #23
nicxvan CreditAttribution: nicxvan commentedComment #24
smustgrave CreditAttribution: smustgrave at Mobomo commentedI 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?
Comment #25
nicxvan CreditAttribution: nicxvan commentedStarterkit 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.
Comment #26
nicxvan CreditAttribution: nicxvan commentedComment #27
nicxvan CreditAttribution: nicxvan commentedComment #28
smustgrave CreditAttribution: smustgrave at Mobomo commentedChange LGTM thanks for adding CR.
Comment #29
nicxvan CreditAttribution: nicxvan commentedComment #30
quietone CreditAttribution: quietone at PreviousNext commentedI'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.
Comment #31
quietone CreditAttribution: quietone at PreviousNext commentedFix links in the issue summary
Comment #32
quietone CreditAttribution: quietone at PreviousNext commentedTry for a better title
Comment #33
nicxvan CreditAttribution: nicxvan commentedI 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.
Comment #34
nicxvan CreditAttribution: nicxvan commentedComment #35
smustgrave CreditAttribution: smustgrave at Mobomo commentedCR 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
Comment #39
nod_Committed ecac251 and pushed to 11.x. Thanks!
Comment #40
nod_