[fix] Moving group items inside of a frame (dropping) (#1886)

closes: #1884 

Fix bug: ungroup when moving a shape in a group in a frame.

### before 


https://github.com/tldraw/tldraw/assets/24749358/994a871a-5da2-4d8b-b80e-a4db43e66142


### after

https://github.com/tldraw/tldraw/assets/24749358/4395d416-b7f6-4af0-a4c2-0bf77c29d7e2



### Change Type

- [x] `patch` — Bug fix
- [ ] `minor` — New feature
- [ ] `major` — Breaking change
- [ ] `dependencies` — Changes to package dependencies[^1]
- [ ] `documentation` — Changes to the documentation only[^2]
- [ ] `tests` — Changes to any test code only[^2]
- [ ] `internal` — Any other changes that don't affect the published
package[^2]
- [ ] I don't know

[^1]: publishes a `patch` release, for devDependencies use `internal`
[^2]: will not publish a new version

### Test Plan

1. Add a step-by-step description of how to test your PR here.
2.

- [ ] Unit Tests
- [ ] End to end tests

### Release Notes

- Fix bug: ungroup when moving a shape in a group in a frame.

---------

Co-authored-by: Steve Ruiz <steveruizok@gmail.com>
Co-authored-by: David Sheldrick <d.j.sheldrick@gmail.com>

packages/editor/src/lib/editor/Editor.ts

@@ -4900,6 +4900,7 @@ export class Editor extends EventEmitter<TLEventMap> {
 			// Only allow dropping into the masked page bounds of the shape, e.g. when a frame is
 			// partially clipped by its own parent frame
 			const maskedPageBounds = this.getShapeMaskedPageBounds(shape.id)
+
 			if (
 				maskedPageBounds &&
 				maskedPageBounds.containsPoint(point) &&

packages/tldraw/src/lib/shapes/frame/FrameShapeUtil.tsx

@@ -205,16 +205,11 @@ export class FrameShapeUtil extends BaseBoxShapeUtil<TLFrameShape> {
 
 		// If frame is in a group, keep the shape
 		// moved out in that group
+
 		if (isInGroup) {
-			this.editor.reparentShapes(
-				shapes.map((shape) => shape.id),
-				parent.id
-			)
+			this.editor.reparentShapes(shapes, parent.id)
 		} else {
-			this.editor.reparentShapes(
-				shapes.map((shape) => shape.id),
-				this.editor.currentPageId
-			)
+			this.editor.reparentShapes(shapes, this.editor.currentPageId)
 		}
 	}
 

packages/tldraw/src/lib/tools/SelectTool/DragAndDropManager.ts

@@ -1,4 +1,4 @@
-import { Editor, TLShape, TLShapeId, compact } from '@tldraw/editor'
+import { Editor, TLShape, TLShapeId, Vec2d, compact } from '@tldraw/editor'
 
 const LAG_DURATION = 100
 
@@ -9,15 +9,20 @@ export class DragAndDropManager {
 	}
 
 	prevDroppingShapeId: TLShapeId | null = null
-	currDroppingShapeId: TLShapeId | null = null
 
 	droppingNodeTimer: ReturnType<typeof setTimeout> | null = null
 
+	first = true
+
 	updateDroppingNode(movingShapes: TLShape[], cb: () => void) {
+		if (this.first) {
+			this.prevDroppingShapeId =
+				this.editor.getDroppingOverShape(this.editor.inputs.originPagePoint, movingShapes)?.id ??
+				null
+			this.first = false
+		}
+
 		if (this.droppingNodeTimer === null) {
-			const { currentPagePoint } = this.editor.inputs
-			this.currDroppingShapeId =
-				this.editor.getDroppingOverShape(currentPagePoint, movingShapes)?.id ?? null
 			this.setDragTimer(movingShapes, LAG_DURATION * 10, cb)
 		} else if (this.editor.inputs.pointerVelocity.len() > 0.5) {
 			clearInterval(this.droppingNodeTimer)
@@ -28,39 +33,31 @@ export class DragAndDropManager {
 	private setDragTimer(movingShapes: TLShape[], duration: number, cb: () => void) {
 		this.droppingNodeTimer = setTimeout(() => {
 			this.editor.batch(() => {
-				this.handleDrag(movingShapes, cb)
+				this.handleDrag(this.editor.inputs.currentPagePoint, movingShapes, cb)
 			})
 			this.droppingNodeTimer = null
 		}, duration)
 	}
 
-	private handleDrag(movingShapes: TLShape[], cb?: () => void) {
-		const { currentPagePoint } = this.editor.inputs
-
+	private handleDrag(point: Vec2d, movingShapes: TLShape[], cb?: () => void) {
 		movingShapes = compact(movingShapes.map((shape) => this.editor.getShape(shape.id)))
 
-		const currDroppingShapeId =
-			this.editor.getDroppingOverShape(currentPagePoint, movingShapes)?.id ?? null
+		const nextDroppingShapeId = this.editor.getDroppingOverShape(point, movingShapes)?.id ?? null
 
-		if (currDroppingShapeId !== this.currDroppingShapeId) {
-			this.prevDroppingShapeId = this.currDroppingShapeId
-			this.currDroppingShapeId = currDroppingShapeId
-		}
-
-		const { prevDroppingShapeId } = this
-
-		if (currDroppingShapeId === prevDroppingShapeId) {
-			// we already called onDragShapesOver on this node, no need to do it again
+		// is the next dropping shape id different than the last one?
+		if (nextDroppingShapeId === this.prevDroppingShapeId) {
 			return
 		}
 
+		// the old previous one
+		const { prevDroppingShapeId } = this
+
 		const prevDroppingShape = prevDroppingShapeId && this.editor.getShape(prevDroppingShapeId)
-		const nextDroppingShape = currDroppingShapeId && this.editor.getShape(currDroppingShapeId)
+		const nextDroppingShape = nextDroppingShapeId && this.editor.getShape(nextDroppingShapeId)
 
 		// Even if we don't have a next dropping shape id (i.e. if we're dropping
 		// onto the page) set the prev to the current, to avoid repeat calls to
 		// the previous parent's onDragShapesOut
-		this.prevDroppingShapeId = this.currDroppingShapeId
 
 		if (prevDroppingShape) {
 			this.editor.getShapeUtil(prevDroppingShape).onDragShapesOut?.(prevDroppingShape, movingShapes)
@@ -80,15 +77,18 @@ export class DragAndDropManager {
 		}
 
 		cb?.()
+
+		// next -> curr
+		this.prevDroppingShapeId = nextDroppingShapeId
 	}
 
 	dropShapes(shapes: TLShape[]) {
-		const { currDroppingShapeId } = this
+		const { prevDroppingShapeId } = this
 
-		this.handleDrag(shapes)
+		this.handleDrag(this.editor.inputs.currentPagePoint, shapes)
 
-		if (currDroppingShapeId) {
-			const shape = this.editor.getShape(currDroppingShapeId)
+		if (prevDroppingShapeId) {
+			const shape = this.editor.getShape(prevDroppingShapeId)
 			if (!shape) return
 			this.editor.getShapeUtil(shape).onDropShapesOver?.(shape, shapes)
 		}
@@ -96,7 +96,6 @@ export class DragAndDropManager {
 
 	clear() {
 		this.prevDroppingShapeId = null
-		this.currDroppingShapeId = null
 
 		if (this.droppingNodeTimer !== null) {
 			clearInterval(this.droppingNodeTimer)
@@ -104,6 +103,7 @@ export class DragAndDropManager {
 
 		this.droppingNodeTimer = null
 		this.editor.setHintingShapes([])
+		this.first = true
 	}
 
 	dispose = () => {

packages/tldraw/src/lib/tools/SelectTool/children/Translating.ts

@@ -202,6 +202,7 @@ export class Translating extends StateNode {
 		if (changes.length > 0) {
 			this.editor.updateShapes(changes)
 		}
+
 		this.editor.setHoveredShape(null)
 	}
 

packages/tldraw/src/test/frames.test.ts

@@ -248,9 +248,9 @@ describe('frame shapes', () => {
 		expect(editor.onlySelectedShape!.parentId).toBe(editor.currentPageId)
 
 		editor.setCurrentTool('select')
-		editor.pointerDown(275, 275, ids.boxA).pointerMove(150, 150)
+		editor.pointerDown(275, 275).pointerMove(150, 150)
 
-		jest.advanceTimersByTime(250)
+		jest.advanceTimersByTime(300)
 
 		expect(editor.onlySelectedShape!.id).toBe(ids.boxA)
 		expect(editor.onlySelectedShape!.parentId).toBe(frameId)
@@ -712,3 +712,66 @@ test('arrows bound to a shape within a group within a frame are reparented if th
 	// expect arrow index to be greater than group index
 	expect(editor.getShape(arrowId)?.index.localeCompare(editor.getShape(groupId)!.index)).toBe(1)
 })
+
+describe('When dragging a shape inside a group inside a frame', () => {
+	const ids = {
+		frame1: createShapeId('frame'),
+		box1: createShapeId('geo1'),
+		box2: createShapeId('geo2'),
+		group1: createShapeId('group1'),
+	}
+
+	beforeEach(() => {
+		editor.createShapes([
+			{ id: ids.frame1, type: 'frame', x: 0, y: 0, props: { w: 500, h: 500 } },
+			{ id: ids.box1, type: 'geo', parentId: ids.frame1, x: 100, y: 100 },
+			{ id: ids.box2, type: 'geo', parentId: ids.frame1, x: 300, y: 300 },
+		])
+	})
+
+	it('When dragging a shape out of a frame', () => {
+		editor.select(ids.box1, ids.box2)
+
+		expect(editor.selectedShapeIds).toHaveLength(2)
+
+		editor.groupShapes(editor.selectedShapeIds, ids.group1)
+
+		expect(editor.getShape(ids.box1)!.parentId).toBe(ids.group1)
+
+		editor.pointerMove(100, 100).click().click()
+
+		expect(editor.onlySelectedShape?.id).toBe(ids.box1)
+
+		editor.pointerMove(150, 150).pointerDown().pointerMove(140, 140)
+
+		jest.advanceTimersByTime(300)
+
+		expect(editor.getShape(ids.box1)!.parentId).toBe(ids.group1)
+	})
+
+	it('reparents the shape to the page if it leaves the frame', () => {
+		editor.select(ids.box1, ids.box2)
+
+		expect(editor.selectedShapeIds).toHaveLength(2)
+
+		editor.groupShapes(editor.selectedShapeIds, ids.group1)
+
+		expect(editor.getShape(ids.box1)!.parentId).toBe(ids.group1)
+
+		editor.pointerMove(100, 100).click().click()
+
+		expect(editor.onlySelectedShape?.id).toBe(ids.box1)
+		expect(editor.focusedGroupId).toBe(ids.group1)
+
+		editor
+			.pointerMove(150, 150)
+			.pointerDown()
+			.pointerMove(-200, -200)
+			.pointerMove(-200, -200)
+			.pointerUp()
+
+		jest.advanceTimersByTime(300)
+
+		expect(editor.getShape(ids.box1)!.parentId).toBe(editor.currentPageId)
+	})
+})