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

[Bug][a11y]: mixed-value state of swatches must be conveyed to screen readers using ARIA #3298

Closed
1 task done
dineshy87 opened this issue Jun 8, 2023 · 17 comments · Fixed by #3455
Closed
1 task done
Assignees
Labels
a11y Issues related to accessibility bug Something isn't working Component: Swatch

Comments

@dineshy87
Copy link

dineshy87 commented Jun 8, 2023

Code of conduct

  • I agree to follow this project's code of conduct.

Impacted component(s)

sp-swatch

Expected behavior

Currently, mixed state is not conveyed to screen reader users. Please use aria-checked="mixed" when mixed-value is used.

Please also note that mixed value is supported by only certain tri-state inputs such as a checkbox or menuitemcheckbox.

The mixed value is not supported on radio or menuitemradio

Actual behavior

mixed-value is not communicated to screen reader users

@dineshy87 dineshy87 added bug Something isn't working triage An issue needing triage labels Jun 8, 2023
@najikahalsema najikahalsema added a11y Issues related to accessibility Component: Swatch and removed triage An issue needing triage labels Jun 9, 2023
@najikahalsema
Copy link
Collaborator

Do we only want this for the mixed-value attribute or would the nothing attribute benefit from this usage, too? https://opensource.adobe.com/spectrum-web-components/components/swatch/

@dineshy87
Copy link
Author

dineshy87 commented Jun 9, 2023

Only mixed-value.

nothing means no color or transparent. And i think that should be communicated by the label for all swatches using 'nothing'. That said, i have a question about labelling these swatches. Documentation doesn't have labels for these swatches even though the api allows label attribute. Does it make sense to use color value as the value for aria-label on swatches conditionally if label attribute is absent? If yes, let me know, I'll create a separate issue for that.

@najikahalsema
Copy link
Collaborator

Good question. I think what's become clear through this audit, as well as comments we've had in the past from other consumers, is that our label attribute is inconsistent between components. I'm currently taking inventory of all of the differences, so it's good to know that this one will be included. I think your logic makes sense though, so I would appreciate a new issue for that :) Thank you!

@dineshy87 dineshy87 changed the title [Bug][a11y]: mixed state of swatch must be conveyed to screen readers using ARIA [Bug][a11y]: mixed-value state of swatches must be conveyed to screen readers using ARIA Jun 15, 2023
@Rajdeepc
Copy link
Contributor

PR fix: #3330

@Rajdeepc Rajdeepc self-assigned this Jun 20, 2023
@najikahalsema
Copy link
Collaborator

I was looking at Rajdeep's PR, and I have some questions about fixing the issue:

Swatch currently has the role of "button" by default, not checkbox, so it will only denote aria-checked="mixed" when the Swatch is in a SwatchGroup with selection="multiple". This seems relatively specific and doesn't account for the other use cases, like a standalone swatch, a swatch that isn't selectable, or a swatch that is by itself. In the case of a single mixed-value swatch element, the screenreader only denotes that it's a button. What more information could we add to clarify what this is?

I did see you added #3320 so perhaps adding this kind of label will clarify some things in the screen reader. @Rajdeepc, let's chat tomorrow about adding this functionality.

@dineshy87
Copy link
Author

I think the intent of the mixed state should be made clear in the documentation as well. That the mixed state is meaningful with a swatchgroup that has selection="multiple". And as you said, if they're using it in other scenarios then the burden is on them to use appropriate labels to convey what they're trying to do.

@Rajdeepc
Copy link
Contributor

PR is merged and waiting for testing.

@Rajdeepc
Copy link
Contributor

Rajdeepc commented Jul 4, 2023

@dineshy87 Can you please verify this from your end and let us know?
cc @jnjosh

@Rajdeepc
Copy link
Contributor

@dineshy87 Awaiting your confirmation on this. Thank you

@dineshy87
Copy link
Author

Problem with this implementation of mixed state is that aria-mixed is not allowed on elements with button role. The best way to fix this issue is by rendering a group of checkboxes when swatch group uses selects='multiple'. And then, if one of the swatches uses mixed-value, it can then communicate the mixed state using aria-checked="mixed" Current implementation in PR will result in violations on Axe browser extension.
image

@Rajdeepc
Copy link
Contributor

@najikahalsema Let me know your thoughts on the above issue raised by Dinesh during testing. Should we track this here or on another issue?

@najikahalsema
Copy link
Collaborator

I think I'm confused now 😅 My understanding was that that aria-checked = mixed only applies in a limited circumstance, and I thought that @Rajdeepc's PR only accounted for this circumstance and didn't apply the aria attribute in circumstances where it did not apply. Based on @dineshy87's comment, it seems like he is seeing this attribute in other places besides that limited situation (ie a swatch that isn't in a swatch group), so we may need to go back and add a few more gates to the logic to make sure the attribute is only applied when the swatch is in a group where selects = multiple... So I think this issue can still be open even though it does have a solution. That solution just isn't applicable to all circumstances where the Swatch has a mixed value. Does that make sense?

@dineshy87
Copy link
Author

Let me clarify a little bit more. The problem will persist as long as the swatches have role="button" on them when selects="multiple" is used. My recommendation is to render individual swatches in swatch-group with selects="multiple" as checkboxes instead of buttons. Only then, aria-checked="mixed" will make sense.

@najikahalsema
Copy link
Collaborator

Do they not convert to checkbox state when selects = multiple? I believe what you suggest is already in the code.

@dineshy87
Copy link
Author

dineshy87 commented Jul 14, 2023

Oops sorry. It seems that they do. However, the screenshot i shared above is from documentation for swatch which has examples for mixed state that need to be updated. Because they're being used in places other than where selects="multiple" is used. And if you run Axe on the below page, you'll find those errors. https://opensource.adobe.com/spectrum-web-components/components/swatch/

I'd recommend the example be corrected to use selects='multiple'.

image

@Rajdeepc
Copy link
Contributor

@najikahalsema PR for the changes requested above.
#3455

@Rajdeepc Rajdeepc linked a pull request Jul 17, 2023 that will close this issue
13 tasks
@Rajdeepc
Copy link
Contributor

Follow up issue tracking here: #3458

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a11y Issues related to accessibility bug Something isn't working Component: Swatch
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants