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

fix(Switch): updated a11y by removing dynamic labeling #10646

Merged
merged 8 commits into from
Jul 3, 2024
Merged

fix(Switch): updated a11y by removing dynamic labeling #10646

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
4e14d21
fix(Switch): updated a11y by removing dynamic labeling
thatblindgeye Jun 24, 2024
cc6c3f8
Updated snapshots
thatblindgeye Jun 24, 2024
3b905ef
Removed remaining labelOff props in examples/demos
thatblindgeye Jun 24, 2024
42660ee
Updated prop description
thatblindgeye Jun 24, 2024
b232b80
Updated logic for applying arialabelledby attribute
thatblindgeye Jun 25, 2024
b93307b
Added docs verbiage for new a11y
thatblindgeye Jul 2, 2024
74efdfd
Updated yarn lock
thatblindgeye Jul 2, 2024
f8f28c8
Updated tests and verbiage
thatblindgeye Jul 3, 2024
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
36 changes: 12 additions & 24 deletions packages/react-charts/src/components/Skeletons/examples/skeletons.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,7 @@ export const ChartAreaSkeleton: React.FunctionComponent = () => {
<>
<Switch
id="area-skeleton-switch"
label="Skeleton on"
labelOff="Skeleton off"
label="Enable skeleton"
isChecked={isChecked}
onChange={handleChange}
/>
Expand Down Expand Up @@ -131,8 +130,7 @@ export const ChartBarSkeleton: React.FunctionComponent = () => {
<>
<Switch
id="bar-skeleton-switch"
label="Skeleton on"
labelOff="Skeleton off"
label="Enable skeleton"
isChecked={isChecked}
onChange={handleChange}
/>
Expand Down Expand Up @@ -188,8 +186,7 @@ export const ChartBoxPlotSkeleton: React.FunctionComponent = () => {
<>
<Switch
id="boxplot-skeleton-switch"
label="Skeleton on"
labelOff="Skeleton off"
label="Enable skeleton"
isChecked={isChecked}
onChange={handleChange}
/>
Expand Down Expand Up @@ -246,8 +243,7 @@ export const ChartBulletSkeleton: React.FunctionComponent = () => {
<>
<Switch
id="bullet-skeleton-switch"
label="Skeleton on"
labelOff="Skeleton off"
label="Enable skeleton"
isChecked={isChecked}
onChange={handleChange}
/>
Expand Down Expand Up @@ -300,8 +296,7 @@ export const ChartDonutSkeleton: React.FunctionComponent = () => {
<>
<Switch
id="donut-skeleton-switch"
label="Skeleton on"
labelOff="Skeleton off"
label="Enable skeleton"
isChecked={isChecked}
onChange={handleChange}
/>
Expand Down Expand Up @@ -339,8 +334,7 @@ export const ChartDonutUtilizationSkeleton: React.FunctionComponent = () => {
<>
<Switch
id="donut-utilization-skeleton-switch"
label="Skeleton on"
labelOff="Skeleton off"
label="Enable skeleton"
isChecked={isChecked}
onChange={handleChange}
/>
Expand Down Expand Up @@ -388,8 +382,7 @@ export const ChartDonutUtilizationSkeleton: React.FunctionComponent = () => {
<>
<Switch
id="donut-utilization-threshold-skeleton-switch"
label="Skeleton on"
labelOff="Skeleton off"
label="Enable skeleton"
isChecked={isChecked}
onChange={handleChange}
/>
Expand Down Expand Up @@ -432,8 +425,7 @@ export const ChartLineSkeleton: React.FunctionComponent = () => {
<>
<Switch
id="line-skeleton-switch"
label="Skeleton on"
labelOff="Skeleton off"
label="Enable skeleton"
isChecked={isChecked}
onChange={handleChange}
/>
Expand Down Expand Up @@ -522,8 +514,7 @@ export const ChartPieSkeleton: React.FunctionComponent = () => {
<>
<Switch
id="pie-skeleton-switch"
label="Skeleton on"
labelOff="Skeleton off"
label="Enable skeleton"
isChecked={isChecked}
onChange={handleChange}
/>
Expand Down Expand Up @@ -570,8 +561,7 @@ export const ChartScatterSkeleton: React.FunctionComponent = () => {
<>
<Switch
id="scatter-skeleton-switch"
label="Skeleton on"
labelOff="Skeleton off"
label="Enable skeleton"
isChecked={isChecked}
onChange={handleChange}
/>
Expand Down Expand Up @@ -627,8 +617,7 @@ export const ChartStackSkeleton: React.FunctionComponent = () => {
<>
<Switch
id="stack-skeleton-switch"
label="Skeleton on"
labelOff="Skeleton off"
label="Enable skeleton"
isChecked={isChecked}
onChange={handleChange}
/>
Expand Down Expand Up @@ -684,8 +673,7 @@ export const ChartThresholdSkeleton: React.FunctionComponent = () => {
<>
<Switch
id="threshold-skeleton-switch"
label="Skeleton on"
labelOff="Skeleton off"
label="Enable skeleton"
isChecked={isChecked}
onChange={handleChange}
/>
Expand Down
45 changes: 24 additions & 21 deletions packages/react-core/src/components/Switch/Switch.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,12 @@ export interface SwitchProps
id?: string;
/** Additional classes added to the switch */
className?: string;
/** Text value for the visible label when on */
/** Text value for the visible label */
label?: React.ReactNode;
/** Text value for the visible label when off */
labelOff?: React.ReactNode;
/** Adds an accessible name to the switch when the label prop is not passed, and must describe the isChecked="true" state. */
'aria-label'?: string;
/** Adds an accessible name to the switch via one or more referenced id(s). The computed accessible name must describe the isChecked="true" state. */
'aria-labelledby'?: string;
/** Flag to show if the switch is checked when it is controlled by React state.
* To make the switch uncontrolled instead use the defaultChecked prop, but do not use both.
*/
Expand All @@ -30,9 +32,7 @@ export interface SwitchProps
isDisabled?: boolean;
/** A callback for when the switch selection changes. (event, isChecked) => {} */
onChange?: (event: React.FormEvent<HTMLInputElement>, checked: boolean) => void;
/** Adds accessible text to the switch, and should describe the isChecked="true" state. When label is defined, aria-label should be set to the text string that is visible when isChecked is true. */
'aria-label'?: string;
/** Flag to reverse the layout of toggle and label (toggle on right). */
/** Flag to reverse the layout of toggle and label (label at start, toggle at end). */
isReversed?: boolean;
/** Value to overwrite the randomly generated data-ouia-component-id.*/
ouiaId?: number | string;
Expand All @@ -48,15 +48,18 @@ class Switch extends React.Component<SwitchProps & OUIAProps, { ouiaStateId: str
isChecked: true,
isDisabled: false,
isReversed: false,
'aria-label': '',
'aria-label': undefined,
'aria-labelledby': undefined,
onChange: () => undefined as any
};

constructor(props: SwitchProps & OUIAProps) {
super(props);
if (!props.label && !props['aria-label']) {
if (!props.label && !props['aria-label'] && !props['aria-labelledby']) {
// eslint-disable-next-line no-console
console.error('Switch: Switch requires either a label or an aria-label to be specified');
console.error(
Copy link
Contributor

Choose a reason for hiding this comment

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

I would as unit test for this. That can be follow up. If we want to get this in.

'Switch: Switch requires at least one of label, aria-labelledby, or aria-label props to be specified'
);
}

this.id = props.id || getUniqueId();
Expand All @@ -71,7 +74,6 @@ class Switch extends React.Component<SwitchProps & OUIAProps, { ouiaStateId: str
id,
className,
label,
labelOff,
isChecked,
defaultChecked,
hasCheckIcon,
Expand All @@ -80,10 +82,16 @@ class Switch extends React.Component<SwitchProps & OUIAProps, { ouiaStateId: str
isReversed,
ouiaId,
ouiaSafe,
'aria-label': ariaLabel,
'aria-labelledby': ariaLabelledBy,
...props
} = this.props;

const isAriaLabelledBy = props['aria-label'] === '';
const hasAccessibleName = label || ariaLabel || ariaLabelledBy;
const isAriaLabelledBy = hasAccessibleName && (!ariaLabel || ariaLabelledBy);
const useDefaultAriaLabelledBy = !ariaLabelledBy && !ariaLabel;
const ariaLabelledByIds = ariaLabelledBy ?? `${this.id}-label`;

return (
<label
className={css(styles.switch, isReversed && styles.modifiers.reverse, className)}
Expand All @@ -94,10 +102,12 @@ class Switch extends React.Component<SwitchProps & OUIAProps, { ouiaStateId: str
id={this.id}
className={css(styles.switchInput)}
type="checkbox"
role="switch"
onChange={(event) => onChange(event, event.target.checked)}
{...(defaultChecked !== undefined ? { defaultChecked } : { checked: isChecked })}
disabled={isDisabled}
aria-labelledby={!isAriaLabelledBy ? null : `${this.id}-${isChecked !== true ? 'off' : 'on'}`}
aria-labelledby={isAriaLabelledBy ? ariaLabelledByIds : null}
aria-label={ariaLabel}
{...props}
/>
{label !== undefined ? (
Expand All @@ -110,19 +120,12 @@ class Switch extends React.Component<SwitchProps & OUIAProps, { ouiaStateId: str
)}
</span>
<span
className={css(styles.switchLabel, styles.modifiers.on)}
id={isAriaLabelledBy ? `${this.id}-on` : null}
className={css(styles.switchLabel)}
id={isAriaLabelledBy && useDefaultAriaLabelledBy ? `${this.id}-label` : null}
aria-hidden="true"
>
{label}
</span>
<span
className={css(styles.switchLabel, styles.modifiers.off)}
id={isAriaLabelledBy ? `${this.id}-off` : null}
aria-hidden="true"
>
{labelOff !== undefined ? labelOff : label}
</span>
</React.Fragment>
) : (
<span className={css(styles.switchToggle)}>
Expand Down
21 changes: 14 additions & 7 deletions packages/react-core/src/components/Switch/__tests__/Switch.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,15 +26,13 @@ describe('Switch', () => {
});

test('switch is checked', () => {
const { asFragment } = render(
<Switch id="switch-is-checked" label="On" labelOff="Off" isChecked aria-label="Switch label" />
);
const { asFragment } = render(<Switch id="switch-is-checked" label="On" isChecked aria-label="Switch label" />);
expect(asFragment()).toMatchSnapshot();
});

test('switch is not checked', () => {
const { asFragment } = render(
<Switch id="switch-is-not-checked" label="On" labelOff="Off" isChecked={false} aria-label="Switch label" />
<Switch id="switch-is-not-checked" label="On" isChecked={false} aria-label="Switch label" />
);
expect(asFragment()).toMatchSnapshot();
});
Expand Down Expand Up @@ -84,7 +82,7 @@ describe('Switch', () => {
expect(props.onChange).toHaveBeenCalledWith(expect.any(Object), true);
});

test('should throw console error when no aria-label or label is given', () => {
test('should throw console error when no aria-label, aria-labelledby, or label is given', () => {
const myMock = jest.fn();

global.console = { ...global.console, error: myMock };
Expand All @@ -93,7 +91,7 @@ describe('Switch', () => {
expect(myMock).toHaveBeenCalled();
});

test('should not throw console error when label is given but no aria-label', () => {
test('should not throw console error when label is given', () => {
const myMock = jest.fn();
global.console = { ...global.console, error: myMock };

Expand All @@ -102,7 +100,7 @@ describe('Switch', () => {
expect(myMock).not.toHaveBeenCalled();
});

test('should not throw console error when aria-label is given but no label', () => {
test('should not throw console error when aria-label is given', () => {
const myMock = jest.fn();
global.console = { ...global.console, error: myMock };

Expand All @@ -111,6 +109,15 @@ describe('Switch', () => {
expect(myMock).not.toHaveBeenCalled();
});

test('should not throw console error when aria-labelledby is given', () => {
const myMock = jest.fn();
global.console = { ...global.console, error: myMock };

render(<Switch {...props} aria-labelledby="some-id" />);

expect(myMock).not.toHaveBeenCalled();
});

test('should apply reversed modifier', () => {
render(<Switch id="reversed-switch" label="reversed switch" isReversed aria-label="Switch label" />);
expect(screen.getByLabelText('Switch label').parentElement).toHaveClass('pf-m-reverse');
Expand Down
Loading
Loading