ColorPicker a11y fixes

[xman343]

Changes

#1148 - Editing ColorPicker examples so that they meet a11y requirements.

Testing

a11y testing will be conducted.

Docs

N/A

[xman343]

Will add changelog shortly.

[mayank99]

how did you decide on using role=listbox and role=option?

[xman343]

It was the set of roles that was worked the best for the swatch array. Implementing a grid / gridcell system would have required too much refactoring. Meanwhile, giving the swatches the role of 'button' changes their appearance. 'Checkbox' didn't work for them either, because only one swatch is meant to be selected at a time.

'Listbox' / 'option' was the best structure I could find for a set of swatches. If there's another setup that you find works better, I can implement that instead.

[mayank99]

That makes sense, but previously there was no role, so I'm asking why/how you decided to add one. Which specific a11y violation is this addressing? Would have been nice if you made the PR description more... descriptive.

[xman343]

It specifically addresses the aria-input-fieldname rule, which was violated in MainExample and AdvancedExample, because the Sliders needed an accessible name.

[mayank99]

this looks unrelated, aria-input-field-name is only violated for the sliders, whereas i'm asking about the changes in ColorPalette.

[xman343]

That's totally my bad. Lot of threads to keep track of!

The listbox and option roles were added to the ColorPalette and the ColorSwatches, respectively, because without those roles, it violated the 'aria-allowed-attribute' rule. This is because the Color Swatch component originally had the aria-selected attribute, despite the component not being given a role.

Other roles that can take aria-selected - Tab, Row, and Grid - either weren't appropriate for ColorPalette & ColorSwatch, or (in the case of Grid) would have required a more extensive refactoring of the code than I figured would be necessary. (More info here.)

[mayank99]

ids should not be hardcoded like this. why do you need it?

[xman343]

There wasn't a way to set the IDs for these nodes from example.

[r100-stack]

Do you need to give an aria-label to the Slider div? Instead, I believe you can pass an aria-label to the thumbs directly (https://github.com/iTwin/iTwinUI/pull/1470/files#r1287238103). Then there is no need for an id.

[mayank99]

• ids are meant to be globally unique. Hardcoding a static id will result in every instance of this component getting the same clashing id.
• this id is getting applied on the top-level container div, so i want to reiterate my question: why do you need it?

[xman343]

I'll follow Rohan's suggestion.

[r100-stack]

Should we have a default aria-label that the ColorSwatch adds? Like swatchLabel could be calculated inside ColorSwatch. If the user provided an aria-label to ColorSwatch, we use it. But if not, we use swatchLabel as the aria-label.

[xman343]

I'd like to implement this, although it might be a bit too involved of a refactor for the scope of this PR. I'll see what I can do.

[r100-stack]

Actually, it is very small change, sorry if I didn't explain it clearly in my comment.

I meant like instead of calculating swatchLabel on L153, you can do it in ColorSwatch.tsx instead. Because you are passing the color to ColorSwatch on L159.

Then in ColorSwatch.tsx, you can do something like this:

[r100-stack]

Ohh, actually colorString (L42-48 of the screenshot) already calculates the string representation of the color. You could try reusing that instead of swatchLabel.

[xman343]

colorString return's the color's hex value, which wouldn't be very understandable. That's one of the main reasons I made swatchLabel its own thing - it returns the color's value in HSL form, which (in my experience) is easier to learn to interpret than the hex form.

[xman343]

Switched from hsl to hex

[mayank99]

please read the comment again, the suggestion is not to switch from hsl to hex, but to use colorString which preserves the source value (can be hex or hsl or whatever string the color was instantiated with)

[xman343]

I had tried using colorString for the swatch label already, but I couldn't figure out how to use it in ColorPalette.tsx. When I try using colorString in ColorSwatches.tsx, its original scope, ColorPickerBasicExample throws an aria-allowed-attribute error for the selected color swatch on top.

This is because that swatch doesn't have a role, since the roles are distributed in ColorPalette.tsx. I could try giving that swatch its own role in the examples file, but it's hard for me to tell what role is appropriate. I could also continue to assign the swatch labels the way I have been, even if it is redundant.

[mayank99]

should be fixable if you use visually-hidden text instead of aria-label

<Box
  className={cx('iui-color-swatch', { 'iui-active': isActive }, className)}
  // ...
  {...rest}
>
  <VisuallyHidden>{colorString.toUpperCase()}</VisuallyHidden>
</Box>

[xman343]

Done. It works!