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

Proposed changes based on ada feedback #116

Merged
merged 7 commits into from
Mar 7, 2023
Merged

Proposed changes based on ada feedback #116

merged 7 commits into from
Mar 7, 2023

Conversation

mattaebersold
Copy link
Contributor

@mattaebersold mattaebersold commented Jan 24, 2023

@weotch pushed this up as a draft so you can see what I'm trying to do. I got some accessibility feedback regarding the carousel for COR...

  1. The label of the dots weren't descriptive of their action, for example, they say 'Slide 1', and they wanted them to state the action, like "Go to slide 1" so the screen reader tells the user what's going to happen.

PROBLEM:
The accessible name of the carousel buttons "Slide 1" and "Slide 2" doesn't clearly describe its function or purpose.
Note: Instances of this issue were discovered. Please refer to the screenshots for details.

SOLUTION:
When a user reaches the button, there should be enough information announced for the user to understand the purpose of the button.
Change aria-label attribute values within the
tags.
For example: "Slide 1" and "Slide 2".
Note that ARIA should only be used if the button cannot be sufficiently labeled using HTML only.


  1. They asked for a visually hidden text element when the button is selected. You can see my attempt at this, let me know if you have any thoughts.

PROBLEM:
The screen reader doesn't announce the current slide button with selected state but rather announcing incorrectly with disabled state.

SOLUTION:
Make sure that when the carousel button is currently active, it is programmatically exposed.
To fix this issue, do the following:

  • Remove the unnecessary disabled attribute from inside the <button> tag.
  • Add visually hidden text such as "selected" within the element having class="visuallyhidden" inside the <button> elements and use the javascript to define the currently active state of buttons using the class="visuallyhidden".

@mattaebersold mattaebersold requested a review from weotch January 24, 2023 22:58
@codesandbox
Copy link

codesandbox bot commented Jan 24, 2023

CodeSandbox logoCodeSandbox logo  Open in CodeSandbox Web Editor | VS Code | VS Code Insiders

@mattaebersold
Copy link
Contributor Author

@weotch the cypress test failed because the aria-label on navigating by dot was different now that I updated the language of the label. Just wanted to let you know why that failed. If you are OK with these changes, then it should be OK now.

@cypress
Copy link

cypress bot commented Jan 25, 2023

Passing run #46 ↗︎

0 18 0 0 Flakiness 0

Details:

Fixing test
Project: vue-ssr-carousel Commit: 2fd6b59909
Status: Passed Duration: 02:36 💡
Started: Feb 24, 2023 12:46 AM Ended: Feb 24, 2023 12:49 AM

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

src/ssr-carousel-dots.vue Outdated Show resolved Hide resolved
src/ssr-carousel-dots.vue Outdated Show resolved Hide resolved
Copy link
Member

@weotch weotch left a comment

Choose a reason for hiding this comment

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

@mattaebersold I made the following changes to your initial work

  • I got rid of the visually-hidden stuff. When I tested it using MacOS Voiceover, it never read the "Selected" bit anyway
  • I replaced all the disabled attributes with aria-disabled. That does seem like a smart change, it was frustrating when navigating by keyboard and like the "next" button would lose focus when you got to the last page. This had the side effect of the screen reader announcing that the button was "dimmed" which I think accomplishes the goal of the "Selected" label
  • I added the pagination-label prop so someone could be more specific with what the labels are

This video shows the cumulative effect of these changes, it feels pretty accessible to me.

Aria.labels.mov

If you agree, I'll tag a new version. I think it should be major version release because the removal of the [disabled] selector could break styling on sites.

@weotch weotch merged commit 4cf5ecc into main Mar 7, 2023
@weotch weotch deleted the ada-updates branch March 7, 2023 22:32
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.

2 participants