fix: improve drag behavior outside of tree boundaries or legal drop positions (#417)

Author: lukasbach

next-release-notes.md

@@ -1,9 +1,3 @@
-<!--
-### Breaking Changes
-
-### Features
-
 ### Bug Fixes and Improvements
-
-### Other Changes
--->
+- Don't show drag line when dragging outside of the tree container (#417)
+- Fix a bug where items where dropped on the last valid position when dragging items on an invalid position and then dropping (#417)

packages/core/src/controlledEnvironment/layoutUtils.ts

@@ -16,7 +16,7 @@ export const computeItemHeight = (treeId: string) => {
 };
 
 export const isOutsideOfContainer = (e: DragEvent, treeBb: DOMRect) =>
-  e.clientX < treeBb.left ||
-  e.clientX > treeBb.right ||
-  e.clientY < treeBb.top ||
-  e.clientY > treeBb.bottom;
+  e.clientX <= treeBb.left ||
+  e.clientX >= treeBb.right ||
+  e.clientY <= treeBb.top ||
+  e.clientY >= treeBb.bottom;

packages/core/src/drag/DragAndDropProvider.tsx

@@ -15,6 +15,7 @@ import { useCallSoon } from '../useCallSoon';
 import { useStableHandler } from '../useStableHandler';
 import { useGetOriginalItemOrder } from '../useGetOriginalItemOrder';
 import { useDraggingPosition } from './useDraggingPosition';
+import { isOutsideOfContainer } from '../controlledEnvironment/layoutUtils';
 
 const DragAndDropContext = React.createContext<DragAndDropContextProps>(
   null as any
@@ -177,6 +178,20 @@ export const DragAndDropProvider: React.FC<React.PropsWithChildren> = ({
     }
   );
 
+  const onDragLeaveContainerHandler = useStableHandler(
+    (
+      e: DragEvent,
+      containerRef: React.MutableRefObject<HTMLElement | undefined>
+    ) => {
+      if (!containerRef.current) return;
+      if (
+        isOutsideOfContainer(e, containerRef.current.getBoundingClientRect())
+      ) {
+        setDraggingPosition(undefined);
+      }
+    }
+  );
+
   const onDropHandler = useStableHandler(() => {
     if (!draggingItems || !draggingPosition || !environment.onDrop) {
       return;
@@ -287,6 +302,7 @@ export const DragAndDropProvider: React.FC<React.PropsWithChildren> = ({
       itemHeight: itemHeight.current,
       isProgrammaticallyDragging,
       onDragOverTreeHandler,
+      onDragLeaveContainerHandler,
       viableDragPositions,
     }),
     [
@@ -297,6 +313,7 @@ export const DragAndDropProvider: React.FC<React.PropsWithChildren> = ({
       isProgrammaticallyDragging,
       itemHeight,
       onDragOverTreeHandler,
+      onDragLeaveContainerHandler,
       onStartDraggingItems,
       programmaticDragDown,
       programmaticDragUp,

packages/core/src/drag/DraggingPositionEvaluation.ts

@@ -227,7 +227,7 @@ export class DraggingPositionEvaluation {
     return true;
   }
 
-  getDraggingPosition(): DraggingPosition | undefined {
+  getDraggingPosition(): DraggingPosition | 'invalid' | undefined {
     if (this.env.linearItems[this.treeId].length === 0) {
       return this.getEmptyTreeDragPosition();
     }
@@ -251,11 +251,11 @@ export class DraggingPositionEvaluation {
     }
 
     if (this.areDraggingItemsDescendantOfTarget()) {
-      return undefined;
+      return 'invalid';
     }
 
     if (!this.canDropAtCurrentTarget()) {
-      return undefined;
+      return 'invalid';
     }
 
     const { parent } = this.getParentOfLinearItem(

packages/core/src/tree/TreeManager.tsx

@@ -48,6 +48,9 @@ export const TreeManager = (): JSX.Element => {
       e.preventDefault(); // Allow drop. Also implicitly set by items, but needed here as well for dropping on empty space
       dnd.onDragOverTreeHandler(e as any, treeId, containerRef);
     },
+    onDragLeave: e => {
+      dnd.onDragLeaveContainerHandler(e as any, containerRef);
+    },
     onMouseDown: () => dnd.abortProgrammaticDrag(),
     ref: containerRef,
     style: { position: 'relative' },

packages/core/src/types.ts

@@ -328,6 +328,10 @@ export interface DragAndDropContextProps<T = any> {
     treeId: string,
     containerRef: React.MutableRefObject<HTMLElement | undefined>
   ) => void;
+  onDragLeaveContainerHandler: (
+    e: DragEvent,
+    containerRef: React.MutableRefObject<HTMLElement | undefined>
+  ) => void;
 }
 
 export type DraggingPosition =

packages/core/test/dnd-basics.spec.tsx

@@ -654,4 +654,35 @@ describe('dnd basics', () => {
       });
     });
   });
+
+  it('doesnt drop on last valid location when moving drop to invalid location', async () => {
+    const test = await new TestUtil().renderOpenTree({});
+    await test.startDrag('aab');
+    await test.dragOver('aa');
+    await test.dragOver('aab');
+    await test.drop();
+    await test.expectItemContentsUnchanged('aa');
+  });
+
+  it('sets drop position correctly', async () => {
+    const test = await new TestUtil().renderOpenTree({});
+    await test.startDrag('target');
+    await test.dragOver('aa');
+    expect(test.treeRef?.dragAndDropContext?.draggingPosition).toStrictEqual({
+      depth: 1,
+      linearIndex: 5,
+      parentItem: 'a',
+      targetItem: 'aa',
+      targetType: 'item',
+      treeId: 'tree-1',
+    });
+  });
+
+  it('unsets drop position when dragging out', async () => {
+    const test = await new TestUtil().renderOpenTree({});
+    await test.startDrag('target');
+    await test.dragOver('aa');
+    await test.dragLeave();
+    expect(test.treeRef?.dragAndDropContext?.draggingPosition).toBe(undefined);
+  });
 });

packages/core/test/helpers/TestUtil.tsx

@@ -171,6 +171,20 @@ export class TestUtil {
     });
   }
 
+  public async dragLeave() {
+    (isOutsideOfContainer as jest.Mock).mockReturnValue(true);
+    await act(async () => {
+      this.environmentRef?.dragAndDropContext.onDragLeaveContainerHandler(
+        {
+          clientX: 9999,
+          clientY: 9999,
+        } as any,
+        { current: this.containerRef ?? undefined }
+      );
+    });
+    (isOutsideOfContainer as jest.Mock).mockReturnValue(false);
+  }
+
   public async drop() {
     await act(async () => {
       fireEvent.drop(window);