Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix collision detection issues #337

Merged
merged 7 commits into from
Jun 22, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions .changeset/collision-detection-issues.md
Original file line number Diff line number Diff line change
@@ -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)
3 changes: 2 additions & 1 deletion cypress.json
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
16 changes: 13 additions & 3 deletions cypress/support/commands.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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});
}
);

Expand Down
74 changes: 51 additions & 23 deletions packages/core/src/components/DndContext/DndContext.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import React, {
useRef,
useState,
} from 'react';
import {unstable_batchedUpdates} from 'react-dom';
import {
add,
Transform,
Expand Down Expand Up @@ -59,6 +60,7 @@ import {
getEventCoordinates,
rectIntersection,
} from '../../utilities';
import {getMeasurableNode} from '../../utilities/nodes';
import {applyModifiers, Modifiers} from '../../modifiers';
import type {
Active,
Expand Down Expand Up @@ -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},
Expand Down Expand Up @@ -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,
});
Expand Down Expand Up @@ -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,
Expand All @@ -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)
Expand Down Expand Up @@ -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({
Expand All @@ -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() {
Expand All @@ -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);
}
});
};
}
},
Expand Down Expand Up @@ -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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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();
Expand Down
14 changes: 14 additions & 0 deletions packages/core/src/utilities/nodes/getMeasurableNode.ts
Original file line number Diff line number Diff line change
@@ -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;
}
1 change: 1 addition & 0 deletions packages/core/src/utilities/nodes/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export {getMeasurableNode} from './getMeasurableNode';
2 changes: 1 addition & 1 deletion stories/2 - Presets/Sortable/1-Vertical.story.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ export const RerenderBeforeSorting = () => {
{...props}
wrapperStyle={({isDragging}) => {
return {
height: isDragging ? 80 : undefined,
height: isDragging ? undefined : 200,
};
}}
/>
Expand Down
2 changes: 0 additions & 2 deletions stories/2 - Presets/Sortable/3-Grid.story.tsx
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import React from 'react';
import {LayoutMeasuringStrategy} from '@dnd-kit/core';
import {restrictToWindowEdges} from '@dnd-kit/modifiers';
import {
AnimateLayoutChanges,
defaultAnimateLayoutChanges,
Expand All @@ -22,7 +21,6 @@ const props: Partial<SortableProps> = {
width: 140,
height: 140,
}),
modifiers: [restrictToWindowEdges],
};

export const BasicSetup = () => <Sortable {...props} />;
Expand Down
3 changes: 3 additions & 0 deletions stories/2 - Presets/Sortable/5-Virtualized.story.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,9 @@ function Sortable({
overIndex: -1,
isDragOverlay: true,
})}
wrapperStyle={{
padding: 5,
}}
dragOverlay
/>
) : null}
Expand Down