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

Accessibility issues with the announcements carousel #19

Open
NateWr opened this issue Jun 5, 2019 · 7 comments
Open

Accessibility issues with the announcements carousel #19

NateWr opened this issue Jun 5, 2019 · 7 comments

Comments

@NateWr
Copy link
Contributor

NateWr commented Jun 5, 2019

The carousel used for the announcements (see this demo) has a couple issues that probably need to be addressed for accessibility.

  1. When using the keyboard to focus on the prev/next buttons and then hitting enter, focus is dropped. It needs to stay on the same prev/next button. It looks like this is the case with the default bootstrap4 carousel.

  2. There's no indication of what the buttons control. I'm not sure what the correct approach is here. With the pagination component I've used aria-live="polite". But I think that only works because the content is being replaced. Since the carousel works by selectively showing/hiding things, I'm not 100% sure that aria-live="polite" will signal the change to the user. In this inclusive carousel component article he suggests using something like this:

<div role="region" aria-label="Announcements" aria-describedby="instructions">
  <!-- slides -->
  <div id="instructions">
    <!-- prev/next buttons -->
  </div>
</div>

I'm not totally sure that nails our use case, and sometimes poorly applied aria can be worse than no aria, but I'm not aware of a better alternative here. These kinds of controls are tricky.

Of course, the other option is to get rid of the carousel and simply put one announcement up with a "View All Announcements" link.

@sssoz
Copy link
Contributor

sssoz commented Jul 3, 2019

I thought about simply displaying a "View all announcements" link, but don’t users get to select the number of announcements they want to display on the homepage? I am not sure I want to mess too much with that -- unless we can completely remove that option for this theme.

@NateWr
Copy link
Contributor Author

NateWr commented Jul 3, 2019

Yeah they do get to choose. (One of many settings I'd like to turn into a theme option, but that's for the future...) I was just offering an easy escape route. :)

@sssoz
Copy link
Contributor

sssoz commented Jul 3, 2019

Actually the fix was much easier than I thought -- just need to move the controls outside the carousel’s slide content! Fixed in ca197ad

@NateWr
Copy link
Contributor Author

NateWr commented Jul 4, 2019

Re-opening this issue because the second point still needs to be addressed.

@NateWr NateWr reopened this Jul 4, 2019
@sssoz
Copy link
Contributor

sssoz commented Jul 4, 2019

@NateWr is the sr-only class in a span indicating "next" and "previous" not clear enough? Currently this is what the theme should be displaying for an arrow button:

<a href="#announcementsCarouselControls" class="btn" role="button" data-slide="next">
	<span aria-hidden="true"></span>
	<span class="sr-only">Next</span>
</a>

@NateWr
Copy link
Contributor Author

NateWr commented Jul 4, 2019

The next/previous buttons look good! 👍 I'm referering specifically to the role="region" aria-label="Announcements" aria-describedby="instructions" attributes that link the controls to the content in the code snippet I posted at the start. Let me know if it's not clear what's happening there.

@thinkbulecount2
Copy link

Hi @sssoz ! Am I here for further UI/UX Review?

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

No branches or pull requests

4 participants