dxc-technology / halstack-react

Library of components for building SPAs with React and Halstack Design System
https://developer.dxc.com/halstack/
Apache License 2.0
15 stars 14 forks source link

Fixed contrast problems for disabled elements #1921

Closed Mil4n0r closed 5 months ago

Mil4n0r commented 6 months ago

Checklist

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

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:

Those are pending to be solved by the designers.

Related to #1899.

Mil4n0r commented 6 months ago

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 commented 6 months ago

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 commented 5 months ago

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 commented 5 months ago

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.