Skip to content
This repository has been archived by the owner on Sep 26, 2020. It is now read-only.

Accessibility improvements #7

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

lenart91
Copy link
Contributor

Sorry for the delay on this new PR..

Couple of suggested improvements here, see what you think.

I've introduced a span element with role="img" on the 'MatCarouselSlideComponent' template. This new span element is responsible for housing the background image, whereas previously it was set on the parent div.

The 'image:string' input parameter on 'MatCarouselSlideComponent' is now of type 'MatCarouselImage'. This new interface encapsulates 'url' and 'altText' members. If 'altText' has a value, then an 'aria-label' attribute is set on the span element, in order to describe the image it houses.

Finally, 'aria-label' attributes have been set on the arrow and mini-fab buttons found in the 'MatCarouselComponent' template.

I hope this is of some value.

Cheers.

@ghost
Copy link

ghost commented May 27, 2019

Hello,

Any plan to move this PR forward or anything that could be done to help? We are planning to use the carousel in a project that need to get to AA level of accessibility so it looks like the PR would improve the carousel in that regard.

Thanks,
Eric

@gbrlsnchs
Copy link
Owner

Hi, this might be merged this week if everything is fine. I have finally taken a week off from my job and will happily review everything pending in this library. Sorry for the delay.

@ghost
Copy link

ghost commented May 27, 2019

Great, thanks for the confirmation!
I'll do my best to report issues and do PR too if I end fixing something.

Have a nice vacation and thanks for sharing material2-carousel.

@ghost
Copy link

ghost commented May 29, 2019

Hello,

Just a quick feedback.

I have played a bit more with the Carousel in our app and while is doing alot of things right there would be quite a few things to change / improve to get to AA level of accessibility (our project have that requirement) so we have decided to take more time to consider the options (investing time to improve the carousel or considering another option without carousel (more likely at this point). We have another project that could need a carousel so maybe we will come back later!

ps : I have used that article as a reference : https://www.w3.org/WAI/tutorials/carousels/

Eric

@gbrlsnchs gbrlsnchs added the enhancement New feature or request label Jun 3, 2019
@gbrlsnchs gbrlsnchs added this to the Launch stable version milestone Jun 3, 2019
@gbrlsnchs gbrlsnchs added help wanted Extra attention is needed needs investigation This issue needs further investigation labels Jun 3, 2019
@gbrlsnchs
Copy link
Owner

@eric-gagnon You're right, after reading the resource you posted, I realized there are missing features regarding a11y. I'll check https://material.angular.io/cdk/a11y/overview to see what I can do. a11y is a must and should be my focus until v1 is released.

@ghost
Copy link

ghost commented Jun 11, 2019

Hello!

We have discussed about this more here. We have people that still strongly want a carousel, so we cannot easily dismiss it! The problem that we also have is with the understanding of the level that we need to get to. As a public organisation, we have a law (in Québec, Canada) that is simular to Section 508 in US.

The problem with conformance is that we should aim for no reported issues (automatic and manual testing) but we can argue that a web site is still accessibible if there is a few titles missings here and there. It's a simple exemple, but some widget can require a bit more work, like the carousel. Some rules are more difficult to do right than it seems (ex. supporting 200% text zoom with text and UI in fixed space).

Currently the decision is to keep the carousel as it is (so our site will not be conformant just from that) with few adjustments that could be done without changing the code from the inside but it's possible that we decide later to put the required work to make it accessibile (could decide to disable animation instead of adding a pause button to simplify by ex.).

To my best undestanding (by testing with Carousel in our app and checking the "good" exemple with the explanation of w3c) I noted something like 13 issues more or less related to accessibility. I didnt want to report them in issue section as you have no obligation to support A and AA but I can give you a idea of what I have found :

  • Swipe problem on mobile (does not always work as expected (length of swipe required?).
  • Animation in conflit with manual page change (timer should be disabled / reset when there is a manual page chage + should probably disabled on rollover (animation should be disabled on mobile as rollover hint is impossible).
  • (not accessibility related) Image are not lazy loaded.
  • (not accessibility related) I had a bug where the display was shaking (maybe a conflict with our display grid, I was not able to reproduce it).
  • (not accessibility related) The carousel seem to be fixed by width by default, we needed fixed by heigh (with breakpoints).
  • Element position of UI / display title action and action button : zoom text 200% not working so well have some issues.
  • Keyboard navigation is not intuitive and missing focurs (main container). I think it would be better to have the focus on arrow + enter instead of navigation with arrow. I don't like that the sample of W3C goes to slide content before arrow but maybe there is a rationale behind this... would surely take the time to think about this more before changing anything!
  • Change of slide should be advertised using aria role (implemented in w3c demo).
  • On some image, the provided widget breaks the contrast rules (ex. black navigation arrow over a black and white photo).
  • Reading by screen reader seem difficult to understand (quick assesment, would need to double check that).
  • Empty buttons (detected by Wave extension).
  • (?) I dont remember if localisation was supported but we would need accessibility labels / titles to be in our language.

If we decide to put some work on that later there would surely be more verifications / discussions to minimise to work to be done. I'll consider a pull request if we move forward and can improve without doing too agressive changes that could break where you would like to bring your project.

@gbrlsnchs
Copy link
Owner

gbrlsnchs commented Jun 11, 2019

Hi @eric-gagnon, again, thanks for your feedback. I'm glad you could list those issues, as I would not have been able to do so myself. Unfortunately, front-end is not my specialty, what means my pace may be slow for fixing those issues.

However, I do believe they are important and I truly want to fix them. I would be glad if you could open an issue listing those items in detail, so we could break them into small issues or PRs later. You could split them into two issues if desired, one for a11y and the other one for bugs.

Also, I'm very open to PRs, if you guys have fixes to suggest, please, don't hesitate to send PRs! Or even code fix snippets (and then I know where to start when fixing issues myself).

@ghost
Copy link

ghost commented Jun 11, 2019

Ok, great! Thanks @gbrlsnchs!

@ralftar
Copy link

ralftar commented Apr 26, 2020

Is this project stalled?

@gbrlsnchs
Copy link
Owner

Is this project stalled?

I'm currently unable to prioritize this project over my other open source ones, unfortunately.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request help wanted Extra attention is needed needs investigation This issue needs further investigation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants