From ce4d441da2c63284a2c1c519a59875c22cd3552f Mon Sep 17 00:00:00 2001 From: Makoto Morimoto Date: Thu, 25 Mar 2021 12:05:58 -0700 Subject: [PATCH] Button: Beefing up accessibility tests and cleaning up state management (#17155) * Button: Beefing up accessibility tests and cleaning up state management. * Change files * Updating change file. * Updating snapshots. * Removing unused imports. * Fixing lint error. Co-authored-by: KHMakoto --- ...-e1acb3ed-0311-42c0-b9ec-468f986666b9.json | 7 ++ .../a11y-testing/src/definitions/index.ts | 6 +- .../buttonAccessibilityBehaviorDefinition.ts | 76 ++++++++++++ packages/react-button/etc/react-button.api.md | 2 +- .../src/components/Button/Button.test.tsx | 109 +++++++++++++----- .../Button/__snapshots__/Button.test.tsx.snap | 16 +-- .../src/components/Button/useButtonState.ts | 73 +++++++----- 7 files changed, 220 insertions(+), 69 deletions(-) create mode 100644 change/@fluentui-react-button-e1acb3ed-0311-42c0-b9ec-468f986666b9.json create mode 100644 packages/a11y-testing/src/definitions/react-button/buttonAccessibilityBehaviorDefinition.ts diff --git a/change/@fluentui-react-button-e1acb3ed-0311-42c0-b9ec-468f986666b9.json b/change/@fluentui-react-button-e1acb3ed-0311-42c0-b9ec-468f986666b9.json new file mode 100644 index 00000000000000..558d23e974866a --- /dev/null +++ b/change/@fluentui-react-button-e1acb3ed-0311-42c0-b9ec-468f986666b9.json @@ -0,0 +1,7 @@ +{ + "type": "prerelease", + "comment": "Button: Beefing up accessibility tests and cleaning up state management.", + "packageName": "@fluentui/react-button", + "email": "Humberto.Morimoto@microsoft.com", + "dependentChangeType": "patch" +} diff --git a/packages/a11y-testing/src/definitions/index.ts b/packages/a11y-testing/src/definitions/index.ts index d7a0236044f6dd..fed4c8f24b42fb 100644 --- a/packages/a11y-testing/src/definitions/index.ts +++ b/packages/a11y-testing/src/definitions/index.ts @@ -2,7 +2,9 @@ export * from './Button/buttonBehaviorDefinition'; export * from './Button/buttonGroupBehaviorDefinition'; export * from './Button/toggleButtonBehaviorDefinition'; export * from './Link/linkBehaviorDefinition'; -export * from './Popup/popupBehaviorDefinition'; export * from './MenuButton/menuButtonBehaviorDefinition'; -export * from './Pill/pillBehaviorDefinition'; export * from './Pill/pillActionBehaviorDefinition'; +export * from './Pill/pillBehaviorDefinition'; +export * from './Popup/popupBehaviorDefinition'; + +export * from './react-button/buttonAccessibilityBehaviorDefinition'; diff --git a/packages/a11y-testing/src/definitions/react-button/buttonAccessibilityBehaviorDefinition.ts b/packages/a11y-testing/src/definitions/react-button/buttonAccessibilityBehaviorDefinition.ts new file mode 100644 index 00000000000000..6df5c5e455f195 --- /dev/null +++ b/packages/a11y-testing/src/definitions/react-button/buttonAccessibilityBehaviorDefinition.ts @@ -0,0 +1,76 @@ +import { Rule } from './../../types'; +import { BehaviorRule } from './../../rules/rules'; + +export const buttonAccessibilityBehaviorDefinition: Rule[] = [ + BehaviorRule.root() + .doesNotHaveAttribute('role') + .doesNotHaveAttribute('tabindex') + .description(`if element is rendered as a default 'button'.`), + // BehaviorRule.root() + // .forProps({ href: '#' }) + // .hasAttribute('role', 'button') + // .hasAttribute('tabindex', '0') + // .description(`if element has href and is rendered as an 'anchor'.`), + BehaviorRule.root() + .forProps({ as: 'div' }) + .hasAttribute('data-is-focusable', 'true') + .hasAttribute('role', 'button') + .hasAttribute('tabindex', '0') + .description(`if element type is other than the defaults 'anchor' and 'button'.`), + BehaviorRule.root() + .forProps({ disabled: true }) + .hasAttribute('disabled') + .description(`if element is rendered as a default 'button' and is disabled.`), + // BehaviorRule.root() + // .forProps({ disabled: true, href: '#' }) + // .doesNotHaveAttribute('disabled') + // .description(`if element has href and is rendered as an 'anchor' and is disabled.`), + BehaviorRule.root() + .forProps({ as: 'div', disabled: true }) + .doesNotHaveAttribute('disabled') + .description(`if element type is other than the defaults 'anchor' and 'button' and is disabled.`), + // BehaviorRule.root() + // .forProps({ disabledFocusable: true }) + // .doesNotHaveAttribute('disabled') + // .hasAttribute('aria-disabled', 'true') + // .description(`if element is rendered as a default 'button' and is disabled but focusable.`), + // BehaviorRule.root() + // .forProps({ disabledFocusable: true, href: '#' }) + // .doesNotHaveAttribute('disabled') + // .hasAttribute('aria-disabled', 'true') + // .hasAttribute('tabindex', '0') + // .description(`if element has href and is rendered as an 'anchor' and is disabled but focusable.`), + // BehaviorRule.root() + // .forProps({ as: 'div', disabledFocusable: true }) + // .doesNotHaveAttribute('disabled') + // .hasAttribute('aria-disabled', 'true') + // .hasAttribute('tabindex', '0') + // .description(`if element type is other than the defaults 'anchor' and 'button' and is disabled but focusable.`), + // BehaviorRule.root() + // .forProps({ disabled: true, disabledFocusable: true }) + // .doesNotHaveAttribute('disabled') + // .hasAttribute('aria-disabled', 'true') + // .description(`if element is rendered as a default 'button' and is disabled but focusable.`), + // BehaviorRule.root() + // .forProps({ disabled: true, disabledFocusable: true, href: '#' }) + // .doesNotHaveAttribute('disabled') + // .hasAttribute('aria-disabled', 'true') + // .hasAttribute('tabindex', '0') + // .description(`if element has href and is rendered as an 'anchor' and is disabled but focusable.`), + // BehaviorRule.root() + // .forProps({ as: 'div', disabled: true, disabledFocusable: true }) + // .doesNotHaveAttribute('disabled') + // .hasAttribute('aria-disabled', 'true') + // .hasAttribute('tabindex', '0') + // .description(`if element type is other than the defaults 'anchor' and 'button' and is disabled but focusable.`), + BehaviorRule.root() + .forProps({ as: 'div' }) + .pressSpaceKey() + .verifyOnclickExecution() + .description(`if element type is other than the defaults 'anchor' and 'button'.`), + BehaviorRule.root() + .forProps({ as: 'div' }) + .pressEnterKey() + .verifyOnclickExecution() + .description(`if element type is other than the defaults 'anchor' and 'button'.`), +]; diff --git a/packages/react-button/etc/react-button.api.md b/packages/react-button/etc/react-button.api.md index a15353eb600238..70e7d7b761242e 100644 --- a/packages/react-button/etc/react-button.api.md +++ b/packages/react-button/etc/react-button.api.md @@ -140,7 +140,7 @@ export const renderCompoundButton: (state: CompoundButtonState) => JSX.Element; export const useButton: (props: ButtonProps, ref: React.Ref, defaultProps?: ButtonProps | undefined) => ButtonState; // @public -export const useButtonState: (draftState: ButtonState) => void; +export const useButtonState: (state: ButtonState) => ButtonState; // @public (undocumented) export const useButtonStyles: (state: ButtonState, selectors: ButtonStyleSelectors) => void; diff --git a/packages/react-button/src/components/Button/Button.test.tsx b/packages/react-button/src/components/Button/Button.test.tsx index 8b9831f993f49e..316573a2d5b72d 100644 --- a/packages/react-button/src/components/Button/Button.test.tsx +++ b/packages/react-button/src/components/Button/Button.test.tsx @@ -1,15 +1,13 @@ import * as React from 'react'; -import { mount, shallow, ReactWrapper } from 'enzyme'; -import * as renderer from 'react-test-renderer'; +import { mount, ReactWrapper } from 'enzyme'; +import { + buttonAccessibilityBehaviorDefinition, + // buttonBehaviorDefinition, + validateBehavior, + ComponentTestFacade, +} from '@fluentui/a11y-testing'; import { isConformant } from '../../common/isConformant'; import { Button } from './Button'; -// import { validateBehavior, ComponentTestFacade, buttonBehaviorDefinition } from '@fluentui/a11y-testing'; - -describe('Button (isConformant)', () => - isConformant({ - Component: Button, - displayName: 'Button', - })); describe('Button', () => { let wrapper: ReactWrapper | undefined; @@ -21,19 +19,48 @@ describe('Button', () => { } }); - /** - * Note: see more visual regression tests for Button in /apps/vr-tests. - */ - it('renders a default state', () => { - const component = renderer.create(); - const tree = component.toJSON(); + isConformant({ + Component: Button, + displayName: 'Button', + }); + + describe('meets accessibility requirements', () => { + const testFacade = new ComponentTestFacade(Button, {}); + + // let errors; + const errors = validateBehavior(buttonAccessibilityBehaviorDefinition, testFacade); + expect(errors).toEqual([]); + + // errors = validateBehavior(buttonBehaviorDefinition, testFacade); + // expect(errors).toEqual([]); + }); + + it('renders a default button', () => { + wrapper = mount(); + const button = wrapper.find('button'); + const anchor = wrapper.find('a'); + expect(button.length).toBe(1); + expect(anchor.length).toBe(0); + + const tree = button.debug(); expect(tree).toMatchSnapshot(); }); + // it('renders as an anchor when href is provided', () => { + // wrapper = mount(); + // const button = wrapper.find('button'); + // const anchor = wrapper.find('a'); + // expect(button.length).toBe(0); + // expect(anchor.length).toBe(1); + + // const tree = anchor.debug(); + // expect(tree).toMatchSnapshot(); + // }); + it('can be focused', () => { const rootRef = React.createRef(); - wrapper = mount(); + wrapper = mount(); expect(typeof rootRef.current).toEqual('object'); expect(document.activeElement).not.toEqual(rootRef.current); @@ -43,13 +70,41 @@ describe('Button', () => { expect(document.activeElement).toEqual(rootRef.current); }); + // it('can be focused when rendered as an anchor', () => { + // const rootRef = React.createRef(); + + // wrapper = mount( + // , + // ); + + // expect(typeof rootRef.current).toEqual('object'); + // expect(document.activeElement).not.toEqual(rootRef.current); + + // rootRef.current?.focus(); + + // expect(document.activeElement).toEqual(rootRef.current); + // }); + + it('can trigger a function by being clicked', () => { + const onClick = jest.fn(); + wrapper = mount(); + + wrapper.find('button').simulate('click'); + + expect(onClick).toHaveBeenCalled(); + }); + it('does not trigger a function by being clicked when button is disabled', () => { const onClick = jest.fn(); - shallow( + wrapper = mount( , - ).simulate('click'); + ); + + wrapper.find('button').simulate('click'); expect(onClick).not.toHaveBeenCalled(); }); @@ -57,18 +112,14 @@ describe('Button', () => { // it(`does not trigger a function by being clicked when button is disabled, even when disabledFocusable has been // provided`, () => { // const onClick = jest.fn(); - // shallow( + // wrapper = mount( // , - // ).simulate('click'); - // - // expect(onClick).not.toHaveBeenCalled(); - // }); + // ); - // describe('AccessibilityButtonBehavior', () => { - // const testFacade = new ComponentTestFacade(Button, {}); - // const errors = validateBehavior(buttonBehaviorDefinition, testFacade); - // expect(errors).toEqual([]); + // wrapper.find('button').simulate('click'); + + // expect(onClick).not.toHaveBeenCalled(); // }); }); diff --git a/packages/react-button/src/components/Button/__snapshots__/Button.test.tsx.snap b/packages/react-button/src/components/Button/__snapshots__/Button.test.tsx.snap index fa632a37db360d..25e809338fc7f7 100644 --- a/packages/react-button/src/components/Button/__snapshots__/Button.test.tsx.snap +++ b/packages/react-button/src/components/Button/__snapshots__/Button.test.tsx.snap @@ -1,14 +1,10 @@ // Jest Snapshot v1, https://goo.gl/fbAQLP -exports[`Button renders a default state 1`] = ` - +" `; diff --git a/packages/react-button/src/components/Button/useButtonState.ts b/packages/react-button/src/components/Button/useButtonState.ts index 7070723b3e7165..2362d3eb37e271 100644 --- a/packages/react-button/src/components/Button/useButtonState.ts +++ b/packages/react-button/src/components/Button/useButtonState.ts @@ -4,40 +4,56 @@ import { ButtonState } from './Button.types'; /** * The useButton hook processes the Button draft state. - * @param draftState - Button draft state to mutate. + * @param state - Button draft state to mutate. */ -export const useButtonState = (draftState: ButtonState) => { - if (draftState.as !== 'button') { - draftState.role = 'button'; +export const useButtonState = (state: ButtonState): ButtonState => { + const { as, disabled, /*disabledFocusable,*/ onClick, onKeyDown: onKeyDownCallback } = state; - if (draftState.as !== 'a') { - const { onClick: onClickCallback, onKeyDown: onKeyDownCallback } = draftState; + const onNonAnchorOrButtonKeyDown = (ev: React.KeyboardEvent) => { + onKeyDownCallback?.(ev); - draftState.tabIndex = 0; + const keyCode = getCode(ev); + if (!ev.defaultPrevented && onClick && (keyCode === EnterKey || keyCode === SpacebarKey)) { + // Translate the keydown enter/space to a click. + ev.preventDefault(); + ev.stopPropagation(); - draftState.onKeyDown = (ev: React.KeyboardEvent) => { - if (onKeyDownCallback) { - onKeyDownCallback(ev); - } + onClick((ev as unknown) as React.MouseEvent); + } + }; - const keyCode = getCode(ev); - if (!ev.defaultPrevented && onClickCallback && (keyCode === EnterKey || keyCode === SpacebarKey)) { - // Translate the keydown enter/space to a click. - ev.preventDefault(); - ev.stopPropagation(); + // Adjust props depending on the root type. + if (typeof as === 'string') { + // Add 'role=button' and 'tabIndex=0' for all non-button elements. + if (as !== 'button') { + state.role = 'button'; + state.tabIndex = disabled /*&& !disabledFocusable*/ ? undefined : 0; - (ev.target as HTMLElement).click(); - } - }; + // Add keydown event handler for all other non-anchor elements. + if (as !== 'a') { + state.onKeyDown = onNonAnchorOrButtonKeyDown; + } } } - - // Disallow click and keyboard events when component is disabled and eat events when disabledFocusable is set to true. - const { disabled, /* disabledFocusable, */ onKeyDown } = draftState; - if (disabled) { - draftState.onClick = undefined; + // Add keydown event handler, 'role=button' and 'tabIndex=0' for all other elements. + else { + state.onKeyDown = onNonAnchorOrButtonKeyDown; + state.role = 'button'; + state.tabIndex = disabled /*&& !disabledFocusable*/ ? undefined : 0; } - draftState.onKeyDown = (ev: React.KeyboardEvent) => { + + // Disallow click event when component is disabled and eat events when disabledFocusable is set to true. + state.onClick = (ev: React.MouseEvent) => { + if (disabled) { + ev.preventDefault(); + } else { + onClick?.(ev); + } + }; + + // Disallow keydown event when component is disabled and eat events when disabledFocusable is set to true. + const { onKeyDown } = state; + state.onKeyDown = (ev: React.KeyboardEvent) => { const keyCode = getCode(ev); if (disabled && (keyCode === EnterKey || keyCode === SpacebarKey)) { ev.preventDefault(); @@ -47,6 +63,9 @@ export const useButtonState = (draftState: ButtonState) => { } }; - draftState['aria-disabled'] = disabled /* || disabledFocusable*/; - draftState.disabled = draftState.as === 'button' ? disabled /* && !disabledFocusable */ : undefined; + // Set the aria-disabled and disabled props correctly. + state['aria-disabled'] = disabled /*|| disabledFocusable*/; + state.disabled = as === 'button' ? disabled /* && !disabledFocusable*/ : undefined; + + return state; };