Fix collision detection issues (#337)

- Batch React updates in non-synthetic event handlers - Defer measurement of droppable node rects until second render after dragging - Use DragOverlay's width and height in collision rect (if it is used)
clauderic · Jun 22, 2021 · 05d6a78 · 05d6a78
1 parent 6fdef7f
commit 05d6a78
Show file tree

Hide file tree

Showing 10 changed files with 96 additions and 31 deletions.
diff --git a/.changeset/collision-detection-issues.md b/.changeset/collision-detection-issues.md
@@ -0,0 +1,9 @@
+---
+"@dnd-kit/core": major
+---
+
+React updates in non-synthetic event handlers are now batched to reduce re-renders and prepare for React 18.
+
+Also fixed issues with collision detection:
+- Defer measurement of droppable node rects until second render after dragging.
+- Use DragOverlay's width and height in collision rect (if it is used)
diff --git a/cypress.json b/cypress.json
@@ -2,5 +2,6 @@
   "$schema": "https://on.cypress.io/cypress.schema.json",
   "supportFile": "cypress/support/index.ts",
   "projectId": "zkn9qu",
-  "baseUrl": "http://localhost:6006/"
+  "baseUrl": "http://localhost:6006/",
+  "viewportHeight": 1000
 }
diff --git a/cypress/support/commands.ts b/cypress/support/commands.ts
@@ -135,10 +135,20 @@ Cypress.Commands.add(
 
     cy.wrap(subject, {log: false})
       .focus({log: false})
-      .type(Keys.Space, {force: true, log: false})
+      .type(Keys.Space, {
+        delay: 150,
+        scrollBehavior: false,
+        force: true,
+        log: false,
+      })
       .closest('body')
-      .type(arrowKey.repeat(times), {delay: 150, force: true})
-      .type(Keys.Space, {log: false, force: true});
+      .type(arrowKey.repeat(times), {
+        scrollBehavior: false,
+        delay: 150,
+        force: true,
+      })
+      .wait(150)
+      .type(Keys.Space, {scrollBehavior: false, log: false, force: true});
   }
 );
 

diff --git a/packages/core/src/components/DndContext/DndContext.tsx b/packages/core/src/components/DndContext/DndContext.tsx
@@ -8,6 +8,7 @@ import React, {
   useRef,
   useState,
 } from 'react';
+import {unstable_batchedUpdates} from 'react-dom';
 import {
   add,
   Transform,
@@ -59,6 +60,7 @@ import {
   getEventCoordinates,
   rectIntersection,
 } from '../../utilities';
+import {getMeasurableNode} from '../../utilities/nodes';
 import {applyModifiers, Modifiers} from '../../modifiers';
 import type {
   Active,
@@ -142,6 +144,7 @@ export const DndContext = memo(function DndContext({
     type: null,
     event: null,
   }));
+  const [isDragging, setIsDragging] = useState(false);
   const {
     draggable: {active: activeId, nodes: draggableNodes, translate},
     droppable: {containers: droppableContainers},
@@ -173,7 +176,7 @@ export const DndContext = memo(function DndContext({
     recomputeLayouts,
     willRecomputeLayouts,
   } = useLayoutMeasuring(droppableContainers, {
-    dragging: activeId != null,
+    dragging: isDragging,
     dependencies: [translate.x, translate.y],
     config: layoutMeasuring,
   });
@@ -216,11 +219,11 @@ export const DndContext = memo(function DndContext({
 
   const [overlayNodeRef, setOverlayNodeRef] = useNodeRef();
   const overlayNodeRect = useClientRect(
-    activeId ? overlayNodeRef.current : null,
+    activeId ? getMeasurableNode(overlayNodeRef.current) : null,
     willRecomputeLayouts
   );
 
-  const draggingNodeRect = overlayNodeRect ?? activeNodeClientRect;
+  const draggingNodeRect = overlayNodeRect ?? activeNodeRect;
   const modifiedTranslate = applyModifiers(modifiers, {
     transform: {
       x: translate.x - nodeRectDelta.x,
@@ -247,9 +250,20 @@ export const DndContext = memo(function DndContext({
 
   const scrollAdjustedTranslate = add(modifiedTranslate, scrollAdjustment);
 
-  const translatedRect = activeNodeRect
-    ? getAdjustedRect(activeNodeRect, modifiedTranslate)
-    : null;
+  // The overlay node's position is based on the active node's position.
+  // We assume that any difference in positioning is for visual purposes only
+  // and shouldn't affect collision detection. However, the computed height of
+  // the overlay node should affect the collision rect.
+  const rect =
+    overlayNodeRect && activeNodeRect
+      ? {
+          ...activeNodeRect,
+          height: overlayNodeRect.height,
+          width: overlayNodeRect.width,
+        }
+      : activeNodeRect;
+
+  const translatedRect = rect ? getAdjustedRect(rect, modifiedTranslate) : null;
 
   const collisionRect = translatedRect
     ? getAdjustedRect(translatedRect, scrollAdjustment)
@@ -320,13 +334,15 @@ export const DndContext = memo(function DndContext({
             active: {id, data: node.data, rect: activeRects},
           };
 
-          dispatch({
-            type: Action.DragStart,
-            initialCoordinates,
-            active: id,
+          unstable_batchedUpdates(() => {
+            dispatch({
+              type: Action.DragStart,
+              initialCoordinates,
+              active: id,
+            });
+            setMonitorState({type: Action.DragStart, event});
+            onDragStart?.(event);
           });
-          setMonitorState({type: Action.DragStart, event});
-          onDragStart?.(event);
         },
         onMove(coordinates) {
           dispatch({
@@ -338,8 +354,10 @@ export const DndContext = memo(function DndContext({
         onCancel: createHandler(Action.DragCancel),
       });
 
-      setActiveSensor(sensorInstance);
-      setActivatorEvent(event.nativeEvent);
+      unstable_batchedUpdates(() => {
+        setActiveSensor(sensorInstance);
+        setActivatorEvent(event.nativeEvent);
+      });
 
       function createHandler(type: Action.DragEnd | Action.DragCancel) {
         return async function handler() {
@@ -366,17 +384,21 @@ export const DndContext = memo(function DndContext({
 
           activeRef.current = null;
 
-          dispatch({type});
-          setActiveSensor(null);
-          setActivatorEvent(null);
+          unstable_batchedUpdates(() => {
+            dispatch({type});
+            setIsDragging(false);
+            setActiveSensor(null);
+            setActivatorEvent(null);
 
-          if (event) {
-            const {onDragCancel, onDragEnd} = latestProps.current;
-            const handler = type === Action.DragEnd ? onDragEnd : onDragCancel;
+            if (event) {
+              const {onDragCancel, onDragEnd} = latestProps.current;
+              const handler =
+                type === Action.DragEnd ? onDragEnd : onDragCancel;
 
-            setMonitorState({type, event});
-            handler?.(event);
-          }
+              setMonitorState({type, event});
+              handler?.(event);
+            }
+          });
         };
       }
     },
@@ -427,6 +449,12 @@ export const DndContext = memo(function DndContext({
     Object.values(props)
   );
 
+  useEffect(() => {
+    if (activeId != null) {
+      setIsDragging(true);
+    }
+  }, [activeId]);
+
   useEffect(() => {
     if (!active) {
       initialActiveNodeRectRef.current = null;

diff --git a/packages/core/src/components/DragOverlay/hooks/useDropAnimation.ts b/packages/core/src/components/DragOverlay/hooks/useDropAnimation.ts
@@ -4,6 +4,7 @@ import {CSS, Transform, useIsomorphicLayoutEffect} from '@dnd-kit/utilities';
 import type {UniqueIdentifier} from '../../../types';
 import type {DraggableNodes} from '../../../store';
 import {getViewRect} from '../../../utilities';
+import {getMeasurableNode} from '../../../utilities/nodes';
 
 export interface DropAnimation {
   duration: number;
@@ -49,7 +50,7 @@ export function useDropAnimation({
       const finalNode = draggableNodes[activeId]?.node.current;
 
       if (transform && node && finalNode && finalNode.parentNode !== null) {
-        const fromNode = node.children.length > 1 ? node : node.children[0];
+        const fromNode = getMeasurableNode(node);
 
         if (fromNode) {
           const from = fromNode.getBoundingClientRect();

diff --git a/packages/core/src/utilities/nodes/getMeasurableNode.ts b/packages/core/src/utilities/nodes/getMeasurableNode.ts
@@ -0,0 +1,14 @@
+export function getMeasurableNode(
+  node: HTMLElement | undefined | null
+): HTMLElement | null {
+  if (!node) {
+    return null;
+  }
+
+  if (node.children.length > 1) {
+    return node;
+  }
+  const firstChild = node.children[0];
+
+  return firstChild instanceof HTMLElement ? firstChild : node;
+}
diff --git a/packages/core/src/utilities/nodes/index.ts b/packages/core/src/utilities/nodes/index.ts
@@ -0,0 +1 @@
+export {getMeasurableNode} from './getMeasurableNode';
diff --git a/stories/2 - Presets/Sortable/1-Vertical.story.tsx b/stories/2 - Presets/Sortable/1-Vertical.story.tsx
@@ -140,7 +140,7 @@ export const RerenderBeforeSorting = () => {
       {...props}
       wrapperStyle={({isDragging}) => {
         return {
-          height: isDragging ? 80 : undefined,
+          height: isDragging ? undefined : 200,
         };
       }}
     />

diff --git a/stories/2 - Presets/Sortable/3-Grid.story.tsx b/stories/2 - Presets/Sortable/3-Grid.story.tsx
@@ -1,6 +1,5 @@
 import React from 'react';
 import {LayoutMeasuringStrategy} from '@dnd-kit/core';
-import {restrictToWindowEdges} from '@dnd-kit/modifiers';
 import {
   AnimateLayoutChanges,
   defaultAnimateLayoutChanges,
@@ -22,7 +21,6 @@ const props: Partial<SortableProps> = {
     width: 140,
     height: 140,
   }),
-  modifiers: [restrictToWindowEdges],
 };
 
 export const BasicSetup = () => <Sortable {...props} />;

diff --git a/stories/2 - Presets/Sortable/5-Virtualized.story.tsx b/stories/2 - Presets/Sortable/5-Virtualized.story.tsx
@@ -112,6 +112,9 @@ function Sortable({
                 overIndex: -1,
                 isDragOverlay: true,
               })}
+              wrapperStyle={{
+                padding: 5,
+              }}
               dragOverlay
             />
           ) : null}