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.

Sign up for GitHub

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

Jump to bottom

fix(swatch): mixed-value state must be conveyed to screen readers using ARIA #3330

Merged
merged 15 commits into from
Jun 27, 2023
Merged

fix(swatch): mixed-value state must be conveyed to screen readers using ARIA #3330

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
c627b87
fix: mixed value state of swatch support aria
Jun 20, 2023
7157405
chore: added test to support mixed-value for aria
Jun 20, 2023
97bbbdc
chore: updated readme
Jun 20, 2023
9361601
fix: removed aria checked from readme
Jun 21, 2023
5915a28
Merge branch 'main' into bug/aria-mixed-value
Jun 21, 2023
970db2a
Merge branch 'main' into bug/aria-mixed-value
Jun 22, 2023
562a502
fix: aria-label fix for swatch in screen reader
Jun 23, 2023
d835675
Merge branch 'main' into bug/aria-mixed-value
Jun 23, 2023
5646c0c
fix: aria-label fix for swatch in screen reader
Jun 23, 2023
e1d0c2c
chore: added tests
Jun 23, 2023
1848f63
chore: updated readme and removed comments
Jun 26, 2023
91a85d1
Merge branch 'main' into bug/aria-mixed-value
Jun 26, 2023
257e22b
fix: tests
Jun 26, 2023
8a281fe
Merge branch 'main' into bug/aria-mixed-value
Jun 27, 2023
e851fd9
Merge branch 'main' into bug/aria-mixed-value
najikahalsema Jun 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions packages/swatch/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,6 +208,8 @@ The `mixed-value` attribute and `mixedValue` property outline when an `<sp-swatc
</sp-swatch-group>
```

Please note that the `aria-checked="mixed"` value only applies when the swatch is in a group with `selects="multiple"`

### Nothing

The `nothing` attribute/property outlines that the `<sp-swatch>` represents no color or that it represents "transparent".
Expand Down
9 changes: 8 additions & 1 deletion packages/swatch/src/Swatch.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,12 +223,19 @@ export class Swatch extends SizedMixin(Focusable, {
);
}
if (changes.has('label')) {
if (this.label) {
if (this.color !== '') {
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be good now except for the fact that few NumberFields tests are failing. Switching the order of if else did the trick!
Thanks

this.setAttribute('aria-label', this.color);
} else if (this.label !== this.color && this.label?.length) {
this.setAttribute('aria-label', this.label);
} else {
this.removeAttribute('aria-label');
}
}
if (changes.has('mixedValue')) {
if (this.mixedValue) {
this.setAttribute('aria-checked', 'mixed');
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

The 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 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().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@najikahalsema Thanks!!

Screenshot 2023-06-23 at 3 41 23 PM

Copy link
Collaborator

Choose a reason for hiding this comment

The 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 {
Expand Down
5 changes: 5 additions & 0 deletions packages/swatch/test/swatch.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,11 @@ describe('Swatch', () => {
it(`loads default swatch accessibly`, async () => {
await expect(el).to.be.accessible();
});
it('loads [mixed-value] swatch accessibly', async () => {
el.mixedValue = true;
await expect(el).to.be.accessible();
expect(el.getAttribute('aria-checked')).to.equal('mixed');
});
it('loads [nothing] swatch accessibly', async () => {
el.nothing = true;
el.removeAttribute('color');
Expand Down