diff --git a/packages/@react-aria/dnd/src/useDrag.ts b/packages/@react-aria/dnd/src/useDrag.ts index 6e1f51d7c28..19da16c2411 100644 --- a/packages/@react-aria/dnd/src/useDrag.ts +++ b/packages/@react-aria/dnd/src/useDrag.ts @@ -12,13 +12,13 @@ import {AriaButtonProps} from '@react-types/button'; import {DragEndEvent, DragItem, DragMoveEvent, DragPreviewRenderer, DragStartEvent, DropOperation, PressEvent, RefObject} from '@react-types/shared'; -import {DragEvent, HTMLAttributes, useRef, useState} from 'react'; +import {DragEvent, HTMLAttributes, version as ReactVersion, useEffect, useRef, useState} from 'react'; import * as DragManager from './DragManager'; import {DROP_EFFECT_TO_DROP_OPERATION, DROP_OPERATION, EFFECT_ALLOWED} from './constants'; 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, useDescription, useGlobalListeners} from '@react-aria/utils'; import {useLocalizedStringFormatter} from '@react-aria/i18n'; export interface DragOptions { @@ -82,11 +82,11 @@ export function useDrag(options: DragOptions): DragResult { y: 0 }).current; state.options = options; - let isDraggingRef = useRef(false); + let isDraggingRef = useRef(null); let [isDragging, setDraggingState] = useState(false); - let setDragging = (isDragging) => { - isDraggingRef.current = isDragging; - setDraggingState(isDragging); + let setDragging = (element: Element | null) => { + isDraggingRef.current = element; + setDraggingState(!!element); }; let {addGlobalListener, removeAllGlobalListeners} = useGlobalListeners(); let modalityOnPointerDown = useRef(null); @@ -186,8 +186,9 @@ export function useDrag(options: DragOptions): DragResult { // Wait a frame before we set dragging to true so that the browser has time to // render the preview image before we update the element that has been dragged. + let target = e.target; requestAnimationFrame(() => { - setDragging(true); + setDragging(target as Element); }); }; @@ -231,7 +232,7 @@ export function useDrag(options: DragOptions): DragResult { options.onDragEnd(event); } - setDragging(false); + setDragging(null); removeAllGlobalListeners(); setGlobalAllowedDropOperations(DROP_OPERATION.none); setGlobalDropEffect(undefined); @@ -240,9 +241,12 @@ export function useDrag(options: DragOptions): DragResult { // If the dragged element is removed from the DOM via onDrop, onDragEnd won't fire: https://bugzilla.mozilla.org/show_bug.cgi?id=460801 // In this case, we need to manually call onDragEnd on cleanup - useLayoutEffect(() => { + useEffect(() => { return () => { - if (isDraggingRef.current) { + // Check that the dragged element has actually unmounted from the DOM and not a React Strict Mode false positive. + // https://github.com/facebook/react/issues/29585 + // React 16 ran effect cleanups before removing elements from the DOM but did not have this issue. + if (isDraggingRef.current && (!isDraggingRef.current.isConnected || parseInt(ReactVersion, 10) < 17)) { if (typeof state.options.onDragEnd === 'function') { let event: DragEndEvent = { type: 'dragend', @@ -253,7 +257,7 @@ export function useDrag(options: DragOptions): DragResult { state.options.onDragEnd(event); } - setDragging(false); + setDragging(null); setGlobalAllowedDropOperations(DROP_OPERATION.none); setGlobalDropEffect(undefined); } @@ -285,14 +289,14 @@ export function useDrag(options: DragOptions): DragResult { ? state.options.getAllowedDropOperations() : ['move', 'copy', 'link'], onDragEnd(e) { - setDragging(false); + setDragging(null); if (typeof state.options.onDragEnd === 'function') { state.options.onDragEnd(e); } } }, stringFormatter); - setDragging(true); + setDragging(target); }; let modality = useDragModality(); diff --git a/packages/@react-aria/dnd/test/useDroppableCollection.test.js b/packages/@react-aria/dnd/test/useDroppableCollection.test.js index 137e311a534..672c415bced 100644 --- a/packages/@react-aria/dnd/test/useDroppableCollection.test.js +++ b/packages/@react-aria/dnd/test/useDroppableCollection.test.js @@ -228,36 +228,38 @@ describe('useDroppableCollection', () => { let dataTransfer = new DataTransfer(); fireEvent(draggable, new DragEvent('dragstart', {dataTransfer, clientX: 0, clientY: 0})); - act(() => jest.advanceTimersToNextTimer()); + act(() => jest.advanceTimersByTime(50)); expect(draggable).toHaveAttribute('data-dragging', 'true'); fireEvent(cells[0], new DragEvent('dragenter', {dataTransfer, clientX: 30, clientY: 30})); - act(() => jest.advanceTimersToNextTimer()); + act(() => jest.advanceTimersByTime(50)); expect(scrollTop).not.toHaveBeenCalled(); fireEvent(cells[2], new DragEvent('dragover', {dataTransfer, clientX: 30, clientY: 100})); - act(() => jest.advanceTimersToNextTimer()); + act(() => jest.advanceTimersByTime(50)); expect(scrollTop).not.toHaveBeenCalled(); fireEvent(cells[4], new DragEvent('dragover', {dataTransfer, clientX: 30, clientY: 135})); - act(() => jest.advanceTimersToNextTimer()); - expect(scrollTop).toHaveBeenCalledTimes(1); - act(() => jest.advanceTimersToNextTimer()); - expect(scrollTop).toHaveBeenCalledTimes(2); + act(() => jest.advanceTimersByTime(50)); + expect(scrollTop).toHaveBeenCalled(); + scrollTop.mockReset(); + act(() => jest.advanceTimersByTime(50)); + expect(scrollTop).toHaveBeenCalled(); jest.clearAllTimers(); fireEvent(cells[2], new DragEvent('dragover', {dataTransfer, clientX: 30, clientY: 100})); - act(() => jest.advanceTimersToNextTimer()); - expect(scrollTop).toHaveBeenCalledTimes(2); + act(() => jest.advanceTimersByTime(50)); + scrollTop.mockReset(); fireEvent(cells[2], new DragEvent('dragover', {dataTransfer, clientX: 30, clientY: 15})); - act(() => jest.advanceTimersToNextTimer()); - expect(scrollTop).toHaveBeenCalledTimes(3); + act(() => jest.advanceTimersByTime(50)); + expect(scrollTop).toHaveBeenCalled(); + scrollTop.mockReset(); jest.clearAllTimers(); fireEvent(cells[2], new DragEvent('dragover', {dataTransfer, clientX: 30, clientY: 30})); - act(() => jest.advanceTimersToNextTimer()); - expect(scrollTop).toHaveBeenCalledTimes(3); + act(() => jest.advanceTimersByTime(50)); + expect(scrollTop).not.toHaveBeenCalled(); }); it('supports dropping on an item', async () => { diff --git a/packages/@react-spectrum/list/test/ListViewDnd.test.js b/packages/@react-spectrum/list/test/ListViewDnd.test.js index 9d3e8bdfddc..7fa741629b4 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(onDrop).toHaveBeenCalledTimes(1); act(() => jest.runAllTimers());