-
Notifications
You must be signed in to change notification settings - Fork 1.4k
fix(dnd): update useDescription to prevent unnecessary re-renders #9738
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
Changes from all commits
db44f62
f0f8bce
7b59821
cb150bd
ebc40ce
4e95ffd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,43 +12,101 @@ | |
|
|
||
| import {AriaLabelingProps} from '@react-types/shared'; | ||
| import {useLayoutEffect} from './useLayoutEffect'; | ||
| import {useState} from 'react'; | ||
| import {useRef, useState} from 'react'; | ||
|
|
||
| let descriptionId = 0; | ||
| const descriptionNodes = new Map<string, {refCount: number, element: Element}>(); | ||
|
|
||
| export function useDescription(description?: string): AriaLabelingProps { | ||
| interface DescriptionNode { | ||
| refCount: number, | ||
| element: HTMLElement | ||
| } | ||
|
|
||
| interface DescriptionSubscription { | ||
| key: string, | ||
| node: DescriptionNode, | ||
| nodes: Map<string, DescriptionNode> | ||
| } | ||
|
|
||
| const descriptionNodes = new Map<string, DescriptionNode>(); | ||
| const dynamicDescriptionNodes = new Map<string, DescriptionNode>(); | ||
|
|
||
| function createDescriptionNode(id: string, description: string): HTMLElement { | ||
| let node = document.createElement('div'); | ||
| node.id = id; | ||
| node.style.display = 'none'; | ||
| node.textContent = description; | ||
| document.body.appendChild(node); | ||
| return node; | ||
| } | ||
|
|
||
| function getOrCreateDescriptionNode(nodes: Map<string, DescriptionNode>, descriptionKey: string, description: string) { | ||
| let desc = nodes.get(descriptionKey); | ||
| if (!desc) { | ||
| let id = `react-aria-description-${descriptionId++}`; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. in the case that multiple copies are loaded, this could create conflicting ids, better to use id generation like
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. or put the id generation into the hooks and make use of |
||
| let node = createDescriptionNode(id, description); | ||
| desc = {refCount: 0, element: node}; | ||
| nodes.set(descriptionKey, desc); | ||
| } | ||
|
|
||
| return desc; | ||
| } | ||
|
|
||
| function cleanupDescriptionSubscription(subscription: DescriptionSubscription, nodeRef: {current: HTMLElement | null}) { | ||
| if (--subscription.node.refCount === 0) { | ||
| subscription.node.element.remove(); | ||
| subscription.nodes.delete(subscription.key); | ||
| } | ||
|
|
||
| if (nodeRef.current === subscription.node.element) { | ||
| nodeRef.current = null; | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Provides an `aria-describedby` reference to a shared hidden description node. | ||
| * By default, descriptions are shared by exact text. If `descriptionKey` is provided, | ||
| * a stable node is shared by key and its text content updates in place as the | ||
| * description changes. | ||
| */ | ||
| export function useDescription(description?: string, descriptionKey?: string): AriaLabelingProps { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In theory we could do this without a special key - basically try to keep the id the same over time. When the description changes, if the ref count is zero, reuse the same id for the new description. |
||
| let [id, setId] = useState<string | undefined>(); | ||
| let subscriptionRef = useRef<DescriptionSubscription | null>(null); | ||
| let nodeRef = useRef<HTMLElement | null>(null); | ||
| let isDynamic = descriptionKey != null; | ||
|
|
||
| useLayoutEffect(() => { | ||
| if (!description) { | ||
| return; | ||
| let subscription = subscriptionRef.current; | ||
| let key = descriptionKey ?? description; | ||
| let nodes = isDynamic ? dynamicDescriptionNodes : descriptionNodes; | ||
| if (subscription && (subscription.key !== key || subscription.nodes !== nodes)) { | ||
| cleanupDescriptionSubscription(subscription, nodeRef); | ||
| subscriptionRef.current = null; | ||
| subscription = null; | ||
| } | ||
|
|
||
| if (!subscription && description && key) { | ||
| let node = getOrCreateDescriptionNode(nodes, key, isDynamic ? '' : description); | ||
| node.refCount++; | ||
| subscription = {key, node, nodes}; | ||
| subscriptionRef.current = subscription; | ||
| nodeRef.current = node.element; | ||
| setId(node.element.id); | ||
| } | ||
|
|
||
| let desc = descriptionNodes.get(description); | ||
| if (!desc) { | ||
| let id = `react-aria-description-${descriptionId++}`; | ||
| setId(id); | ||
|
|
||
| let node = document.createElement('div'); | ||
| node.id = id; | ||
| node.style.display = 'none'; | ||
| node.textContent = description; | ||
| document.body.appendChild(node); | ||
| desc = {refCount: 0, element: node}; | ||
| descriptionNodes.set(description, desc); | ||
| } else { | ||
| setId(desc.element.id); | ||
| if (isDynamic && description && nodeRef.current) { | ||
| nodeRef.current.textContent = description; | ||
| } | ||
| }, [description, descriptionKey, isDynamic]); | ||
|
|
||
| desc.refCount++; | ||
| useLayoutEffect(() => { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I still think this can be moved up into the previous layout effect, negating the need to call cleanup on line 82 |
||
| return () => { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if you move this up to be in the first useLayoutEffect, then you can de-duplicate the refCount removal, always handle it in the cleanup of that effect |
||
| if (desc && --desc.refCount === 0) { | ||
| desc.element.remove(); | ||
| descriptionNodes.delete(description); | ||
| let subscription = subscriptionRef.current; | ||
| if (subscription) { | ||
| cleanupDescriptionSubscription(subscription, nodeRef); | ||
| subscriptionRef.current = null; | ||
| } | ||
| }; | ||
| }, [description]); | ||
| }, []); | ||
|
|
||
| return { | ||
| 'aria-describedby': description ? id : undefined | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,145 @@ | ||
| /* | ||
| * 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} 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('useDescription with a description key', () => { | ||
| it('should return an id if description is provided', () => { | ||
| let {result} = renderHook(() => useDescription('Test description', 'dynamic-1')); | ||
| expect(result.current['aria-describedby']).toMatch(/^react-aria-description-\d+$/); | ||
| }); | ||
|
|
||
| it('should return undefined if no description is provided', () => { | ||
| let {result} = renderHook(() => useDescription(undefined, 'dynamic-2')); | ||
| expect(result.current['aria-describedby']).toBeUndefined(); | ||
| }); | ||
|
|
||
| it('should reuse the same id for the same description key', () => { | ||
| let {result: result1} = renderHook(() => useDescription('Test description', 'shared-key')); | ||
| let {result: result2} = renderHook(() => useDescription('Test description', 'shared-key')); | ||
| expect(result1.current['aria-describedby']).toBe(result2.current['aria-describedby']); | ||
| }); | ||
|
|
||
| it('should create a new id for a different description key', () => { | ||
| let {result: result1} = renderHook(() => useDescription('Test description', 'dynamic-3')); | ||
| let {result: result2} = renderHook(() => useDescription('Test description', 'dynamic-4')); | ||
| expect(result1.current['aria-describedby']).not.toBe(result2.current['aria-describedby']); | ||
| }); | ||
|
|
||
| it('should keep the same id and update text content when description changes for the same key', () => { | ||
| let {result, rerender} = renderHook( | ||
| ({description}: {description?: string}) => useDescription(description, 'dynamic-5'), | ||
| {initialProps: {description: 'Test description 1'}} | ||
| ); | ||
|
|
||
| let id = result.current['aria-describedby']; | ||
| let node = document.getElementById(id!); | ||
| expect(node?.textContent).toBe('Test description 1'); | ||
|
|
||
| act(() => { | ||
| rerender({description: 'Test description 2'}); | ||
| }); | ||
|
|
||
| expect(result.current['aria-describedby']).toBe(id); | ||
| expect(document.getElementById(id!)).toBe(node); | ||
| expect(node?.textContent).toBe('Test description 2'); | ||
| }); | ||
|
|
||
| it('should keep the same id for the lifetime of the component', () => { | ||
| let {result, rerender} = renderHook( | ||
| ({description}: {description?: string}) => useDescription(description, 'dynamic-6'), | ||
| {initialProps: {description: 'Test description'}} | ||
| ); | ||
|
|
||
| let id = result.current['aria-describedby']; | ||
| let node = document.getElementById(id!); | ||
|
|
||
| act(() => { | ||
| rerender({description: ''}); | ||
| }); | ||
|
|
||
| expect(result.current['aria-describedby']).toBeUndefined(); | ||
| expect(document.getElementById(id!)).toBe(node); | ||
| expect(node?.textContent).toBe('Test description'); | ||
|
|
||
| act(() => { | ||
| rerender({description: 'Updated description'}); | ||
| }); | ||
|
|
||
| expect(result.current['aria-describedby']).toBe(id); | ||
| expect(document.getElementById(id!)).toBe(node); | ||
| expect(node?.textContent).toBe('Updated description'); | ||
| }); | ||
|
|
||
| it('should not clean up if other components are using the same description key', () => { | ||
| let {result: result1, unmount: unmount1} = renderHook(() => useDescription('Test description', 'shared-cleanup')); | ||
| let {unmount: unmount2} = renderHook(() => useDescription('Test description', 'shared-cleanup')); | ||
| 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(); | ||
| }); | ||
|
|
||
| it('should clean up description node on unmount', () => { | ||
| let {result, unmount} = renderHook(() => useDescription('Test description', 'dynamic-7')); | ||
| let id = result.current['aria-describedby']; | ||
| expect(document.getElementById(id!)).not.toBeNull(); | ||
| unmount(); | ||
| expect(document.getElementById(id!)).toBeNull(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If half the app is in ja-JP and the other half is in en-US, these keys will conflict and some english descriptions will end up in jp or vice versa?
Probably have to use the translated string itself as the key