diff --git a/UNRELEASED.md b/UNRELEASED.md index acd3984ee67..6b9c0642f3e 100644 --- a/UNRELEASED.md +++ b/UNRELEASED.md @@ -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 diff --git a/src/components/CheckableButton/tests/CheckableButton.test.tsx b/src/components/CheckableButton/tests/CheckableButton.test.tsx index b9ba21a6405..95b71aa1abc 100644 --- a/src/components/CheckableButton/tests/CheckableButton.test.tsx +++ b/src/components/CheckableButton/tests/CheckableButton.test.tsx @@ -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', @@ -73,18 +72,5 @@ describe('', () => { element.find('div')!.trigger('onClick'); expect(spy).toHaveBeenCalled(); }); - - it('is called when the CheckableButton pressed with spacebar', () => { - const spy = jest.fn(); - const element = mountWithApp( - , - ); - - element.find(Checkbox)!.find('input')!.trigger('onKeyUp', { - keyCode: Key.Space, - }); - - expect(spy).toHaveBeenCalled(); - }); }); }); diff --git a/src/components/Checkbox/Checkbox.tsx b/src/components/Checkbox/Checkbox.tsx index 60b47a639f5..433eeb8491b 100644 --- a/src/components/Checkbox/Checkbox.tsx +++ b/src/components/Checkbox/Checkbox.tsx @@ -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'; @@ -92,20 +92,21 @@ export const Checkbox = forwardRef( setKeyFocused(false); }; - 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[] = []; @@ -152,13 +153,11 @@ export const Checkbox = forwardRef( helpText={helpText} error={error} disabled={disabled} - onClick={handleInput} onMouseOver={handleMouseOver} onMouseOut={handleMouseOut} > ( 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} /> - + @@ -189,6 +193,8 @@ export const Checkbox = forwardRef( function noop() {} -function stopPropagation(event: React.MouseEvent) { +function stopPropagation( + event: React.MouseEvent | React.KeyboardEvent | React.FormEvent, +) { event.stopPropagation(); } diff --git a/src/components/Checkbox/tests/Checkbox.test.tsx b/src/components/Checkbox/tests/Checkbox.test.tsx index e87b9ff2e66..af8a8bded99 100644 --- a/src/components/Checkbox/tests/Checkbox.test.tsx +++ b/src/components/Checkbox/tests/Checkbox.test.tsx @@ -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('', () => { @@ -32,18 +31,6 @@ describe('', () => { }); }); - it('does not propagate click events from input element', () => { - const spy = jest.fn(); - const element = mountWithApp( - , - ); - - 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(); @@ -52,57 +39,25 @@ describe('', () => { ); (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( - , - ); - - element.find('input')!.trigger('onKeyUp', { - keyCode: Key.Space, - }); - - expect(spy).toHaveBeenCalledTimes(1); - }); - - it('is not from keys other than space', () => { - const spy = jest.fn(); - const element = mountWithApp( - , - ); - - 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.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.find('input')!.trigger('onKeyUp', { - keyCode: Key.Enter, - }); - expect(spy).not.toHaveBeenCalled(); - }); - it('is not called from click events when disabled', () => { const spy = jest.fn(); const checkbox = mountWithApp( @@ -175,22 +130,6 @@ describe('', () => { disabled: false, }); }); - - it('can change values when disabled', () => { - const spy = jest.fn(); - const checkbox = mountWithApp( - , - ); - - checkbox.find('input')!.trigger('onKeyUp', { - keyCode: Key.Enter, - }); - checkbox.setProps({checked: true}); - - expect(checkbox).toContainReactComponent('input', { - checked: true, - }); - }); }); describe('helpText', () => { diff --git a/src/components/ChoiceList/tests/ChoiceList.test.tsx b/src/components/ChoiceList/tests/ChoiceList.test.tsx index e5fa0958e72..afd30baa95d 100644 --- a/src/components/ChoiceList/tests/ChoiceList.test.tsx +++ b/src/components/ChoiceList/tests/ChoiceList.test.tsx @@ -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('', () => { @@ -314,12 +313,18 @@ describe('', () => { 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', @@ -327,7 +332,7 @@ describe('', () => { choiceList.setProps({selected}); - getChoiceElement(0).find(Choice)!.trigger('onClick'); + getChoiceElement(0).find('input')!.domNode?.dispatchEvent(event); expect(spy).toHaveBeenLastCalledWith(['two', 'three'], 'MyChoiceList'); }); }); diff --git a/src/components/IndexTable/components/Checkbox/Checkbox.tsx b/src/components/IndexTable/components/Checkbox/Checkbox.tsx index d11b4de6b2a..d00f5726b40 100644 --- a/src/components/IndexTable/components/Checkbox/Checkbox.tsx +++ b/src/components/IndexTable/components/Checkbox/Checkbox.tsx @@ -36,8 +36,7 @@ export const Checkbox = memo(function Checkbox() {
', () => { }); }); - it('prevents onChange propagation', () => { - let stopPropagationSpy = false; - const checkbox = mountWithTable(); - - triggerCheckboxEvent(checkbox, 'onChange', { - stopPropagation: () => { - stopPropagationSpy = true; - }, - }); - - expect(stopPropagationSpy).toBe(true); - }); - it('toggles the checkbox value when clicked', () => { const onSelectionChange = jest.fn(); const checkbox = mountWithTable(, { @@ -82,21 +69,6 @@ describe('', () => { expect(onSelectionChange).toHaveBeenCalledWith('single', false, defaultId); }); - it('toggles the checkbox when spacebar is pressed', () => { - const onSelectionChange = jest.fn(); - const checkbox = mountWithTable(, { - 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 = { diff --git a/src/components/IndexTable/components/Row/Row.tsx b/src/components/IndexTable/components/Row/Row.tsx index 5a7d46a8f9f..aac8ffe81b3 100644 --- a/src/components/IndexTable/components/Row/Row.tsx +++ b/src/components/IndexTable/components/Row/Row.tsx @@ -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 @@ -97,7 +98,6 @@ export const Row = memo(function Row({ if (!tableRowRef.current || isNavigating.current) { return; } - event.stopPropagation(); event.preventDefault();