fix: React 19 strict mode drag and drop bug (#8562)

* fix: React 19 strict mode drag and drop bug

* fix test in React 16

* revert the change in react 16

Author: devongovett

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<Element | null>(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<string>(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();

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 () => {

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());