From 0c0f464b16a6781df0ecbe3a33b4b3152fa4f8f8 Mon Sep 17 00:00:00 2001 From: KHMakoto Date: Tue, 23 Feb 2021 19:03:53 -0800 Subject: [PATCH 1/6] Button: Beefing up accessibility tests and cleaning up state management. --- .../a11y-testing/src/definitions/index.ts | 4 +- .../buttonAccessibilityBehaviorDefinition.ts | 76 ++++++++++++++++ packages/react-button/etc/react-button.api.md | 2 +- packages/react-button/package.json | 4 +- .../src/components/Button/Button.test.tsx | 83 +++++++++++++----- .../Button/__snapshots__/Button.test.tsx.snap | 11 ++- .../src/components/Button/useButtonState.ts | 87 +++++++++++-------- 7 files changed, 201 insertions(+), 66 deletions(-) create mode 100644 packages/a11y-testing/src/definitions/react-button/buttonAccessibilityBehaviorDefinition.ts diff --git a/packages/a11y-testing/src/definitions/index.ts b/packages/a11y-testing/src/definitions/index.ts index acda1a0691cdf2..0d5870b6a09ee7 100644 --- a/packages/a11y-testing/src/definitions/index.ts +++ b/packages/a11y-testing/src/definitions/index.ts @@ -1,5 +1,7 @@ export * from './Button/buttonBehaviorDefinition'; export * from './Button/buttonGroupBehaviorDefinition'; export * from './Button/toggleButtonBehaviorDefinition'; -export * from './Popup/popupBehaviorDefinition'; export * from './MenuButton/menuButtonBehaviorDefinition'; +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..a25037d323e04d --- /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 a431e95a875604..0c68aefae2d5d0 100644 --- a/packages/react-button/etc/react-button.api.md +++ b/packages/react-button/etc/react-button.api.md @@ -322,7 +322,7 @@ export const useButton: (props: ButtonProps, ref: React.Ref, defaul export const useButtonClasses: (state: ButtonState, options?: import("@fluentui/react-theme-provider/lib/compat").UseStylesOptions | undefined) => void; // @public -export const useButtonState: (draftState: ButtonState) => void; +export const useButtonState: (state: ButtonState) => ButtonState; // @public export const useChecked: (draftState: TDraftState) => void; diff --git a/packages/react-button/package.json b/packages/react-button/package.json index a724fa50f6a41d..100cf283beb52a 100644 --- a/packages/react-button/package.json +++ b/packages/react-button/package.json @@ -27,6 +27,7 @@ "update-snapshots": "just-scripts jest -u" }, "devDependencies": { + "@fluentui/a11y-testing": "^0.1.0", "@fluentui/common-styles": "^1.0.0-beta.19", "@fluentui/eslint-plugin": "^1.0.0-beta.2", "@fluentui/react-conformance": "^1.0.0", @@ -42,8 +43,7 @@ "react": "16.8.6", "react-app-polyfill": "~1.0.1", "react-dom": "16.8.6", - "react-test-renderer": "^16.3.0", - "@fluentui/a11y-testing": "^0.1.0" + "react-test-renderer": "^16.3.0" }, "dependencies": { "@fluentui/keyboard-key": "^0.2.13", diff --git a/packages/react-button/src/components/Button/Button.test.tsx b/packages/react-button/src/components/Button/Button.test.tsx index f4deea131826b0..c42217c6cad4cb 100644 --- a/packages/react-button/src/components/Button/Button.test.tsx +++ b/packages/react-button/src/components/Button/Button.test.tsx @@ -1,16 +1,15 @@ import * as React from 'react'; import { mount, ReactWrapper } from 'enzyme'; import * as renderer from 'react-test-renderer'; +import { + buttonAccessibilityBehaviorDefinition, + buttonBehaviorDefinition, + validateBehavior, + ComponentTestFacade, +} from '@fluentui/a11y-testing'; import { isConformant } from '../../common/isConformant'; import { Button } from './Button'; import { GlobalClassNames } from './useButtonClasses'; -import { validateBehavior, ComponentTestFacade, buttonBehaviorDefinition } from '@fluentui/a11y-testing'; - -describe('Button (isConformant)', () => - isConformant({ - Component: Button, - displayName: 'Button', - })); describe('Button', () => { let wrapper: ReactWrapper | undefined; @@ -22,17 +21,42 @@ describe('Button', () => { } }); - /** - * Note: see more visual regression tests for Button in /apps/vr-tests. - */ - it('renders a default state', () => { - const component = renderer.create(); + isConformant({ + Component: Button, + displayName: 'Button', + }); + + describe('meets accessibility requirements', () => { + const testFacade = new ComponentTestFacade(Button, {}); + + let errors; + 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 component = renderer.create(); const tree = component.toJSON(); expect(tree).toMatchSnapshot(); }); - it('renders anchor when href prop is provided', () => { - const component = renderer.create(); + 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 component = renderer.create(); const tree = component.toJSON(); expect(tree).toMatchSnapshot(); }); @@ -40,7 +64,24 @@ describe('Button', () => { 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); + + rootRef.current?.focus(); + + 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); @@ -52,7 +93,7 @@ describe('Button', () => { it('can trigger a function by being clicked', () => { const onClick = jest.fn(); - wrapper = mount(); + wrapper = mount(); wrapper.find(`.${GlobalClassNames.root}`).simulate('click'); @@ -63,7 +104,7 @@ describe('Button', () => { const onClick = jest.fn(); wrapper = mount( , ); @@ -77,7 +118,7 @@ describe('Button', () => { const onClick = jest.fn(); wrapper = mount( , ); @@ -85,10 +126,4 @@ describe('Button', () => { expect(onClick).not.toHaveBeenCalled(); }); - - describe('AccessibilityButtonBehavior', () => { - const testFacade = new ComponentTestFacade(Button, {}); - const errors = validateBehavior(buttonBehaviorDefinition, testFacade); - expect(errors).toEqual([]); - }); }); 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 ecb96b4df3c4d5..c6af8a300dfcea 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,6 +1,6 @@ // Jest Snapshot v1, https://goo.gl/fbAQLP -exports[`Button renders a default state 1`] = ` +exports[`Button renders a default button 1`] = ` `; -exports[`Button renders anchor when href prop is provided 1`] = ` +exports[`Button renders as an anchor when href is provided 1`] = ` - Default button + This is a button `; diff --git a/packages/react-button/src/components/Button/useButtonState.ts b/packages/react-button/src/components/Button/useButtonState.ts index 2e1a04750bdb94..b913545944fd25 100644 --- a/packages/react-button/src/components/Button/useButtonState.ts +++ b/packages/react-button/src/components/Button/useButtonState.ts @@ -4,42 +4,58 @@ 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) => { - // Update the button's tab-index, keyboard handling, and aria attributes. - if (draftState.as !== 'button') { - draftState.role = 'button'; - - if (draftState.as !== 'a') { - const { onClick: onClickCallback, onKeyDown: onKeyDownCallback } = draftState; - - draftState['data-is-focusable'] = true; - draftState.tabIndex = 0; - - draftState.onKeyDown = (ev: React.KeyboardEvent) => { - if (onKeyDownCallback) { - onKeyDownCallback(ev); - } - - const keyCode = getCode(ev); - if (!ev.defaultPrevented && onClickCallback && (keyCode === EnterKey || keyCode === SpacebarKey)) { - // Translate the keydown enter/space to a click. - ev.preventDefault(); - ev.stopPropagation(); - - (ev.target as HTMLElement).click(); - } - }; +export const useButtonState = (state: ButtonState): ButtonState => { + const { as, disabled, disabledFocusable, onClick, onKeyDown: onKeyDownCallback } = state; + + const onNonAnchorOrButtonKeyDown = (ev: React.KeyboardEvent) => { + onKeyDownCallback?.(ev); + + const keyCode = getCode(ev); + if (!ev.defaultPrevented && onClick && (keyCode === EnterKey || keyCode === SpacebarKey)) { + // Translate the keydown enter/space to a click. + ev.preventDefault(); + ev.stopPropagation(); + + onClick((ev as unknown) as React.MouseEvent); } - } + }; + + // 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; - // 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 'data-is-focusable=true' and keydown event handler for all other non-anchor elements. + if (as !== 'a') { + state['data-is-focusable'] = true; + state.onKeyDown = onNonAnchorOrButtonKeyDown; + } + } + } + // Add 'data-is-focusable=true', keydown event handler, 'role=button' and 'tabIndex=0' for all other elements. + else { + state['data-is-focusable'] = true; + 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(); @@ -49,6 +65,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; }; From 2e070a3d244073d4fa5e893cb810a3b3f4b9e310 Mon Sep 17 00:00:00 2001 From: KHMakoto Date: Tue, 23 Feb 2021 19:06:45 -0800 Subject: [PATCH 2/6] Change files --- ...-react-button-e1acb3ed-0311-42c0-b9ec-468f986666b9.json | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 change/@fluentui-react-button-e1acb3ed-0311-42c0-b9ec-468f986666b9.json 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..4b8f054bb386e8 --- /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_makoto@hotmail.com", + "dependentChangeType": "patch" +} From 243f9ee45828c7b69b4ba518f4a7ead412f7ff8e Mon Sep 17 00:00:00 2001 From: KHMakoto Date: Tue, 23 Feb 2021 19:26:18 -0800 Subject: [PATCH 3/6] Updating change file. --- ...entui-react-button-e1acb3ed-0311-42c0-b9ec-468f986666b9.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/change/@fluentui-react-button-e1acb3ed-0311-42c0-b9ec-468f986666b9.json b/change/@fluentui-react-button-e1acb3ed-0311-42c0-b9ec-468f986666b9.json index 4b8f054bb386e8..558d23e974866a 100644 --- a/change/@fluentui-react-button-e1acb3ed-0311-42c0-b9ec-468f986666b9.json +++ b/change/@fluentui-react-button-e1acb3ed-0311-42c0-b9ec-468f986666b9.json @@ -2,6 +2,6 @@ "type": "prerelease", "comment": "Button: Beefing up accessibility tests and cleaning up state management.", "packageName": "@fluentui/react-button", - "email": "humberto_makoto@hotmail.com", + "email": "Humberto.Morimoto@microsoft.com", "dependentChangeType": "patch" } From 4d0776f69727b48bee0b3e00d61ebf9bff06543c Mon Sep 17 00:00:00 2001 From: KHMakoto Date: Thu, 25 Feb 2021 12:41:56 -0800 Subject: [PATCH 4/6] Updating snapshots. --- .../src/components/Button/Button.test.tsx | 6 ++---- .../Button/__snapshots__/Button.test.tsx.snap | 13 ++++--------- 2 files changed, 6 insertions(+), 13 deletions(-) diff --git a/packages/react-button/src/components/Button/Button.test.tsx b/packages/react-button/src/components/Button/Button.test.tsx index f91b3f9e34abc0..4ea70f13389a52 100644 --- a/packages/react-button/src/components/Button/Button.test.tsx +++ b/packages/react-button/src/components/Button/Button.test.tsx @@ -43,8 +43,7 @@ describe('Button', () => { expect(button.length).toBe(1); expect(anchor.length).toBe(0); - const component = renderer.create(); - const tree = component.toJSON(); + const tree = button.debug(); expect(tree).toMatchSnapshot(); }); @@ -55,8 +54,7 @@ describe('Button', () => { // expect(button.length).toBe(0); // expect(anchor.length).toBe(1); - // const component = renderer.create(); - // const tree = component.toJSON(); + // const tree = anchor.debug(); // expect(tree).toMatchSnapshot(); // }); 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 b4f768e0d88323..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,15 +1,10 @@ // Jest Snapshot v1, https://goo.gl/fbAQLP exports[`Button renders a default button 1`] = ` - +" `; From a788474450b3ebf65e0bfb8f1e3a8d14444f2658 Mon Sep 17 00:00:00 2001 From: KHMakoto Date: Thu, 25 Feb 2021 12:50:41 -0800 Subject: [PATCH 5/6] Removing unused imports. --- packages/react-button/src/components/Button/Button.test.tsx | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/react-button/src/components/Button/Button.test.tsx b/packages/react-button/src/components/Button/Button.test.tsx index 4ea70f13389a52..bd3e50bb11c886 100644 --- a/packages/react-button/src/components/Button/Button.test.tsx +++ b/packages/react-button/src/components/Button/Button.test.tsx @@ -1,6 +1,5 @@ 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, From a54f817f75a9d470bb2e8acabcb767dd6100f9b2 Mon Sep 17 00:00:00 2001 From: KHMakoto Date: Thu, 25 Feb 2021 13:27:49 -0800 Subject: [PATCH 6/6] Fixing lint error. --- packages/react-button/src/components/Button/Button.test.tsx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/react-button/src/components/Button/Button.test.tsx b/packages/react-button/src/components/Button/Button.test.tsx index bd3e50bb11c886..316573a2d5b72d 100644 --- a/packages/react-button/src/components/Button/Button.test.tsx +++ b/packages/react-button/src/components/Button/Button.test.tsx @@ -27,8 +27,8 @@ describe('Button', () => { describe('meets accessibility requirements', () => { const testFacade = new ComponentTestFacade(Button, {}); - let errors; - errors = validateBehavior(buttonAccessibilityBehaviorDefinition, testFacade); + // let errors; + const errors = validateBehavior(buttonAccessibilityBehaviorDefinition, testFacade); expect(errors).toEqual([]); // errors = validateBehavior(buttonBehaviorDefinition, testFacade);