From a6bb6c4e76322b581deeb23305c97e073013ef33 Mon Sep 17 00:00:00 2001 From: Josh Black Date: Thu, 29 Oct 2026 15:22:26 -0500 Subject: [PATCH 01/12] refactor: use ariaNotify for Announce --- package-lock.json | 7 + packages/react/package.json | 7 +- packages/react/src/live-region/Announce.tsx | 60 +++--- .../live-region/__tests__/Announce.test.tsx | 203 +++++++++++++++--- packages/react/src/live-region/polyfill.ts | 17 ++ 5 files changed, 233 insertions(+), 61 deletions(-) create mode 100644 packages/react/src/live-region/polyfill.ts diff --git a/package-lock.json b/package-lock.json index 7781e7e497d..a48beb0b34e 100644 --- a/package-lock.json +++ b/package-lock.json @@ -4781,6 +4781,12 @@ } } }, + "node_modules/@github/arianotify-polyfill": { + "version": "2.4.0", + "resolved": "https://registry.npmjs.org/@github/arianotify-polyfill/-/arianotify-polyfill-2.4.0.tgz", + "integrity": "sha512-pUhesPIflu3FpabgU55EUZVdowrsT5ax1k818Ffoo0TgxBKXkTy+HT74I3wk9VZKf++JdbBikHB2+tTwu2GwWg==", + "license": "MIT" + }, "node_modules/@github/axe-github": { "version": "0.7.0", "dev": true, @@ -31937,6 +31943,7 @@ "version": "38.43.0", "license": "MIT", "dependencies": { + "@github/arianotify-polyfill": "^2.4.0", "@github/mini-throttle": "^2.1.1", "@github/relative-time-element": "^5.0.0", "@lit-labs/react": "1.2.1", diff --git a/packages/react/package.json b/packages/react/package.json index 97027d07ccc..11138b78c48 100644 --- a/packages/react/package.json +++ b/packages/react/package.json @@ -33,7 +33,9 @@ "sideEffects": [ "dist/**/*.css", "src/**/test-helpers.tsx", - "dist/**/test-helpers.js" + "src/**/polyfill.ts", + "dist/**/test-helpers.js", + "dist/**/polyfill.js" ], "scripts": { "build": "./script/build", @@ -73,6 +75,7 @@ "npm": ">=7" }, "dependencies": { + "@github/arianotify-polyfill": "^2.4.0", "@github/mini-throttle": "^2.1.1", "@github/relative-time-element": "^5.0.0", "@lit-labs/react": "1.2.1", @@ -98,9 +101,9 @@ "@babel/core": "8.0.7", "@babel/generator": "8.0.6", "@babel/parser": "8.0.7", + "@babel/plugin-transform-modules-commonjs": "8.0.1", "@babel/plugin-transform-nullish-coalescing-operator": "8.0.1", "@babel/plugin-transform-optional-chaining": "8.0.5", - "@babel/plugin-transform-modules-commonjs": "8.0.1", "@babel/preset-react": "8.0.1", "@babel/preset-typescript": "8.0.7", "@babel/types": "8.0.6", diff --git a/packages/react/src/live-region/Announce.tsx b/packages/react/src/live-region/Announce.tsx index ae67d2fc51a..cb0fdc7210c 100644 --- a/packages/react/src/live-region/Announce.tsx +++ b/packages/react/src/live-region/Announce.tsx @@ -1,9 +1,8 @@ -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' +import './polyfill' export type AnnounceProps = PolymorphicProps< As, @@ -44,9 +43,9 @@ export type AnnounceProps = PolymorphicProps< export function Announce(props: AnnounceProps) { const { as: BaseComponent = 'div', - announceOnShow = true, + announceOnShow: shouldAnnounceOnShow = true, children, - delayMs, + delayMs = 0, hidden = false, politeness = 'polite', ...rest @@ -56,8 +55,13 @@ export function Announce(props: AnnouncePr // 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). const previousAnnouncementText = useRef(null) - const savedAnnouncement = useRef | null>(null) + const savedAnnouncement = useRef(null) const announce = useEffectCallback(() => { + if (savedAnnouncement.current !== null) { + clearTimeout(savedAnnouncement.current) + savedAnnouncement.current = null + } + const {current: element} = ref if (!element) { return @@ -84,29 +88,25 @@ export function Announce(props: AnnouncePr return } - savedAnnouncement.current?.cancel() - savedAnnouncement.current = announceFromElement( - element, - politeness === 'assertive' - ? { - politeness, - } - : { - politeness, - delayMs, - }, - ) - previousAnnouncementText.current = textContent + const timeoutMs = politeness === 'assertive' ? 0 : delayMs + savedAnnouncement.current = window.setTimeout(() => { + element.ariaNotify(textContent, { + priority: politeness === 'assertive' ? 'high' : 'normal', + }) + savedAnnouncement.current = null + previousAnnouncementText.current = textContent + }, timeoutMs) }) - - // Announce the initial message, this is wrapped in `useEffectOnce` so that it - // does not announce twice in StrictMode - useEffectOnce(() => { - if (announceOnShow) { + const announceOnShow = useEffectCallback(() => { + if (shouldAnnounceOnShow) { announce() } }) + useEffect(() => { + announceOnShow() + }, [announceOnShow]) + useEffect(() => { const {current: container} = ref if (container === null) { @@ -132,9 +132,13 @@ export function Announce(props: AnnouncePr useEffect(() => { return () => { if (savedAnnouncement.current !== null) { - savedAnnouncement.current.cancel() + clearTimeout(savedAnnouncement.current) savedAnnouncement.current = null } + + if (previousAnnouncementText.current !== null) { + previousAnnouncementText.current = null + } } }, []) @@ -146,12 +150,16 @@ export function Announce(props: AnnouncePr } function getTextContent(element: HTMLElement): string { - let value = '' + let value: string | null = '' + if (element.hasAttribute('aria-label')) { - value = element.getAttribute('aria-label')! + value = element.getAttribute('aria-label') + } else if (element.innerText) { + value = element.innerText } else if (element.textContent) { value = element.textContent } + return value ? value.trim() : '' } diff --git a/packages/react/src/live-region/__tests__/Announce.test.tsx b/packages/react/src/live-region/__tests__/Announce.test.tsx index 5512adec0ef..6c6209984df 100644 --- a/packages/react/src/live-region/__tests__/Announce.test.tsx +++ b/packages/react/src/live-region/__tests__/Announce.test.tsx @@ -1,37 +1,48 @@ -import {describe, expect, it, beforeEach, afterEach} from 'vitest' -import {render, screen, act, waitFor} from '@testing-library/react' +import {StrictMode} from 'react' +import {describe, expect, it, beforeEach, afterEach, vi, type MockInstance} from 'vitest' +import {render, act} from '@testing-library/react' +import {page} from 'vitest/browser' import {Announce} from '../Announce' -import {getLiveRegion} from './test-helpers' import {implementsClassName} from '../../utils/testing' import {createRenderCounter} from '../../utils/testing/profiler' describe('Announce', () => { + let ariaNotify: MockInstance + implementsClassName(Announce) + beforeEach(() => { - const liveRegion = document.createElement('live-region') - document.body.appendChild(liveRegion) + vi.useFakeTimers() + ariaNotify = vi.spyOn(Element.prototype, 'ariaNotify').mockImplementation(() => {}) }) afterEach(() => { - // Reset the live-region after each test so that we do not have overlapping - // messages from previous tests - const liveRegion = getLiveRegion() - document.body.removeChild(liveRegion) + vi.useRealTimers() + vi.restoreAllMocks() }) - it('should have a default politeness of `polite`', () => { + it('should have a default politeness of `polite`', async () => { render(test) - const liveRegion = getLiveRegion() - expect(liveRegion.getMessage('polite')).toBe('test') + await act(async () => { + await vi.runAllTimersAsync() + }) + + expect(ariaNotify).toHaveBeenCalledExactlyOnceWith('test', { + priority: 'normal', + }) }) - it('should support an `assertive` politeness', () => { + it('should support an `assertive` politeness', async () => { render(test) - const liveRegion = getLiveRegion() - expect(liveRegion.getMessage('assertive')).toBe('test') - expect(liveRegion.getMessage('polite')).toBe('') + await act(async () => { + await vi.runAllTimersAsync() + }) + + expect(ariaNotify).toHaveBeenCalledExactlyOnceWith('test', { + priority: 'high', + }) }) it('should pass additional props to the container element', () => { @@ -46,17 +57,20 @@ describe('Announce', () => { test , ) - expect(screen.getByTestId('container').tagName).toBe('SPAN') + expect(page.getByTestId('container').element().tagName).toBe('SPAN') }) - it('should not announce the contents of the container if `hidden={false}`', () => { + it('should not announce the contents of the container if `hidden={true}`', async () => { render() - const liveRegion = getLiveRegion() - expect(liveRegion.getMessage('polite')).not.toBe('test') + await act(async () => { + await vi.runAllTimersAsync() + }) + + expect(ariaNotify).not.toHaveBeenCalled() }) - it('should not announce the contents of the container if `display: none`', () => { + it('should not announce the contents of the container if `display: none`', async () => { render( { , ) - const liveRegion = getLiveRegion() - expect(liveRegion.getMessage('polite')).not.toBe('test') + await act(async () => { + await vi.runAllTimersAsync() + }) + + expect(ariaNotify).not.toHaveBeenCalled() }) - it('should not announce the contents of the container if `visibility: hidden`', () => { + it('should not announce the contents of the container if `visibility: hidden`', async () => { render( { , ) - const liveRegion = getLiveRegion() - expect(liveRegion.getMessage('polite')).not.toBe('test') + await act(async () => { + await vi.runAllTimersAsync() + }) + + expect(ariaNotify).not.toHaveBeenCalled() + }) + + it('does not announce hidden descendant text', async () => { + render( + + Visible message + + Hidden message + + , + ) + + await act(async () => { + await vi.runAllTimersAsync() + }) + + expect(ariaNotify).toHaveBeenCalledExactlyOnceWith('Visible message', { + priority: 'normal', + }) + }) + + it.each(['polite', 'assertive'] as const)( + 'announces once on mount in StrictMode with %s politeness', + async politeness => { + render( + + test + , + ) + + await act(async () => { + await vi.runAllTimersAsync() + }) + + expect(ariaNotify).toHaveBeenCalledExactlyOnceWith('test', { + priority: politeness === 'assertive' ? 'high' : 'normal', + }) + }, + ) + + it('delays polite announcements and replaces pending content', async () => { + const {rerender} = render(First message) + + await act(async () => { + await vi.advanceTimersByTimeAsync(99) + }) + expect(ariaNotify).not.toHaveBeenCalled() + + rerender(Updated message) + + await act(async () => { + await Promise.resolve() + await vi.advanceTimersByTimeAsync(99) + }) + expect(ariaNotify).not.toHaveBeenCalled() + + await act(async () => { + await vi.advanceTimersByTimeAsync(1) + }) + expect(ariaNotify).toHaveBeenCalledExactlyOnceWith('Updated message', { + priority: 'normal', + }) + }) + + it('ignores delayMs for assertive announcements', async () => { + render( + + test + , + ) + + await act(async () => { + await vi.advanceTimersByTimeAsync(0) + }) + + expect(ariaNotify).toHaveBeenCalledExactlyOnceWith('test', { + priority: 'high', + }) + }) + + it.each(['message A', ''])('cancels pending content when the content changes back to "%s"', async message => { + const {rerender} = render(message A) + + await act(async () => { + await vi.runAllTimersAsync() + }) + expect(ariaNotify).toHaveBeenCalledExactlyOnceWith('message A', { + priority: 'normal', + }) + ariaNotify.mockClear() + + rerender(message B) + await act(async () => {}) + + rerender({message}) + await act(async () => { + await vi.runAllTimersAsync() + }) + + expect(ariaNotify).not.toHaveBeenCalled() + }) + + it('cancels a pending announcement on unmount', async () => { + const {unmount} = render(test) + + unmount() + + await act(async () => { + await vi.runAllTimersAsync() + }) + + expect(ariaNotify).not.toHaveBeenCalled() }) it('does not commit an extra render when its content changes', async () => { @@ -99,7 +235,9 @@ describe('Announce', () => { , ) // Flush the initial mount announcement before measuring. - await act(async () => {}) + await act(async () => { + await vi.runAllTimersAsync() + }) counter.reset() const changes = 5 @@ -113,13 +251,12 @@ describe('Announce', () => { await act(async () => {}) } - // The announcement path actually ran (so the render-count assertion is not - // vacuous): wait for the live region's throttle queue to drain and surface - // the latest content... - await waitFor(() => { - expect(getLiveRegion().getMessage('polite')).toBe('message 5') + await act(async () => { + await vi.runAllTimersAsync() + }) + expect(ariaNotify).toHaveBeenLastCalledWith('message 5', { + priority: 'normal', }) - // ...and it did so without any extra commit beyond the prop updates. expect(counter.updateCount).toBe(changes) }) }) diff --git a/packages/react/src/live-region/polyfill.ts b/packages/react/src/live-region/polyfill.ts new file mode 100644 index 00000000000..65b996413e8 --- /dev/null +++ b/packages/react/src/live-region/polyfill.ts @@ -0,0 +1,17 @@ +import '@github/arianotify-polyfill' + +type AriaNotifyOptions = { + priority?: 'normal' | 'high' +} + +declare global { + interface Document { + ariaNotify(announcement: string, options?: AriaNotifyOptions): void + } + + interface Element { + ariaNotify(announcement: string, options?: AriaNotifyOptions): void + } +} + +export type {AriaNotifyOptions} From 2f026ce699d799c6952907e88038aca37656b0a9 Mon Sep 17 00:00:00 2001 From: Josh Black Date: Thu, 29 Oct 2026 15:25:40 -0500 Subject: [PATCH 02/12] test: update AriaAlert and AriaStatus tests --- .../live-region/__tests__/AriaAlert.test.tsx | 70 +++++----- .../live-region/__tests__/AriaStatus.test.tsx | 126 ++++++++---------- 2 files changed, 88 insertions(+), 108 deletions(-) diff --git a/packages/react/src/live-region/__tests__/AriaAlert.test.tsx b/packages/react/src/live-region/__tests__/AriaAlert.test.tsx index df5605a22c8..e293831c31c 100644 --- a/packages/react/src/live-region/__tests__/AriaAlert.test.tsx +++ b/packages/react/src/live-region/__tests__/AriaAlert.test.tsx @@ -1,30 +1,34 @@ -import {describe, expect, it, beforeEach, afterEach} from 'vitest' -import {render, screen} from '@testing-library/react' -import React from 'react' +import {describe, expect, it, beforeEach, afterEach, vi, type MockInstance} from 'vitest' +import {render, act} from '@testing-library/react' +import {page} from 'vitest/browser' import {AriaAlert} from '../AriaAlert' -import {userEvent} from '@testing-library/user-event' -import {getLiveRegion} from './test-helpers' import {implementsClassName} from '../../utils/testing' describe('AriaAlert', () => { + let ariaNotify: MockInstance + implementsClassName(AriaAlert) + beforeEach(() => { - const liveRegion = document.createElement('live-region') - document.body.appendChild(liveRegion) + vi.useFakeTimers() + ariaNotify = vi.spyOn(Element.prototype, 'ariaNotify').mockImplementation(() => {}) }) afterEach(() => { - // Reset the live-region after each test so that we do not have overlapping - // messages from previous tests - const liveRegion = getLiveRegion() - document.body.removeChild(liveRegion) + vi.useRealTimers() + vi.restoreAllMocks() }) - it('should have a default politeness of `assertive`', () => { + it('should have a default politeness of `assertive`', async () => { render(test) - const liveRegion = getLiveRegion() - expect(liveRegion.getMessage('assertive')).toBe('test') + await act(async () => { + await vi.runAllTimersAsync() + }) + + expect(ariaNotify).toHaveBeenCalledExactlyOnceWith('test', { + priority: 'high', + }) }) it('should pass additional props to the container element', () => { @@ -39,34 +43,24 @@ describe('AriaAlert', () => { test , ) - expect(screen.getByTestId('container').tagName).toBe('SPAN') + expect(page.getByTestId('container').element().tagName).toBe('SPAN') }) - it('should update live-region element when AriaAlert goes from empty to populated', async () => { - function TestComponent() { - const [show, setShow] = React.useState(false) - return ( - <> - {show ? 'Failed to export data!' : null} - - - ) - } - const user = userEvent.setup() + it('should announce when AriaAlert goes from empty to populated', async () => { + const {rerender} = render({null}) - render() + await act(async () => { + await vi.runAllTimersAsync() + }) + expect(ariaNotify).not.toHaveBeenCalled() - const liveRegion = getLiveRegion() - expect(liveRegion.getMessage('assertive')).toBe('') + rerender(Failed to export data!) - await user.click(screen.getByText('Export data')) - expect(liveRegion.getMessage('assertive')).toBe('Failed to export data!') + await act(async () => { + await vi.runAllTimersAsync() + }) + expect(ariaNotify).toHaveBeenCalledExactlyOnceWith('Failed to export data!', { + priority: 'high', + }) }) }) diff --git a/packages/react/src/live-region/__tests__/AriaStatus.test.tsx b/packages/react/src/live-region/__tests__/AriaStatus.test.tsx index 583f74b555a..5015e370bb3 100644 --- a/packages/react/src/live-region/__tests__/AriaStatus.test.tsx +++ b/packages/react/src/live-region/__tests__/AriaStatus.test.tsx @@ -1,66 +1,62 @@ -import {describe, expect, it, beforeEach, afterEach} from 'vitest' -import {render, screen} from '@testing-library/react' -import React from 'react' +import {describe, expect, it, beforeEach, afterEach, vi, type MockInstance} from 'vitest' +import {render, act} from '@testing-library/react' +import {page} from 'vitest/browser' import {AriaStatus} from '../AriaStatus' -import {userEvent} from '@testing-library/user-event' -import {getLiveRegion} from './test-helpers' import {implementsClassName} from '../../utils/testing' describe('AriaStatus', () => { + let ariaNotify: MockInstance + implementsClassName(AriaStatus) + beforeEach(() => { - const liveRegion = document.createElement('live-region') - document.body.appendChild(liveRegion) + vi.useFakeTimers() + ariaNotify = vi.spyOn(Element.prototype, 'ariaNotify').mockImplementation(() => {}) }) afterEach(() => { - // Reset the live-region after each test so that we do not have overlapping - // messages from previous tests - const liveRegion = getLiveRegion() - document.body.removeChild(liveRegion) + vi.useRealTimers() + vi.restoreAllMocks() }) - it('should not announce on show by default', () => { + it('should not announce on show by default', async () => { render(test) - const liveRegion = getLiveRegion() - expect(liveRegion.getMessage('polite')).not.toBe('test') + await act(async () => { + await vi.runAllTimersAsync() + }) + + expect(ariaNotify).not.toHaveBeenCalled() }) - it('should have a default politeness of `polite`', () => { + it('should have a default politeness of `polite`', async () => { render(test) - const liveRegion = getLiveRegion() - expect(liveRegion.getMessage('polite')).toBe('test') + await act(async () => { + await vi.runAllTimersAsync() + }) + + expect(ariaNotify).toHaveBeenCalledExactlyOnceWith('test', { + priority: 'normal', + }) }) it('should announce on content change', async () => { - function TestComponent() { - const [message, setMessage] = React.useState('default message') - return ( - <> - {message} - - - ) - } - - const user = userEvent.setup() - - render() - - const liveRegion = getLiveRegion() - expect(liveRegion.getMessage('polite')).not.toBe('test') - - await user.click(screen.getByText('Update message')) - expect(liveRegion.getMessage('polite')).toBe('updated message') + const {rerender} = render(default message) + + await act(async () => { + await vi.runAllTimersAsync() + }) + expect(ariaNotify).not.toHaveBeenCalled() + + rerender(updated message) + + await act(async () => { + await vi.runAllTimersAsync() + }) + expect(ariaNotify).toHaveBeenCalledExactlyOnceWith('updated message', { + priority: 'normal', + }) }) it('should pass additional props to the container element', () => { @@ -75,34 +71,24 @@ describe('AriaStatus', () => { test , ) - expect(screen.getByTestId('container').tagName).toBe('SPAN') + expect(page.getByTestId('container').element().tagName).toBe('SPAN') }) - it('should update live-region element when AriaStatus goes from empty to populated', async () => { - function TestComponent() { - const [show, setShow] = React.useState(false) - return ( - <> - {show ? 'Export completed' : null} - - - ) - } - const user = userEvent.setup() - - render() - - const liveRegion = getLiveRegion() - expect(liveRegion.getMessage('polite')).toBe('') - - await user.click(screen.getByText('Export data')) - expect(liveRegion.getMessage('polite')).toBe('Export completed') + it('should announce when AriaStatus goes from empty to populated', async () => { + const {rerender} = render({null}) + + await act(async () => { + await vi.runAllTimersAsync() + }) + expect(ariaNotify).not.toHaveBeenCalled() + + rerender(Export completed) + + await act(async () => { + await vi.runAllTimersAsync() + }) + expect(ariaNotify).toHaveBeenCalledExactlyOnceWith('Export completed', { + priority: 'normal', + }) }) }) From 17492fbdf117985639318694510e179fd820931b Mon Sep 17 00:00:00 2001 From: Josh Black Date: Thu, 29 Oct 2026 15:52:59 -0500 Subject: [PATCH 03/12] refactor: replace live-region-element usage with useAnnounce --- .../src/Autocomplete/AutocompleteMenu.tsx | 36 ++++++------- packages/react/src/live-region/index.ts | 2 + packages/react/src/live-region/useAnnounce.ts | 51 +++++++++++++++++++ 3 files changed, 71 insertions(+), 18 deletions(-) create mode 100644 packages/react/src/live-region/useAnnounce.ts diff --git a/packages/react/src/Autocomplete/AutocompleteMenu.tsx b/packages/react/src/Autocomplete/AutocompleteMenu.tsx index 1e9ea6addbb..5ae9c37117a 100644 --- a/packages/react/src/Autocomplete/AutocompleteMenu.tsx +++ b/packages/react/src/Autocomplete/AutocompleteMenu.tsx @@ -1,19 +1,18 @@ -import React, {useContext, useEffect, useMemo, useRef, useState} from 'react' -import {debounce} from '@github/mini-throttle' -import {announce} from '@primer/live-region-element' import {scrollIntoView} from '@primer/behaviors' import type {ScrollIntoViewOptions} from '@primer/behaviors' +import type {IconProps} from '@primer/octicons-react' +import {PlusIcon} from '@primer/octicons-react' +import React, {useContext, useEffect, useMemo, useRef, useState} from 'react' +import {isElement} from 'react-is' import type {ActionListItemProps} from '../ActionList' import {ActionList} from '../ActionList' import {useFocusZone} from '../hooks/useFocusZone' -import type {ComponentProps, MandateProps, AriaRole} from '../utils/types' +import {AutocompleteContext, AutocompleteDeferredInputContext} from './AutocompleteContext' import Spinner from '../Spinner' +import {useAnnounce} from '../live-region' import {useId} from '../hooks/useId' -import {AutocompleteContext, AutocompleteDeferredInputContext} from './AutocompleteContext' -import type {IconProps} from '@primer/octicons-react' -import {PlusIcon} from '@primer/octicons-react' +import type {ComponentProps, MandateProps, AriaRole} from '../utils/types' import VisuallyHidden from '../_VisuallyHidden' -import {isElement} from 'react-is' import classes from './AutocompleteMenu.module.css' @@ -122,14 +121,6 @@ export type AutocompleteMenuInternalProps = { ['aria-labelledby']: string } -/** - * Announces a message to screen readers at a slowed-down rate. This is useful when you want to announce don't want to - * overwhelm the user with too many announcements in rapid succession. - */ -const debounceAnnouncement = debounce((announcement: string) => { - announce(announcement) -}, 250) - function AutocompleteMenu(props: AutocompleteMenuInternalProps) { const autocompleteContext = useContext(AutocompleteContext) const deferredInputContext = useContext(AutocompleteDeferredInputContext) @@ -170,6 +161,7 @@ function AutocompleteMenu(props: AutocompleteMe const [highlightedItem, setHighlightedItem] = useState() const [sortedItemIds, setSortedItemIds] = useState>(items.map(({id: itemId}) => itemId)) const generatedUniqueId = useId(id) + const {announce, cancel} = useAnnounce() const selectableItems = useMemo( () => @@ -280,9 +272,17 @@ function AutocompleteMenu(props: AutocompleteMe React.useEffect(() => { if (allItemsToRender.length === 0) { - debounceAnnouncement(emptyStateText as string) + /** + * Announces a message to screen readers at a slowed-down rate. This is useful when you want to announce don't want to + * overwhelm the user with too many announcements in rapid succession. + */ + announce(emptyStateText as string, { + delayMs: 250, + }) + } else { + cancel() } - }, [allItemsToRender, emptyStateText]) + }, [allItemsToRender, announce, cancel, emptyStateText]) useFocusZone( { diff --git a/packages/react/src/live-region/index.ts b/packages/react/src/live-region/index.ts index 5b98e55dbbe..2c5bfd5b0a6 100644 --- a/packages/react/src/live-region/index.ts +++ b/packages/react/src/live-region/index.ts @@ -6,3 +6,5 @@ export type {AriaAlertProps} from './AriaAlert' export {AriaStatus} from './AriaStatus' export type {AriaStatusProps} from './AriaStatus' + +export {useAnnounce} from './useAnnounce' diff --git a/packages/react/src/live-region/useAnnounce.ts b/packages/react/src/live-region/useAnnounce.ts new file mode 100644 index 00000000000..a494180bed0 --- /dev/null +++ b/packages/react/src/live-region/useAnnounce.ts @@ -0,0 +1,51 @@ +import {useEffect, useRef} from 'react' + +type AnnounceOptions = { + priority?: 'high' | 'normal' + delayMs?: number +} + +const defaultAnnounceOptions: AnnounceOptions = {} + +export function useAnnounce() { + const timeoutId = useRef(null) + + useEffect(() => { + return () => { + if (timeoutId.current !== null) { + clearTimeout(timeoutId.current) + timeoutId.current = null + } + } + }, []) + + return { + announce(message: string, {priority = 'normal', delayMs}: AnnounceOptions = defaultAnnounceOptions) { + if (timeoutId.current !== null) { + clearTimeout(timeoutId.current) + timeoutId.current = null + } + + if (priority === 'high') { + document.ariaNotify(message, { + priority: 'high', + }) + } else if (typeof delayMs !== 'number') { + document.ariaNotify(message, { + priority: 'normal', + }) + } else { + timeoutId.current = window.setTimeout(() => { + document.ariaNotify(message, {priority: 'normal'}) + timeoutId.current = null + }, delayMs) + } + }, + cancel() { + if (timeoutId.current !== null) { + clearTimeout(timeoutId.current) + timeoutId.current = null + } + }, + } +} From 32955d80f25338603f32d503bd6a3d51b8fbe771 Mon Sep 17 00:00:00 2001 From: Josh Black Date: Thu, 29 Oct 2026 16:33:01 -0500 Subject: [PATCH 04/12] refactor: use ariaNotify for SelectPanel, FilteredActionList --- .changeset/select-panel-announcements.md | 5 + .../useAnnouncements.test.tsx | 193 +++++++++--- .../FilteredActionList/useAnnouncements.tsx | 67 ++-- .../src/SelectPanel/SelectPanel.test.tsx | 288 +++++++++++++++--- .../react/src/SelectPanel/SelectPanel.tsx | 64 ++-- 5 files changed, 481 insertions(+), 136 deletions(-) create mode 100644 .changeset/select-panel-announcements.md diff --git a/.changeset/select-panel-announcements.md b/.changeset/select-panel-announcements.md new file mode 100644 index 00000000000..59fe9359fd4 --- /dev/null +++ b/.changeset/select-panel-announcements.md @@ -0,0 +1,5 @@ +--- +'@primer/react': patch +--- + +SelectPanel: Prevent stale loading and list announcements after results arrive or the panel closes diff --git a/packages/react/src/FilteredActionList/useAnnouncements.test.tsx b/packages/react/src/FilteredActionList/useAnnouncements.test.tsx index ced1cf137b7..9f8ed1abfc6 100644 --- a/packages/react/src/FilteredActionList/useAnnouncements.test.tsx +++ b/packages/react/src/FilteredActionList/useAnnouncements.test.tsx @@ -1,15 +1,18 @@ -import {act, renderHook, waitFor} from '@testing-library/react' -import {announce} from '@primer/live-region-element' -import {beforeEach, describe, expect, it, vi} from 'vitest' +import {act, renderHook} from '@testing-library/react' +import {afterEach, beforeEach, describe, expect, it, vi, type MockInstance} from 'vitest' import {useAnnouncements} from './useAnnouncements' -vi.mock('@primer/live-region-element', () => ({ - announce: vi.fn(), -})) - describe('useAnnouncements', () => { + let ariaNotify: MockInstance + beforeEach(() => { - vi.mocked(announce).mockClear() + vi.useFakeTimers() + ariaNotify = vi.spyOn(document, 'ariaNotify').mockImplementation(() => {}) + }) + + afterEach(() => { + vi.useRealTimers() + vi.restoreAllMocks() }) it('includes the input label in the initial focus announcement', async () => { @@ -25,20 +28,31 @@ describe('useAnnouncements', () => { list.append(activeOption) const listRef: {current: HTMLUListElement | null} = {current: null} - const {result} = renderHook(() => - useAnnouncements([{text: 'Start date'}], listRef, {current: input}, true, false, undefined, 'active-descendant'), - ) + const {result} = renderHook(() => { + return useAnnouncements( + [{text: 'Start date'}], + listRef, + {current: input}, + true, + false, + undefined, + 'active-descendant', + ) + }) act(() => { result.current() listRef.current = list }) - await waitFor(() => - expect(announce).toHaveBeenCalledWith( - 'Filter fields, filter text box and list of items, Focused item: Start date, not selected, 1 of 1', - {delayMs: 500, from: undefined}, - ), + await act(async () => { + await vi.runAllTimersAsync() + }) + expect(ariaNotify).toHaveBeenCalledExactlyOnceWith( + 'Filter fields, filter text box and list of items, Focused item: Start date, not selected, 1 of 1', + { + priority: 'normal', + }, ) }) @@ -60,8 +74,8 @@ describe('useAnnouncements', () => { activeOption.textContent = 'Start date' list.append(activeOption) - const {result} = renderHook(() => - useAnnouncements( + const {result} = renderHook(() => { + return useAnnouncements( [{text: 'Start date'}], {current: list}, {current: input}, @@ -69,16 +83,21 @@ describe('useAnnouncements', () => { false, undefined, 'active-descendant', - ), - ) + ) + }) - act(() => result.current()) + act(() => { + result.current() + }) - await waitFor(() => - expect(announce).toHaveBeenCalledWith( - 'Issue fields, filter text box and list of items, Focused item: Start date, not selected, 1 of 1', - {delayMs: 500, from: undefined}, - ), + await act(async () => { + await vi.runAllTimersAsync() + }) + expect(ariaNotify).toHaveBeenCalledExactlyOnceWith( + 'Issue fields, filter text box and list of items, Focused item: Start date, not selected, 1 of 1', + { + priority: 'normal', + }, ) label.remove() @@ -101,8 +120,8 @@ describe('useAnnouncements', () => { activeOption.textContent = 'Start date' list.append(activeOption) - const {result} = renderHook(() => - useAnnouncements( + const {result} = renderHook(() => { + return useAnnouncements( [{text: 'Start date'}], {current: list}, {current: input}, @@ -110,23 +129,28 @@ describe('useAnnouncements', () => { false, undefined, 'active-descendant', - ), - ) + ) + }) - act(() => result.current()) + act(() => { + result.current() + }) - await waitFor(() => - expect(announce).toHaveBeenCalledWith( - 'Filter fields, filter text box and list of items, Focused item: Start date, not selected, 1 of 1', - {delayMs: 500, from: undefined}, - ), + await act(async () => { + await vi.runAllTimersAsync() + }) + expect(ariaNotify).toHaveBeenCalledExactlyOnceWith( + 'Filter fields, filter text box and list of items, Focused item: Start date, not selected, 1 of 1', + { + priority: 'normal', + }, ) label.remove() input.remove() }) - it('does not announce when items are recreated with the same announcement state', () => { + it('does not announce when items are recreated with the same announcement state', async () => { const list = document.createElement('ul') const activeOption = document.createElement('li') activeOption.setAttribute('role', 'option') @@ -134,26 +158,105 @@ describe('useAnnouncements', () => { list.append(activeOption) const {rerender} = renderHook( - ({items}) => - useAnnouncements(items, {current: list}, {current: null}, true, false, undefined, 'active-descendant'), - {initialProps: {items: [{id: 'start-date', text: 'Start date', selected: false}]}}, + ({items}) => { + return useAnnouncements(items, {current: list}, {current: null}, true, false, undefined, 'active-descendant') + }, + { + initialProps: { + items: [ + { + id: 'start-date', + text: 'Start date', + selected: false, + }, + ], + }, + }, ) rerender({items: [{id: 'start-date', text: 'Start date', selected: false}]}) - expect(announce).not.toHaveBeenCalled() + await act(async () => { + await vi.runAllTimersAsync() + }) + expect(ariaNotify).not.toHaveBeenCalled() }) + it.each(['active-descendant', 'roving-tabindex'] as const)( + 'cancels a pending %s announcement when loading starts', + async focusManagement => { + const items = [ + { + text: 'Start date', + }, + ] + const list = document.createElement('ul') + const activeOption = document.createElement('li') + activeOption.setAttribute('role', 'option') + activeOption.setAttribute('data-is-active-descendant', 'true') + activeOption.textContent = 'Start date' + list.append(activeOption) + + const {result, rerender} = renderHook( + ({loading}) => { + return useAnnouncements(items, {current: list}, {current: null}, true, loading, undefined, focusManagement) + }, + { + initialProps: { + loading: false, + }, + }, + ) + + act(() => { + result.current() + }) + await act(async () => { + await vi.advanceTimersByTimeAsync(250) + }) + expect(ariaNotify).not.toHaveBeenCalled() + + rerender({ + loading: true, + }) + + await act(async () => { + await vi.runAllTimersAsync() + }) + + expect(ariaNotify).not.toHaveBeenCalled() + }, + ) + it('announces an unchanged empty state when the filter value changes', async () => { const message = {title: 'Nothing found', description: "There's nothing here."} const {rerender} = renderHook( - ({filterValue}) => - useAnnouncements([], {current: null}, {current: null}, true, false, message, 'active-descendant', filterValue), - {initialProps: {filterValue: ''}}, + ({filterValue}) => { + return useAnnouncements( + [], + {current: null}, + {current: null}, + true, + false, + message, + 'active-descendant', + filterValue, + ) + }, + { + initialProps: { + filterValue: '', + }, + }, ) rerender({filterValue: 'zero'}) - await waitFor(() => expect(announce).toHaveBeenCalledWith("Nothing found. There's nothing here.", {delayMs: 500})) + await act(async () => { + await vi.runAllTimersAsync() + }) + expect(ariaNotify).toHaveBeenCalledExactlyOnceWith("Nothing found. There's nothing here.", { + priority: 'normal', + }) }) }) diff --git a/packages/react/src/FilteredActionList/useAnnouncements.tsx b/packages/react/src/FilteredActionList/useAnnouncements.tsx index b34bf87f793..c77d6bc83ab 100644 --- a/packages/react/src/FilteredActionList/useAnnouncements.tsx +++ b/packages/react/src/FilteredActionList/useAnnouncements.tsx @@ -1,10 +1,10 @@ // Announcements for FilteredActionList (and SelectPanel) based // on https://github.com/github/multi-select-user-testing -import {announce as liveRegionAnnounce} from '@primer/live-region-element' import {useCallback, useEffect, useRef} from 'react' import type {FilteredActionListProps} from './index' import type {ItemInput} from '../SelectPanel' +import {useAnnounce} from '../live-region' // we add a delay so that it does not interrupt default screen reader announcement and queues after it const delayMs = 500 @@ -78,32 +78,51 @@ export const useAnnouncements = ( filterValue?: string, ) => { const usingRovingTabindex = focusManagement === 'roving-tabindex' - - const liveRegion = document.querySelector('live-region') - // Notify user of the number of items available const selectedItems = items.filter(item => item.selected).length - + const {announce: ariaNotify, cancel} = useAnnounce() + const focusTimeoutId = useRef(null) + const updateFrameId = useRef(null) + + useEffect(() => { + return () => { + if (focusTimeoutId.current !== null) { + clearTimeout(focusTimeoutId.current) + focusTimeoutId.current = null + } + if (updateFrameId.current !== null) { + cancelAnimationFrame(updateFrameId.current) + updateFrameId.current = null + } + } + }, []) const announce = useCallback( - (...args: Parameters): ReturnType | undefined => { + (...args: Parameters) => { if (enabled) { - return liveRegionAnnounce(...args) + ariaNotify(...args) } }, - [enabled], + [ariaNotify, enabled], ) const onInputFocus = useCallback(() => { + if (loading) { + return + } + if (usingRovingTabindex) { const announcementText = `${items.length} item${items.length > 1 ? 's' : ''} available, ${selectedItems} selected.` announce(announcementText, { delayMs, - from: liveRegion ? liveRegion : undefined, // announce will create a liveRegion if it doesn't find one }) return } - window.setTimeout(() => { + if (focusTimeoutId.current !== null) { + clearTimeout(focusTimeoutId.current) + } + focusTimeoutId.current = window.setTimeout(() => { + focusTimeoutId.current = null const activeItem = getItemWithActiveDescendant(listContainerRef, items) if (!activeItem) return const {index, text, selected} = activeItem @@ -117,10 +136,9 @@ export const useAnnouncements = ( ].join(', ') announce(announcementText, { delayMs, - from: liveRegion ? liveRegion : undefined, // announce will create a liveRegion if it doesn't find one }) }) - }, [announce, inputRef, items, listContainerRef, liveRegion, selectedItems, usingRovingTabindex]) + }, [announce, inputRef, items, listContainerRef, loading, selectedItems, usingRovingTabindex]) const announcementState = getAnnouncementState(items, loading, message, filterValue) const previousAnnouncementState = useRef(announcementState) @@ -129,10 +147,24 @@ export const useAnnouncements = ( if (previousAnnouncementState.current === announcementState) return previousAnnouncementState.current = announcementState - liveRegion?.clear() // clear previous announcements + // clear previous announcements + cancel() + if (focusTimeoutId.current !== null) { + clearTimeout(focusTimeoutId.current) + focusTimeoutId.current = null + } + if (updateFrameId.current !== null) { + cancelAnimationFrame(updateFrameId.current) + updateFrameId.current = null + } + + // eslint-disable-next-line react-you-might-not-need-an-effect/no-event-handler + if (loading) { + return + } // eslint-disable-next-line react-you-might-not-need-an-effect/no-event-handler - if (items.length === 0 && !loading) { + if (items.length === 0) { announce(`${message?.title}. ${message?.description}`, {delayMs}) return } @@ -142,11 +174,11 @@ export const useAnnouncements = ( announce(announcementText, { delayMs, - from: liveRegion ? liveRegion : undefined, }) } else { // give @primer/behaviors a moment to update active-descendant - window.requestAnimationFrame(() => { + updateFrameId.current = window.requestAnimationFrame(() => { + updateFrameId.current = null const activeItem = getItemWithActiveDescendant(listContainerRef, items) if (!activeItem) return const {index, text, selected} = activeItem @@ -160,17 +192,16 @@ export const useAnnouncements = ( announce(announcementText, { delayMs, - from: liveRegion ? liveRegion : undefined, // announce will create a liveRegion if it doesn't find one }) }) } }, [ announce, + cancel, announcementState, items, listContainerRef, - liveRegion, usingRovingTabindex, message?.title, message?.description, diff --git a/packages/react/src/SelectPanel/SelectPanel.test.tsx b/packages/react/src/SelectPanel/SelectPanel.test.tsx index f5f70445d63..3a518d142e8 100644 --- a/packages/react/src/SelectPanel/SelectPanel.test.tsx +++ b/packages/react/src/SelectPanel/SelectPanel.test.tsx @@ -1,5 +1,5 @@ -import {act, fireEvent, render, screen, waitFor} from '@testing-library/react' -import {describe, expect, it, beforeEach, vi} from 'vitest' +import {act, fireEvent, render, renderHook, screen, waitFor} from '@testing-library/react' +import {describe, expect, it, beforeEach, afterEach, vi, type MockInstance} from 'vitest' import {page} from 'vitest/browser' import React from 'react' import {flushSync} from 'react-dom' @@ -7,7 +7,6 @@ import {SelectPanel, type SelectPanelProps, type ItemInput, type GroupedListProp import {userEvent} from '@testing-library/user-event' import {FeatureFlags} from '../FeatureFlags' import type {InitialLoadingType} from './SelectPanel' -import type {LiveRegionElement} from '@primer/live-region-element' import {IconButton} from '../Button' import {ArrowLeftIcon} from '@primer/octicons-react' import classes from './SelectPanel.test.module.css' @@ -15,6 +14,7 @@ import {implementsClassName} from '../utils/testing' import {getAnchoredPosition} from '@primer/behaviors' import type {AnchorPosition} from '@primer/behaviors' import {createRenderCounter} from '../utils/testing/profiler' +import {useAnnounce} from '../live-region' // Mock getAnchoredPosition to verify displayInViewport is forwarded vi.mock('@primer/behaviors', async () => { @@ -37,14 +37,15 @@ vi.mock('@primer/behaviors', async () => { } }) -// Instead of importing from live-region/__tests__/test-helpers.ts, we define our own getLiveRegion function -export function getLiveRegion(): LiveRegionElement { - const liveRegion = document.querySelector('live-region') - if (liveRegion) { - return liveRegion as LiveRegionElement - } - throw new Error('No live-region found') -} +let ariaNotify: MockInstance + +beforeEach(() => { + ariaNotify = vi.spyOn(document, 'ariaNotify').mockImplementation(() => {}) +}) + +afterEach(() => { + vi.restoreAllMocks() +}) const getFocusManagement = (flag?: boolean) => { // true = 'use roving tabindex' @@ -112,11 +113,13 @@ describe('SelectPanel state synchronization', () => { function MultiSelectPanel({ items: panelItems, loading, + notice, onOpenChange = () => {}, open, }: { items: ItemInput[] loading?: boolean + notice?: SelectPanelProps['notice'] onOpenChange?: SelectPanelProps['onOpenChange'] open: boolean }) { @@ -130,6 +133,7 @@ describe('SelectPanel state synchronization', () => { onSelectedChange={() => {}} onFilterChange={() => {}} loading={loading} + notice={notice} open={open} onOpenChange={onOpenChange} /> @@ -304,11 +308,6 @@ describe('SelectPanel state synchronization', () => { it('announces internally managed loading when opened', async () => { vi.useFakeTimers() - for (const element of document.querySelectorAll('live-region')) { - element.remove() - } - const liveRegion = document.createElement('live-region') as LiveRegionElement - document.body.appendChild(liveRegion) try { function LoadingPanel() { @@ -322,11 +321,205 @@ describe('SelectPanel state synchronization', () => { await vi.runAllTimersAsync() }) - expect(liveRegion.getMessage('polite')).toBe('Loading.') + expect(ariaNotify).toHaveBeenCalledWith('Loading.', { + priority: 'normal', + }) } finally { vi.useRealTimers() } }) + + describe('announcement cancellation', () => { + beforeEach(() => { + vi.useFakeTimers() + }) + + afterEach(() => { + vi.useRealTimers() + }) + + it('replaces pending loading with the available results', async () => { + const {rerender} = render() + + await act(async () => { + await vi.advanceTimersByTimeAsync(1000) + }) + expect(ariaNotify).not.toHaveBeenCalled() + + rerender() + + await act(async () => { + await vi.runAllTimersAsync() + }) + + expect(ariaNotify).not.toHaveBeenCalledWith('Loading.', { + priority: 'normal', + }) + expect(ariaNotify).toHaveBeenCalledWith('List updated, Focused item: item one, not selected, 1 of 3', { + priority: 'normal', + }) + }) + + it.each(['close', 'unmount'] as const)('cancels pending loading on %s', async action => { + const {rerender, unmount} = render() + + await act(async () => { + await vi.advanceTimersByTimeAsync(1000) + }) + expect(ariaNotify).not.toHaveBeenCalled() + + if (action === 'close') { + rerender() + } else { + unmount() + } + + await act(async () => { + await vi.runAllTimersAsync() + }) + + expect(ariaNotify).not.toHaveBeenCalled() + }) + + it('does not start a loading announcement after the panel closes', async () => { + const emptyItems: SelectPanelProps['items'] = [] + const {rerender} = render() + + rerender() + + await act(async () => { + await vi.runAllTimersAsync() + }) + + expect(ariaNotify).not.toHaveBeenCalled() + }) + + it.each(['close', 'unmount'] as const)('cancels deferred focus announcements on %s', async action => { + const {rerender, unmount} = render() + + if (action === 'close') { + rerender() + } else { + unmount() + } + + await act(async () => { + await vi.runAllTimersAsync() + }) + + expect(ariaNotify).not.toHaveBeenCalled() + }) + + it('cancels a deferred results announcement when the panel closes', async () => { + const {rerender} = render() + + await act(async () => { + await vi.advanceTimersByTimeAsync(1000) + }) + + rerender() + rerender() + + await act(async () => { + await vi.runAllTimersAsync() + }) + + expect(ariaNotify).not.toHaveBeenCalled() + }) + + it('replaces pending loading with a notice', async () => { + const emptyItems: SelectPanelProps['items'] = [] + const {rerender} = render() + + await act(async () => { + await vi.advanceTimersByTimeAsync(1000) + }) + expect(ariaNotify).not.toHaveBeenCalled() + + rerender( + , + ) + + await act(async () => { + await vi.advanceTimersByTimeAsync(500) + }) + + expect(ariaNotify).toHaveBeenCalledExactlyOnceWith(expect.stringContaining('Updated notice'), { + priority: 'normal', + }) + }) + + it('does not cancel a pending list announcement when a notice appears', async () => { + const {rerender} = render() + + await act(async () => { + await vi.advanceTimersByTimeAsync(100) + }) + expect(ariaNotify).not.toHaveBeenCalled() + + rerender( + , + ) + + await act(async () => { + await vi.advanceTimersByTimeAsync(400) + }) + + expect(ariaNotify).toHaveBeenCalledWith(expect.stringContaining('Updated notice'), { + priority: 'normal', + }) + expect(ariaNotify).toHaveBeenCalledWith( + 'Filter items, filter text box and list of items, Focused item: item one, not selected, 1 of 3', + { + priority: 'normal', + }, + ) + expect(ariaNotify).toHaveBeenCalledTimes(2) + }) + + it('does not cancel an unrelated announcement when results arrive', async () => { + const {result} = renderHook(() => { + return useAnnounce() + }) + act(() => { + result.current.announce('Unrelated announcement', { + delayMs: 2000, + }) + }) + const {rerender} = render() + + await act(async () => { + await vi.advanceTimersByTimeAsync(1000) + }) + + rerender() + + await act(async () => { + await vi.runAllTimersAsync() + }) + + expect(ariaNotify).toHaveBeenCalledWith('Unrelated announcement', { + priority: 'normal', + }) + expect(ariaNotify).not.toHaveBeenCalledWith('Loading.', { + priority: 'normal', + }) + }) + }) }) for (const usingRemoveActiveDescendant of [false, true]) { @@ -970,11 +1163,6 @@ for (const usingRemoveActiveDescendant of [false, true]) { }) describe('screen reader announcements', () => { - beforeEach(() => { - const liveRegion = document.createElement('live-region') - document.body.appendChild(liveRegion) - }) - function LoadingSelectPanel({ initialLoadingType = 'spinner', items = [], @@ -1051,10 +1239,15 @@ for (const usingRemoveActiveDescendant of [false, true]) { await waitFor( async () => { if (usingRemoveActiveDescendant) { - expect(getLiveRegion().getMessage('polite')!.trim()).toEqual('3 items available, 0 selected.') + expect(ariaNotify).toHaveBeenLastCalledWith('3 items available, 0 selected.', { + priority: 'normal', + }) } else { - expect(getLiveRegion().getMessage('polite')!.trim()).toEqual( + expect(ariaNotify).toHaveBeenLastCalledWith( 'Filter items, filter text box and list of items, Focused item: item one, not selected, 1 of 3', + { + priority: 'normal', + }, ) } }, @@ -1104,7 +1297,9 @@ for (const usingRemoveActiveDescendant of [false, true]) { await user.click(screen.getByText('Select items')) expect(screen.getByLabelText('Filter items')).toHaveFocus() - expect(getLiveRegion().getMessage('polite').trim()).toContain('This is a notice') + expect(ariaNotify).toHaveBeenCalledWith(expect.stringContaining('This is a notice'), { + priority: 'normal', + }) }) it('should announce filtered results', async () => { @@ -1117,10 +1312,15 @@ for (const usingRemoveActiveDescendant of [false, true]) { await waitFor( async () => { if (usingRemoveActiveDescendant) { - expect(getLiveRegion().getMessage('polite')!.trim()).toEqual('3 items available, 0 selected.') + expect(ariaNotify).toHaveBeenLastCalledWith('3 items available, 0 selected.', { + priority: 'normal', + }) } else { - expect(getLiveRegion().getMessage('polite')!.trim()).toEqual( + expect(ariaNotify).toHaveBeenLastCalledWith( 'Filter items, filter text box and list of items, Focused item: item one, not selected, 1 of 3', + { + priority: 'normal', + }, ) } }, @@ -1133,10 +1333,15 @@ for (const usingRemoveActiveDescendant of [false, true]) { await waitFor( async () => { if (usingRemoveActiveDescendant) { - expect(getLiveRegion().getMessage('polite')).toBe('2 items available, 0 selected.') + expect(ariaNotify).toHaveBeenLastCalledWith('2 items available, 0 selected.', { + priority: 'normal', + }) } else { - expect(getLiveRegion().getMessage('polite')).toBe( + expect(ariaNotify).toHaveBeenLastCalledWith( 'List updated, Focused item: item one, not selected, 1 of 2', + { + priority: 'normal', + }, ) } }, @@ -1149,10 +1354,15 @@ for (const usingRemoveActiveDescendant of [false, true]) { await waitFor( async () => { if (usingRemoveActiveDescendant) { - expect(getLiveRegion().getMessage('polite')!.trim()).toBe('1 item available, 0 selected.') + expect(ariaNotify).toHaveBeenLastCalledWith('1 item available, 0 selected.', { + priority: 'normal', + }) } else { - expect(getLiveRegion().getMessage('polite').trim()).toBe( + expect(ariaNotify).toHaveBeenLastCalledWith( 'List updated, Focused item: item one, not selected, 1 of 1', + { + priority: 'normal', + }, ) } }, @@ -1171,11 +1381,9 @@ for (const usingRemoveActiveDescendant of [false, true]) { await waitFor( async () => { - if (usingRemoveActiveDescendant) { - expect(getLiveRegion().getMessage('polite')!.trim()).toBe('No items available.') - } else { - expect(getLiveRegion().getMessage('polite').trim()).toBe('No items available.') - } + expect(ariaNotify).toHaveBeenLastCalledWith(expect.stringMatching(/^No items available\.\s*$/), { + priority: 'normal', + }) }, {timeout: 3000}, ) @@ -1223,11 +1431,9 @@ for (const usingRemoveActiveDescendant of [false, true]) { await waitFor( async () => { - if (usingRemoveActiveDescendant) { - expect(getLiveRegion().getMessage('polite')!.trim()).toBe("Nothing found. There's nothing here.") - } else { - expect(getLiveRegion().getMessage('polite').trim()).toBe("Nothing found. There's nothing here.") - } + expect(ariaNotify).toHaveBeenLastCalledWith("Nothing found. There's nothing here.", { + priority: 'normal', + }) }, {timeout: 3000}, ) diff --git a/packages/react/src/SelectPanel/SelectPanel.tsx b/packages/react/src/SelectPanel/SelectPanel.tsx index f3b655065d1..5d99184c0d3 100644 --- a/packages/react/src/SelectPanel/SelectPanel.tsx +++ b/packages/react/src/SelectPanel/SelectPanel.tsx @@ -20,7 +20,7 @@ import useSafeTimeout from '../hooks/useSafeTimeout' import type {FilteredActionListLoadingType} from '../FilteredActionList/FilteredActionListLoaders' import {FilteredActionListLoadingTypes} from '../FilteredActionList/FilteredActionListLoaders' import {useFeatureFlag} from '../FeatureFlags' -import {announce, announceFromElement} from '@primer/live-region-element' +import {useAnnounce} from '../live-region' import classes from './SelectPanel.module.css' import {clsx} from 'clsx' import {debounce} from '@github/mini-throttle' @@ -44,21 +44,6 @@ const DefaultEmptyMessage = ( ) -async function announceText(text: string, delayMs = SHORT_DELAY_MS) { - const liveRegion = document.querySelector('live-region') - - liveRegion?.clear() // clear previous announcements - - await announce(text, { - delayMs, - from: liveRegion ? liveRegion : undefined, // announce will create a liveRegion if it doesn't find one - }) -} - -async function announceLoading() { - await announceText('Loading.') -} - interface SelectPanelSingleSelection { selected: ItemInput | undefined onSelectedChange: (selected: ItemInput | undefined) => void @@ -225,6 +210,13 @@ function Panel({ const [filterValue, setInternalFilterValue] = useProvidedStateOrCreate(externalFilterValue, undefined, '') const {safeSetTimeout, safeClearTimeout} = useSafeTimeout() const loadingDelayTimeoutId = useRef(null) + const {announce, cancel} = useAnnounce() + const announceLoading = useCallback(() => { + announce('Loading.', { + delayMs: SHORT_DELAY_MS, + }) + }, [announce]) + const loadingManagedInternally = loading === undefined const loadingManagedExternally = !loadingManagedInternally const [loadingState, setLoadingState] = useState(() => ({ @@ -259,6 +251,18 @@ function Panel({ } const {dataLoadedOnce, isLoading} = loadingState + + useEffect(() => { + // eslint-disable-next-line react-you-might-not-need-an-effect/no-event-handler + if (!open || !(isLoading || loading)) { + cancel() + if (loadingDelayTimeoutId.current !== null) { + safeClearTimeout(loadingDelayTimeoutId.current) + loadingDelayTimeoutId.current = null + } + } + }, [cancel, isLoading, loading, open, safeClearTimeout]) + const [inputRef, setInputRef] = React.useState | null>(null) const [listContainerElement, setListContainerElement] = useState(null) const [needsNoItemsAnnouncement, setNeedsNoItemsAnnouncement] = useState(false) @@ -352,6 +356,7 @@ function Panel({ const onFilterChange: FilteredActionListProps['onFilterChange'] = useCallback( (value, e) => { + cancel() if (loadingManagedInternally) { if (loadingDelayTimeoutId.current) { safeClearTimeout(loadingDelayTimeoutId.current) @@ -400,6 +405,8 @@ function Panel({ safeClearTimeout, items.length, resetSort, + announceLoading, + cancel, ], ) @@ -464,6 +471,7 @@ function Panel({ }, [isNarrowScreenSize, open, usingFullScreenOnNarrow, variant]) useEffect(() => { + cancel() // eslint-disable-next-line react-you-might-not-need-an-effect/no-event-handler if (open) { // eslint-disable-next-line react-you-might-not-need-an-effect/no-event-handler @@ -476,11 +484,12 @@ function Panel({ if (loadingDelayTimeoutId.current) { safeClearTimeout(loadingDelayTimeoutId.current) + loadingDelayTimeoutId.current = null } // Only fire this effect if items have changed // eslint-disable-next-line react-hooks/exhaustive-deps - }, [items]) + }, [cancel, items]) useEffect(() => { // eslint-disable-next-line react-you-might-not-need-an-effect/no-event-handler @@ -513,7 +522,7 @@ function Panel({ } } } - }, [isLoading, loadingManagedExternally, safeSetTimeout, safeClearTimeout]) + }, [isLoading, loadingManagedExternally, safeSetTimeout, safeClearTimeout, announceLoading]) // Populate panel with items on first open useEffect(() => { @@ -578,22 +587,13 @@ function Panel({ }, [open, isNarrowScreenSize]) useEffect(() => { - const announceNotice = async () => { - if (!noticeRef.current) return - const liveRegion = document.querySelector('live-region') - - liveRegion?.clear() - - await announceFromElement(noticeRef.current, { - from: liveRegion ? liveRegion : undefined, - }) - } - + // eslint-disable-next-line github/no-innerText -- Exclude hidden notice content from announcements. + const text = noticeRef.current?.innerText || noticeRef.current?.textContent // eslint-disable-next-line react-you-might-not-need-an-effect/no-event-handler - if (open && notice) { - announceNotice() + if (open && notice && text?.trim()) { + announce(text.trim()) } - }, [notice, open]) + }, [announce, notice, open]) const anchorRef = useProvidedRefOrCreate(externalAnchorRef) const onOpen: AnchoredOverlayProps['onOpen'] = useCallback( From b74f8105ab6864bda660631632af3e034bac4cba Mon Sep 17 00:00:00 2001 From: Josh Black Date: Thu, 29 Oct 2026 16:36:52 -0500 Subject: [PATCH 05/12] test: update tests to use ariaNotify over live-region-element --- .../react/src/TextInput/TextInput.test.tsx | 59 +++++++++++-------- packages/react/src/Textarea/Textarea.test.tsx | 59 +++++++++++-------- packages/react/src/TreeView/TreeView.test.tsx | 40 +++++++++---- .../__tests__/SelectPanelLoading.test.tsx | 32 ++++++---- .../src/live-region/__tests__/test-helpers.ts | 9 --- 5 files changed, 119 insertions(+), 80 deletions(-) delete mode 100644 packages/react/src/live-region/__tests__/test-helpers.ts diff --git a/packages/react/src/TextInput/TextInput.test.tsx b/packages/react/src/TextInput/TextInput.test.tsx index 0cbe72bfec3..ecce2e25fbc 100644 --- a/packages/react/src/TextInput/TextInput.test.tsx +++ b/packages/react/src/TextInput/TextInput.test.tsx @@ -1,19 +1,13 @@ import {SearchIcon} from '@primer/octicons-react' import userEvent from '@testing-library/user-event' import {render, fireEvent, screen, act} from '@testing-library/react' -import {describe, it, expect, vi, afterEach} from 'vitest' +import {describe, it, expect, vi, beforeEach, afterEach, type MockInstance} from 'vitest' import React, {createRef} from 'react' import TextInput from '../TextInput' import {FeatureFlags} from '../FeatureFlags' import {implementsClassName} from '../utils/testing' import {SCREEN_READER_DELAY} from '../utils/character-counter' import {createRenderCounter} from '../utils/testing/profiler' -import type {LiveRegionElement} from '@primer/live-region-element' - -function getPoliteAnnouncement(): string { - const liveRegion = document.querySelector('live-region') as LiveRegionElement | null - return liveRegion?.getMessage('polite') ?? '' -} describe('TextInput', () => { implementsClassName(TextInput, 'TextInput-wrapper') @@ -299,10 +293,15 @@ describe('TextInput', () => { }) describe('character counter', () => { + let ariaNotify: MockInstance + + beforeEach(() => { + ariaNotify = vi.spyOn(Element.prototype, 'ariaNotify').mockImplementation(() => {}) + }) + afterEach(() => { - // The announcement live region is shared and appended to the document, so - // remove it between tests to avoid leaking messages across cases. - document.querySelector('live-region')?.remove() + vi.useRealTimers() + ariaNotify.mockRestore() }) it('should render character counter when characterLimit is provided', () => { @@ -387,9 +386,15 @@ describe('TextInput', () => { expect(describedBy).toContain(staticMessage?.id) }) - it('does not announce the counter on show', () => { + it('does not announce the counter on show', async () => { + vi.useFakeTimers() render() - expect(getPoliteAnnouncement()).toBe('') + + await act(async () => { + await vi.runAllTimersAsync() + }) + + expect(ariaNotify).not.toHaveBeenCalled() }) it('should have static screen reader message', () => { @@ -402,32 +407,36 @@ describe('TextInput', () => { expect(container.textContent).toContain('You can enter up to 1 character') }) - it('should not announce on initial load', () => { + it('should not announce on initial load', async () => { + vi.useFakeTimers() render() - expect(getPoliteAnnouncement()).toBe('') + + await act(async () => { + await vi.runAllTimersAsync() + }) + + expect(ariaNotify).not.toHaveBeenCalled() }) it('announces the updated remaining count after a change', async () => { - // The remaining-count message is derived in render and announced (debounced) - // through the shared live region. Uses fireEvent + fake timers because - // userEvent deadlocks with fake timers in the browser test environment and - // the announcement is debounced (real timers would be slow and flaky). + // Use fireEvent with fake timers to check the announcement debounce. vi.useFakeTimers() try { const {getByRole} = render() fireEvent.change(getByRole('textbox'), {target: {value: 'Hello'}}) - // Nothing is announced until the debounce delay elapses. - expect(getPoliteAnnouncement()).toBe('') - await act(async () => { - // Let the MutationObserver schedule the debounced announcement, then - // advance past the delay so the live region updates. await Promise.resolve() - await vi.advanceTimersByTimeAsync(SCREEN_READER_DELAY) + await vi.advanceTimersByTimeAsync(SCREEN_READER_DELAY - 1) }) + expect(ariaNotify).not.toHaveBeenCalled() - expect(getPoliteAnnouncement()).toBe('15 characters remaining') + await act(async () => { + await vi.advanceTimersByTimeAsync(1) + }) + expect(ariaNotify).toHaveBeenCalledExactlyOnceWith('15 characters remaining', { + priority: 'normal', + }) } finally { vi.useRealTimers() } diff --git a/packages/react/src/Textarea/Textarea.test.tsx b/packages/react/src/Textarea/Textarea.test.tsx index c626bc9ec2b..6e7ecafc816 100644 --- a/packages/react/src/Textarea/Textarea.test.tsx +++ b/packages/react/src/Textarea/Textarea.test.tsx @@ -2,18 +2,12 @@ import React from 'react' import Textarea from '../Textarea' import {render, screen, fireEvent, act} from '@testing-library/react' import userEvent from '@testing-library/user-event' -import {describe, expect, it, vi, beforeEach, afterEach} from 'vitest' +import {describe, expect, it, vi, beforeEach, afterEach, type MockInstance} from 'vitest' import classes from './TextArea.module.css' import {implementsClassName} from '../utils/testing' import textInputClasses from '../internal/components/TextInputWrapper.module.css' import {SCREEN_READER_DELAY} from '../utils/character-counter' import {createRenderCounter} from '../utils/testing/profiler' -import type {LiveRegionElement} from '@primer/live-region-element' - -function getPoliteAnnouncement(): string { - const liveRegion = document.querySelector('live-region') as LiveRegionElement | null - return liveRegion?.getMessage('polite') ?? '' -} function getCSSRules(selector: string): Array { return Array.from(document.styleSheets).flatMap(sheet => { @@ -193,10 +187,15 @@ describe('Textarea', () => { }) describe('character counter', () => { + let ariaNotify: MockInstance + + beforeEach(() => { + ariaNotify = vi.spyOn(Element.prototype, 'ariaNotify').mockImplementation(() => {}) + }) + afterEach(() => { - // The announcement live region is shared and appended to the document, so - // remove it between tests to avoid leaking messages across cases. - document.querySelector('live-region')?.remove() + vi.useRealTimers() + ariaNotify.mockRestore() }) it('should render character counter when characterLimit is provided', () => { @@ -283,9 +282,15 @@ describe('Textarea', () => { expect(describedBy).toContain(staticMessage?.id) }) - it('does not announce the counter on show', () => { + it('does not announce the counter on show', async () => { + vi.useFakeTimers() render(