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(a11y): Multiple accessibility/quality fix #156

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

nicolasambroise
Copy link

@nicolasambroise nicolasambroise commented Apr 27, 2022

Hey !
On this pull request I do 5 modifications :

  1. Few accessibility tools report the use of "background" attribut on the lottie-player tag as a main criteria failure.
    (RGAA Criteria 10.1)
    To fix it, I remove the "background" attribut and I create a CSS var instead. (If you would like to keep it as attribut, can you rename it as data-background or something similar)
  2. I also have a warning with the use of "tabindex=0" attribut on button/input element because they are already focusable. So I remove them.
  3. I rename the class .main to .animation-wrapper because I have some conflict on many website.
  4. I add the "aria-pressed" attribut on toggle button (more info on aria-pressed)
  5. The last one is more a code quality warning with the presence of inline style. So I move them to the CSS file.

Thanks

@nicolasambroise nicolasambroise changed the title feat(a11y): Add background attribut as CSS var feat(a11y): Multiple accessibility/quality fix Apr 27, 2022
@samuelOsborne samuelOsborne changed the base branch from master to develop April 28, 2022 08:37
@samuelOsborne
Copy link
Member

Hi @nicolasambroise seeing as this would be a breaking change (removing background attribute) we need to see how we want to accommodate these changes. Cheers!

Copy link
Member

@theashraf theashraf left a comment

Choose a reason for hiding this comment

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

Hi @nicolasambroise, thanks for your PR
there are few things i just want to clarify;

Regarding the first point; as @samuelOsborne mentioned that removing the background attr would bring breaking changes, and at the current moment the background attribute is optional and maybe if you want to alter the background without using the background attr, you can embed the background with the styles passed in style attribute

 <lottie-player autoplay controls loop mode="normal" style="width: 320px; background: red;" ></lottie-player>
  1. Few accessibility tools report the use of "background" attribut on the lottie-player tag as a main criteria failure.
    (RGAA Criteria 10.1)
    To fix it, I remove the "background" attribut and I create a CSS var instead. (If you would like to keep it as attribut, can you rename it as data-background or something similar)

Awesome!!

  1. I also have a warning with the use of "tabindex=0" attribut on button/input element because they are already focusable. So I remove them.

Regarding point no. 3; i do believe that styles within shadow dom are scoped so there shouldn't be any conflicts.
Do you mind sharing an example in a codesandbox with the conflict ?

  1. I rename the class .main to .animation-wrapper because I have some conflict on many website.

Regarding point no.4; Nice catch!! do you think we should also alter the aria-label value based on the current state of the player? so if the player state isPlayer , aria-label should be 'pause' and vice versa.

  1. I add the "aria-pressed" attribut on toggle button (more info on aria-pressed)

@nicolasambroise
Copy link
Author

If you can alter the aria-label value based on the current state of the player it will be really nice !

@yananym
Copy link

yananym commented Dec 12, 2023

Hey folks, you ever were able to finalize and merge this PR?

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.

None yet

4 participants