Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

USWDS - Pagination: Remove presentation role #5197

Merged
merged 7 commits into from
May 10, 2023

Conversation

mahoneycm
Copy link
Contributor

@mahoneycm mahoneycm commented Mar 23, 2023

Summary

This PR removes the role attribute from our pagination component. It also updates axe-core and axe-storybook-testing dev dependencies.

Breaking change

⚠️ This is a breaking change.

Related issue

Closes #5022

Preview link

Pagination →

Preview link:

Problem statement

WCAG 2.1 & 2.0 guidelines require ul & ol to contain only li content elements. Elements containing role=presentation are not allowed under this guideline.

Solution

Remove role from pagination component.

Testing and review

  1. Use pagination component with screen reader
  2. There should be no change in readouts from its develop counterpart
  3. Run npm test:a11y and ensure no failing tests
  4. Use axe-core browser plugin on component page to ensure no failing tests

Dependency updates

Dependency name Previous version New version
axe-core [4.3.4] [^4.6.3]
[@chanzuckerberg/axe-storybook-testing] [5.0.1] [^6.3.0]

Before opening this PR, make sure you’ve done whichever of these applies to you:

  • Confirm that this code follows the 18F Front End Coding Style Guide and Accessibility Guide.
  • Run git pull origin [base branch] to pull in the most recent updates from your base and check for merge conflicts. (Often, the base branch is develop).
  • Run npm test and confirm that all tests pass.
  • Run your code through HTML_CodeSniffer and make sure it’s error free.

Copy link
Contributor

@amyleadem amyleadem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good! I had one question about pinning the dependencies. Let me know if you had questions.

Tests completed:

  • Confirm the error in develop
  • Confirm dependency updates are most current available
  • Run npm install, npm start, npm run test:a11y without error
  • Confirm no errors in axe devTools

package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
Co-authored-by: Amy Leadem <93996430+amyleadem@users.noreply.github.com>
Copy link
Contributor

@amyleadem amyleadem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me!
Small change to the accessibility tree, but I didn't notice a change in the readout for VoiceOver.

@amycole501
Copy link

Listening in NVDA I heard on this link:
https://federalist-3b6ba08e-0df4-44c9-ac73-6fc193b0e19c.sites.pages.cloud.gov/preview/uswds/uswds/cm-pagination-remove-role/iframe.html?args=&id=components-pagination--default&viewMode=story

"list with 9 items"

while on this link
https://federalist-3b6ba08e-0df4-44c9-ac73-6fc193b0e19c.sites.pages.cloud.gov/preview/uswds/uswds/develop/iframe.html?id=components-pagination--default&viewMode=story

I heard
"List with 7 items"

I think NVDA ignores the ellipses as links which is fine as longs they aren't clickable for anyone.

They sounded the same to me in JAWS, however. They both said "7 items"

From a user perspective it's a little confusing to be on the last page in this case, 24, and still see the "next" option when there's no next page. In JAWS and NVDA it also says "page 24 last page" then "Next page". Weird but mostly confusing.

Copy link

@amycole501 amycole501 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I put some comments into the ticket; overall I didn't hear any "roles" announced but just noticed a very minor inconsistency in how the links were announced.

@mahoneycm
Copy link
Contributor Author

@amycole501 The example link is only partially a live demo. Since there are no pages to paginate through, it does not update the pagination when clicked

If you edit the controls to make the "current" page the last page, the next button is removed to avoid confusion :)

image

As for the ellipses, they should never be clickable, only used to separate the first and last page links from the current pages if they are many pages away.

@amycole501
Copy link

That helps, thanks. I think the way it's coded now is fine, it's usable and clear enough to be able to navigate and it doesn't have the "role" included which is good. I will say that JAWS does read "elipses" out loud but they aren't clickable. If we want to change that wording that's read to something clearer like "ellipses indicating non-visible pages" that might be clearer.

Copy link
Contributor

@mejiaj mejiaj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC part of the reason we added role="presentation" was to convey there are pages in between being represented by the dots.

So I like @amycole501's suggestion of adding an aria label that tells the user non-visible pages exist and are being represented by ellipsis. This not only gets rid of the error, but improves the experience.

@mahoneycm
Copy link
Contributor Author

mahoneycm commented Apr 6, 2023

@amycole501 @mejiaj

I added aria-label="ellipses indicating non-visible pages" to the ellipses <li> tag. With VoiceOver it reads the label and then "ellipses" for the span itself

Ellipses indicating non-visible pages... (press right arrow) ...ellipses

Alternatively, I could hide the ellipses from the screen reader and add a second sr-only span for the screen reader

  <li class="usa-pagination__item usa-pagination__overflow" >
    <span aria-hidden="true">
      …
    </span>
    <span class="usa-sr-only">ellipses indicating non-visible pages</span>
  </li>

This causes the screen reader to not read both the ellipses and the label.

Let me know which direction is preferred or if there is a better option that I'm unaware of!

@mejiaj mejiaj requested a review from thisisdano April 7, 2023 14:46
@thisisdano
Copy link
Member

thisisdano commented May 8, 2023

I'll draw up a changelog PR for this change.
Changelog PR: uswds/uswds-site#2096

Co-authored-by: Dan O. Williams <11464021+thisisdano@users.noreply.github.com>
@thisisdano thisisdano merged commit 6078061 into develop May 10, 2023
@thisisdano thisisdano deleted the cm-pagination-remove-role branch May 10, 2023 21:37
@thisisdano thisisdano mentioned this pull request Jun 6, 2023
@timveld
Copy link

timveld commented Aug 7, 2024

While redesigning our own pagination I came across this discussion.
I absolutely agree that you had to remove the role="presentation", this violated the aria specification.
However, I think the aria-label on the list items containing the ellipses is superfluous.

Technically, Aria permits the placement of an aria-label attribute on a li element.
However, all screen readers I tested (JAWS, NVDA, iPhone) ignore this attribute and just read "...".

Those screen readers do read an aria-label if placed on an ul element, but apparently screen reader designers agree that aria-labels on list items should be ignored and I agree with them. Naming list items just doesn't make sense semantically, and allowing authors to do that could result in terribly complicated and senseless designs which would greatly confuse users. Therefore I'd make the point that the aria specification should prohibit aria-label on list items, but this is not the right venue for that discussion. Point is that the text "ellipses indicating non-visible pages" does not describe the list item.Instead, it describes the list item's content so it should replace or somehow be linked to that content.

The aria-label is, at least from a practical point of view, superfluous as screen readers intentionally ignore it. Superfluous aria should be removed in all cases, "the first rule of aria is not to use it".

I see two alternatives:

  • Implement the second suggestion @mahoneycm made in his apr 6 comment i.e. make "..." aria-hidden and instead place the sr-only text "ellipses indicating non-visible pages". While this could help some blind users to better understand the component, it comes at the cost of reading a substantial amount of extra text which many screen reader users won't appreciate. Also, it amounts to providing a text alternative for punctuation.
  • Just remove the aria-label and assume that blind users will understand that in this context "page 1, page 2, …, page 9" means pages 3 - 8 are not currently visible. I think that's a reasonable assumption, so for now that's the design I'd go for.

But I realize that opinions may vary. @amycole501 which were your reasons for suggesting the additional text of "ellipses indicating non-visible pages"?

@amycole501
Copy link

I appreciate your insight and comments, this is a great discussion.

Given the fact that we now try to test these scenarios with AT users, I would suggest that @jaclinec add this to a testing pattern/scenario in any of our upcoming usability testing sessions.

For context: In my experience, AT users interact with ARIA differently based on their AT settings. For those who may be new to AT usage I suggested the ARIA label to add clarity as ellipses may not be an element they are familiar with much less pagination.

That said, some AT users may find it superfluous as you suggested. I made the suggestion based on an underlying need of providing clarity to the user about orientation on the page and within the website. It is still my assumption that the ARIA label does not causing harm nor prevent someone from performing a task. However that is my experience and not representative of a wider variety of participants.

Our current process is to add these suggestions or enhancements to usability testing and let our research with real world participants dictate our final decision. The rules of ARIA are fine to follow from a broad standpoint but if users do find the label helpful in this case (or in any of our testing) we would most likely recommend keeping it in tact. On the other hand if they do find it confusing or unhelpful we would of course remove it from our code and share our findings with the public for their decision-making.

@mahoneycm
Copy link
Contributor Author

Oh I do see what @timveld is saying though. It is more descriptive for the "ellipses indicating non-visible pages" to be on the ... element instead of it's parent. WIth VO, it currently reads:

"ellipses indicating non-visible pages, group, 3 of 9",
"ellipses",
"end of ellipses indicating non-visible pages"

I would agree that either of the suggested methods would likely be a better user experience than this. This feels unnecessarily verbose.

@amycole501
Copy link

I want to make sure I'm testing the same thing as when I listen to the live and test versions of pagination I don't hear anything verbose in neither JAWS nor NVDA. I'm using Chrome; I can test in other browsers, however. I just want to make sure we're comparing apples to apples. But in practice I agree that putting the ARIA on the ellipses instead of the parent makes the most sense.

@timveld
Copy link

timveld commented Aug 8, 2024

I'll split my answer up into two parts:

User panel

@amycole501 Excellent suggestion to ask your user panel! Though we don't have a formal user panel I'll also discuss this with some of our regular testers. I suggest asking them two questions:
Given a pagination component contained in a landmark labelled "pagination" and ellipses indicating non-visible pages:

  1. How would you prefer the ellipses, visually represented as "...", to be read?
    A) "..." (pronunciation will depend on your screen reader settings and speech synthesizer)
    B) "Ellipses"
    C) "... indicating non-visible pages"
    D) "ellipses indicating non-visible pages"
  2. How would you prefer the links to the pages, visually represented as the page numbers ("1", "2" and so on) to be read?
    A) "1"
    B) "page 1"

My opinion as an expert and screen reader user:

  1. A, because this use of "..." is a commonly understood textual convention, matches the visual representation and respects my preferences as reflected in my screen reader's speech settings and choice of speech synthesizer
  2. A, because the necessary context is provided by the "pagination" label on the landmark and it saves me unnecessary repetitions of the word "page" (which, on our Dutch website, would be "pagina" so it wastes even more time)

Technical

I never suggested "putting the aria on the ellipses", that would also be ignored by screen readers. We can't and don't need to use aria for providing the additional hint, sr-only text is the only way to provide it.

@mahoneycm Interesting that VoiceOver on Mac doesn't ignore the aria-label. But the way it reads the aria-label illustrates my point: semantically this text doesn't belong in an aria-label. And it also illustrates the point that there is no such thing as "superfluous / incorrect yet harmless aria". There are so many different browser and AT combinations that there will Always be one which reads things in a way we didn't expect. Aria is a double-edged sword. Only ever apply it if:

  • You are sure there is no alternative
  • You understand why you need to add it and how exactly it will support users
  • You have made sure you followed the specification
  • You have it tested by actual screen reader users

Short summary: "the first rule of aria is not to use it".

@amycole501
Copy link

@timveld Those are excellent suggestions for posing the preferences, I like how you phrased them.

I agree that we want to strike a balance of providing clear content while being respectful of people's time. That balance will vary depending on the user and their settings and AT experience. Some people new to AT may also be new to technology in general (something I am finding while participating as an observer with recent usability research panels) so we want to ensure that the ellipses convention is encountered often enough so as to not need the additional ARIA. Like you, I think ARIA should be used sparingly, so I look forward to learning more when we conduct usability research on the component with a wider base of participants.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

USWDS - Bug: Axe a11y error in pagination
6 participants