Pager template fails accessibility tests for Ellipsis ("...")

I have removed the role="presentation" and added the aria-label="additional pages". please review

This affects all themes.

Since it covers all it should have a test.

Closed #3347326: Axe error reported on role=presentation in pagination as a duplicate

Thanks for closing that duplicate.

This is also being discussed here with the USWDS:
https://github.com/uswds/uswds/issues/5022

-        <li class="pager__item pager__item--ellipsis" role="presentation">&hellip;</li>
+        <li class="pager__item pager__item--ellipsis" aria-label="additional pages">&hellip;</li>

"additional pages" needs to be translated.

AFAIK aria-label does not make sense on non-interactive elements. Most of screen readers will simply ignore it. I think aria-hidden attribute would be more suitable here.

Updated #4 as per #9.

aria-hidden="additional pages"

aria-hidden is a boolean attribute

It is likely that the direction that the USWDS is going is to just remove the presentation role:
https://github.com/uswds/uswds/issues/5022#issuecomment-1476832757

I don't know that hiding them will give us the desired result.

Also, can we provide some credit to @flanneryla in the commit (when it happens) so that her contributions from the other thread can be recgnized?

I have uploaded the patch
and add aria-hidden in a boolean format

As it affects all the themes, I have made changes into all other themes. Attached interdiff for same.

Think it needs to be documented what approach was taken and why.

We discussed the aria-hidden attribute but not sure it was decided upon.

Do we need that?

The same issue also exists for all templates (even outside of Olivero). I'm seeing it in Claro, in the system module, etc. I'm willing to bet it's everywhere.

If we update this, we should update it everywhere.

Adding WCAG SC 4.1.2 https://www.w3.org/WAI/WCAG21/Understanding/name-role-value

Made a merge request with the US WDS design linked by mgifford. That seems like a logical approach to me and I couldn't any suggestion of using aria-hidden anywhere in my googling.

Huge fan of USWDS, but if that's the solution then definitely needs to be updated in issue summary that a new solution is being done.

Also will need a CR for the twig changes so contrib themes know to update theirs also.

The USWDS solution was actually still the proposed solution in the IS, the only difference was the actual content of the label. Went ahead and linked to the issue in the proposed resolution though.

CR Created.

CR reads great!

Is it possible to add a simple assert somewhere. Shouldn't need it's own but could extend an existing one.

I wonder if this would be caught by core/tests/Drupal/Nightwatch/Tests/a11yTestDefault.js if we tested a page with pager.

Sounds like a better candidate then me writing a browser test. I don't really have any much experience with the nightwatch tests and none with this test which seems to maybe have some magic I don't understand.

@laurii Took a look at the a11yTestDefault test again today and I'm really unsure what to do. It "Sounds" like the correct location because it would ensure we're testing this as standards change. I have lots of questions though because its just a list of URLs which doesn't provide a lot to work with and the core documentation doesn't seem to cover it.

• How is the site is setup? What's available to test against?
• How to affect things so that there is a URL that has enough pages to trigger the pager overflow? My guess is the site is probably empty or minimally populated so most urls with a pager won't have overflow in the pager.
• Can you somehow assert the element is on the page so the accessibility test is doing something? If we're mocking a bunch of content and then just have a url in the test file, it would be very easy to regress to the point of not testing this anymore without knowing it by removing(optimizing) content.

I'm running tests on our project with axe-core (not core's Nightwatch) and I'm getting this violation related to pager. Here's the upstream issue I've opened https://github.com/dequelabs/axe-core/issues/4247 but it's duplication of https://github.com/dequelabs/axe-core/issues/3935. In the later there's also a proposed solution:

Putting role=presentation on an li means the li is hidden, but its content is not. You should use aria-hidden="true" instead if you want it ignored.

Yeah, hidden was one of the approaches investigated earlier. The quote summarizes things pretty well.

"the li is hidden, but its content is not"

Without this weird presentation behavior, there is a fork in the road.

1. Keep the content by keeping the li and making it more accessible.
2. Hide the content with the li.

"... if you want it ignored"

Since both approaches address the issue from a test perspective, I went with the USWDS approach assuming it provided a better experience in their testing. I think the idea is the ellipsis provides information for why the numbering skips. So instead of hiding it completely, we take the first approach, keep the li and provide more information.

Perhaps it could say 'x skipped pages'

@neclimdul, I think for this pager example it would be better to create a new test file (e.g. filename a11yTestPager.js or something) that scaffolds enough content (say 5 items) and modifies the view to display 1 item at a time so you get a pager, or something like that.

* How is the site is setup? What's available to test against?

The install profile 'nightwatch_a11y_testing' is used to setup the nightwatch a11y tests in the pipeline and provides some structure to test against.

* How to affect things so that there is a URL that has enough pages to trigger the pager overflow? My guess is the site is probably empty or minimally populated so most urls with a pager won't have overflow in the pager.

Nightwatch tests have access to create nodes and users like others tests including calling setup scripts. Here is an example https://git.drupalcode.org/project/drupal/-/blob/10.2.5/core/tests/Drupa... that calls https://git.drupalcode.org/project/drupal/-/blob/10.2.5/core/tests/Drupa...

* Can you somehow assert the element is on the page so the accessibility test is doing something? If we're mocking a bunch of content and then just have a url in the test file, it would be very easy to regress to the point of not testing this anymore without knowing it by removing(optimizing) content.

Yes in a sense. Instead of testing the entire page you can just test the pager block (which I would recommend for a new test file). In this https://git.drupalcode.org/project/drupal/-/blob/10.2.5/core/tests/Drupa... example checks only the 'body' for accessibility issues.

There might be a way to include the pager test module in the nightwatch test https://git.drupalcode.org/project/drupal/-/tree/11.x/core/modules/syste... and/or build of other pager tests https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/views... or https://git.drupalcode.org/project/drupal/-/blob/11.x/core/tests/Drupal/...

Thanks! That's really helpful, I think I can take a first pass based on this.

On the last point, I think I understand I could test the pager block and that makes sense and seems like the right approach. I'm not sure that fully addresses my concern though. The ellipse could be failing but the test pass if the environment doesn't have enough content or configuration changed in a way as so the ellipse isn't rendered. Should it target the ellipse directly?

@neclimdul

On the last point, I think I understand I could test the pager block and that makes sense and seems like the right approach. I'm not sure that fully addresses my concern though. The ellipse could be failing but the test pass if the environment doesn't have enough content or configuration changed in a way as so the ellipse isn't rendered. Should it target the ellipse directly?

Ah, yes targeting the ellipse directly makes sense for the test as the test should fail if no ellipse is shown.

Got a test that's failing against core:

faddb2be4808:/app/core$ yarn test:nightwatch --test tests/Drupal/Nightwatch/Tests/a11yTestPager.js 

[Tests/A11y Test Pager] Test Suite
────────────────────────────────────────────────────────────────────
ℹ Connected to chromedriver on port 9515 (93ms).
  Using: chrome (106.0.5249.103) on LINUX.

  ℹ Loaded url http://d10.lndo.site in 259ms
  ℹ Loaded url http://d10.lndo.site/user/reset/1/1714417934/bnXISZHcCy0Vaj7yqQkhlbKvsb2pb2R8YKembDx4hMY/login
 in 384ms
  ℹ Loaded url http://d10.lndo.site/admin/modules in 381ms
  ✔ Element <form.system-modules [name="modules[pager_test][enable]"]> was visible after 528 milliseconds.
  ✔ Element <#system-modules-confirm-form> was present after 514 milliseconds.
  ✔ Element <form.system-modules [name="modules[pager_test][enable]"]:disabled> was present after 8 milliseconds.
  ℹ Loaded url http://d10.lndo.site/user/logout/confirm in 168ms

  Running pager ellipsis is accessible:
─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
  ℹ Loaded url http://d10.lndo.site/pager-test/ellipsis in 81ms
  ✔ Testing if element <.pager__item--ellipsis> is visible (20ms)
  ✔ Passed [ok]: aXe rule: aria-allowed-attr (1 elements checked)
  ✔ Passed [ok]: aXe rule: aria-allowed-role (2 elements checked)
  ✔ Passed [ok]: aXe rule: aria-conditional-attr (1 elements checked)
  ✔ Passed [ok]: aXe rule: aria-deprecated-role (2 elements checked)
  ✔ Passed [ok]: aXe rule: aria-prohibited-attr (1 elements checked)
  ✔ Passed [ok]: aXe rule: aria-required-attr (2 elements checked)
  ✔ Passed [ok]: aXe rule: aria-roles (2 elements checked)
  ✔ Passed [ok]: aXe rule: aria-valid-attr-value (1 elements checked)
  ✔ Passed [ok]: aXe rule: aria-valid-attr (1 elements checked)
  ✔ Passed [ok]: aXe rule: color-contrast (2 elements checked)
  ✔ Passed [ok]: aXe rule: duplicate-id-aria (1 elements checked)
  ✔ Passed [ok]: aXe rule: empty-heading (1 elements checked)
  ✔ Passed [ok]: aXe rule: landmark-unique (1 elements checked)
  ✔ Passed [ok]: aXe rule: link-name (3 elements checked)
  ✔ Passed [ok]: aXe rule: listitem (4 elements checked)
  ✔ Passed [ok]: aXe rule: presentation-role-conflict (1 elements checked)
  ✖ NightwatchAssertError
   aXe rule: list - <ul> and <ol> must only directly contain <li>, <script> or <template> elements
        In element: .pager__items

  FAILED: 1 assertions failed and  17 passed (385ms)

────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────

  ️TEST FAILURE (7.154s):  
   - 1 assertions failed; 20 passed

   ✖ 1) Tests/a11yTestPager

   – pager ellipsis is accessible (385ms)

   → ✖ NightwatchAssertError
   aXe rule: list - <ul> and <ol> must only directly contain <li>, <script> or <template> elements
        In element: .pager__items

 Wrote HTML report file to: /app/core/reports/nightwatch/nightwatch-html-report/index.html

Rebased and pushed test to the MR.

Thanks @neclimdul. The tests looks great. Looks like there is one minor code quality issue preventing the pipeline from running. I ran it locally though and the test is passing!

dmundra@drupal-core-web:/var/www/html/web/core$ yarn test:nightwatch --test tests/Drupal/Nightwatch/Tests/a11yTestPager.js 
Corepack is about to download https://repo.yarnpkg.com/4.1.1/packages/yarnpkg-cli/bin/yarn.js.

Do you want to continue? [Y/n] y

[Tests/A11y Test Pager] Test Suite
────────────────────────────────────────────────────────────────────
ℹ Connected to chromedriver on port 9515 (2116ms).
  Using: chrome (74.0.3729.157) on LINUX.

  ℹ Loaded url http://web in 1844ms
  ℹ Loaded url http://web/user/reset/1/1714426166/CkrAHSO_GVGNGP73wkkbB25lBkkiWpTlwMX_UER82qc/login
 in 1197ms
  ℹ Loaded url http://web/admin/modules in 1376ms
  ✔ Element <form.system-modules [name="modules[pager_test][enable]"]> was visible after 564 milliseconds.
  ✔ Element <#system-modules-confirm-form> was present after 551 milliseconds.
  ✔ Element <form.system-modules [name="modules[pager_test][enable]"]:disabled> was present after 25 milliseconds.
  ℹ Loaded url http://web/user/logout/confirm in 487ms

  Running pager ellipsis is accessible:
───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
  ℹ Loaded url http://web/pager-test/ellipsis in 279ms
  ✔ Testing if element <.pager__item--ellipsis> is visible (38ms)
  ✔ Passed [ok]: aXe rule: aria-allowed-attr (2 elements checked)
  ✔ Passed [ok]: aXe rule: aria-allowed-role (1 elements checked)
  ✔ Passed [ok]: aXe rule: aria-conditional-attr (2 elements checked)
  ✔ Passed [ok]: aXe rule: aria-deprecated-role (1 elements checked)
  ✔ Passed [ok]: aXe rule: aria-prohibited-attr (2 elements checked)
  ✔ Passed [ok]: aXe rule: aria-required-attr (1 elements checked)
  ✔ Passed [ok]: aXe rule: aria-roles (1 elements checked)
  ✔ Passed [ok]: aXe rule: aria-valid-attr-value (2 elements checked)
  ✔ Passed [ok]: aXe rule: aria-valid-attr (2 elements checked)
  ✔ Passed [ok]: aXe rule: color-contrast (2 elements checked)
  ✔ Passed [ok]: aXe rule: duplicate-id-aria (1 elements checked)
  ✔ Passed [ok]: aXe rule: empty-heading (1 elements checked)
  ✔ Passed [ok]: aXe rule: landmark-unique (1 elements checked)
  ✔ Passed [ok]: aXe rule: link-name (3 elements checked)
  ✔ Passed [ok]: aXe rule: list (1 elements checked)
  ✔ Passed [ok]: aXe rule: listitem (5 elements checked)

  ✨ PASSED. 17 assertions. (686ms)
 Wrote HTML report file to: /var/www/html/web/core/reports/nightwatch/nightwatch-html-report/index.html

The current implementation would need a review although the failing pipelines are a bit of mystery because they don't seem related to code changes in this issue. Perhaps the issue branch needs a rebase?

I did notice that the comment from #9 is not taken into account in the current implementation where it still uses aria-label and not aria-hidden. Instead of using the aria-label that the screen readers might ignore, we could perhaps also use the visually-hidden class with a span to add more context for screen readers or CSS content property as suggested by @simohell in Drupal Slack #accessibility channel.

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Thank you @HeikkiY. Changes look good to me.

Rebased and all unit tests are green now. Marking as Needs review.

Comments on MR.

Changed according to comments in the MR from @smustgrave. Changing back to Needs review.

Currently the Nightwatch test seems to fail after the latest commit. Seems a bit odd because it passed after the previous commit and the latest commit shouldn't change anything related to the test code.

Tried rerunning the Nightwatch part several times but it keeps failing.

@heikkiy did you try it locally to see what the issue could be when you try a sandbox site directly?

No, not yet. That was on my to do list to try to reproduce the problem in local outside of the pipelines. Are there any instructions how to test that in the sandbox site?

I believe you need to run 'yarn install' in the core directory and then run the command like 'yarn test:nightwatch tests/Drupal/Nightwatch/Tests/exampleA11yTest.js' to execute a single test.

I wasn't able to get it working in local but I did test it with https://github.com/justafish/ddev-drupal-core-dev.

Running ddev nightwatch tests/Drupal/Nightwatch/Tests/a11yTestPager.js seems to yield similar errors as with the pipeline Nightwatch tests.

It might be that my local test environment is not yet setup correctly for the full Nighwatch test so perhaps it would make sense for someone who has originally been able to run the tests to give this a go. It is still notable that the tests showed green before I made the most recent small out of scope change which IMO should not affect the Nightwatch tests.

@HeikkiY @smustgrave I was poking around with this locally, also using ddev-drupal-core-dev and found that my test was failing when navigating to '/pager-test/ellipsis'. I was receiving an error that the watchdog table didn't exist. I left a comment on the MR, but I believe the dependency on dblog is required. This is passing for me locally with the dependency added back.

Pipelines are now working thanks to @mcgovernm. Good for review again.

Edited the issue summary.

Edited pager.html.twig, minor edits to comments

Adding a parent to help keep track of changes for this conversion.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Sorry for the late response. Not sure how related this is to the parent unless the scope of that issue is different then the title.

Technically we're already using the … entity so which is probably better then the UTF8 character. This is issue adds information to the pager to explain what an ellipsis means in the context of the pager list. Specifically explaining what the missing information is.

What is the status of the original accessibility issue?

Axe is still throwing an issue because there is a role=presentation element as a direct child of a <ul> in the "full" format. See screenshot issue1.png

Axe is also throwing an issue about the list structure of the "mini" format. There is an <a> as a direct child of the <ul> See screenshot issue2.png

Sorry to jump-in like this but the approach taken in the "Proposed resolution" is not a real fix for this accessibility issue.

Adding aria-label to a plain <li> generally does nothing because list items without interactive or landmark roles are not typically focusable or announced individually by most of the screen readers.
With the current solution, a screen reader will just read "dot dot dot" so, if you want to communicate “there are pages in between”, don’t rely on aria-label on an <li> element. Instead, provide visually hidden text inside the element:

<li>
  <span aria-hidden="true">…</span>
  <span class="visually-hidden">Ellipsis indicating non-visible pages</span>
</li>

We had rough buy in for the previous approach including a CR documenting the change. Additionally I've been running the USWDS version of this for 2 years through several accessibility audits and its still the suggested solution provided by the USWDS.

So I think we should at least discuss before taking a completely different approach. Going to revert the current MR to the previous approach but feel free to open a different MR with the alternate approach to discuss.

That said, the exact suggestion you provided was (and it still) being discussed and so far has not been accepted.

• https://github.com/uswds/uswds/pull/5197
• https://github.com/uswds/uswds/issues/6015

Some users also suggested it made their SR experience worse https://github.com/uswds/uswds/pull/5197#issuecomment-2275809180

So unless we have a better reference for choosing the more verbose solution, I think it would be prudent to take their accepted solution and follow up if something changes.

rebased as well to fix merge conflict.

last NW was from bot so moving back to NR for discussion.

re: #61, after this patch core doesn't provide that role on anything and your li class also doesn't match those provided in core so I don't think that's related to this issue. I would check your theme. This documentation will help you narrow down what template is causing that https://www.drupal.org/docs/develop/theming-drupal/twig-in-drupal/debugg...

Changes look good to me and the tests and passing. I think the one failure is unrelated to the change.

Reading the code it looks good. We tried to test it in the A11y Office Hours Meeting, but there needs to be a new MR due to the new annotation change in Core.

not a new MR i guess a rebase should be enough.

Some links from Slack for future reference
https://design-system.service.gov.uk/components/pagination/
https://design-system.service.gov.uk/components/pagination/for-list-pages/

Moving it back to RTBC.

Pipeline had a random test failure. It's green now.

The MR is "mergeable". Does it still need a rebase?

You can always rebase it from gitlab. For larger MRs or when it gets 100+ commits back I try and rebase. But it’s at 12 and don’t think anything major changed that would need to be updated here

@smustgrave thanks.

I didn't notice before, but it looks like @neclimdul updated the fork from 11.x after #67.

So, with that and #70, I'm removing the Needs rebase tag.

I meant looks like @neclimdul updated the fork after #67 (where Needs rebase was added).

Edited my comment.

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Hi All,

I tried reading page with nvda for checking whether it is giving wrong idea about the content.
That implies Assistive technology is reading the pager correctly and rightly skipping elipsss. so what is the issue?

@drupalite1411

I tried reading page with nvda for checking whether it is giving wrong idea about the content.
That implies Assistive technology is reading the pager correctly and rightly skipping elipsss. so what is the issue?

According to the IS, the problem is that the li item has an invalid role (role="presentation").
MDN shows the valid roles for li elements: https://developer.mozilla.org/en-US/docs/Web/HTML/Reference/Elements/li

It fails WCAG SC 1.3.1 Info and Relationships (Level A): https://www.w3.org/WAI/WCAG21/Techniques/failures/F92

This failure occurs when a role of presentation is applied to an element whose purpose is to convey information or relationships in the content. Elements such as table, can convey information about the content contained in them via their semantic markup. The WAI-ARIA role of presentation on the other hand, is intended to suppress semantic information of content from the accessibility API and prevent user agents from conveying that information to the user. Use of role="presentation" for content which should convey semantic information may prevent the user from understanding that content.

Drupal aims to conform to WCAG 2.2 Level AA.

I am more inclined on the comment #62 here.
https://www.drupal.org/project/drupal/issues/3348552#comment-16318835
It is not correct to add aria-label to elipsis. As it is a non-interactive element and might be missed by some assistive technology and in that case, it will read it as dot dot dot.
Better approach would be use aria-hidden and using Ellipsis indicating non-visible pages like suggested in the comment. Any thoughts?

Per comment #74, this was failing PHPStan and was set back from RTBC. When I view the latest run, this MR is passing PHPStan tests and is marked as ready for merge. I'm going to set back to RTBC status.

There's obviously a handful of comments coming in questioning the approach still, and I believe per comment #63 it was suggested to create another MR to discuss an alternative approach, which has not been done in the almost 5 months since that comment was posted. Hoping that moving this back to RTBC could get this another look from anyone with some feedback on the appropriateness of this change to core.

There issue with stable theme as well I think we need to fix.
https://www.drupal.org/project/stable/issues/3579429

Stable is a different project. That issue needs to be fixed within that project.

dpagini I just posted that issue here just to be noticed by other maintainers.

pager.html.twig in the new Default Admin theme also needs this change.

Regarding #77:

I am more inclined on the comment #62 here.
https://www.drupal.org/project/drupal/issues/3348552#comment-16318835
It is not correct to add aria-label to elipsis. As it is a non-interactive element and might be missed by some assistive technology and in that case, it will read it as dot dot dot.
Better approach would be use aria-hidden and using Ellipsis indicating non-visible pages like suggested in the comment. Any thoughts?

It was RTBC by @mgifford (Accessibility Maintainer) in #68.

The discussion in the #accessibility Slack channel is here.