Pager h4 causes accessibility flag on many pages

lauriii → closed merge request !3242

Committed 01de0a8 and pushed to 10.1.x. Cherry-picked to 10.0.x and 9.5.x since this is a non-disruptive bug fix. Thanks!

I think this broke 9.5.x: https://www.drupal.org/pift-ci-job/2576989 →

Reverted and queuing the patch against that branch.

I think this is due to how classy was removed from 10, there was a related issue: https://www.drupal.org/project/drupal/issues/3232222 🐛 Pager template h4 can be out of document hierarchy order Closed: duplicate that addressed the content hash, I'm not sure how to provide a MR that fixes tests in 9.5 that would have been removed in 10.
I closed the related issue yesterday when this was merged.

Let me know how I should proceed and I'm happy to address it.

I think I may have gotten it:

I used the wrong hashing function - here is the correct one.

For some reason the page.css file is now triggering a failure even though nothing changed, I regenerated the hash locally.

Not sure why that was failed this time.

Sorry for the back and forth on the patches, this is the first time I've generated content hashes and one of them was different than the test was generating, I think due to my local php version, this should be right.

I missed updating two themes.

I had inverted the pager hash

Ok I got it, the confusion on my end was I didn't realize more than one theme was failing so I flipped back and forth a couple of times.

Some of the folks on https://www.drupal.org/project/drupal/issues/3232222 🐛 Pager template h4 can be out of document hierarchy order Closed: duplicate should get credit cause I pulled from that issue to get this right.

Looks good to me. Tested on Simlytest.me and does what is expected. Moving to RTBC!

I Saw this on the Drupal 9.5.3 release notes yesterday, including the reverting commit.

I think the proposal here is rather brutal. It doesn't take account of well-attested user behaviours. I don't think we should remove the heading.

@nicxvan - Thanks for reporting the issue. I appreciate the work you've done to maintain the patch here.

I'd like to take issue with the appraisal so far. Comment #8 by @bnjmnm moved this to RTBC:

To assistive tech, this solution works identically so it's not at all disruptive.

Emphasis mine. This remark is concerned with the assistive technology software, rather than the software's human users. The proposal may not be disruptive to software, but it is likely to be disruptive for users.

Previously, the pagination could be found by screen reader users in a number of ways:

1. Peruse headings. Many screen readers provide tools for this, but no widely used web browsers do so.
2. Using the web browser's find-in-page tool. It can match the content of the heading element. (Aside, you don't need actually need a screen reader for this.)
3. Peruse landmark regions. Many screen readers provide tools for this, but no widely used web browsers do so.

After implementing the proposal here, two of these methods no longer work:

1. The heading is gone, so you can't find the pager by perusing headings.
2. Using the browser's find-in-page tool no longer works. It doesn't match the content of the aria-label attribute.
3. The only method still available is to peruse landmark regions.

Will this be disruptive for actual users? Let's look at some user data.

The WebAIM Screen Reader User Survey #9 asked respondents:

When trying to find information on a lengthy web page, which of the following are you most likely to do first?

1. Navigate by headings was by far the most popular method, with approximately two-thirds of users preferring it.
2. Using a find-in-page tool was the next most popular, with approx 15% of users.
3. Navigating by landmark regions was the least popular method, at under 4% of users.

The same question was asked in earlier WebAIM surveys:

• Screen reader user surveys #8, #7, #5, and #4 showed roughly similar results to survey #9. It's been very consistent for a decade.
• Curiously, the question was omitted from survey #6.
• In surveys #2 and #3, headings and find-in-page were also the most popular methods. These surveys didn't offer landmark regions as an answer.

But the proposal here puts the kibosh on the two most popular methods, and doubles down on the least popular method!

Concerning the report from the aXe tests:

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

• Deque's documentation for the Heading levels should only increase by one test describes it as a "Deque best practice" of "moderate" impact.
• Deque's commentary describes why a sensible heading outline is useful, and gives recommendations for writing effective heading text.
• What the commentary doesn't address is: It completely fails to say how the skipped level presents a problem.
• The commentary doesn't encourage the removal of headings as a way to address the skipped level. On the contrary, the article goes to a lot of effort to advocate for the use of headings.

It's also worth noting that the skipped heading level IS NOT a failure of any WCAG success criteria. For a detailed explanation, see David Swallow's article: Heading off confusion: When do headings fail WCAG?

So, what's the impact (for users) of skipping a heading level? It's actually not as bad as the aXe rule title implies.

Screen readers are quite forgiving of missing heading levels. The missing heading level isn't a complete disaster.

Screen reader users can interact with headings in various ways:

1. All the screen readers I've ever used provide tools to move to the next/previous headings regardless of the heading level. Indeed, when using a touchscreen on a mobile OS, this is typically the only mechanism for moving amongst headings. The skipped heading level has no impact here whatsoever; the only thing that matters is the order of headings.
2. Many screen readers also provide commands to move among headings of a particular level, when using a keyboard. Most desktop screen readers have this feature, as do iOS VoiceOver and Android Talkback when a Bluetooth keyboard is connected. In practice this is very useful for jumping to the major sections of the page, while ignoring the intervening lower levels. It isn't quite as much use for navigating among the lower nested levels of headings; sometimes you ask for the next H4, only to be told that there isn't one.
3. A few screen readers (e.g. NVDA) provide a hierarchical tool to navigate headings as a tree. The NVDA tool is forgiving of skipped levels. It will show a H2 and H4, and still let you access the H4. AFAIK, no screen reader for a mobile OS provides a hierarchical heading browser, so the skipped heading level has no practical impact.

I'd like to stress that accessibility is about helping actual users, not satisfying opinionated best-practice rules in automated tests. It certainly isn't about keeping developer's dashboards green. Has any screen reader user asked for the heading to be removed?

Should we continue to provide a heading for the pagination? Yes. It facilitates the two most popular methods of finding information, as reported by the WebAim screen reader user survey.

Should we continue to use a landmark region? Yes. A moderate number of landmarks are very useful for finding the major sections of the page, and/or small-but-important tools on the page. Pagination for the main content is a good candidate for a landmark region.

Whilst landmark regions aren't the first tool that all users reach for, a simple landmark structure is a great accompaniment to a detailed heading outline. The belt-and-braces approach of using landmarks and headings offers choice for users.

For these reasons, I'd say no to the current proposal to remove the heading.

Alternative proposal: keep the heading. Can we improve the heading level?

We could deal with the missing level by promoting it to h3, and aXe would stop complaining.

The devil is in the details though...

Headings assume ownership over ALL of the content which follows, until another heading of the same level (or higher) is encountered.

For example, consider the admin/content page, with the default configuration from Standard profile.

Currently it looks like the pagination (h4) is a sub-heading under the breadcrumb navigation (the preceding h2). This is clearly nonsense, and promoting the pagination heading to h3 wouldn't improve that at all. It would still be a misleading heading structure, even though we'd eliminated the missing level which aXe complained about.

We could promote the pagination to a h2 level. Then it would be a subheading of the main page heading (h1), and a sibling of the breadcrumb and primary tabs. Much better.

Yet, a h2 assumes that the pagination relates to the main content of the page. For a views block in a sidebar region, the h2 would be too important; the views block would have it's own h2. Or, the views block might have no heading at all. Either way, giving the pagination a h2 in a views block would produce a misleading outline, suggesting it related to the main content.

Managing heading levels with a template-driven publishing system is a pain-in-the-architecture. There is no single correct level for every use of the template.

A more ambitious approach would be to compute heading levels, for the context of the assembled page. I'm not aware of prior PHP work which does this, but there's an interesting article from Heydon Pickering about how to do it in a React Javascript application: Managing Heading Levels In Design Systems. I expect there are still devils lurking in the details there too. Heydon's use of aria-level could complicate our existing CSS, but a server-side PHP approach might avoid using aria-level if we computed the heading levels late in the page render process.

Thank you @andrewmacpherson for your comprehensive and insightful response. Your thoughts and experiences are greatly appreciated, and your point about the potential regressions with some assistive technology users seems to warrant a revert.

I completely agree with you that real users should always be our top priority, and their needs and experiences should always be at the forefront of our considerations. Automated tools and tests are certainly helpful, but they are ultimately secondary to the needs of the people who are using the system.

If we were to revisit this and bring back the heading, my instinct would be to go via JS post-load -- querySelectorAll for H1, H2, H3, H4 and their aria equivalents. Then iterate the array and each time you hit a pagination heading, modify the pagination header's level to be one less than the previous level in the array.

That's what I do in Editoria11y -- I walk the tree comparing levels and flag things as needed.

And then...add a field to Views for "Pagination heading level." Let people manually pick a level if they know more than the machine, and have the default be auto, which pulls the JS library. To avoid breaking CSS changes to existing sites, the upgrade script could set existing views to "4" and only new views/installs would use the "auto..."

Re. #34 - Aha... thanks lauriii. I saw in the D9.5.3 release notes it was reverted, but I didn't realize it was still there in the D10 branches.

Re. #35 - We still have the heading, barring a brief blip in D10.0.3

That JS logic could be useful to study. I think server-side logic to write the correct level in HTML would be preferable to JS though.

Thank you @andrewmacpherson for the detailed write up, I'll have to read through those survey's more closely.

I completely agree with you and @laurii that real users are the priority.

If there is a way to prevent false negatives on almost every page of a real site it removes white noise when addressing accessibility issues, so if there is a way to address the heading issue without affecting usability then we should do that.

After reading your write up, there doesn't seem to really be a way to remove the heading without significantly affecting usability negatively which means we need to see if there is a way to set it in a way that responds to content as mentioned.

I do have a couple of questions / comments about the alternative solutions mentioned:
I'm not sure how this could be addressed at the template level since it's so contextual to what content the page and the pager could be split out from the content of the view. It could also be affected by rewriting fields.

That points closer to using javascript, but I'm not sure the javascript solution is much better as it's just the inverse of the false negative from the automated test. Also I'd be wary of adding js just to prevent an accessibility flag in axe tools.

It sounds like the correct solution is to do nothing in core and if we need to change heading levels on an individual case we just use separate templates for pagers as needed.

If there is something to be done still I'd be happy to take a crack at it.

I don't really mind the white noise, or the Drupal core issues that result from it. It's nice to know people run automated tests.

aXe has some ways to turn off certain tests; the browser plugin has a button to disable the "best practice" category, and if you run it from the CLI you can exclude specific tests I think. What you can't do is flag a specific test result as a false flag.

The thing is, the aXe report here ISN'T a false report. It really would be better if we didn't skip a heading level. It's just that the impact isn't actually very severe in practice, and the proposed cure was worse than the disease.

Also I'd be wary of adding js just to prevent an accessibility flag in axe tools.

Yeah, I think that would be folly, too. It would also make us subject to ridicule and suspicion in the wider accessibility community.

Aside: there have been some disturbing reports alleging that an accessibility overlay company interfered with the WAVE accessibility checker, and WAVE's developers noticed this. Some nice bedtime reading at #accessiBe Spoofs Automated Checkers.

There is no way we can allow JS in Drupal core just to silence a test suite.

Ah, great points!

After re-reading #35, maybe the solution is a new option to set the heading in the pager options in views. I missed that suggestion, so it would default to h4, but you could set the heading level like you do in fields.

I'm going to make an attempt at making the pager level configurable at the individual view level.

I'll make sure this tests both 9.5 and 10 so we don't have the same test failures.

I created a new branch since the previous one was merged and doesn't really match this approach at all.
I took a first pass at adding the option to the pager.
I added a new settings value for views so that we can restrict the tags to headings.
This only adds and saves the value in the pager form, it does not do anything yet to the templates, I had some questions.

I'd welcome feedback on naming / process so far.
Things I am not sure the best way to approach:
How do I get that pager option in the views.theme.inc when the pager is instantiated?
I assume I have to override it in theme.inc as well.
Where would the update hook to create that config go? views.install?

This only attempts to provide the fix in 10.x since I will have to regenerate the classy hashes again and those don't exist on this branch.

Simple, elegant; I like the approach.

Dodging the update script question...

• I'd go for an on-the-nose name for the machine name for the variable. E.g.: "pagination-heading-level" and "heading level." And probably best to go with heading rather than header, unless there is local precedent. I tend to get these confused myself...
• For the description; perhaps make it clear what people should do? E.g.: "Choose a heading level equal to or one lower than the section heading for your view."
• Miiiight also make sense to change the default level for *new* views to H3. That is more likely be correct out of the box, and avoids breaking themes. The update script could apply H4 to all existing views.
• Another approach that would dodge breaking existing sites would be leaving the H4 in code, and just modifying an attribute to override its level in the page outline: (aria-level="pagination-heading-level"
• You can leave the H1 off the option list in my opinion. A pager is going to be descendant of something, so it shouldn't ever need to be an H1.

Choose a heading level equal to or one lower than the section heading for your view.

This text made me realise that the functionality presumes the site admin can predict the context that the view will be rendered in. In some cases (eg a <drupal-view> embedded in content) this won't be the case. I'm not pitching the dynamic approach here, just that assigning a heading level at view config makes assumptions about the content structure the view will be used in later which can't be relied on. For the case of an embed, there are at least two edge cases:

• The view appears several times in a document which contains headings at several levels
• The view appears in a content section which gets edited over time

Instead of fixing to headings here, should we permit views to use headings or <nav>, allowing site admins to determine an appropriate method?

For the proposed description there's also the case where views have section headings disabled. If we stick with headings, I propose a less specific text for that description, eg "Choose a heading level appropriate to the context of the view placement". (It's not great, but to me it doesn't over-specify.)

@nicxvan, I don't see the new branch mentioned in #42 here - did that get missed? The only MR branch showing on the issue (for me) is the previously merged one.

Taking the opportunity in this comment to accept your offer in #3333401-28: Pager h4 causes accessibility flag on many pages → for credit from 🐛 Pager template h4 can be out of document hierarchy order Closed: duplicate previously :)

@itmaybejj good points on terminology, I'll update that.

@xurizaemon The branch is at the top, I didn't create an MR yet since I need advice. But here is the branch: https://git.drupalcode.org/issue/drupal-3333401/-/tree/3333401-configura... and the Diff https://git.drupalcode.org/issue/drupal-3333401/-/compare/10.1.x...33334...

It's right below where it calls the fork.

I updated the pull request with most of the feedback.

I think updating the actual tag is better than just a aria heading.

Tagging for WCAG SC 2.4.6 https://www.w3.org/WAI/WCAG21/Understanding/headings-and-labels.html

The attached patch expands on the work done in the issue fork - injecting the 'pagination_heading_level' into both full and mini pagers, as well as updating all core theme pager templates. For the life of me, I could not access 'pager_rewrite_elements' from the view config, so instead I added a method to filter out the heading elements from 'field_rewrite_elements'.

Blank line was missing necessary whitespace.

I think I may have a crlf issue

Submitted patch works, but fails a test -- Drupal\Tests\datetime\Functional\Views\FilterDateTest, due to a schema error despite numerous attempts to patch (views.pager.schema.yml).

Views Configuration Form:

Pager State after Update:

Moving to NW for test failure

Also imagine this will need test coverage.

Also tagging for CR for changes to twig files.

Re-rolled patch to add guards for non-view pagers.

I'm going to port your changes back to the branch and take a crack at the tests. I think we can hide the 9.5 changes since 9 is end of life.

@tsquared212 I moved your changes to the branch again since that's where initial testing was going. I made two changes.

1. I added the pager rewrite schema back since it gives more flexibility in the future rather than using the field rewrite.
2. I changed the default to h4 since that is what it was before. Let me know if there was a specific reason for that change.

For 2 I think it makes sense to change if others do too, I think the first point is important to keep though.

I'm getting the same error in the gitlab ci. I tried adding the new option to the pager to see if that was it, but it did not fix it.

Failure copied here for convenience:

Fail      Other      phpunit-51.xml       0 Drupal\Tests\datetime\Functional\Vi
    PHPUnit Test failed to complete; Error: PHPUnit 9.6.13 by Sebastian
    Bergmann and contributors.
    
    Testing Drupal\Tests\datetime\Functional\Views\FilterDateTest
    .F                                                                  2 / 2
    (100%)
    
    Time: 00:34.826, Memory: 4.00 MB
    
    There was 1 failure:
    
    1)
    Drupal\Tests\datetime\Functional\Views\FilterDateTest::testExposedFilterWithPager
    Failed asserting that actual size 0 matches expected size 2.
    
    /builds/issue/drupal-3333401/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:121
    /builds/issue/drupal-3333401/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:55
    /builds/issue/drupal-3333401/core/modules/datetime/tests/src/Functional/Views/FilterDateTest.php:227
    /builds/issue/drupal-3333401/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
    
    FAILURES!
    Tests: 2, Assertions: 38, Failures: 1.

This is the test that is failing: core/modules/datetime/tests/src/Functional/Views/FilterDateTest.php

/**
   * Tests exposed date filters with a pager.
   */
  public function testExposedFilterWithPager() {
    // Expose the empty and not empty operators in a grouped filter.
    $this->drupalGet('admin/structure/views/nojs/handler/test_filter_datetime/default/filter/' . $this->fieldName . '_value');
    $this->submitForm([], t('Expose filter'));

    $edit = [];
    $edit['options[operator]'] = '>';

    $this->submitForm($edit, 'Apply');

    // Expose the view and set the pager to 2 items.
    $path = 'test_filter_datetime-path';
    $this->drupalGet('admin/structure/views/view/test_filter_datetime/edit');
    $this->submitForm([], 'Add Page');
    $this->drupalGet('admin/structure/views/nojs/display/test_filter_datetime/page_1/path');
    $this->submitForm(['path' => $path], 'Apply');
    $this->drupalGet('admin/structure/views/nojs/display/test_filter_datetime/default/pager_options');
    $this->submitForm(['pager_options[items_per_page]' => 2, 'pager_options[pagination_heading_level]' => 'h4'], 'Apply');
    $this->submitForm([], t('Save'));

    // Assert the page without filters.
    $this->drupalGet($path);
    $results = $this->cssSelect('.views-row');
    $this->assertCount(2, $results);
    $this->assertSession()->pageTextContains('Next');

    // Assert the page with filter in the future, one results without pager.
    $page = $this->getSession()->getPage();
    $now = \Drupal::time()->getRequestTime();
    $page->fillField($this->fieldName . '_value', DrupalDateTime::createFromTimestamp($now + 1)->format('Y-m-d H:i:s'));
    $page->pressButton('Apply');

    $results = $this->cssSelect('.views-row');
    $this->assertCount(1, $results);
    $this->assertSession()->pageTextNotContains('Next');

    // Assert the page with filter in the past, 3 results with pager.
    $page->fillField($this->fieldName . '_value', DrupalDateTime::createFromTimestamp($now - 1000000)->format('Y-m-d H:i:s'));
    $this->getSession()->getPage()->pressButton('Apply');
    $results = $this->cssSelect('.views-row');
    $this->assertCount(2, $results);
    $this->assertSession()->pageTextContains('Next');
    $page->clickLink('2');
    $results = $this->cssSelect('.views-row');
    $this->assertCount(1, $results);

  }

Any guidance would be appreciated, I'm not sure why that would not be returning 2 rows

It looks like the pages are not rendering, if you want to see them you can go here https://git.drupalcode.org/issue/drupal-3333401/-/jobs/290017/artifacts/... and look for the ones with the id 93301068.
https://issue.pages.drupalcode.org/-/drupal-3333401/-/jobs/290017/artifa...

I found the error: https://issue.pages.drupalcode.org/-/drupal-3333401/-/jobs/290017/artifa...

Drupal\Core\Config\Schema\SchemaIncompleteException: Schema errors for views.view.test_filter_datetime with the following errors: views.view.test_filter_datetime:display.default.display_options.pager.options.pagination_heading_level missing schema, 0 [display.default.display_options.pager.options.pagination_heading_level] 'pagination_heading_level' is not a supported key. in Drupal\Core\Config\Development\ConfigSchemaChecker->onConfigSave() (line 94 of core/lib/Drupal/Core/Config/Development/ConfigSchemaChecker.php).

call_user_func(Array, Object, 'config.save', Object) (Line: 111)
Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher->dispatch(Object, 'config.save') (Line: 229)
Drupal\Core\Config\Config->save() (Line: 278)
Drupal\Core\Config\Entity\ConfigEntityStorage->doSave('test_filter_datetime', Object) (Line: 486)
Drupal\Core\Entity\EntityStorageBase->save(Object) (Line: 257)
Drupal\Core\Config\Entity\ConfigEntityStorage->save(Object) (Line: 352)
Drupal\Core\Entity\EntityBase->save() (Line: 609)
Drupal\Core\Config\Entity\ConfigEntityBase->save() (Line: 1001)
Drupal\views_ui\ViewUI->save() (Line: 345)
Drupal\views_ui\ViewEditForm->save(Array, Object)
call_user_func_array(Array, Array) (Line: 129)
Drupal\Core\Form\FormSubmitter->executeSubmitHandlers(Array, Object) (Line: 67)
Drupal\Core\Form\FormSubmitter->doSubmitForm(Array, Object) (Line: 597)
Drupal\Core\Form\FormBuilder->processForm('view_edit_form', Array, Object) (Line: 325)
Drupal\Core\Form\FormBuilder->buildForm(Object, Object) (Line: 48)
Drupal\Core\Entity\EntityFormBuilder->getForm(Object, 'edit', Array) (Line: 230)
Drupal\views_ui\Controller\ViewsUIController->edit(Object, 'default')
call_user_func_array(Array, Array) (Line: 123)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 627)
Drupal\Core\Render\Renderer->executeInRenderContext(Object, Object) (Line: 121)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext(Array, Array) (Line: 97)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 181)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1) (Line: 76)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1) (Line: 37)
Drupal\Core\Test\StackMiddleware\TestWaitTerminateMiddleware->handle(Object, 1, 1) (Line: 58)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1) (Line: 48)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1) (Line: 106)
Drupal\page_cache\StackMiddleware\PageCache->pass(Object, 1, 1) (Line: 85)
Drupal\page_cache\StackMiddleware\PageCache->handle(Object, 1, 1) (Line: 48)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1) (Line: 51)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1) (Line: 51)
Drupal\Core\StackMiddleware\StackedHttpKernel->handle(Object, 1, 1) (Line: 704)
Drupal\Core\DrupalKernel->handle(Object) (Line: 19)

@tsquared212 I figured out the testing issue, this is ready for review. Please use the merge request for review.

With the schema changes will require a post_update hook in addition to a change record.

I removed the heading options from views.settings per conversation in slack with @lendude.

I am also adding a first pass at updating the pager config.
There is no test yet and the update is not working. This seems to be due to the pager returning multiple handlers and not being plural.

One schema question:
Does views_pager_sql need the pagination_heading_level setting?

Should I split processDisplayHandlerSingular or find a way to get processDisplayHandlers to work with pagers. (I would lean towards calling it process pager handlers)

This still has several items todo
1. Write a test that confirms changing the header style works
2. Investigate broken tests
3. Confirm the update test is working as expected

Now that the options are moved to the pager options the default config tests are failing in the kernel. I updated the core views to include the correct config but they are still failing.

Here is the failure:
1) Drupal\KernelTests\Config\DefaultConfigTest::testModuleConfig with data
set "user" ('user')
Exception: views.view.user_admin_people:
\Drupal\Component\Diff\Engine\DiffOpAdd::__set_state(array(
'type' => 'add',
'orig' => false,
'closing' =>
array (
0 => ' pagination_heading_level: h4',
),
))

It looks like it's getting added in core/lib/Drupal/Component/Diff/Engine/DiffOpAdd.php I haven't tracked down why though.

Ok I figured out that there were two things that were triggering the kernel failures.

The new schema shouldn't be last and there were a few items missing the config.

I've updated the MR with Kernel fixes.

I've added a test for setting the value and for checking claro, olivero and starterkit for the full pager.

I'm working on the change record.

Here is the first draft of the change record for review: https://www.drupal.org/node/3402642 →

I also rebased and fixed the multilingual tests again.

There was a js functional failure, but I think that was random / unrelated so I'm running it again.

All tests are passing

Great work on the update hook!

There's a change to a fixture that I don't believe needs to be there.

Also since this is introducing a new UI element could a screenshot be added the User interface section.

Again great work!

Thanks! I learned so much digging into this!

I assume you mean the UI section of the change record, I've added the screenshot. If you mean somewhere else let me know.

That fixture update is intentional and necessary, I left a comment on the MR and resolved, but I'll comment here too. I needed to add the new schema setting to the views in that fixture cause the test loads them and looks for changes to config globally and only allows the system mail change.

If any other changes happen then the test fails. The update hook for this change adds the pagination heading value so those are detected as changes and fail the test.

I extracted the fixture, added the schema to the views and re compressed it. I had to pull from the latest on 11.x since it was updated last week.

After taking another look there was a missing test for stark. The changes I made would work for the full pager, but I created a new assertion for stark for the mini pager.

I ran the tests locally and they passed so this should too, but I'll watch the pipeline and move to needs work if they don't pass.

Tests pass, there was a random failure on the js side again but rerunning it succeeded.

Working on this

I've added more complete testing for pager types in the post update test.

Before applying the MR I did an export of the config to compare the views. Obviously didn't have the header config changes.

Applied the MR and the update hooks ran without issue
Did another config export and all my views have the header settings now.

Using the content view for manual testing I see the default H4
Changed it in the config to H2 and that worked like a charm.

Running test-only feature shows a few failures so think test coverage is there https://git.drupalcode.org/issue/drupal-3333401/-/jobs/376849

Believe this one is good!

Great work!

Added issue credits

@andrewmacpherson 💙💙💙 thank you so much for your thorough explanation, including data to support it. This community never ceases to amaze me.

Left some comments on the MR, great work here folks!

I addressed all of larowlan's feedback and tests are all passing so putting back into needs review.

Appears feedback from @larowlan has been addressed.

Issue credits

Rebased this, will commit if passes

Thanks for all the changes @nicxvan

Committed to 11.x after rebase

I added an extra unit-test case for an invalid value and waited for a green run

diff --git a/core/modules/system/tests/src/Unit/Pager/PreprocessPagerTest.php b/core/modules/system/tests/src/Unit/Pager/PreprocessPagerTest.php
index b0fa383d800..24a12a53b47 100644
--- a/core/modules/system/tests/src/Unit/Pager/PreprocessPagerTest.php
+++ b/core/modules/system/tests/src/Unit/Pager/PreprocessPagerTest.php
@@ -158,4 +158,26 @@ public function testPaginationHeadingLevelSet() {
     $this->assertEquals('h5', $variables['pagination_heading_level']);
   }
 
+  /**
+   * Test template_preprocess_pager() with an invalid #pagination_heading_level.
+   *
+   * @covers ::template_preprocess_pager
+   */
+  public function testPaginationHeadingLevelInvalid() {
+    require_once $this->root . '/core/includes/theme.inc';
+    $variables = [
+      'pager' => [
+        '#element' => '2',
+        '#pagination_heading_level' => 'not-a-heading-element',
+        '#parameters' => [],
+        '#quantity' => '2',
+        '#route_name' => '',
+        '#tags' => '',
+      ],
+    ];
+    template_preprocess_pager($variables);
+
+    $this->assertEquals('h4', $variables['pagination_heading_level']);
+  }
+

Published the change record

Because this adds an update path, it can only be added in a minor so isn't eligible for backport.

Thanks again all.

larowlan → closed merge request !5282

Came here looking for explanation of why an additional option was added to Drupal, instead of the initial approach discussed on this issue of moving the heading text into an aria-label attribute on the parent tag, and thereby removing the Heading element altogether.

Had to dig for the answer buried in comment #30 and learned something in the process.

I've updated the issue summary with a sidenote about this point to help others not have to dig for the gold!

Thank you @andrewmacpherson; always enlightening.

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

Tagging for release note inclusion.

This change adds "layout--content-medium" to the element in the Olivero pager.html.twig file.
Not sure why it was added, but it messed up the layout of my site on a views page with pagination. I had to override the template and remove the class.

https://git.drupalcode.org/project/drupal/-/commit/197d8bae7ddcab6093d30...

Leaving a comment here instead of opening a new ticket. Not sure if this is a bug, but some one else out there might be dealing with the same problem when updating to 10.3