Skip to content

[Checkbox] properly support screen readers #4631

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

Merged
merged 1 commit into from
Nov 23, 2021
Merged

[Checkbox] properly support screen readers #4631

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
1 change: 1 addition & 0 deletions UNRELEASED.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ Use [the changelog guidelines](/documentation/Versioning%20and%20changelog.md) t
- Fixed try-catch syntax error in `Modal` ([#4553](https://github.com/Shopify/polaris-react/pull/4553))
- Fixed an issue with `TextField` where date and time were uneditable on click ([#4671](https://github.com/Shopify/polaris-react/pull/4671))
- Fixed an issue with `Popover` where the transform property interfered with descendants positioning ([#4685](https://github.com/Shopify/polaris-react/pull/4685))
- Fixed screen reader accessibility issue of the `Checkbox` component ([#4631](https://github.com/Shopify/polaris-react/pull/4631))

### Documentation

Expand Down
14 changes: 0 additions & 14 deletions src/components/CheckableButton/tests/CheckableButton.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ import {mountWithApp} from 'tests/utilities';
import {Checkbox} from 'components';

import {CheckableButton} from '../CheckableButton';
import {Key} from '../../../types';

const CheckableButtonProps = {
label: 'Test-Label',
Expand Down Expand Up @@ -73,18 +72,5 @@ describe('<CheckableButton />', () => {
element.find('div')!.trigger('onClick');
expect(spy).toHaveBeenCalled();
});

it('is called when the CheckableButton pressed with spacebar', () => {
const spy = jest.fn();
const element = mountWithApp(
<CheckableButton {...CheckableButtonProps} onToggleAll={spy} />,
);

element.find(Checkbox)!.find('input')!.trigger('onKeyUp', {
keyCode: Key.Space,
});

expect(spy).toHaveBeenCalled();
});
Comment on lines -77 to -88
Copy link
Contributor Author

@patrickracicot patrickracicot Nov 12, 2021

Choose a reason for hiding this comment

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

Since we no longer manually handler the keyUp events, we no longer need to test this.

Copy link
Contributor

@dleroux dleroux Nov 23, 2021

Choose a reason for hiding this comment

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

Same here? Since KeyUp is back, does this still stand?

Copy link
Contributor Author

@patrickracicot patrickracicot Nov 23, 2021

Choose a reason for hiding this comment

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

Although the onKeyUp handler is back on the Checkbox.Input element, we aren't managing it the same way this test intends. The onKeyUp only manages the focusable state and no longer triggers value change (we let the subsequent click event triggered by the browser do this).

});
});
40 changes: 23 additions & 17 deletions src/components/Checkbox/Checkbox.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import {useUniqueId} from '../../utilities/unique-id';
import {Choice, helpTextID} from '../Choice';
import {errorTextID} from '../InlineError';
import {Icon} from '../Icon';
import {Error, Key, CheckboxHandles} from '../../types';
import {Error, CheckboxHandles, Key} from '../../types';
import {WithinListboxContext} from '../../utilities/listbox/context';

import styles from './Checkbox.scss';
Expand Down Expand Up @@ -92,20 +92,21 @@ export const Checkbox = forwardRef<CheckboxHandles, CheckboxProps>(
setKeyFocused(false);
Copy link
Contributor

@dleroux dleroux Nov 17, 2021

Choose a reason for hiding this comment

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

The reason we have this keyFocused on checkboxes is 2 folds:

  1. We won't want the styles (focus ring) to be applied on click. Only when tabbing. This may be fixed with :focus-visible having better support now?

  2. Also, since the focus right is on the backdrop, tabbing to the checkbox did not trigger the event to bubble and apply the styles. I believe this still an issue.

Copy link
Contributor

@dleroux dleroux Nov 17, 2021

Choose a reason for hiding this comment

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

I just had a look and yes does show the focus rectangle on click now. so the classname should only be applied when it's a keyboard click.

Copy link
Contributor Author

@patrickracicot patrickracicot Nov 18, 2021

Choose a reason for hiding this comment

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

Alright so I've added back the onKeyUp logic, but made some adjustments to it.

Instead of calling setKeyFocused(true) for every key up event, I only call it for the Space and Tab keyboard events. This will prevent an issue we had in the current implementation where if a user clicked on the checkbox and then type any key other then Tab or Space, it would still apply the focus style.

};

const handleInput = () => {
if (onChange == null || inputNode.current == null || disabled) {
return;
const handleKeyUp = (event: React.KeyboardEvent) => {
const {keyCode} = event;

if (keyCode === Key.Space || keyCode === Key.Tab) {
!keyFocused && setKeyFocused(true);
}
onChange(!inputNode.current.checked, id);
inputNode.current.focus();
};

const handleKeyUp = (event: React.KeyboardEvent) => {
const {keyCode} = event;
!keyFocused && setKeyFocused(true);
if (keyCode === Key.Space) {
handleInput();
const handleOnClick = () => {
if (onChange == null || inputNode.current == null || disabled) {
return;
}

onChange(inputNode.current.checked, id);
inputNode.current.focus();
};

const describedBy: string[] = [];
Expand Down Expand Up @@ -152,13 +153,11 @@ export const Checkbox = forwardRef<CheckboxHandles, CheckboxProps>(
helpText={helpText}
error={error}
disabled={disabled}
onClick={handleInput}
onMouseOver={handleMouseOver}
onMouseOut={handleMouseOut}
>
<span className={wrapperClassName}>
<input
onKeyUp={handleKeyUp}
ref={inputNode}
id={id}
name={name}
Expand All @@ -167,17 +166,22 @@ export const Checkbox = forwardRef<CheckboxHandles, CheckboxProps>(
checked={isChecked}
disabled={disabled}
className={inputClassName}
onFocus={onFocus}
onBlur={handleBlur}
onClick={stopPropagation}
onChange={noop}
onClick={handleOnClick}
onFocus={onFocus}
onKeyUp={handleKeyUp}
aria-invalid={error != null}
aria-controls={ariaControls}
aria-describedby={ariaDescribedBy}
role={isWithinListbox ? 'presentation' : 'checkbox'}
{...indeterminateAttributes}
/>
<span className={backdropClassName} />
<span
className={backdropClassName}
onClick={stopPropagation}
onKeyUp={stopPropagation}
/>
<span className={styles.Icon}>
<Icon source={iconSource} />
</span>
Expand All @@ -189,6 +193,8 @@ export const Checkbox = forwardRef<CheckboxHandles, CheckboxProps>(

function noop() {}

function stopPropagation<E>(event: React.MouseEvent<E>) {
Copy link
Contributor

@dleroux dleroux Nov 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

IIRC this was also part of the focus issue

Copy link
Contributor

@dleroux dleroux Nov 17, 2021

Choose a reason for hiding this comment

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

Pretty sure this had to do with the bulk actions. I would remove this.

Copy link
Contributor Author

@patrickracicot patrickracicot Nov 18, 2021

Choose a reason for hiding this comment

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

Please refer to other PR comments on the IndexTable component in which I have added a fix to properly support the bulk actions.

TLDR; the root cause of the issue were the same as for the checkbox hence managing events on other DOM nodes then the originating element.

function stopPropagation(
event: React.MouseEvent | React.KeyboardEvent | React.FormEvent,
) {
event.stopPropagation();
}
75 changes: 7 additions & 68 deletions src/components/Checkbox/tests/Checkbox.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ import React, {AllHTMLAttributes} from 'react';
import {mountWithApp} from 'tests/utilities';

import {Key} from '../../../types';
import {Choice} from '../../Choice';
import {Checkbox} from '../Checkbox';

describe('<Checkbox />', () => {
Expand Down Expand Up @@ -32,18 +31,6 @@ describe('<Checkbox />', () => {
});
});

it('does not propagate click events from input element', () => {
const spy = jest.fn();
const element = mountWithApp(
<Checkbox id="MyCheckbox" label="Checkbox" onChange={spy} />,
);

element.find('input')!.trigger('onClick', {
stopPropagation: () => {},
});
expect(spy).not.toHaveBeenCalled();
});

describe('onChange()', () => {
it('is called with the updated checked value of the input on click', () => {
const spy = jest.fn();
Expand All @@ -52,57 +39,25 @@ describe('<Checkbox />', () => {
);

(element.find('input')!.domNode as HTMLInputElement).checked = true;
element.find(Choice)?.trigger('onClick');

expect(spy).toHaveBeenCalledWith(false, 'MyCheckbox');
});

it('is called when space is pressed', () => {
const spy = jest.fn();
const element = mountWithApp(
<Checkbox id="MyCheckbox" label="Checkbox" onChange={spy} />,
);

element.find('input')!.trigger('onKeyUp', {
keyCode: Key.Space,
});

expect(spy).toHaveBeenCalledTimes(1);
});
Comment on lines -60 to -71
Copy link
Contributor Author

@patrickracicot patrickracicot Nov 12, 2021

Choose a reason for hiding this comment

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

Since we no longer manually handler the keyUp events, we no longer need to test this.

Copy link
Contributor

@dleroux dleroux Nov 23, 2021

Choose a reason for hiding this comment

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

you brought back the keyUp, does this still stand?

Copy link
Contributor Author

@patrickracicot patrickracicot Nov 23, 2021

Choose a reason for hiding this comment

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

See #4631 (comment)


it('is not from keys other than space', () => {
const spy = jest.fn();
const element = mountWithApp(
<Checkbox id="MyCheckbox" label="Checkbox" onChange={spy} />,
);

element.find('input')!.trigger('onKeyUp', {
keyCode: Key.Enter,
const event = new MouseEvent('click', {
view: window,
bubbles: true,
cancelable: true,
});
element.find('input')!.domNode?.dispatchEvent(event);

expect(spy).not.toHaveBeenCalled();
expect(spy).toHaveBeenCalledWith(false, 'MyCheckbox');
});

it('sets focus on the input when checkbox is toggled off', () => {
const checkbox = mountWithApp(
<Checkbox checked id="checkboxId" label="Checkbox" onChange={noop} />,
);
checkbox.find(Choice)!.trigger('onClick');
checkbox.find('input')!.trigger('onClick');

expect(document.activeElement).toBe(checkbox.find('input')!.domNode);
});

it('is not called from keyboard events when disabled', () => {
const spy = jest.fn();
const checkbox = mountWithApp(
<Checkbox label="label" disabled onChange={spy} />,
);
checkbox.find('input')!.trigger('onKeyUp', {
keyCode: Key.Enter,
});
expect(spy).not.toHaveBeenCalled();
});
Comment on lines -95 to -104
Copy link
Contributor Author

@patrickracicot patrickracicot Nov 12, 2021

Choose a reason for hiding this comment

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

Since we no longer manually handler the keyUp events, we no longer need to test this.


it('is not called from click events when disabled', () => {
const spy = jest.fn();
const checkbox = mountWithApp(
Expand Down Expand Up @@ -175,22 +130,6 @@ describe('<Checkbox />', () => {
disabled: false,
});
});

it('can change values when disabled', () => {
const spy = jest.fn();
const checkbox = mountWithApp(
<Checkbox label="label" disabled onChange={spy} />,
);

checkbox.find('input')!.trigger('onKeyUp', {
keyCode: Key.Enter,
});
checkbox.setProps({checked: true});

expect(checkbox).toContainReactComponent('input', {
checked: true,
});
});
Comment on lines -179 to -193
Copy link
Contributor Author

@patrickracicot patrickracicot Nov 12, 2021

Choose a reason for hiding this comment

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

Since we no longer manually handler the keyUp events, we no longer need to test this.

});

describe('helpText', () => {
Expand Down
13 changes: 9 additions & 4 deletions src/components/ChoiceList/tests/ChoiceList.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ import React from 'react';
import {mountWithApp} from 'tests/utilities';
import {RadioButton, Checkbox, InlineError} from 'components';

import {Choice} from '../../Choice';
import {ChoiceList, ChoiceListProps} from '../ChoiceList';

describe('<ChoiceList />', () => {
Expand Down Expand Up @@ -314,20 +313,26 @@ describe('<ChoiceList />', () => {
const getChoiceElement = (index: number) =>
choiceList.findAll(Checkbox)[index];

getChoiceElement(1).find(Choice)!.trigger('onClick');
const event = new MouseEvent('click', {
view: window,
bubbles: true,
cancelable: true,
});

getChoiceElement(1).find('input')!.domNode?.dispatchEvent(event);
expect(spy).toHaveBeenLastCalledWith(['one', 'two'], 'MyChoiceList');

choiceList.setProps({selected});

getChoiceElement(2).find(Choice)!.trigger('onClick');
getChoiceElement(2).find('input')!.domNode?.dispatchEvent(event);
expect(spy).toHaveBeenLastCalledWith(
['one', 'two', 'three'],
'MyChoiceList',
);

choiceList.setProps({selected});

getChoiceElement(0).find(Choice)!.trigger('onClick');
getChoiceElement(0).find('input')!.domNode?.dispatchEvent(event);
expect(spy).toHaveBeenLastCalledWith(['two', 'three'], 'MyChoiceList');
});
});
Expand Down
9 changes: 2 additions & 7 deletions src/components/IndexTable/components/Checkbox/Checkbox.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,7 @@ export const Checkbox = memo(function Checkbox() {
<div
className={wrapperClassName}
onClick={onInteraction}
onKeyUp={onInteraction}
onChange={stopPropagation}
onKeyUp={noop}
>
<PolarisCheckbox
id={itemId}
Expand Down Expand Up @@ -92,8 +91,4 @@ export function CheckboxWrapper({children}: CheckboxWrapperProps) {
);
}

function stopPropagation(
event: React.MouseEvent | React.KeyboardEvent | React.FormEvent,
) {
event.stopPropagation();
}
function noop() {}
28 changes: 0 additions & 28 deletions src/components/IndexTable/components/Checkbox/tests/Checkbox.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,19 +55,6 @@ describe('<Checkbox />', () => {
});
});

it('prevents onChange propagation', () => {
let stopPropagationSpy = false;
const checkbox = mountWithTable(<Checkbox />);

triggerCheckboxEvent(checkbox, 'onChange', {
stopPropagation: () => {
stopPropagationSpy = true;
},
});

expect(stopPropagationSpy).toBe(true);
});

it('toggles the checkbox value when clicked', () => {
const onSelectionChange = jest.fn();
const checkbox = mountWithTable(<Checkbox />, {
Expand All @@ -82,21 +69,6 @@ describe('<Checkbox />', () => {
expect(onSelectionChange).toHaveBeenCalledWith('single', false, defaultId);
});

it('toggles the checkbox when spacebar is pressed', () => {
const onSelectionChange = jest.fn();
const checkbox = mountWithTable(<Checkbox />, {
indexProps: {onSelectionChange},
rowProps: {selected: true},
});

triggerCheckboxEvent(checkbox, 'onKeyUp', {
key: ' ',
nativeEvent: {shiftKey: false},
});

expect(onSelectionChange).toHaveBeenCalledWith('single', false, defaultId);
});

describe('CheckboxWrapper', () => {
describe('small screen', () => {
const defaultTableProps = {
Expand Down
2 changes: 1 addition & 1 deletion src/components/IndexTable/components/Row/Row.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ export const Row = memo(function Row({
const handleInteraction = useCallback(
(event: React.MouseEvent | React.KeyboardEvent) => {
event.stopPropagation();

if (('key' in event && event.key !== ' ') || !onSelectionChange) return;
const selectionType = event.nativeEvent.shiftKey
? SelectionType.Multi
Expand Down Expand Up @@ -97,7 +98,6 @@ export const Row = memo(function Row({
if (!tableRowRef.current || isNavigating.current) {
return;
}

event.stopPropagation();
event.preventDefault();

Expand Down