- Issue created by @shaal
- Status changed to Needs review
almost 2 years ago 12:48pm 17 March 2023 - 🇮🇳India gauravvvv Delhi, India
I have removed the
role="presentation"and added thearia-label="additional pages". please review - Status changed to Needs work
almost 2 years ago 2:44pm 17 March 2023 - 🇺🇸United States smustgrave
This affects all themes.
Since it covers all it should have a test.
- 🇺🇸United States smustgrave
Closed 🐛 Axe error reported on role=presentation in pagination Closed: duplicate as a duplicate
- 🇨🇦Canada mgifford Ottawa, Ontario
Thanks for closing that duplicate.
This is also being discussed here with the USWDS:
https://github.com/uswds/uswds/issues/5022 - 🇷🇺Russia Chi
-
- …
+
- …
"additional pages" needs to be translated.
AFAIK
aria-labeldoes not make sense on non-interactive elements. Most of screen readers will simply ignore it. I thinkaria-hiddenattribute would be more suitable here. - Status changed to Needs review
almost 2 years ago 4:17am 20 March 2023 The last submitted patch, 10: 3348552-10.patch, failed testing. View results →
- Status changed to Needs work
almost 2 years ago 10:58am 20 March 2023 - 🇷🇺Russia Chi
aria-hidden="additional pages"
aria-hidden is a boolean attribute
- 🇨🇦Canada mgifford Ottawa, Ontario
It is likely that the direction that the USWDS is going is to just remove the
presentationrole:
https://github.com/uswds/uswds/issues/5022#issuecomment-1476832757I don't know that hiding them will give us the desired result.
- 🇨🇦Canada mgifford Ottawa, Ontario
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?
- 🇮🇳India anchal_gupta
I have uploaded the patch
and add aria-hidden in a boolean format - Status changed to Needs review
almost 2 years ago 2:46am 4 April 2023 - 🇮🇳India gauravvvv Delhi, India
As it affects all the themes, I have made changes into all other themes. Attached interdiff for same.
- Status changed to Needs work
almost 2 years ago 2:55am 4 April 2023 - 🇺🇸United States smustgrave
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?
- 🇺🇸United States mherchel Gainesville, FL, US
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.
- 🇨🇦Canada mgifford Ottawa, Ontario
Adding WCAG SC 4.1.2 https://www.w3.org/WAI/WCAG21/Understanding/name-role-value
- First commit to issue fork.
- @neclimdul opened merge request.
- Status changed to Needs review
over 1 year ago 3:06pm 3 November 2023 - 🇺🇸United States neclimdul Houston, TX
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.
- Status changed to Needs work
over 1 year ago 4:38pm 3 November 2023 - 🇺🇸United States smustgrave
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.
- 🇺🇸United States neclimdul Houston, TX
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.
- Status changed to Needs review
over 1 year ago 2:34pm 7 November 2023 - Status changed to Needs work
over 1 year ago 2:59pm 7 November 2023 - 🇺🇸United States smustgrave
CR reads great!
Is it possible to add a simple assert somewhere. Shouldn't need it's own but could extend an existing one.
- 🇫🇮Finland lauriii Finland
I wonder if this would be caught by
core/tests/Drupal/Nightwatch/Tests/a11yTestDefault.jsif we tested a page with pager. - 🇺🇸United States neclimdul Houston, TX
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.
- 🇺🇸United States neclimdul Houston, TX
@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.
- 🇷🇴Romania claudiu.cristea Arad 🇷🇴
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.
- 🇺🇸United States neclimdul Houston, TX
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.
- Keep the content by keeping the li and making it more accessible.
- 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.
- 🇺🇸United States dmundra Eugene, OR
@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.
- 🇺🇸United States dmundra Eugene, OR
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/...
- 🇺🇸United States neclimdul Houston, TX
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?
- 🇺🇸United States dmundra Eugene, OR
@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.
- Status changed to Needs review
10 months ago 7:17pm 29 April 2024 - 🇺🇸United States neclimdul Houston, TX
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.htmlRebased and pushed test to the MR.
- Status changed to Needs work
10 months ago 9:32pm 29 April 2024 - 🇺🇸United States dmundra Eugene, OR
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 - First commit to issue fork.
- 🇫🇮Finland heikkiy Oulu
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.
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.
- 🇺🇸United States dmundra Eugene, OR
Thank you @HeikkiY. Changes look good to me.
- 🇫🇮Finland heikkiy Oulu
Rebased and all unit tests are green now. Marking as Needs review.
- 🇫🇮Finland heikkiy Oulu
Changed according to comments in the MR from @smustgrave. Changing back to Needs review.
- 🇫🇮Finland heikkiy Oulu
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.
- 🇺🇸United States dmundra Eugene, OR
@heikkiy did you try it locally to see what the issue could be when you try a sandbox site directly?
- 🇫🇮Finland heikkiy Oulu
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?
- 🇺🇸United States dmundra Eugene, OR
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.
- 🇫🇮Finland heikkiy Oulu
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.
- 🇺🇸United States mcgovernm North Carolina
@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.
- 🇫🇮Finland heikkiy Oulu
Pipelines are now working thanks to @mcgovernm. Good for review again.
- First commit to issue fork.
- 🇬🇧United Kingdom oily Greater London
Edited pager.html.twig, minor edits to comments
