Skip to content

WIP: fix(DnD): avoid unnecessary re-renders when drag modality changes. #8413

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions packages/@react-aria/dnd/src/useDrag.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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<HTMLElement> = {};
if (!hasDragButton) {
Expand Down
7 changes: 5 additions & 2 deletions packages/@react-aria/dnd/src/useVirtualDrop.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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: {
Expand Down
2 changes: 1 addition & 1 deletion packages/@react-aria/utils/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down
152 changes: 151 additions & 1 deletion packages/@react-aria/utils/src/useDescription.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<string, {refCount: number, element: Element}>();
const dynamicDescriptionNodes = new Map<string, {refCount: number, element: Element}>();

export function useDescription(description?: string): AriaLabelingProps {
let [id, setId] = useState<string | undefined>();
Expand Down Expand Up @@ -54,3 +55,152 @@ 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<string | undefined>();

let elementRef = useRef<Element | null>(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) {
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--;
if (descRef.current.refCount === 0) {
descRef.current.element.remove();
for (let [key, value] of dynamicDescriptionNodes) {
if (value === descRef.current) {
dynamicDescriptionNodes.delete(key);
break;
}
}
}
}
};
}, []);

let setDescription = useCallback((description?: string) => {
if (description === undefined) {
return;
}

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);
break;
}
}
dynamicDescriptionNodes.set(description, descRef.current!);
elementRef.current.textContent = description;
}, [getOrCreateNode]);

return {
descriptionProps: {
'aria-describedby': idState
},
setDescription
};
}
159 changes: 159 additions & 0 deletions packages/@react-aria/utils/test/useDescription.test.tsx
Original file line number Diff line number Diff line change
@@ -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');
});
});