From 0c22731ba9c92ddc528b88077d3dd7aca5914d77 Mon Sep 17 00:00:00 2001 From: Cameron Dutro Date: Fri, 3 Nov 2023 10:22:06 -0700 Subject: [PATCH 1/7] Address additional ToggleSwitch a11y feedback --- script/test-e2e | 3 +- .../ToggleSwitch.features.stories.tsx | 58 +++++++++++++++- src/ToggleSwitch/ToggleSwitch.test.tsx | 18 ++++- src/ToggleSwitch/ToggleSwitch.tsx | 68 ++++++++++++++++--- .../__snapshots__/ToggleSwitch.test.tsx.snap | 4 +- 5 files changed, 134 insertions(+), 17 deletions(-) diff --git a/script/test-e2e b/script/test-e2e index b451e5b0cf2..ca84a40bd71 100755 --- a/script/test-e2e +++ b/script/test-e2e @@ -1,5 +1,6 @@ #!/bin/bash +args="$@" set -x docker run --rm \ @@ -7,4 +8,4 @@ docker run --rm \ -v $(pwd):/workspace \ -w /workspace \ -it mcr.microsoft.com/playwright:v1.37.0-jammy \ - /bin/bash -c "npm install && STORYBOOK_URL=http://host.docker.internal:6006 npx playwright test $@" + /bin/bash -c "npm install && STORYBOOK_URL=http://host.docker.internal:6006 npx playwright test $args" diff --git a/src/ToggleSwitch/ToggleSwitch.features.stories.tsx b/src/ToggleSwitch/ToggleSwitch.features.stories.tsx index 138b6b69b80..2e453ac6aed 100644 --- a/src/ToggleSwitch/ToggleSwitch.features.stories.tsx +++ b/src/ToggleSwitch/ToggleSwitch.features.stories.tsx @@ -1,8 +1,9 @@ -import React from 'react' -import ToggleSwitch from './ToggleSwitch' +import React, {useState} from 'react' +import ToggleSwitch, {ToggleSwitchProps} from './ToggleSwitch' import {Box, Text} from '..' import {action} from '@storybook/addon-actions' import ToggleSwitchStoryWrapper from './ToggleSwitchStoryWrapper' +import {StoryFn} from '@storybook/react' export default { title: 'Components/ToggleSwitch/Features', @@ -67,6 +68,59 @@ export const Loading = () => ( ) +type LoadingWithDelayProps = { + loadingDelay: number +} + +export const LoadingWithDelay: StoryFn = args => { + const {loadingDelay, loadingLabelDelay} = args + + const [isLoading, setIsLoading] = useState(false) + const [timeoutId, setTimeoutId] = useState(null) + + const handleToggleClick = () => { + setIsLoading(true) + + if (timeoutId) { + clearTimeout(timeoutId) + setTimeoutId(null) + } + + setTimeoutId(setTimeout(() => setIsLoading(false), loadingDelay) as unknown as number) + } + + return ( + + + Toggle label + + + + ) +} + +LoadingWithDelay.args = { + loadingDelay: 5000, + loadingLabelDelay: 2000, +} +LoadingWithDelay.argTypes = { + loadingDelay: { + control: { + type: 'number', + }, + }, + loadingLabelDelay: { + control: { + type: 'number', + }, + }, +} + export const LabelEnd = () => ( diff --git a/src/ToggleSwitch/ToggleSwitch.test.tsx b/src/ToggleSwitch/ToggleSwitch.test.tsx index 5704783d588..9112071cf8f 100644 --- a/src/ToggleSwitch/ToggleSwitch.test.tsx +++ b/src/ToggleSwitch/ToggleSwitch.test.tsx @@ -1,5 +1,5 @@ import React from 'react' -import {render} from '@testing-library/react' +import {render, waitFor} from '@testing-library/react' import ToggleSwitch from './' import {behavesAsComponent, checkExports, checkStoriesForAxeViolations} from '../utils/testing' import userEvent from '@testing-library/user-event' @@ -55,7 +55,7 @@ describe('ToggleSwitch', () => { expect(toggleSwitch).toHaveAttribute('aria-pressed', 'false') }) - it("renders a switch who's state is loading", async () => { + it('renders a switch whose state is loading', async () => { const user = userEvent.setup() const {getByLabelText, container} = render( <> @@ -180,5 +180,19 @@ describe('ToggleSwitch', () => { expect(ref).toHaveBeenCalledWith(expect.any(HTMLButtonElement)) }) + it('displays a loading label', async () => { + const TEST_ID = 'a test id' + + const {getByTestId} = render( + <> + label + + , + ) + + const toggleSwitch = getByTestId(TEST_ID) + await waitFor(() => expect(toggleSwitch).toHaveTextContent('Loading')) + }) + checkStoriesForAxeViolations('ToggleSwitch.features', '../ToggleSwitch/') }) diff --git a/src/ToggleSwitch/ToggleSwitch.tsx b/src/ToggleSwitch/ToggleSwitch.tsx index 1bbdb1ff8d7..780892f0cfe 100644 --- a/src/ToggleSwitch/ToggleSwitch.tsx +++ b/src/ToggleSwitch/ToggleSwitch.tsx @@ -1,4 +1,5 @@ import React, {MouseEventHandler, useCallback, useEffect} from 'react' +import {useId} from '../hooks/useId' import styled, {css} from 'styled-components' import {variant} from 'styled-system' import Box from '../Box' @@ -8,11 +9,14 @@ import {get} from '../constants' import {useProvidedStateOrCreate} from '../hooks' import sx, {BetterSystemStyleObject, SxProp} from '../sx' import {CellAlignment} from '../DataTable/column' +import VisuallyHidden from '../_VisuallyHidden' const TRANSITION_DURATION = '80ms' const EASE_OUT_QUAD_CURVE = 'cubic-bezier(0.5, 1, 0.89, 1)' export interface ToggleSwitchProps extends Omit, 'onChange'>, SxProp { + /** The id of the DOM node that labels the switch */ + ['aria-labelledby']: string /** Uncontrolled - whether the switch is turned on */ defaultChecked?: boolean /** Whether the switch is ready for user input */ @@ -31,6 +35,9 @@ export interface ToggleSwitchProps extends Omit` } ${props => { - if (props.disabled) { + if (props['aria-disabled']) { return css` background-color: ${get('colors.switchTrack.disabledBg')}; border-color: transparent; @@ -166,11 +173,12 @@ const SwitchButton = styled.button` ${sx} ${sizeVariants} ` -const ToggleKnob = styled.div<{checked?: boolean; disabled?: boolean}>` +const ToggleKnob = styled.div<{checked?: boolean}>` background-color: ${get('colors.switchKnob.bg')}; border-width: 1px; border-style: solid; - border-color: ${props => (props.disabled ? get('colors.switchTrack.disabledBg') : get('colors.switchKnob.border'))}; + border-color: ${props => + props['aria-disabled'] ? get('colors.switchTrack.disabledBg') : get('colors.switchKnob.border')}; border-radius: calc(${get('radii.2')} - 1px); /* -1px to account for 1px border around the control */ width: 50%; position: absolute; @@ -187,7 +195,7 @@ const ToggleKnob = styled.div<{checked?: boolean; disabled?: boolean}>` } ${props => { - if (props.disabled) { + if (props['aria-disabled']) { return css` border-color: ${get('colors.switchTrack.disabledBg')}; ` @@ -219,27 +227,60 @@ const ToggleSwitch = React.forwardRef(checked, onChange, Boolean(defaultChecked)) const acceptsInteraction = !disabled && !loading + + const [loadingLabelTimeoutId, setLoadingLabelTimeoutId] = React.useState(null) + const [isLoadingLabelVisible, setIsLoadingLabelVisible] = React.useState(false) + const loadingLabelId = useId('loadingLabel') + + const resetLoadingLabel = useCallback(() => { + if (loadingLabelTimeoutId) { + clearTimeout(loadingLabelTimeoutId) + setLoadingLabelTimeoutId(null) + } + + if (isLoadingLabelVisible) { + setIsLoadingLabelVisible(false) + } + }, [loadingLabelTimeoutId, isLoadingLabelVisible]) + const handleToggleClick: MouseEventHandler = useCallback( e => { + if (disabled || loading) return + if (!isControlled) { setIsOn(!isOn) + resetLoadingLabel() } onClick && onClick(e) }, - [onClick, isControlled, isOn, setIsOn], + [disabled, isControlled, loading, onClick, setIsOn, isOn, resetLoadingLabel], ) useEffect(() => { - if (onChange && isControlled) { + if (onChange && isControlled && !disabled) { onChange(Boolean(checked)) } - }, [onChange, checked, isControlled]) + }, [onChange, checked, isControlled, disabled]) + + if (loading) { + if (!loadingLabelTimeoutId) { + setLoadingLabelTimeoutId( + setTimeout(() => { + setLoadingLabelTimeoutId(null) + setIsLoadingLabelVisible(true) + }, loadingLabelDelay) as unknown as number, + ) + } + } else { + resetLoadingLabel() + } return ( - {loading ? : null} + {isLoadingLabelVisible ? ( + + + Loading + + + ) : null} + {loading ? : null} - ) diff --git a/src/ToggleSwitch/__snapshots__/ToggleSwitch.test.tsx.snap b/src/ToggleSwitch/__snapshots__/ToggleSwitch.test.tsx.snap index f5a73758d01..8797aab723f 100644 --- a/src/ToggleSwitch/__snapshots__/ToggleSwitch.test.tsx.snap +++ b/src/ToggleSwitch/__snapshots__/ToggleSwitch.test.tsx.snap @@ -219,10 +219,10 @@ exports[`ToggleSwitch renders consistently 1`] = ` From 2ffa8cbf689fcb04b44a0852d26b365a0ec6cef9 Mon Sep 17 00:00:00 2001 From: Cameron Dutro Date: Fri, 3 Nov 2023 10:37:32 -0700 Subject: [PATCH 2/7] Add test ensuring clicking the status label cannot toggle a disabled switch --- src/ToggleSwitch/ToggleSwitch.test.tsx | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/src/ToggleSwitch/ToggleSwitch.test.tsx b/src/ToggleSwitch/ToggleSwitch.test.tsx index 9112071cf8f..c08a145ed24 100644 --- a/src/ToggleSwitch/ToggleSwitch.test.tsx +++ b/src/ToggleSwitch/ToggleSwitch.test.tsx @@ -103,6 +103,22 @@ describe('ToggleSwitch', () => { expect(toggleSwitch).toHaveAttribute('aria-pressed', 'true') }) + it('ensures the status label cannot toggle a disabled switch', async () => { + const user = userEvent.setup() + const {getByLabelText, getByText} = render( + <> +
{SWITCH_LABEL_TEXT}
+ + , + ) + const toggleSwitch = getByLabelText(SWITCH_LABEL_TEXT) + const toggleSwitchStatusLabel = getByText('Off') + + expect(toggleSwitch).toHaveAttribute('aria-pressed', 'false') + await user.click(toggleSwitchStatusLabel) + expect(toggleSwitch).toHaveAttribute('aria-pressed', 'false') + }) + it('switches from off to on with a controlled prop', async () => { const user = userEvent.setup() const ControlledSwitchComponent = () => { From 0d50aa49e5cfab53424dfa4878a2f8cfcb6e8f9f Mon Sep 17 00:00:00 2001 From: Cameron Dutro Date: Fri, 3 Nov 2023 10:38:02 -0700 Subject: [PATCH 3/7] Add changeset --- .changeset/honest-gorillas-drive.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/honest-gorillas-drive.md diff --git a/.changeset/honest-gorillas-drive.md b/.changeset/honest-gorillas-drive.md new file mode 100644 index 00000000000..d0c76f5db4a --- /dev/null +++ b/.changeset/honest-gorillas-drive.md @@ -0,0 +1,5 @@ +--- +'@primer/react': minor +--- + +Address additional ToggleSwitch a11y feedback From c157d7b456009e76a2f835c5cb59c84cd6f4006a Mon Sep 17 00:00:00 2001 From: Cameron Dutro Date: Wed, 8 Nov 2023 10:24:04 -0800 Subject: [PATCH 4/7] Update src/ToggleSwitch/ToggleSwitch.tsx Co-authored-by: Tyler Jones --- src/ToggleSwitch/ToggleSwitch.tsx | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/ToggleSwitch/ToggleSwitch.tsx b/src/ToggleSwitch/ToggleSwitch.tsx index 780892f0cfe..c2668d02e95 100644 --- a/src/ToggleSwitch/ToggleSwitch.tsx +++ b/src/ToggleSwitch/ToggleSwitch.tsx @@ -197,6 +197,10 @@ const ToggleKnob = styled.div<{checked?: boolean}>` ${props => { if (props['aria-disabled']) { return css` + @media (forced-colors: active) { + color: GrayText; + } + border-color: ${get('colors.switchTrack.disabledBg')}; ` } From af748d49299dcaccd87a09ccbc3c866d8f196297 Mon Sep 17 00:00:00 2001 From: Cameron Dutro Date: Wed, 8 Nov 2023 10:24:10 -0800 Subject: [PATCH 5/7] Update src/ToggleSwitch/ToggleSwitch.tsx Co-authored-by: Tyler Jones --- src/ToggleSwitch/ToggleSwitch.tsx | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/ToggleSwitch/ToggleSwitch.tsx b/src/ToggleSwitch/ToggleSwitch.tsx index c2668d02e95..58cf3f59c9e 100644 --- a/src/ToggleSwitch/ToggleSwitch.tsx +++ b/src/ToggleSwitch/ToggleSwitch.tsx @@ -136,6 +136,10 @@ const SwitchButton = styled.button` ${props => { if (props['aria-disabled']) { return css` + @media (forced-colors: active) { + border-color: GrayText; + } + background-color: ${get('colors.switchTrack.disabledBg')}; border-color: transparent; cursor: not-allowed; From 9b724566168857fb4bde977048c356f92260df0d Mon Sep 17 00:00:00 2001 From: Cameron Dutro Date: Wed, 8 Nov 2023 10:29:13 -0800 Subject: [PATCH 6/7] Move aria-describedby --- src/ToggleSwitch/ToggleSwitch.tsx | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/ToggleSwitch/ToggleSwitch.tsx b/src/ToggleSwitch/ToggleSwitch.tsx index 58cf3f59c9e..52f9b8a508a 100644 --- a/src/ToggleSwitch/ToggleSwitch.tsx +++ b/src/ToggleSwitch/ToggleSwitch.tsx @@ -139,7 +139,7 @@ const SwitchButton = styled.button` @media (forced-colors: active) { border-color: GrayText; } - + background-color: ${get('colors.switchTrack.disabledBg')}; border-color: transparent; cursor: not-allowed; @@ -204,7 +204,7 @@ const ToggleKnob = styled.div<{checked?: boolean}>` @media (forced-colors: active) { color: GrayText; } - + border-color: ${get('colors.switchTrack.disabledBg')}; ` } @@ -305,7 +305,7 @@ const ToggleSwitch = React.forwardRef ) : null} - {loading ? : null} + {loading ? : null} Date: Wed, 8 Nov 2023 10:51:10 -0800 Subject: [PATCH 7/7] Update snapshot --- src/ToggleSwitch/ToggleSwitch.tsx | 5 ++++- src/ToggleSwitch/__snapshots__/ToggleSwitch.test.tsx.snap | 1 + 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/src/ToggleSwitch/ToggleSwitch.tsx b/src/ToggleSwitch/ToggleSwitch.tsx index 52f9b8a508a..9fbc5cc550d 100644 --- a/src/ToggleSwitch/ToggleSwitch.tsx +++ b/src/ToggleSwitch/ToggleSwitch.tsx @@ -290,6 +290,9 @@ const ToggleSwitch = React.forwardRef