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

Fixed contrast problems for disabled elements #1921

Closed
wants to merge 12 commits into from

Conversation

Mil4n0r
Copy link
Collaborator

@Mil4n0r Mil4n0r commented Mar 22, 2024

Checklist

  • Build process is done without errors. All tests pass in the /lib directory.
  • Self-reviewed the code before submitting.
  • Meets accessibility standards.
  • Added/updated documentation to /website as needed.
  • Added/updated tests as needed.

Description
WCAG AA indicates that contrast should be at least 4.5:1 EXCEPT FOR:

  • Large text
  • Incidental content
  • Logotypes

There were some disabled content that was not tagged with aria-disabled={true}, that's the reason why it was being registered as a color-contrast issue in the accessibility automatic testing.

Now it is fixed, leaving only the following contrast issues to be solved:

  • Date Input (Numbers for previous and next month are too light so they don't reach the minimum contrast and they should not be tagged as disabled).
  • StatusLight (Info, warning and success are below 4:1 contrast).
  • Tabs (When multiple Tabs are active, the ones that are not currently selected have a gray color that does not meet minimum contrast).

Those are pending to be solved by the designers.

Related to #1899.

@Mil4n0r Mil4n0r changed the title Fixed accessibility problems for disabled texts Fixed contrast problems for disabled elements Mar 22, 2024
@Mil4n0r Mil4n0r marked this pull request as ready for review April 2, 2024 06:07
@GomezIvann GomezIvann self-requested a review April 2, 2024 07:10
@GomezIvann GomezIvann self-assigned this Apr 2, 2024
Copy link
Collaborator

@GomezIvann GomezIvann left a comment

Choose a reason for hiding this comment

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

I do not agree with these changes. aria-disabled should be placed on the operable element that can be disabled (or on the container of all the operable elements). RadioGroupContainer is not the case because not all of its descendants are focusable (label and helper text are only visual).

I understand why you make this change but I do not know for sure if it is a good use of aria-disabled.

image

@Mil4n0r
Copy link
Collaborator Author

Mil4n0r commented Apr 3, 2024

I do not agree with these changes. aria-disabled should be placed on the operable element that can be disabled (or on the container of all the operable elements). RadioGroupContainer is not the case because not all of its descendants are focusable (label and helper text are only visual).

I understand why you make this change but I do not know for sure if it is a good use of aria-disabled.

image

That's a good point. However, according to WCAG I don't seem to find a more suitable way to cover this scenario. Because setting the helper text as decorative seems like an even worse solution

image

As stated in the PR, this list doesn't cover the case for DateInput's previous/next month days interactuable buttons inside the calendar either, so I am not sure about what approach should be followed instead.

@GomezIvann
Copy link
Collaborator

GomezIvann commented Apr 4, 2024

I do not agree with these changes. aria-disabled should be placed on the operable element that can be disabled (or on the container of all the operable elements). RadioGroupContainer is not the case because not all of its descendants are focusable (label and helper text are only visual).
I understand why you make this change but I do not know for sure if it is a good use of aria-disabled.
image

That's a good point. However, according to WCAG I don't seem to find a more suitable way to cover this scenario. Because setting the helper text as decorative seems like an even worse solution

image

As stated in the PR, this list doesn't cover the case for DateInput's previous/next month days interactuable buttons inside the calendar either, so I am not sure about what approach should be followed instead.

But, I don't think that a visual problem, regarding accessibility, should be solved using arias. If our designers consider that this color is the color we want, maybe we need to disable this check (I don't know if that is possible).

As I stated before, aria-disabled is used for interactive elements that can be disabled. And that case doesn't suit our label and helper text...

@Mil4n0r Mil4n0r marked this pull request as draft April 5, 2024 12:11
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have yet to validate some of these changes with the Design team.

@Mil4n0r
Copy link
Collaborator Author

Mil4n0r commented Apr 5, 2024

Currently facing a strange problem. test-storybook is targetting .test.js files for some reason when it should be targetting only the stories.tsx ones (according to the config file inside .storybook/main.ts). However this problem only occurs in the GitHub flow, I can't replicate it in local.

EDIT: It was a confusion caused by a problem in my end when loading the storybook, after refreshing there is no longer differences in the execution between localhost and CI/CD. Sorry about that.

@Mil4n0r
Copy link
Collaborator Author

Mil4n0r commented Apr 10, 2024

Lara thinks that it is not appropiate to make these changes to meet contrast rules until the new core tokens are defined, I think that totally makes sense as we would have to do the work twice.

I will keep this closed for now.

@Mil4n0r Mil4n0r closed this Apr 10, 2024
@Mil4n0r Mil4n0r added the help wanted Extra attention is needed label Apr 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants