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

feat(carousel): accessibility #3773

Merged
merged 1 commit into from
Jul 10, 2020

Conversation

divdavem
Copy link
Member

@divdavem divdavem commented Jun 15, 2020

This PR adds accessibility to the carousel:

  • the carousel is now automatically paused when the focus is inside it (this can be controlled with the pauseOnFocus property)
  • some roles and aria attributes are added to make it easier to use the carousel with a screen reader. This mostly relies on the carousel indicators that act like tabs.

In the future, it would be nice to integrate assistive-webdriver to execute end-to-end tests with a screen reader.

Before submitting a pull request, please make sure you have at least performed the following:

  • read and followed the CONTRIBUTING.md and DEVELOPER.md guide.
  • built and tested the changes locally.
  • added/updated any applicable tests.
  • added/updated any applicable API documentation.
  • added/updated any applicable demos.

Fixes #3755

@codecov-commenter
Copy link

Codecov Report

Merging #3773 into master will decrease coverage by 0.05%.
The diff coverage is 86.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3773      +/-   ##
==========================================
- Coverage   91.23%   91.18%   -0.06%     
==========================================
  Files         100      100              
  Lines        2988     3016      +28     
  Branches      555      562       +7     
==========================================
+ Hits         2726     2750      +24     
  Misses        192      192              
- Partials       70       74       +4     
Flag Coverage Δ
#e2e 55.90% <20.00%> (-0.33%) ⬇️
#unit 87.73% <86.66%> (-0.02%) ⬇️
Impacted Files Coverage Δ
src/carousel/carousel.ts 95.61% <86.20%> (-3.24%) ⬇️
src/carousel/carousel-config.ts 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a95c910...c659989. Read the comment docs.

src/carousel/carousel.ts Outdated Show resolved Hide resolved
src/carousel/carousel.ts Outdated Show resolved Hide resolved
src/carousel/carousel.ts Show resolved Hide resolved
@divdavem divdavem force-pushed the carouselAccessibility branch 2 times, most recently from 753cf47 to d64e913 Compare June 17, 2020 09:45
@divdavem divdavem requested a review from benouat June 17, 2020 12:29
src/carousel/carousel.ts Outdated Show resolved Hide resolved
src/carousel/carousel.ts Show resolved Hide resolved
@divdavem divdavem requested a review from benouat June 30, 2020 11:54
@benouat
Copy link
Member

benouat commented Jul 1, 2020

I still have one consideration regarding all these changes.

I was having a look a the first demo, if you click on previous/next arrow, I assume that focus will be somewhere caught inside the widget, but there is no visual hint such as an outline. Hence the carousel gets stuck in pause mode, and for the end user you don't know why...

It's not very intuitive that you have to click elsewhere for the carousel to resume.

@divdavem divdavem force-pushed the carouselAccessibility branch 6 times, most recently from 0da4219 to 9dc12de Compare July 9, 2020 08:57
Copy link
Member

@benouat benouat left a comment

Choose a reason for hiding this comment

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

LGTM! I just left a couple nitpick comments.

src/carousel/carousel.ts Outdated Show resolved Hide resolved
src/carousel/carousel.ts Outdated Show resolved Hide resolved
@divdavem
Copy link
Member Author

divdavem commented Jul 9, 2020

@benouat Thank you for your review and your comments. I have updated the code accordingly.

@benouat benouat merged commit 6830d55 into ng-bootstrap:master Jul 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Carousel accessibility
3 participants