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

ColorPicker a11y fixes #1470

Closed
wants to merge 30 commits into from
Closed

ColorPicker a11y fixes #1470

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
8f151a8
initial push
xman343 Aug 2, 2023
cbffb4e
ColorPallete labels & roles
xman343 Aug 3, 2023
00f21c3
working fixes
xman343 Aug 4, 2023
4fc5acf
case fix
xman343 Aug 4, 2023
8449063
Merge branch 'dev' into xander/color-picker-a11y-improvements
xman343 Aug 7, 2023
630a2a2
labels removed & thumbProps implemented
xman343 Aug 8, 2023
38654b3
advanced popover toggle vis label update
xman343 Aug 8, 2023
4993641
aria-labelledby added
xman343 Aug 8, 2023
f7d6feb
color string capitalized
xman343 Aug 9, 2023
94f8b3a
Merge branch 'dev' into xander/color-picker-a11y-improvements
xman343 Aug 9, 2023
9253248
useId implemented
xman343 Aug 9, 2023
db46b62
hsl to hex
xman343 Aug 9, 2023
4dd6396
useId consolidated w/ other imports
xman343 Aug 9, 2023
7119c9e
useId update
xman343 Aug 9, 2023
3de5c17
Update packages/itwinui-react/src/core/ColorPicker/ColorBuilder.tsx
xman343 Aug 9, 2023
6a1fafb
Update packages/itwinui-react/src/core/ColorPicker/ColorBuilder.tsx
xman343 Aug 9, 2023
ee5f503
blank line restored
xman343 Aug 9, 2023
9396da6
Merge branch 'xander/color-picker-a11y-improvements' of https://githu…
xman343 Aug 9, 2023
5600955
aria-label replaced with aria-hidden
xman343 Aug 9, 2023
5c63f45
Merge branch 'dev' into xander/color-picker-a11y-improvements
xman343 Aug 9, 2023
943fc15
commented out code removed
xman343 Aug 9, 2023
4a481ac
changeset added
xman343 Aug 9, 2023
9d32b88
Merge branch 'dev' into xander/color-picker-a11y-improvements
xman343 Aug 11, 2023
31ddf48
Merge branch 'dev' into xander/color-picker-a11y-improvements
xman343 Aug 11, 2023
7f7c197
Merge branch 'dev' into xander/color-picker-a11y-improvements
xman343 Aug 11, 2023
9d44a26
Merge branch 'dev' into xander/color-picker-a11y-improvements
xman343 Aug 14, 2023
9c0df13
Merge branch 'dev' into xander/color-picker-a11y-improvements
xman343 Aug 14, 2023
74f0793
Merge branch 'dev' into xander/color-picker-a11y-improvements
xman343 Aug 14, 2023
0f75854
Merge branch 'dev' into xander/color-picker-a11y-improvements
xman343 Aug 16, 2023
f26b980
Merge branch 'dev' into xander/color-picker-a11y-improvements
xman343 Aug 16, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion examples/ColorPicker.advancedPopover.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,10 @@ export default () => {
visible={isOpen}
placement='bottom-start'
>
<IconButton onClick={() => setIsOpen((open) => !open)}>
<IconButton
r100-stack marked this conversation as resolved.
Show resolved Hide resolved
label='Color Picker Toggle Visibility'
onClick={() => setIsOpen((open) => !open)}
>
<ColorSwatch
style={{ pointerEvents: 'none', margin: 0 }}
color={selectedColor}
Expand Down
10 changes: 10 additions & 0 deletions packages/itwinui-react/src/core/ColorPicker/ColorBuilder.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -287,6 +287,11 @@ export const ColorBuilder = React.forwardRef((props, ref) => {
</Box>

<Slider
thumbProps={() => {
return {
'aria-label': `Hue slider`,
xman343 marked this conversation as resolved.
Show resolved Hide resolved
};
}}
minLabel=''
maxLabel=''
values={[sliderValue]}
Expand All @@ -305,6 +310,11 @@ export const ColorBuilder = React.forwardRef((props, ref) => {

{showAlpha && (
<Slider
thumbProps={() => {
return {
'aria-label': `Opacity slider`,
xman343 marked this conversation as resolved.
Show resolved Hide resolved
};
}}
minLabel=''
maxLabel=''
values={[alphaValue]}
Expand Down
1 change: 1 addition & 0 deletions packages/itwinui-react/src/core/ColorPicker/ColorInputPanel.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -477,6 +477,7 @@ export const ColorInputPanel = React.forwardRef((props, ref) => {
<Box className='iui-color-input'>
{allowedColorFormats.length > 1 && (
<IconButton
label='Color input'
styleType='borderless'
onClick={swapColorFormat}
size='small'
Expand Down
14 changes: 12 additions & 2 deletions packages/itwinui-react/src/core/ColorPicker/ColorPalette.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import type {
import { getColorValue } from './ColorPicker.js';
import { ColorSwatch } from './ColorSwatch.js';
import { useColorPickerContext } from './ColorPickerContext.js';

import { useId } from '../utils/index.js';
export type ColorPaletteProps = {
/**
* Label shown above the palette.
Expand Down Expand Up @@ -45,7 +45,8 @@ export type ColorPaletteProps = {
* </ColorPalette>
*/
export const ColorPalette = React.forwardRef((props, ref) => {
const { colors, label, className, children, ...rest } = props;
const uid = useId();
github-advanced-security[bot] marked this conversation as resolved.
Fixed Show resolved Hide resolved
const { colors, label, className, children, id = uid, ...rest } = props;

const { activeColor, setActiveColor, onChangeComplete } =
useColorPickerContext();
Expand Down Expand Up @@ -136,10 +137,14 @@ export const ColorPalette = React.forwardRef((props, ref) => {
<Box
className={cx('iui-color-palette-wrapper', className)}
ref={ref}
id={id}
{...rest}
>
{label && <Box className='iui-color-picker-section-label'>{label}</Box>}
<Box
role='listbox'
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@xman343 xman343 Aug 8, 2023 •
edited by mayank99
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@mayank99 mayank99 Aug 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.
a11y-allowed-attr

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

aria-labelledby={label ? id : undefined}
aria-label={!label ? 'Color palette' : undefined}
className='iui-color-palette'
onKeyDown={handleKeyDown}
ref={paletteRefs}
Expand All @@ -148,8 +153,13 @@ export const ColorPalette = React.forwardRef((props, ref) => {
{colors &&
colors.map((_color, index) => {
const color = getColorValue(_color);
const swatchLabel = getColorValue(color)
.toHslString(true)
.toUpperCase();
return (
<ColorSwatch
role='option'
aria-label={swatchLabel}
Copy link
Member

@r100-stack r100-stack Aug 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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:
image

Copy link
Member

@r100-stack r100-stack Aug 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Switched from hsl to hex

Copy link
Contributor

@mayank99 mayank99 Aug 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.
image
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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. It works!

key={index}
color={color}
onClick={(event) => {
Expand Down