diff --git a/.changeset/bright-buttons-announce.md b/.changeset/bright-buttons-announce.md new file mode 100644 index 00000000000..a8ff3db8b8e --- /dev/null +++ b/.changeset/bright-buttons-announce.md @@ -0,0 +1,5 @@ +--- +'@primer/react': patch +--- + +Button: Ensure loading state messages are announced when entering loading, including in React Strict Mode diff --git a/packages/react/src/Button/Button.dev.stories.tsx b/packages/react/src/Button/Button.dev.stories.tsx index 8d23de9c81f..f1099d1cc0e 100644 --- a/packages/react/src/Button/Button.dev.stories.tsx +++ b/packages/react/src/Button/Button.dev.stories.tsx @@ -1,4 +1,5 @@ import {SearchIcon, TriangleDownIcon, EyeIcon, HeartFillIcon} from '@primer/octicons-react' +import {useState} from 'react' import {Button, IconButton} from '.' import {Stack} from '../Stack' @@ -86,6 +87,27 @@ export const DisabledButtonVariants = () => { ) } +export const LoadingAnnouncement = () => { + const [isLoading, setIsLoading] = useState(false) + + return ( + +

+ With a screen reader running, activate “Export repository”. The button enters its loading state and announces + “Exporting repository”. Reset the loading state to repeat the test. +

+ + + + +
+ ) +} + export const LinkVariantWithUnderlinePreference = () => { return ( diff --git a/packages/react/src/Button/ButtonBase.tsx b/packages/react/src/Button/ButtonBase.tsx index d4747b94d4d..96abbbfbad4 100644 --- a/packages/react/src/Button/ButtonBase.tsx +++ b/packages/react/src/Button/ButtonBase.tsx @@ -200,7 +200,9 @@ const ButtonBase = forwardRef(({children, as: Component = 'button', ...props}, f {loading && ( - {loadingAnnouncement} + + {loadingAnnouncement} + )} diff --git a/packages/react/src/Button/__tests__/Button.test.tsx b/packages/react/src/Button/__tests__/Button.test.tsx index e95eb5d614d..0784fd3907c 100644 --- a/packages/react/src/Button/__tests__/Button.test.tsx +++ b/packages/react/src/Button/__tests__/Button.test.tsx @@ -7,6 +7,7 @@ import {IconButton, Button, LinkButton} from '../../Button' import classes from '../ButtonBase.module.css' import {implementsClassName} from '../../utils/testing' import {FeatureFlags} from '../../FeatureFlags' +import {getLiveRegion} from '../../live-region/__tests__/test-helpers' type StatefulLoadingButtonProps = { children?: React.ReactNode @@ -295,6 +296,25 @@ describe('Button', () => { .toBe('Action loading') }) + it('announces the loading announcement when entering the loading state', async () => { + for (const region of document.querySelectorAll('live-region')) { + region.remove() + } + const liveRegion = document.createElement('live-region') + document.body.appendChild(liveRegion) + + render( + + content + , + ) + + await userEvent.click(page.getByRole('button')) + + await expect.poll(() => getLiveRegion().getMessage('polite')).toBe('Action loading') + liveRegion.remove() + }) + it('should be described by loading announcement AND whatever is passed to aria-describedby', async () => { const buttonDescriptionId = 'button-description' const buttonId = 'loading-button' diff --git a/packages/react/src/live-region/Announce.tsx b/packages/react/src/live-region/Announce.tsx index ae67d2fc51a..f095c95ef1b 100644 --- a/packages/react/src/live-region/Announce.tsx +++ b/packages/react/src/live-region/Announce.tsx @@ -1,7 +1,6 @@ import {announceFromElement} from '@primer/live-region-element' import type React from 'react' import {useEffect, useRef, type ElementRef} from 'react' -import {useEffectOnce} from '../internal/hooks/useEffectOnce' import {useEffectCallback} from '../internal/hooks/useEffectCallback' import type {PolymorphicProps} from '../utils/modern-polymorphic' @@ -52,6 +51,7 @@ export function Announce(props: AnnouncePr ...rest } = props const ref = useRef>(null) + // Tracked in a ref rather than state because it is only used to dedupe // announcements and is never rendered. Using state here would trigger a React // commit on every content change (e.g. one per keystroke when tied to an input). @@ -99,14 +99,16 @@ export function Announce(props: AnnouncePr previousAnnouncementText.current = textContent }) - // Announce the initial message, this is wrapped in `useEffectOnce` so that it - // does not announce twice in StrictMode - useEffectOnce(() => { + const announceOnMount = useEffectCallback(() => { if (announceOnShow) { announce() } }) + useEffect(() => { + announceOnMount() + }, [announceOnMount]) + useEffect(() => { const {current: container} = ref if (container === null) { @@ -131,10 +133,9 @@ export function Announce(props: AnnouncePr useEffect(() => { return () => { - if (savedAnnouncement.current !== null) { - savedAnnouncement.current.cancel() - savedAnnouncement.current = null - } + savedAnnouncement.current?.cancel() + savedAnnouncement.current = null + previousAnnouncementText.current = null } }, []) diff --git a/packages/react/src/live-region/__tests__/Announce.test.tsx b/packages/react/src/live-region/__tests__/Announce.test.tsx index 5512adec0ef..98d71c1ba2b 100644 --- a/packages/react/src/live-region/__tests__/Announce.test.tsx +++ b/packages/react/src/live-region/__tests__/Announce.test.tsx @@ -1,5 +1,6 @@ import {describe, expect, it, beforeEach, afterEach} from 'vitest' import {render, screen, act, waitFor} from '@testing-library/react' +import {StrictMode} from 'react' import {Announce} from '../Announce' import {getLiveRegion} from './test-helpers' import {implementsClassName} from '../../utils/testing' @@ -34,6 +35,37 @@ describe('Announce', () => { expect(liveRegion.getMessage('polite')).toBe('') }) + it('announces delayed content when rendered in StrictMode', async () => { + render( + + test + , + ) + + await waitFor(() => { + expect(getLiveRegion().getMessage('polite')).toBe('test') + }) + }) + + it('does not announce when announceOnShow changes after mount', () => { + const {rerender} = render(test) + + rerender(test) + + expect(getLiveRegion().getMessage('polite')).toBe('') + }) + + it('cancels delayed content after unmounting', async () => { + const {unmount} = render(test) + + unmount() + await act(async () => { + await new Promise(resolve => setTimeout(resolve, 75)) + }) + + expect(getLiveRegion().getMessage('polite')).toBe('') + }) + it('should pass additional props to the container element', () => { const {container} = render(test)