-
Notifications
You must be signed in to change notification settings - Fork 191
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
fix(swatch): mixed-value state must be conveyed to screen readers using ARIA #3330
Changes from 6 commits
c627b87
7157405
97bbbdc
9361601
5915a28
970db2a
562a502
d835675
5646c0c
e1d0c2c
1848f63
91a85d1
257e22b
8a281fe
e851fd9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -229,6 +229,11 @@ export class Swatch extends SizedMixin(Focusable, { | |
this.removeAttribute('aria-label'); | ||
} | ||
} | ||
if (changes.has('mixedValue')) { | ||
if (this.mixedValue) { | ||
this.setAttribute('aria-checked', 'mixed'); | ||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hey Rajdeep! Just following up on our conversation earlier today. To add a label to the
This obviously doesn't handle situations where our value would be 'mixed' or 'nothing' (since There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @najikahalsema Thanks!! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just one thing--I'd remove the comments from the code! |
||
} | ||
|
||
protected override firstUpdated(changes: PropertyValues): void { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 withselects="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
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks