refactor(tree): clarify move logic

packages/dds/tree/src/feature-libraries/default-schema/defaultEditBuilder.ts

@@ -316,6 +316,7 @@ export class DefaultEditBuilder implements ChangeFamilyEditor, IDefaultEditBuild
 		const detachCellId = this.modularBuilder.generateId(count);
 		const attachCellId: CellId = { localId: this.modularBuilder.generateId(count), revision };
 		if (compareFieldUpPaths(sourceField, destinationField)) {
+			// The source and destination fields are the same.
 			const change = sequence.changeHandler.editor.move(
 				sourceIndex,
 				count,
@@ -331,31 +332,35 @@ export class DefaultEditBuilder implements ChangeFamilyEditor, IDefaultEditBuild
 				revision,
 			);
 		} else {
+			// The source and destination fields are different.
 			const detachPath = topDownPath(sourceField.parent);
 			const attachPath = topDownPath(destinationField.parent);
+			/**
+			 * The number of elements, starting from the root, that both paths have in common.
+			 * This defines the lowest common ancestor node (LCA) of the source and destination fields.
+			 */
 			const sharedDepth = getSharedPrefixLength(detachPath, attachPath);
 			let adjustedAttachField = destinationField;
-			// After the above loop, `sharedDepth` is the number of elements, starting from the root,
-			// that both paths have in common.
 			if (sharedDepth === detachPath.length) {
-				const attachField = attachPath[sharedDepth]?.parentField ?? destinationField.field;
-				if (attachField === sourceField.field) {
-					// The detach occurs in an ancestor field of the field where the attach occurs.
-					let attachAncestorIndex = attachPath[sharedDepth]?.parentIndex ?? sourceIndex;
-					if (attachAncestorIndex < sourceIndex) {
+				const fieldOnDetachPathUnderLCA = sourceField.field;
+				const fieldOnAttachPathUnderLCA =
+					attachPath[sharedDepth]?.parentField ?? destinationField.field;
+				if (fieldOnDetachPathUnderLCA === fieldOnAttachPathUnderLCA) {
+					// We know that the attach path runs deeper because both paths would otherwise bottom-out in the same field,
+					// which is handled in a separate code path above.
+					const firstDifferentAttachAncestor = attachPath[sharedDepth] ?? oob();
+					if (firstDifferentAttachAncestor.parentIndex < sourceIndex) {
 						// The attach path runs through a node located before the detached nodes.
 						// No need to adjust the attach path.
-					} else if (sourceIndex + count <= attachAncestorIndex) {
+					} else if (sourceIndex + count <= firstDifferentAttachAncestor.parentIndex) {
 						// The attach path runs through a node located after the detached nodes.
-						// adjust the index for the node at that depth of the path, so that it is interpreted correctly
+						// Adjust the index for the node at that depth of the path, so that it is interpreted correctly
 						// in the composition performed by `submitChanges`.
-						attachAncestorIndex -= count;
-						let parent: UpPath | undefined = attachPath[sharedDepth - 1];
-						const parentField = attachPath[sharedDepth] ?? oob();
+						let parent = firstDifferentAttachAncestor.parent;
 						parent = {
 							parent,
-							parentIndex: attachAncestorIndex,
-							parentField: parentField.parentField,
+							parentIndex: firstDifferentAttachAncestor.parentIndex - count,
+							parentField: firstDifferentAttachAncestor.parentField,
 						};
 						for (let i = sharedDepth + 1; i < attachPath.length; i += 1) {
 							parent = {

packages/dds/tree/src/test/feature-libraries/default-field-kinds/defaultChangeFamily.spec.ts

@@ -824,6 +824,111 @@ describe("DefaultEditBuilder", () => {
 			assert.deepEqual(treeView, [expected]);
 		});
 
+		it("Can move nodes from before an ancestor of the destination field", () => {
+			const { builder, forest } = initializeEditableForest({
+				type: brand(JsonAsTree.JsonObject.identifier),
+				fields: {
+					foo: [
+						{ type: brand(numberSchema.identifier), value: 0 },
+						{ type: brand(numberSchema.identifier), value: 1 },
+						{ type: brand(numberSchema.identifier), value: 2 },
+						{ type: brand(numberSchema.identifier), value: 3 },
+						{
+							type: brand(JsonAsTree.JsonObject.identifier),
+							fields: {
+								foo: [{ type: brand(numberSchema.identifier), value: 0 }],
+							},
+						},
+					],
+				},
+			});
+			builder.move(
+				// Path to root.foo.
+				{ parent: root, field: fooKey },
+				1,
+				3,
+				// Path to root.foo[4].foo, which is after the moved nodes in their original location.
+				{
+					parent: {
+						parent: root,
+						parentField: fooKey,
+						parentIndex: 4,
+					},
+					field: fooKey,
+				},
+				1,
+			);
+			const treeView = toJsonableTreeFromForest(forest);
+			const expected: JsonableTree = {
+				type: brand(JsonAsTree.JsonObject.identifier),
+				fields: {
+					foo: [
+						{ type: brand(numberSchema.identifier), value: 0 },
+						{
+							type: brand(JsonAsTree.JsonObject.identifier),
+							fields: {
+								foo: [
+									{ type: brand(numberSchema.identifier), value: 0 },
+									{ type: brand(numberSchema.identifier), value: 1 },
+									{ type: brand(numberSchema.identifier), value: 2 },
+									{ type: brand(numberSchema.identifier), value: 3 },
+								],
+							},
+						},
+					],
+				},
+			};
+			assert.deepEqual(treeView, [expected]);
+		});
+
+		it("Can move nodes from after an ancestor of the destination field", () => {
+			const { builder, forest } = initializeEditableForest({
+				type: brand(JsonAsTree.JsonObject.identifier),
+				fields: {
+					foo: [
+						{
+							type: brand(JsonAsTree.JsonObject.identifier),
+							fields: {
+								foo: [{ type: brand(numberSchema.identifier), value: 0 }],
+							},
+						},
+						{ type: brand(numberSchema.identifier), value: 1 },
+						{ type: brand(numberSchema.identifier), value: 2 },
+						{ type: brand(numberSchema.identifier), value: 3 },
+					],
+				},
+			});
+			builder.move(
+				// Path to root.foo.
+				{ parent: root, field: fooKey },
+				1,
+				3,
+				// Path to root.foo[0].foo, which is before the moved nodes in their original location.
+				{ parent: root_foo0, field: fooKey },
+				1,
+			);
+			const treeView = toJsonableTreeFromForest(forest);
+			const expected: JsonableTree = {
+				type: brand(JsonAsTree.JsonObject.identifier),
+				fields: {
+					foo: [
+						{
+							type: brand(JsonAsTree.JsonObject.identifier),
+							fields: {
+								foo: [
+									{ type: brand(numberSchema.identifier), value: 0 },
+									{ type: brand(numberSchema.identifier), value: 1 },
+									{ type: brand(numberSchema.identifier), value: 2 },
+									{ type: brand(numberSchema.identifier), value: 3 },
+								],
+							},
+						},
+					],
+				},
+			};
+			assert.deepEqual(treeView, [expected]);
+		});
+
 		it("Errors when attempting to move a node under itself", () => {
 			const statingState: JsonableTree = {
 				type: brand(JsonAsTree.JsonObject.identifier),

packages/dds/tree/src/test/shared-tree/editing.spec.ts

@@ -1627,13 +1627,6 @@ describe("Editing", () => {
 			expectJsonTree(tree, expectedState);
 		});
 
-		it("can move a node out from a field and into a field under a sibling", () => {
-			const tree = makeTreeFromJsonSequence(["A", {}]);
-			tree.editor.move(rootField, 0, 1, { parent: rootNode2, field: brand("foo") }, 0);
-			const expectedState: JsonCompatible = [{ foo: "A" }];
-			expectJsonTree(tree, expectedState);
-		});
-
 		it("can rebase a move over the deletion of the source parent", () => {
 			const tree = makeTreeFromJson({ src: ["A", "B"], dst: ["C", "D"] });
 			const childBranch = tree.fork();