From b16be4dd9dc69d88b822a202ede2dc4272ae3ad6 Mon Sep 17 00:00:00 2001 From: Reid Barber Date: Wed, 18 Jun 2025 14:32:40 -0500 Subject: [PATCH 1/4] add useDynamicDescription --- packages/@react-aria/dnd/src/useDrag.ts | 8 +- .../@react-aria/dnd/src/useVirtualDrop.ts | 7 +- packages/@react-aria/utils/src/index.ts | 2 +- .../@react-aria/utils/src/useDescription.ts | 118 ++++++++++++- .../utils/test/useDescription.test.tsx | 159 ++++++++++++++++++ 5 files changed, 288 insertions(+), 6 deletions(-) create mode 100644 packages/@react-aria/utils/test/useDescription.test.tsx diff --git a/packages/@react-aria/dnd/src/useDrag.ts b/packages/@react-aria/dnd/src/useDrag.ts index 49cc8f9ec75..dd6cc8f1d1d 100644 --- a/packages/@react-aria/dnd/src/useDrag.ts +++ b/packages/@react-aria/dnd/src/useDrag.ts @@ -18,7 +18,7 @@ import {DROP_EFFECT_TO_DROP_OPERATION, DROP_OPERATION, EFFECT_ALLOWED} from './c import {globalDropEffect, setGlobalAllowedDropOperations, setGlobalDropEffect, useDragModality, writeToDataTransfer} from './utils'; // @ts-ignore import intlMessages from '../intl/*.json'; -import {isVirtualClick, isVirtualPointerEvent, useDescription, useGlobalListeners, useLayoutEffect} from '@react-aria/utils'; +import {isVirtualClick, isVirtualPointerEvent, useDynamicDescription, useGlobalListeners, useLayoutEffect} from '@react-aria/utils'; import {useLocalizedStringFormatter} from '@react-aria/i18n'; export interface DragOptions { @@ -280,7 +280,11 @@ export function useDrag(options: DragOptions): DragResult { let modality = useDragModality(); let message = !isDragging ? MESSAGES[modality].start : MESSAGES[modality].end; - let descriptionProps = useDescription(stringFormatter.format(message)); + let {descriptionProps, setDescription} = useDynamicDescription(stringFormatter.format(message)); + + useLayoutEffect(() => { + setDescription(stringFormatter.format(message)); + }, [message, stringFormatter, setDescription]); let interactions: HTMLAttributes = {}; if (!hasDragButton) { diff --git a/packages/@react-aria/dnd/src/useVirtualDrop.ts b/packages/@react-aria/dnd/src/useVirtualDrop.ts index b74897d3cba..037c02abed9 100644 --- a/packages/@react-aria/dnd/src/useVirtualDrop.ts +++ b/packages/@react-aria/dnd/src/useVirtualDrop.ts @@ -15,8 +15,8 @@ import {DOMAttributes} from 'react'; import * as DragManager from './DragManager'; // @ts-ignore import intlMessages from '../intl/*.json'; -import {useDescription} from '@react-aria/utils'; import {useDragModality} from './utils'; +import {useDynamicDescription, useLayoutEffect} from '@react-aria/utils'; import {useLocalizedStringFormatter} from '@react-aria/i18n'; interface VirtualDropResult { @@ -33,7 +33,10 @@ export function useVirtualDrop(): VirtualDropResult { let stringFormatter = useLocalizedStringFormatter(intlMessages, '@react-aria/dnd'); let modality = useDragModality(); let dragSession = DragManager.useDragSession(); - let descriptionProps = useDescription(dragSession ? stringFormatter.format(MESSAGES[modality]) : ''); + let {descriptionProps, setDescription} = useDynamicDescription(dragSession ? stringFormatter.format(MESSAGES[modality]) : ''); + useLayoutEffect(() => { + setDescription(dragSession ? stringFormatter.format(MESSAGES[modality]) : ''); + }, [dragSession, modality, setDescription, stringFormatter]); return { dropProps: { diff --git a/packages/@react-aria/utils/src/index.ts b/packages/@react-aria/utils/src/index.ts index a567ea89667..d02f0068a31 100644 --- a/packages/@react-aria/utils/src/index.ts +++ b/packages/@react-aria/utils/src/index.ts @@ -34,7 +34,7 @@ export {getScrollParent} from './getScrollParent'; export {getScrollParents} from './getScrollParents'; export {isScrollable} from './isScrollable'; export {useViewportSize} from './useViewportSize'; -export {useDescription} from './useDescription'; +export {useDescription, useDynamicDescription} from './useDescription'; export {isMac, isIPhone, isIPad, isIOS, isAppleDevice, isWebKit, isChrome, isAndroid, isFirefox} from './platform'; export {useEvent} from './useEvent'; export {useValueEffect} from './useValueEffect'; diff --git a/packages/@react-aria/utils/src/useDescription.ts b/packages/@react-aria/utils/src/useDescription.ts index 01f953e9c32..8f4811aebb1 100644 --- a/packages/@react-aria/utils/src/useDescription.ts +++ b/packages/@react-aria/utils/src/useDescription.ts @@ -11,11 +11,12 @@ */ import {AriaLabelingProps} from '@react-types/shared'; +import {useCallback, useRef, useState} from 'react'; import {useLayoutEffect} from './useLayoutEffect'; -import {useState} from 'react'; let descriptionId = 0; const descriptionNodes = new Map(); +const dynamicDescriptionNodes = new Map(); export function useDescription(description?: string): AriaLabelingProps { let [id, setId] = useState(); @@ -54,3 +55,118 @@ export function useDescription(description?: string): AriaLabelingProps { 'aria-describedby': description ? id : undefined }; } + +export type DynamicDescriptionResult = { + /** Props for the description element. */ + descriptionProps: AriaLabelingProps, + /** Setter for updating the description text. */ + setDescription: (description?: string) => void +} + +/** + * Similar to `useDescription`, but optimized for cases where the description text + * changes over time (e.g. drag modality changes) and multiple consumers are on the page. + * Instead of destroying and recreating the description element, this hook keeps the + * same element (and id) for the lifetime of the component and updates the element's text + * content when needed, avoiding unnecessary re-renders (e.g. many drop targets on the page). + */ +export function useDynamicDescription(initialDescription?: string): DynamicDescriptionResult { + let [idState, setIdState] = useState(); + + let elementRef = useRef(null); + let descRef = useRef<{refCount: number, element: Element} | null>(null); + + let getOrCreateNode = useCallback((text: string): Element => { + let desc = dynamicDescriptionNodes.get(text); + if (!desc) { + let node = document.createElement('div'); + node.id = `react-aria-description-${descriptionId++}`; + node.style.display = 'none'; + node.textContent = text; + document.body.appendChild(node); + desc = {refCount: 0, element: node}; + dynamicDescriptionNodes.set(text, desc); + } + + desc.refCount++; + descRef.current = desc; + elementRef.current = desc.element; + setIdState(desc.element.id); + return desc.element; + }, []); + + useLayoutEffect(() => { + if (initialDescription) { + getOrCreateNode(initialDescription); + } + + return () => { + if (descRef.current) { + descRef.current.refCount--; + if (descRef.current.refCount === 0) { + descRef.current.element.remove(); + for (let [key, value] of dynamicDescriptionNodes) { + if (value === descRef.current) { + dynamicDescriptionNodes.delete(key); + break; + } + } + } + } + }; + // eslint-disable-next-line react-hooks/exhaustive-deps + }, []); + + useLayoutEffect(() => { + if (!initialDescription) { + return; + } + + if (!elementRef.current) { + getOrCreateNode(initialDescription); + return; + } + + if (elementRef.current.textContent === initialDescription) { + return; + } + + for (let [key, value] of dynamicDescriptionNodes) { + if (value.element === elementRef.current) { + dynamicDescriptionNodes.delete(key); + break; + } + } + + dynamicDescriptionNodes.set(initialDescription, descRef.current!); + elementRef.current.textContent = initialDescription; + }, [initialDescription, getOrCreateNode]); + + let setDescription = useCallback((description?: string) => { + if (!description) { + return; + } + if (!elementRef.current) { + getOrCreateNode(description); + return; + } + if (elementRef.current.textContent === description) { + return; + } + for (let [key, value] of dynamicDescriptionNodes) { + if (value.element === elementRef.current) { + dynamicDescriptionNodes.delete(key); + break; + } + } + dynamicDescriptionNodes.set(description, descRef.current!); + elementRef.current.textContent = description; + }, [getOrCreateNode]); + + return { + descriptionProps: { + 'aria-describedby': idState + }, + setDescription + }; +} diff --git a/packages/@react-aria/utils/test/useDescription.test.tsx b/packages/@react-aria/utils/test/useDescription.test.tsx new file mode 100644 index 00000000000..774093200ba --- /dev/null +++ b/packages/@react-aria/utils/test/useDescription.test.tsx @@ -0,0 +1,159 @@ +/* + * Copyright 2025 Adobe. All rights reserved. + * This file is licensed to you under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. You may obtain a copy + * of the License at http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software distributed under + * the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR REPRESENTATIONS + * OF ANY KIND, either express or implied. See the License for the specific language + * governing permissions and limitations under the License. + */ + +import {actHook as act, renderHook} from '@react-spectrum/test-utils-internal'; +import {useDescription, useDynamicDescription} from '../src/useDescription'; + +describe('useDescription', () => { + it('should return an id if description is provided', () => { + let {result} = renderHook(() => useDescription('Test description')); + expect(result.current['aria-describedby']).toMatch(/^react-aria-description-\d+$/); + }); + + it('should return undefined if no description is provided', () => { + let {result} = renderHook(() => useDescription()); + expect(result.current['aria-describedby']).toBeUndefined(); + }); + + it('should reuse the same id for the same description', () => { + let {result: result1} = renderHook(() => useDescription('Test description')); + let {result: result2} = renderHook(() => useDescription('Test description')); + expect(result1.current['aria-describedby']).toBe(result2.current['aria-describedby']); + }); + + it('should create a new id for a new description', () => { + let {result: result1} = renderHook(() => useDescription('Test description 1')); + let {result: result2} = renderHook(() => useDescription('Test description 2')); + expect(result1.current['aria-describedby']).not.toBe(result2.current['aria-describedby']); + }); + + it('should clean up description node on unmount', () => { + let {result, unmount} = renderHook(() => useDescription('Test description')); + let id = result.current['aria-describedby']; + expect(document.getElementById(id!)).not.toBeNull(); + unmount(); + expect(document.getElementById(id!)).toBeNull(); + }); + + it('should not clean up if other components are using the same description', () => { + let {result: result1, unmount: unmount1} = renderHook(() => useDescription('Test description')); + let {unmount: unmount2} = renderHook(() => useDescription('Test description')); + let id = result1.current['aria-describedby']; + expect(document.getElementById(id!)).not.toBeNull(); + unmount1(); + expect(document.getElementById(id!)).not.toBeNull(); + unmount2(); + expect(document.getElementById(id!)).toBeNull(); + }); +}); + + +describe('useDynamicDescription', () => { + it('should return a stable aria-describedby id', () => { + const {result, unmount} = renderHook(({text}) => useDynamicDescription(text), { + initialProps: {text: 'Initial description'} + }); + + const {descriptionProps} = result.current; + expect(descriptionProps['aria-describedby']).toBeDefined(); + const id = descriptionProps['aria-describedby']!; + + const node = document.getElementById(id); + expect(node).not.toBeNull(); + expect(node!.textContent).toBe('Initial description'); + expect((node as HTMLElement).style.display).toBe('none'); + + act(() => { + const {setDescription} = result.current; + setDescription('Updated description'); + }); + + const updatedNode = document.getElementById(id); + expect(updatedNode).toBe(node); // Same element instance + expect(updatedNode!.textContent).toBe('Updated description'); + expect(document.querySelectorAll(`#${id}`).length).toBe(1); + + unmount(); + expect(document.getElementById(id)).toBeNull(); + }); + + it('should update text when initialDescription prop changes without changing id', () => { + const {result, rerender} = renderHook(({text}) => useDynamicDescription(text), { + initialProps: {text: 'First'} + }); + + const id = result.current.descriptionProps['aria-describedby']!; + expect(document.getElementById(id)!.textContent).toBe('First'); + + rerender({text: 'Second'}); + + expect(result.current.descriptionProps['aria-describedby']).toBe(id); + expect(document.getElementById(id)!.textContent).toBe('Second'); + }); + + it('should reuse the same node for multiple hooks with identical descriptions and clean up when the last unmounts', () => { + const {result: result1, unmount: unmount1} = renderHook(() => useDynamicDescription('Shared description')); + const {result: result2, unmount: unmount2} = renderHook(() => useDynamicDescription('Shared description')); + + const id1 = result1.current.descriptionProps['aria-describedby']!; + const id2 = result2.current.descriptionProps['aria-describedby']!; + + // Both hooks should reference the same id and therefore the same element. + expect(id1).toBe(id2); + const node = document.getElementById(id1)!; + expect(node).not.toBeNull(); + expect(node.textContent).toBe('Shared description'); + + // Unmounting the first hook should not remove the element since the second still references it. + unmount1(); + expect(document.getElementById(id1)).not.toBeNull(); + + // After the final unmount, the element should be removed. + unmount2(); + expect(document.getElementById(id1)).toBeNull(); + }); + + it('should lazily create a node when setDescription is called after an undefined initial description', () => { + const {result, unmount} = renderHook(() => useDynamicDescription(undefined)); + + expect(result.current.descriptionProps['aria-describedby']).toBeUndefined(); + + act(() => { + result.current.setDescription('Lazy description'); + }); + + const id = result.current.descriptionProps['aria-describedby']!; + expect(id).toMatch(/^react-aria-description-\d+$/); + const node = document.getElementById(id); + expect(node).not.toBeNull(); + expect(node!.textContent).toBe('Lazy description'); + + unmount(); + expect(document.getElementById(id)).toBeNull(); + }); + + it('should ignore undefined values passed to setDescription and keep existing text', () => { + const {result} = renderHook(() => useDynamicDescription('Keep me')); + const id = result.current.descriptionProps['aria-describedby']!; + const nodeBefore = document.getElementById(id); + expect(nodeBefore!.textContent).toBe('Keep me'); + + act(() => { + result.current.setDescription(undefined); + }); + + expect(result.current.descriptionProps['aria-describedby']).toBe(id); + const nodeAfter = document.getElementById(id); + expect(nodeAfter).toBe(nodeBefore); + expect(nodeAfter!.textContent).toBe('Keep me'); + }); +}); From 77295c59fa7f1d4155f9984c602a179d7c6d0974 Mon Sep 17 00:00:00 2001 From: Reid Barber Date: Wed, 18 Jun 2025 16:11:57 -0500 Subject: [PATCH 2/4] handle cleared description --- .../@react-aria/utils/src/useDescription.ts | 84 +++++++++++++------ 1 file changed, 59 insertions(+), 25 deletions(-) diff --git a/packages/@react-aria/utils/src/useDescription.ts b/packages/@react-aria/utils/src/useDescription.ts index 8f4811aebb1..3739017e5a5 100644 --- a/packages/@react-aria/utils/src/useDescription.ts +++ b/packages/@react-aria/utils/src/useDescription.ts @@ -97,9 +97,46 @@ export function useDynamicDescription(initialDescription?: string): DynamicDescr useLayoutEffect(() => { if (initialDescription) { - getOrCreateNode(initialDescription); + if (!elementRef.current) { + getOrCreateNode(initialDescription); + return; + } + + if (elementRef.current.textContent === initialDescription) { + return; + } + + for (let [key, value] of dynamicDescriptionNodes) { + if (value.element === elementRef.current) { + dynamicDescriptionNodes.delete(key); + break; + } + } + + dynamicDescriptionNodes.set(initialDescription, descRef.current!); + elementRef.current.textContent = initialDescription; + return; } + if (elementRef.current && descRef.current) { + descRef.current.refCount--; + if (descRef.current.refCount === 0) { + descRef.current.element.remove(); + for (let [key, value] of dynamicDescriptionNodes) { + if (value === descRef.current) { + dynamicDescriptionNodes.delete(key); + break; + } + } + } + + elementRef.current = null; + descRef.current = null; + setIdState(undefined); + } + }, [initialDescription, getOrCreateNode]); + + useLayoutEffect(() => { return () => { if (descRef.current) { descRef.current.refCount--; @@ -114,45 +151,42 @@ export function useDynamicDescription(initialDescription?: string): DynamicDescr } } }; - // eslint-disable-next-line react-hooks/exhaustive-deps }, []); - useLayoutEffect(() => { - if (!initialDescription) { - return; - } - - if (!elementRef.current) { - getOrCreateNode(initialDescription); - return; - } - - if (elementRef.current.textContent === initialDescription) { + let setDescription = useCallback((description?: string) => { + if (description === undefined) { return; } - for (let [key, value] of dynamicDescriptionNodes) { - if (value.element === elementRef.current) { - dynamicDescriptionNodes.delete(key); - break; - } - } - - dynamicDescriptionNodes.set(initialDescription, descRef.current!); - elementRef.current.textContent = initialDescription; - }, [initialDescription, getOrCreateNode]); - - let setDescription = useCallback((description?: string) => { if (!description) { + if (elementRef.current && descRef.current) { + descRef.current.refCount--; + if (descRef.current.refCount === 0) { + descRef.current.element.remove(); + for (let [key, value] of dynamicDescriptionNodes) { + // eslint-disable-next-line max-depth + if (value === descRef.current) { + dynamicDescriptionNodes.delete(key); + break; + } + } + } + elementRef.current = null; + descRef.current = null; + setIdState(undefined); + } return; } + if (!elementRef.current) { getOrCreateNode(description); return; } + if (elementRef.current.textContent === description) { return; } + for (let [key, value] of dynamicDescriptionNodes) { if (value.element === elementRef.current) { dynamicDescriptionNodes.delete(key); From 91cbb185d6fe07c29eece438e964bf6b53fca597 Mon Sep 17 00:00:00 2001 From: Reid Barber Date: Wed, 18 Jun 2025 16:27:29 -0500 Subject: [PATCH 3/4] fix failing tests --- .../list/test/ListView.test.js | 2 +- .../list/test/ListViewDnd.test.js | 19 +++---------------- 2 files changed, 4 insertions(+), 17 deletions(-) diff --git a/packages/@react-spectrum/list/test/ListView.test.js b/packages/@react-spectrum/list/test/ListView.test.js index 27735a9a7b9..47e1d64d280 100644 --- a/packages/@react-spectrum/list/test/ListView.test.js +++ b/packages/@react-spectrum/list/test/ListView.test.js @@ -1722,7 +1722,7 @@ describe('ListView', function () { act(() => { jest.runAllTimers(); }); - expect(grid.firstChild).toBeEmptyDOMElement(); + expect(grid.firstChild).toBeInTheDocument(); }); }); }); diff --git a/packages/@react-spectrum/list/test/ListViewDnd.test.js b/packages/@react-spectrum/list/test/ListViewDnd.test.js index 9d3e8bdfddc..63d3f42df5c 100644 --- a/packages/@react-spectrum/list/test/ListViewDnd.test.js +++ b/packages/@react-spectrum/list/test/ListViewDnd.test.js @@ -434,12 +434,7 @@ describe('ListView', function () { expect(onDrop).toHaveBeenCalledTimes(1); fireEvent(cell, new DragEvent('dragend', {dataTransfer, clientX: 1, clientY: 110})); - // TODO: fix in strict mode, due to https://github.com/facebook/react/issues/29585 - if (isReact19) { - expect(onDragEnd).toHaveBeenCalledTimes(2); - } else { - expect(onDragEnd).toHaveBeenCalledTimes(1); - } + expect(onDragEnd).toHaveBeenCalledTimes(1); act(() => jest.runAllTimers()); @@ -482,18 +477,10 @@ describe('ListView', function () { fireEvent(grid, new DragEvent('drop', {dataTransfer, clientX: 1, clientY: 150})); act(() => jest.runAllTimers()); await act(async () => Promise.resolve()); - if (isReact19) { - expect(onDrop).toHaveBeenCalledTimes(1); - } else { - expect(onDrop).toHaveBeenCalledTimes(1); - } + expect(onDrop).toHaveBeenCalledTimes(1); fireEvent(cell, new DragEvent('dragend', {dataTransfer, clientX: 1, clientY: 150})); - if (isReact19) { - expect(onDragEnd).toHaveBeenCalledTimes(2); - } else { - expect(onDragEnd).toHaveBeenCalledTimes(1); - } + expect(onDragEnd).toHaveBeenCalledTimes(1); act(() => jest.runAllTimers()); From 5c632015c5cb580c91e12e7bb653803203778633 Mon Sep 17 00:00:00 2001 From: Reid Barber Date: Wed, 18 Jun 2025 17:58:28 -0500 Subject: [PATCH 4/4] revert test fixes --- .../list/test/ListView.test.js | 2 +- .../list/test/ListViewDnd.test.js | 19 ++++++++++++++++--- 2 files changed, 17 insertions(+), 4 deletions(-) diff --git a/packages/@react-spectrum/list/test/ListView.test.js b/packages/@react-spectrum/list/test/ListView.test.js index 47e1d64d280..27735a9a7b9 100644 --- a/packages/@react-spectrum/list/test/ListView.test.js +++ b/packages/@react-spectrum/list/test/ListView.test.js @@ -1722,7 +1722,7 @@ describe('ListView', function () { act(() => { jest.runAllTimers(); }); - expect(grid.firstChild).toBeInTheDocument(); + expect(grid.firstChild).toBeEmptyDOMElement(); }); }); }); diff --git a/packages/@react-spectrum/list/test/ListViewDnd.test.js b/packages/@react-spectrum/list/test/ListViewDnd.test.js index 63d3f42df5c..9d3e8bdfddc 100644 --- a/packages/@react-spectrum/list/test/ListViewDnd.test.js +++ b/packages/@react-spectrum/list/test/ListViewDnd.test.js @@ -434,7 +434,12 @@ describe('ListView', function () { expect(onDrop).toHaveBeenCalledTimes(1); fireEvent(cell, new DragEvent('dragend', {dataTransfer, clientX: 1, clientY: 110})); - expect(onDragEnd).toHaveBeenCalledTimes(1); + // TODO: fix in strict mode, due to https://github.com/facebook/react/issues/29585 + if (isReact19) { + expect(onDragEnd).toHaveBeenCalledTimes(2); + } else { + expect(onDragEnd).toHaveBeenCalledTimes(1); + } act(() => jest.runAllTimers()); @@ -477,10 +482,18 @@ describe('ListView', function () { fireEvent(grid, new DragEvent('drop', {dataTransfer, clientX: 1, clientY: 150})); act(() => jest.runAllTimers()); await act(async () => Promise.resolve()); - expect(onDrop).toHaveBeenCalledTimes(1); + if (isReact19) { + expect(onDrop).toHaveBeenCalledTimes(1); + } else { + expect(onDrop).toHaveBeenCalledTimes(1); + } fireEvent(cell, new DragEvent('dragend', {dataTransfer, clientX: 1, clientY: 150})); - expect(onDragEnd).toHaveBeenCalledTimes(1); + if (isReact19) { + expect(onDragEnd).toHaveBeenCalledTimes(2); + } else { + expect(onDragEnd).toHaveBeenCalledTimes(1); + } act(() => jest.runAllTimers());