Fix race conditions with keyboard sensor (#745)

The keyboard sensor now keeps track of the initial coordinates of the collision rect to provide a translate delta when move events are dispatched.

Previously the keyboard sensor would measure the initial rect of the active node and store its top and left properties as its initial coordinates it would then compare all subsequent move coordinates to calculate the delta.

This approach was flawed on a number of different levels:

- It didn't respect the measuring configuration defined on the `<DndContext>` for the draggable node
- Some consumers re-render the active node after dragging begins, which would lead to stale measurements
- An error had to be thrown if there was no active node during the activation phase of the keyboard sensor. This shouldn't be a concern of the keyboard sensor.

.changeset/keyboard-sensor-raceconditions.md

@@ -0,0 +1,16 @@
+---
+'@dnd-kit/core': major
+---
+
+The keyboard sensor now keeps track of the initial coordinates of the collision rect to provide a translate delta when move events are dispatched.
+
+This is a breaking change that may affect consumers that had created custom keyboard coordinate getters.
+
+Previously the keyboard sensor would measure the initial rect of the active node and store its top and left properties as its initial coordinates it would then compare all subsequent move coordinates to calculate the delta.
+
+This approach suffered from the following issues:
+
+- It didn't respect the measuring configuration defined on the `<DndContext>` for the draggable node
+- Some consumers re-render the active node after dragging begins, which would lead to stale measurements
+- An error had to be thrown if there was no active node during the activation phase of the keyboard sensor. This shouldn't be a concern of the keyboard sensor.
+- The `currentCoordinates` passed to the coordinate getter were often stale and not an accurate representation of the current position of the collision rect, which can be affected by a number of different variables, such as modifiers.

packages/core/src/sensors/keyboard/KeyboardSensor.ts

@@ -9,13 +9,12 @@ import {
 import type {Coordinates} from '../../types';
 import {
   defaultCoordinates,
-  getTransformAgnosticClientRect,
   getScrollPosition,
   getScrollElementRect,
 } from '../../utilities';
 import {scrollIntoViewIfNeeded} from '../../utilities/scroll';
-import {Listeners} from '../utilities';
 import {EventName} from '../events';
+import {Listeners} from '../utilities';
 import type {SensorInstance, SensorProps, SensorOptions} from '../types';
 
 import {KeyboardCoordinateGetter, KeyboardCode, KeyboardCodes} from './types';
@@ -35,7 +34,7 @@ export type KeyboardSensorProps = SensorProps<KeyboardSensorOptions>;
 
 export class KeyboardSensor implements SensorInstance {
   public autoScrollEnabled = false;
-  private coordinates: Coordinates = defaultCoordinates;
+  private referenceCoordinates: Coordinates | undefined;
   private listeners: Listeners;
   private windowListeners: Listeners;
 
@@ -64,29 +63,17 @@ export class KeyboardSensor implements SensorInstance {
 
   private handleStart() {
     const {activeNode, onStart} = this.props;
+    const node = activeNode.node.current;
 
-    if (!activeNode.node.current) {
-      throw new Error('Active draggable node is undefined');
+    if (node) {
+      scrollIntoViewIfNeeded(node);
     }
 
-    scrollIntoViewIfNeeded(activeNode.node.current);
-
-    const activeNodeRect = getTransformAgnosticClientRect(
-      activeNode.node.current
-    );
-    const coordinates = {
-      x: activeNodeRect.left,
-      y: activeNodeRect.top,
-    };
-
-    this.coordinates = coordinates;
-
-    onStart(coordinates);
+    onStart(defaultCoordinates);
   }
 
   private handleKeyDown(event: Event) {
     if (isKeyboardEvent(event)) {
-      const {coordinates} = this;
       const {active, context, options} = this.props;
       const {
         keyboardCodes = defaultKeyboardCodes,
@@ -105,13 +92,26 @@ export class KeyboardSensor implements SensorInstance {
         return;
       }
 
+      const {collisionRect} = context.current;
+      const currentCoordinates = collisionRect
+        ? {x: collisionRect.left, y: collisionRect.top}
+        : defaultCoordinates;
+
+      if (!this.referenceCoordinates) {
+        this.referenceCoordinates = currentCoordinates;
+      }
+
       const newCoordinates = coordinateGetter(event, {
         active,
         context: context.current,
-        currentCoordinates: coordinates,
+        currentCoordinates,
       });
 
       if (newCoordinates) {
+        const coordinatesDelta = getCoordinatesDelta(
+          newCoordinates,
+          currentCoordinates
+        );
         const scrollDelta = {
           x: 0,
           y: 0,
@@ -120,10 +120,6 @@ export class KeyboardSensor implements SensorInstance {
 
         for (const scrollContainer of scrollableAncestors) {
           const direction = event.code;
-          const coordinatesDelta = getCoordinatesDelta(
-            newCoordinates,
-            coordinates
-          );
           const {
             isTop,
             isRight,
@@ -230,7 +226,10 @@ export class KeyboardSensor implements SensorInstance {
 
         this.handleMove(
           event,
-          getAdjustedCoordinates(newCoordinates, scrollDelta)
+          getAdjustedCoordinates(
+            getCoordinatesDelta(newCoordinates, this.referenceCoordinates),
+            scrollDelta
+          )
         );
       }
     }
@@ -241,7 +240,6 @@ export class KeyboardSensor implements SensorInstance {
 
     event.preventDefault();
     onMove(coordinates);
-    this.coordinates = coordinates;
   }
 
   private handleEnd(event: Event) {

stories/3 - Examples/Tree/SortableTree.tsx

@@ -105,7 +105,7 @@ interface Props {
 export function SortableTree({
   collapsible,
   defaultItems = initialItems,
-  indicator,
+  indicator = false,
   indentationWidth = 50,
   removable,
 }: Props) {
@@ -146,7 +146,7 @@ export function SortableTree({
     offset: offsetLeft,
   });
   const [coordinateGetter] = useState(() =>
-    sortableTreeKeyboardCoordinates(sensorContext, indentationWidth)
+    sortableTreeKeyboardCoordinates(sensorContext, indicator, indentationWidth)
   );
   const sensors = useSensors(
     useSensor(PointerSensor),

stories/3 - Examples/Tree/components/TreeItem/TreeItem.module.css

@@ -8,8 +8,8 @@
     display: inline-block;
     pointer-events: none;
     padding: 0;
-    margin-left: 10px;
-    margin-top: 5px;
+    padding-left: 10px;
+    padding-top: 5px;
 
     .TreeItem {
       --vertical-padding: 5px;

stories/3 - Examples/Tree/keyboardCoordinates.ts

@@ -1,6 +1,5 @@
 import {
   closestCorners,
-  getClientRect,
   getFirstCollision,
   KeyboardCode,
   KeyboardCoordinateGetter,
@@ -21,8 +20,9 @@ const horizontal: string[] = [KeyboardCode.Left, KeyboardCode.Right];
 
 export const sortableTreeKeyboardCoordinates: (
   context: SensorContext,
+  indicator: boolean,
   indentationWidth: number
-) => KeyboardCoordinateGetter = (context, indentationWidth) => (
+) => KeyboardCoordinateGetter = (context, indicator, indentationWidth) => (
   event,
   {
     currentCoordinates,
@@ -78,7 +78,7 @@ export const sortableTreeKeyboardCoordinates: (
         return;
       }
 
-      const rect = container?.rect.current;
+      const rect = droppableRects.get(container.id);
 
       if (!rect) {
         return;
@@ -105,16 +105,22 @@ export const sortableTreeKeyboardCoordinates: (
       droppableRects,
       droppableContainers: containers,
     });
-    const closestId = getFirstCollision(collisions, 'id');
+    let closestId = getFirstCollision(collisions, 'id');
+
+    if (closestId === over?.id && collisions.length > 1) {
+      closestId = collisions[1].id;
+    }
 
     if (closestId && over?.id) {
-      const newNode = droppableContainers.get(closestId)?.node.current;
-      const activeNodeRect = droppableContainers.get(active.id)?.rect.current;
+      const activeRect = droppableRects.get(active.id);
+      const newRect = droppableRects.get(closestId);
+      const newDroppable = droppableContainers.get(closestId);
 
-      if (newNode && activeNodeRect) {
-        const newRect = getClientRect(newNode, {ignoreTransform: true});
-        const newItem = items.find(({id}) => id === closestId);
-        const activeItem = items.find(({id}) => id === active.id);
+      if (activeRect && newRect && newDroppable) {
+        const newIndex = items.findIndex(({id}) => id === closestId);
+        const newItem = items[newIndex];
+        const activeIndex = items.findIndex(({id}) => id === active.id);
+        const activeItem = items[activeIndex];
 
         if (newItem && activeItem) {
           const {depth} = getProjection(
@@ -124,14 +130,15 @@ export const sortableTreeKeyboardCoordinates: (
             (newItem.depth - activeItem.depth) * indentationWidth,
             indentationWidth
           );
-          const offset =
-            newRect.top > activeNodeRect.top
-              ? Math.abs(activeNodeRect.height - newRect.height)
-              : 0;
+          const isBelow = newIndex > activeIndex;
+          const modifier = isBelow ? 1 : -1;
+          const offset = indicator
+            ? (collisionRect.height - activeRect.height) / 2
+            : 0;
 
           const newCoordinates = {
             x: newRect.left + depth * indentationWidth,
-            y: newRect.top + offset,
+            y: newRect.top + modifier * offset,
           };
 
           return newCoordinates;

stories/components/Draggable/DraggableOverlay.tsx

@@ -41,8 +41,6 @@ const dropAnimationConfig: DropAnimation = {
           easing: 'ease',
         }
       );
-
-      debugger;
     }
 
     return () => {