From e5fc97622b521469635ed06085f2a8993ae4f870 Mon Sep 17 00:00:00 2001 From: Atlassian Date: Mon, 9 Dec 2024 11:39:22 +0000 Subject: [PATCH] Synchronise latest changes --- .../live-region/__tests__/unit/_utils.tsx | 13 ---- .../live-region/__tests__/unit/announce.tsx | 54 +++++++++++--- .../live-region/__tests__/unit/cleanup.tsx | 17 ++++- packages/live-region/__tests__/unit/dom.tsx | 12 ++-- packages/live-region/src/constants.tsx | 6 ++ packages/live-region/src/index.tsx | 44 +++++++++++- .../__tests__/unit/live-region.test.tsx | 52 ++++++++++++++ .../responders/announcements.test.tsx | 3 +- .../afm-jira/tsconfig.json | 3 - .../afm-post-office/tsconfig.json | 3 - .../package.json | 1 - .../src/drag-drop-context/index.tsx | 3 +- .../src/drag-drop-context/live-region.tsx | 71 +++++++++++++++++++ 13 files changed, 240 insertions(+), 42 deletions(-) delete mode 100644 packages/live-region/__tests__/unit/_utils.tsx create mode 100644 packages/live-region/src/constants.tsx create mode 100644 packages/react-beautiful-dnd-migration/__tests__/unit/live-region.test.tsx create mode 100644 packages/react-beautiful-dnd-migration/src/drag-drop-context/live-region.tsx diff --git a/packages/live-region/__tests__/unit/_utils.tsx b/packages/live-region/__tests__/unit/_utils.tsx deleted file mode 100644 index 4e4552c..0000000 --- a/packages/live-region/__tests__/unit/_utils.tsx +++ /dev/null @@ -1,13 +0,0 @@ -import { screen } from '@testing-library/dom'; - -function queryLiveRegion(): HTMLElement | null { - return screen.queryByRole('alert'); -} - -export function getLiveRegion(): HTMLElement { - return screen.getByRole('alert'); -} - -export function hasLiveRegion(): boolean { - return Boolean(queryLiveRegion()?.isConnected); -} diff --git a/packages/live-region/__tests__/unit/announce.tsx b/packages/live-region/__tests__/unit/announce.tsx index 9794422..6999c2d 100644 --- a/packages/live-region/__tests__/unit/announce.tsx +++ b/packages/live-region/__tests__/unit/announce.tsx @@ -1,37 +1,42 @@ import { screen } from '@testing-library/dom'; import * as liveRegion from '../../src'; +import { announceDelay } from '../../src/constants'; -import { getLiveRegion, hasLiveRegion } from './_utils'; +jest.useFakeTimers(); describe('announce', () => { beforeEach(() => { liveRegion.cleanup(); - expect(hasLiveRegion()).toBe(false); }); it('should create a live region', () => { liveRegion.announce('a message'); - expect(hasLiveRegion()).toBe(true); + expect(screen.getByRole('status')).toBeInTheDocument(); }); - it('should place the message inside of the live region', () => { + it('should place the message inside of the live region after the announceDelay', () => { const msg = 'a message'; liveRegion.announce(msg); - expect(getLiveRegion().textContent).toBe(msg); + const node = screen.getByRole('status'); + expect(node).not.toHaveTextContent(msg); + jest.advanceTimersByTime(announceDelay); + expect(node).toHaveTextContent(msg); }); it('should reuse an existing live region', () => { liveRegion.announce(''); - const node = getLiveRegion(); + const node = screen.getByRole('status'); const msg1 = 'message #1'; liveRegion.announce(msg1); - expect(node.textContent).toBe(msg1); + jest.runOnlyPendingTimers(); + expect(node).toHaveTextContent(msg1); const msg2 = 'message #2'; liveRegion.announce(msg2); - expect(node.textContent).toBe(msg2); + jest.runOnlyPendingTimers(); + expect(node).toHaveTextContent(msg2); }); it('should not create more than one node at a time', () => { @@ -40,6 +45,37 @@ describe('announce', () => { liveRegion.announce('red message'); liveRegion.announce('blue message'); - expect(screen.getAllByRole('alert')).toHaveLength(1); + expect(screen.getAllByRole('status')).toHaveLength(1); + }); + + // We might want to change this in the future, but this is capturing existing behavior + it('should use the latest message', () => { + liveRegion.announce('first'); + liveRegion.announce('second'); + + jest.advanceTimersByTime(announceDelay); + expect(screen.getByRole('status')).toHaveTextContent('second'); + }); + + // We might want to change this in the future, but this is capturing existing behavior + it('should clear pending messages, but still wait the full announceDelay', () => { + liveRegion.announce('first'); + + setTimeout(() => { + liveRegion.announce('second'); + }, announceDelay / 2); + + const node = screen.getByRole('status'); + + jest.advanceTimersByTime(announceDelay); + // The queued first message was cleared by the second before it was shown + expect(node).not.toHaveTextContent('first'); + // There has not been a full `announceDelay` since the second message was queued + expect(node).not.toHaveTextContent('second'); + + // Advancing by the delay we used to queue the second message + jest.advanceTimersByTime(announceDelay / 2); + // Now there has been a complete `announceDelay` since the second message was queued + expect(node).toHaveTextContent('second'); }); }); diff --git a/packages/live-region/__tests__/unit/cleanup.tsx b/packages/live-region/__tests__/unit/cleanup.tsx index 86fc3eb..547bb11 100644 --- a/packages/live-region/__tests__/unit/cleanup.tsx +++ b/packages/live-region/__tests__/unit/cleanup.tsx @@ -1,6 +1,8 @@ +import { screen } from '@testing-library/dom'; + import * as liveRegion from '../../src'; -import { hasLiveRegion } from './_utils'; +jest.useFakeTimers(); describe('cleanup', () => { it('should do nothing if a node does not exist', () => { @@ -12,6 +14,17 @@ describe('cleanup', () => { it('should remove the live region node if it exists', () => { liveRegion.announce('a message'); liveRegion.cleanup(); - expect(hasLiveRegion()).toBe(false); + expect(screen.queryByRole('status')).not.toBeInTheDocument(); + }); + + // We might want to change this in the future, but this is capturing existing behavior + it('should clear any pending messages', () => { + liveRegion.announce('one message'); + liveRegion.cleanup(); + + // The pending message is never announced + jest.runAllTimers(); + expect(screen.queryByRole('status')).not.toBeInTheDocument(); + expect(screen.queryByText('one message')).not.toBeInTheDocument(); }); }); diff --git a/packages/live-region/__tests__/unit/dom.tsx b/packages/live-region/__tests__/unit/dom.tsx index 6c6504d..7ff8203 100644 --- a/packages/live-region/__tests__/unit/dom.tsx +++ b/packages/live-region/__tests__/unit/dom.tsx @@ -1,10 +1,14 @@ -import * as liveRegion from '../../src'; +import { screen } from '@testing-library/dom'; -import { getLiveRegion } from './_utils'; +import * as liveRegion from '../../src'; describe('DOM node', () => { - it('should have role="alert"', () => { + beforeEach(() => { + liveRegion.cleanup(); + }); + + it('should have role="status"', () => { liveRegion.announce('a message'); - expect(getLiveRegion()).toHaveAttribute('role', 'alert'); + expect(screen.getByRole('status')).toBeInTheDocument(); }); }); diff --git a/packages/live-region/src/constants.tsx b/packages/live-region/src/constants.tsx new file mode 100644 index 0000000..6cef538 --- /dev/null +++ b/packages/live-region/src/constants.tsx @@ -0,0 +1,6 @@ +/** + * Delay until the text content of the live region is updated. + * + * This value was reached through some experimentation, and may need tweaking in the future. + */ +export const announceDelay = 1000; diff --git a/packages/live-region/src/index.tsx b/packages/live-region/src/index.tsx index baf5885..672e1f8 100644 --- a/packages/live-region/src/index.tsx +++ b/packages/live-region/src/index.tsx @@ -1,3 +1,5 @@ +import { announceDelay } from './constants'; + let node: HTMLElement | null = null; const size = '1px'; @@ -26,7 +28,14 @@ const visuallyHiddenStyles = { */ function createNode(): HTMLElement { const node = document.createElement('div'); - node.setAttribute('role', 'alert'); + /** + * Using `role="status"` instead of `role="alert"` so that the message + * can be queued and read when able. + * + * We found with `role="alert"` the message was not reliably read when + * focus changed. + */ + node.setAttribute('role', 'status'); Object.assign(node.style, visuallyHiddenStyles); document.body.append(node); return node; @@ -42,18 +51,47 @@ function getNode(): HTMLElement { return node; } +let timerId: ReturnType | null = null; + +function tryClearTimer() { + if (timerId !== null) { + clearTimeout(timerId); + } + timerId = null; +} + /** * Announces the provided message to assistive technology. */ export function announce(message: string) { - const node = getNode(); - node.textContent = message; + /** + * Calling this immediately to ensure a node exists and has time to be parsed + * and exposed in the accessibility tree. + */ + getNode(); + + /** + * Updating the message in a timeout so that it's less likely to be interrupted. + * + * This function is often called right before focus changes, + * because the user has just taken an action. + * This focus change would often cause the message to be skipped / interrupted. + */ + tryClearTimer(); + timerId = setTimeout(() => { + timerId = null; + const node = getNode(); + node.textContent = message; + }, announceDelay); + + return; } /** * Removes the created live region. If there is no live region this is a no-op. */ export function cleanup() { + tryClearTimer(); node?.remove(); node = null; } diff --git a/packages/react-beautiful-dnd-migration/__tests__/unit/live-region.test.tsx b/packages/react-beautiful-dnd-migration/__tests__/unit/live-region.test.tsx new file mode 100644 index 0000000..2cfe9f3 --- /dev/null +++ b/packages/react-beautiful-dnd-migration/__tests__/unit/live-region.test.tsx @@ -0,0 +1,52 @@ +import { screen } from '@testing-library/dom'; + +import * as liveRegion from '../../src/drag-drop-context/live-region'; + +describe('live region', () => { + beforeEach(() => { + liveRegion.cleanup(); + }); + + describe('announce', () => { + it('should create a live region with role="alert"', () => { + liveRegion.announce('a message'); + expect(screen.getByRole('alert')).toHaveTextContent('a message'); + }); + + it('should reuse an existing live region', () => { + liveRegion.announce(''); + const node = screen.getByRole('alert'); + + const msg1 = 'message #1'; + liveRegion.announce(msg1); + expect(node).toHaveTextContent(msg1); + + const msg2 = 'message #2'; + liveRegion.announce(msg2); + expect(node).toHaveTextContent(msg2); + }); + + it('should not create more than one node at a time', () => { + liveRegion.announce('one message'); + liveRegion.announce('two message'); + liveRegion.announce('red message'); + liveRegion.announce('blue message'); + + expect(screen.getAllByRole('alert')).toHaveLength(1); + }); + }); + + describe('cleanup', () => { + it('should do nothing if a node does not exist', () => { + const snapshot = document.documentElement.outerHTML; + liveRegion.cleanup(); + expect(document.documentElement.outerHTML).toEqual(snapshot); + }); + + it('should remove the live region node if it exists', () => { + liveRegion.announce('a message'); + liveRegion.cleanup(); + expect(screen.queryByRole('alert')).not.toBeInTheDocument(); + }); + }); +}); diff --git a/packages/react-beautiful-dnd-migration/__tests__/unit/ported-from-react-beautiful-dnd/unit/state/middleware/responders/announcements.test.tsx b/packages/react-beautiful-dnd-migration/__tests__/unit/ported-from-react-beautiful-dnd/unit/state/middleware/responders/announcements.test.tsx index ed430bd..841265f 100644 --- a/packages/react-beautiful-dnd-migration/__tests__/unit/ported-from-react-beautiful-dnd/unit/state/middleware/responders/announcements.test.tsx +++ b/packages/react-beautiful-dnd-migration/__tests__/unit/ported-from-react-beautiful-dnd/unit/state/middleware/responders/announcements.test.tsx @@ -4,8 +4,7 @@ import { fireEvent, render, type RenderResult } from '@testing-library/react'; import type { DragStart, DragUpdate, ResponderProvided } from 'react-beautiful-dnd'; import invariant from 'tiny-invariant'; -import * as liveRegion from '@atlaskit/pragmatic-drag-and-drop-live-region'; - +import * as liveRegion from '../../../../../../../src/drag-drop-context/live-region'; import * as screenReader from '../../../../../../../src/drag-drop-context/screen-reader'; import { Board } from '../../../../../_utils/board'; import { keyboard } from '../../../integration/_utils/controls'; diff --git a/packages/react-beautiful-dnd-migration/afm-jira/tsconfig.json b/packages/react-beautiful-dnd-migration/afm-jira/tsconfig.json index 2cd97cf..5b42a5a 100644 --- a/packages/react-beautiful-dnd-migration/afm-jira/tsconfig.json +++ b/packages/react-beautiful-dnd-migration/afm-jira/tsconfig.json @@ -23,9 +23,6 @@ { "path": "../../hitbox/afm-jira/tsconfig.json" }, - { - "path": "../../live-region/afm-jira/tsconfig.json" - }, { "path": "../../react-beautiful-dnd-autoscroll/afm-jira/tsconfig.json" }, diff --git a/packages/react-beautiful-dnd-migration/afm-post-office/tsconfig.json b/packages/react-beautiful-dnd-migration/afm-post-office/tsconfig.json index 1eefe0e..692a8ed 100644 --- a/packages/react-beautiful-dnd-migration/afm-post-office/tsconfig.json +++ b/packages/react-beautiful-dnd-migration/afm-post-office/tsconfig.json @@ -23,9 +23,6 @@ { "path": "../../hitbox/afm-post-office/tsconfig.json" }, - { - "path": "../../live-region/afm-post-office/tsconfig.json" - }, { "path": "../../react-beautiful-dnd-autoscroll/afm-post-office/tsconfig.json" }, diff --git a/packages/react-beautiful-dnd-migration/package.json b/packages/react-beautiful-dnd-migration/package.json index 1113a44..4d075e7 100644 --- a/packages/react-beautiful-dnd-migration/package.json +++ b/packages/react-beautiful-dnd-migration/package.json @@ -20,7 +20,6 @@ "dependencies": { "@atlaskit/pragmatic-drag-and-drop": "^1.4.0", "@atlaskit/pragmatic-drag-and-drop-hitbox": "^1.0.0", - "@atlaskit/pragmatic-drag-and-drop-live-region": "^1.1.0", "@atlaskit/pragmatic-drag-and-drop-react-beautiful-dnd-autoscroll": "^1.2.0", "@atlaskit/tokens": "^2.4.0", "@babel/runtime": "^7.0.0", diff --git a/packages/react-beautiful-dnd-migration/src/drag-drop-context/index.tsx b/packages/react-beautiful-dnd-migration/src/drag-drop-context/index.tsx index b88986c..a67ce83 100644 --- a/packages/react-beautiful-dnd-migration/src/drag-drop-context/index.tsx +++ b/packages/react-beautiful-dnd-migration/src/drag-drop-context/index.tsx @@ -21,8 +21,6 @@ import type { MovementMode, } from 'react-beautiful-dnd'; -import { announce } from '@atlaskit/pragmatic-drag-and-drop-live-region'; - import { getDraggableDimensions } from '../hooks/use-captured-dimensions'; import { useCleanupFn } from '../hooks/use-cleanup-fn'; import { attributes, getAttribute } from '../utils/attributes'; @@ -40,6 +38,7 @@ import { usePointerControls } from './hooks/use-pointer-controls'; import useStyleMarshal from './hooks/use-style-marshal'; import { DragDropContextProvider } from './internal-context'; import { LifecycleContextProvider, useLifecycle } from './lifecycle-context'; +import { announce } from './live-region'; import { rbdInvariant } from './rbd-invariant'; import { defaultDragHandleUsageInstructions, getProvided } from './screen-reader'; import type { DragController, DragState } from './types'; diff --git a/packages/react-beautiful-dnd-migration/src/drag-drop-context/live-region.tsx b/packages/react-beautiful-dnd-migration/src/drag-drop-context/live-region.tsx new file mode 100644 index 0000000..71fbdac --- /dev/null +++ b/packages/react-beautiful-dnd-migration/src/drag-drop-context/live-region.tsx @@ -0,0 +1,71 @@ +/** + * This is defined in the migration package instead of using `@atlaskit/pragmatic-drag-and-drop-live-region` + * because RBD-style dragging has different needs to the alternative flows of PDND. + * + * RBD can make a lot of announcements in a short period, so delaying messages is not feasible. + * RBD also maintains focus while dragging, so messages being skipped is less of a concern. + * + * `@atlaskit/pragmatic-drag-and-drop-live-region` has been tailored for PDND-specific alternative flows, + * where focus usually changes around the time `announce()` is called. So in `@atlaskit/pragmatic-drag-and-drop-live-region` + * messages have delays to avoid them being skipped. + */ + +let node: HTMLElement | null = null; + +const size = '1px'; + +const visuallyHiddenStyles = { + // Standard visually hidden styles. + // Copied from our VisuallyHidden (react) package. + width: size, + height: size, + padding: '0', + position: 'absolute', + border: '0', + clip: `rect(${size}, ${size}, ${size}, ${size})`, + overflow: 'hidden', + whiteSpace: 'nowrap', + // Pulling upwards slightly to prevent the page + // from growing when appended to a body that contains + // an element with height:100% + marginTop: `-${size}`, + // Just being safe and letting this element not interfere with hitboxes + pointerEvents: 'none', +}; + +/** + * Creates a live region node, appends it to the body, and returns it. + */ +function createNode(): HTMLElement { + const node = document.createElement('div'); + node.setAttribute('role', 'alert'); + Object.assign(node.style, visuallyHiddenStyles); + document.body.append(node); + return node; +} + +/** + * Returns the live region node, creating one if necessary. + */ +function getNode(): HTMLElement { + if (node === null) { + node = createNode(); + } + return node; +} + +/** + * Announces the provided message to assistive technology. + */ +export function announce(message: string) { + const node = getNode(); + node.textContent = message; +} + +/** + * Removes the created live region. If there is no live region this is a no-op. + */ +export function cleanup() { + node?.remove(); + node = null; +}