fix(swatch): mixed-value state must be conveyed to screen readers using ARIA #3330
Conversation
Rajdeepc
added
bug
packages/swatch/README.md
Outdated
| @@ -116,6 +118,7 @@ import { Swatch } from '@spectrum-web-components/swatch'; | |||
| rounding="full" | |||
| shape="rectangle" | |||
| mixed-value | |||
| aria-checked="mixed" | |||
There was a problem hiding this comment.
The internal logic should be enough to cover this delivery. Adding it manually here could confuse consumers as to their responsibilities with this pattern. If for some reason these are not getting the attribute at runtime after the below change, then that’s a sign there’s more work to do.
There was a problem hiding this comment.
Make sense. Thank you
Tachometer resultsChromeswatch permalink
Firefoxswatch permalink
|
| if (this.mixedValue) { | ||
| this.setAttribute('aria-checked', 'mixed'); | ||
| } | ||
| } |
There was a problem hiding this comment.
Hey Rajdeep! Just following up on our conversation earlier today.
To add a label to the Swatch that matches the color attribute, you can add the following in the Swatch code's willUpdate function:
if (changes.has('label')) {
// if there is a colour set,
if (this.color !== '') {
this.setAttribute('aria-label', this.color); // set the aria-label to be this.color
// otherwise, if the label is different from the colour and is a length longer than 0,
} else if (this.label !== this.color && this.label.length) {
// override the label with this.label
this.setAttribute('aria-label', this.label);
}
else {
this.removeAttribute('aria-label');
}
}
This obviously doesn't handle situations where our value would be 'mixed' or 'nothing' (since color='' in that situation), but it's better than no label at all. Hopefully, we can get some guidance on @dineshy87 's side as to what kind of aria-label would be best for those situations, too. 😄 Depending on how many different types of labels we need, we may want to abstract this out of willUpdate().
There was a problem hiding this comment.
@najikahalsema Thanks!!
There was a problem hiding this comment.
Just one thing--I'd remove the comments from the code!
| if (this.mixedValue) { | ||
| this.setAttribute('aria-checked', 'mixed'); | ||
| } | ||
| } |
There was a problem hiding this comment.
Just one thing--I'd remove the comments from the code!
packages/swatch/README.md
Outdated
| <sp-swatch mixed-value></sp-swatch> | ||
| <sp-swatch mixed-value rounding="full"></sp-swatch> | ||
| <sp-swatch mixed-value shape="rectangle"></sp-swatch> | ||
| <sp-swatch mixed-value aria-checked="mixed"></sp-swatch> |
There was a problem hiding this comment.
I believe that westbrook touched on this before, but you shouldn't need to add aria-checked="mixed" here since it's doing it now (you would have to add selects="multiple" to the swatch group, though)? Or maybe you could say something like "Please note that the aria-checked="mixed" value only applies when the swatch is in a group with selects="multiple".
I believe that even if you add it manually, like you have here, it still won't display on the screen reader because the role is still button.
packages/swatch/src/Swatch.ts
Outdated
| @@ -223,12 +223,19 @@ export class Swatch extends SizedMixin(Focusable, { | |||
| ); | |||
| } | |||
| if (changes.has('label')) { | |||
| if (this.label) { | |||
| if (this.color !== '') { | |||
There was a problem hiding this comment.
I think you placed this logic in the wrong order and it's what's causing the test failures in CI right now. Can you take a look at those?
There was a problem hiding this comment.
Should be good now except for the fact that few NumberFields tests are failing. Switching the order of if else did the trick!
Thanks
|
@Westbrook @najikahalsema Let me know if this PR looks good to merge? |
Westbrook
changed the title
Description
Added a logic to check for mixedValue property and add aria-checked="mixed"
Related issue(s)
Motivation and context
Currently, mixed state is not conveyed to screen reader users. Please use aria-checked="mixed" when mixed-value is used.
How has this been tested?
Screenshots (if appropriate)
Types of changes
Checklist
Best practices
This repository uses conventional commit syntax for each commit message; note that the GitHub UI does not use this by default so be cautious when accepting suggested changes. Avoid the "Update branch" button on the pull request and opt instead for rebasing your branch against
main.