Fix move_node dirtyPath and PathUtils.transform calculations (#2593)

* test and changes to test runner for move-node edge case

* fix dirtyPaths calculation for move_node

* prettier

* remove commented out code, remove needless toArray call

* remove test assertion

* more lint fixes

* possibly correct test for deleting all nested blocks

* pretty test

* fix PathUtils.transform edge case with move_node

* tests for PathUtils.transform for some move_node cases

* prettier

* element moveNode also uses PathUtils.transform, fix sibling edge case

* refactor for ease of comprehension

* new edge case test, fix move_node invert

* prettier

* revert last move_node transform case for clarity

packages/slate/src/controllers/editor.js

@@ -524,29 +524,21 @@ function getDirtyPaths(operation) {
     }
 
     case 'move_node': {
-      let parentPath = PathUtils.lift(path)
-      let newParentPath = PathUtils.lift(newPath)
-
       if (PathUtils.isEqual(path, newPath)) {
         return []
       }
 
-      // HACK: this clause only exists because the `move_path` logic isn't
-      // consistent when it deals with siblings.
-      if (!PathUtils.isSibling(path, newPath)) {
-        if (newParentPath.size && PathUtils.isYounger(path, newPath)) {
-          newParentPath = PathUtils.decrement(newParentPath, 1, path.size - 1)
-        }
+      const oldAncestors = PathUtils.getAncestors(path).reduce((arr, p) => {
+        arr.push(...PathUtils.transform(p, operation).toArray())
+        return arr
+      }, [])
 
-        if (parentPath.size && PathUtils.isYounger(newPath, path)) {
-          parentPath = PathUtils.increment(parentPath, 1, newPath.size - 1)
-        }
-      }
+      const newAncestors = PathUtils.getAncestors(newPath).reduce((arr, p) => {
+        arr.push(...PathUtils.transform(p, operation).toArray())
+        return arr
+      }, [])
 
-      const oldAncestors = PathUtils.getAncestors(parentPath).toArray()
-      const newAncestors = PathUtils.getAncestors(newParentPath).toArray()
-
-      return [...oldAncestors, parentPath, ...newAncestors, newParentPath]
+      return [...oldAncestors, ...newAncestors]
     }
 
     case 'remove_node': {

packages/slate/src/interfaces/element.js

@@ -14,6 +14,7 @@ import Point from '../models/point'
 import Range from '../models/range'
 import Selection from '../models/selection'
 import Value from '../models/value'
+import Operation from '../models/operation'
 
 /**
  * The interface that `Document`, `Block` and `Inline` all implement, to make
@@ -1763,13 +1764,15 @@ class ElementInterface {
     const newParentPath = PathUtils.lift(newPath)
     this.assertNode(newParentPath)
 
-    const position = PathUtils.compare(path, newPath)
-
-    // If the old path ends above and before a node in the new path, then
-    // removing it will alter the target, so we need to adjust the new path.
-    if (path.size < newPath.size && position === -1) {
-      newPath = PathUtils.decrement(newPath, 1, path.size - 1)
-    }
+    // TODO: this is a bit hacky, re-creating the operation that led to this method being called
+    // Alternative 1: pass the operation through from apply -> value.moveNode
+    // Alternative 2: add a third property to the operation called "transformedNewPath", pass that through
+    const op = Operation.create({
+      type: 'move_node',
+      path,
+      newPath,
+    })
+    newPath = PathUtils.transform(path, op).first()
 
     let ret = this
     ret = ret.removeNode(path)

packages/slate/src/operations/invert.js

@@ -42,23 +42,21 @@ function invertOperation(op) {
         return op
       }
 
-      let inversePath = newPath
-      let inverseNewPath = path
+      // Get the true path that the moved node ended up at
+      const inversePath = PathUtils.transform(path, op).first()
 
-      const position = PathUtils.compare(path, newPath)
+      // Get the true path we are trying to move back to
+      // We transform the right-sibling of the path
+      // This will end up at the operation.path most of the time
+      // But if the newPath is a left-sibling or left-ancestor-sibling, this will account for it
+      const transformedSibling = PathUtils.transform(
+        PathUtils.increment(path),
+        op
+      ).first()
 
-      // If the node's old position was a left sibling of an ancestor of
-      // its new position, we need to adjust part of the path by -1.
-      // If the node's new position is an ancestor of the old position,
-      // or a left sibling of an ancestor of its old position, we need
-      // to adjust part of the path by 1.
-      if (path.size < newPath.size && position === -1) {
-        inversePath = PathUtils.decrement(newPath, 1, path.size - 1)
-      } else if (path.size > newPath.size && position !== -1) {
-        inverseNewPath = PathUtils.increment(path, 1, newPath.size - 1)
-      }
-
-      const inverse = op.set('path', inversePath).set('newPath', inverseNewPath)
+      const inverse = op
+        .set('path', inversePath)
+        .set('newPath', transformedSibling)
       return inverse
     }
 

packages/slate/src/utils/path-utils.js

@@ -84,11 +84,11 @@ function decrement(path, n = 1, index = path.size - 1) {
  */
 
 function getAncestors(path) {
-  let ancestors = new List()
-
-  for (let i = 0; i < path.size; i++) {
-    ancestors = ancestors.push(path.slice(0, i))
-  }
+  const ancestors = List().withMutations(list => {
+    for (let i = 0; i < path.size; i++) {
+      list.push(path.slice(0, i))
+    }
+  })
 
   return ancestors
 }
@@ -340,35 +340,28 @@ function transform(path, operation) {
 
   if (type === 'move_node') {
     const { newPath: np } = operation
-    const npIndex = np.size - 1
-    const npEqual = isEqual(np, path)
 
     if (isEqual(p, np)) {
       return List([path])
     }
 
-    const npYounger = isYounger(np, path)
-    const npAbove = isAbove(np, path)
-
-    if (pAbove) {
-      if (isAfter(np, p)) {
+    if (pAbove || pEqual) {
+      // We are comparing something that was moved
+      // The new path is unaffected unless the old path was the left-sibling of an ancestor
+      if (isYounger(p, np) && p.size < np.size) {
         path = decrement(np, 1, min(np, p) - 1).concat(path.slice(p.size))
       } else {
         path = np.concat(path.slice(p.size))
       }
-    } else if (pEqual) {
-      if (isAfter(np, p)) {
-        path = decrement(np, 1, min(np, p) - 1)
-      } else {
-        path = np
-      }
     } else {
+      // This is equivalent logic to remove_node for path
       if (pYounger) {
         path = decrement(path, 1, pIndex)
       }
 
-      if (npEqual || npYounger || npAbove) {
-        path = increment(path, 1, npIndex)
+      // This is the equivalent logic to insert_node for newPath
+      if (isYounger(np, path) || isEqual(np, path) || isAbove(np, path)) {
+        path = increment(path, 1, np.size - 1)
       }
     }
   }

packages/slate/test/commands/at-current-range/delete-backward/with-plugin-normalization.js

@@ -0,0 +1,62 @@
+/** @jsx h */
+
+import h from '../../../helpers/h'
+import { Block, Text } from 'slate'
+
+function normalizeNode(node, editor, next) {
+  if (node.type === 'container' && node.nodes.first().type === 'container') {
+    return () =>
+      editor.insertNodeByKey(
+        node.key,
+        0,
+        Block.create({
+          type: 'paragraph',
+          nodes: [Text.create()],
+        })
+      )
+  }
+
+  return next()
+}
+
+export const plugins = [{ normalizeNode }]
+
+export default function(editor) {
+  editor.deleteBackward()
+}
+
+export const input = (
+  <value>
+    <document>
+      <block type="container" key="c1">
+        <paragraph key="p1">1 </paragraph>
+        <block type="container" key="c2">
+          <paragraph key="p2">
+            <cursor />1.1
+          </paragraph>
+          <block type="container" key="c3">
+            <paragraph key="p3">1.1.1</paragraph>
+          </block>
+        </block>
+      </block>
+    </document>
+  </value>
+)
+
+export const output = (
+  <value>
+    <document>
+      <block type="container" key="c1">
+        <paragraph key="p1">
+          1 <cursor />1.1
+        </paragraph>
+        <block type="container" key="c2">
+          <paragraph />
+          <block type="container" key="c3">
+            <paragraph key="p3">1.1.1</paragraph>
+          </block>
+        </block>
+      </block>
+    </document>
+  </value>
+)

packages/slate/test/commands/at-current-range/delete/all-with-nested-blocks.js

@@ -0,0 +1,44 @@
+/** @jsx h */
+
+import h from '../../../helpers/h'
+
+export default function(editor) {
+  editor.delete()
+}
+
+export const input = (
+  <value>
+    <document>
+      <block type="container">
+        <paragraph key="original paragraph">
+          <anchor />one
+        </paragraph>
+        <block type="container">
+          <paragraph> two</paragraph>
+          <paragraph> three</paragraph>
+          <block type="container">
+            <paragraph> four</paragraph>
+            <block type="container">
+              <paragraph>
+                {' '}
+                five<focus />
+              </paragraph>
+            </block>
+          </block>
+        </block>
+      </block>
+    </document>
+  </value>
+)
+
+export const output = (
+  <value>
+    <document>
+      <block type="container">
+        <paragraph key="original paragraph">
+          <cursor />
+        </paragraph>
+      </block>
+    </document>
+  </value>
+)

packages/slate/test/history/undo/insert-fragment.js

@@ -0,0 +1,47 @@
+/** @jsx h */
+
+import h from '../../helpers/h'
+
+const fragment = (
+  <document>
+    <block type="d">
+      <paragraph>A</paragraph>
+      <block type="c">
+        <block type="d">
+          <paragraph>B</paragraph>
+          <paragraph>
+            <block type="d">
+              <paragraph>C</paragraph>
+            </block>
+          </paragraph>
+        </block>
+        <block type="d">
+          <paragraph>D</paragraph>
+        </block>
+      </block>
+    </block>
+  </document>
+)
+
+export default function(editor) {
+  editor
+    .insertFragment(fragment)
+    .flush()
+    .undo()
+}
+
+export const input = (
+  <value>
+    <document>
+      <block type="d">
+        <paragraph>
+          <text>
+            <cursor />
+          </text>
+        </paragraph>
+      </block>
+    </document>
+  </value>
+)
+
+export const output = input

packages/slate/test/index.js

@@ -112,9 +112,11 @@ describe('slate', () => {
   // editor doesn't! It needs to live in the tests instead.
 
   fixtures(__dirname, 'commands', ({ module }) => {
-    const { input, output, options = {} } = module
+    const { input, output, options = {}, plugins: module_plugins } = module
     const fn = module.default
-    const editor = new Editor({ plugins })
+    const editor = new Editor({
+      plugins: module_plugins ? plugins.concat(module_plugins) : plugins,
+    })
     const opts = { preserveSelection: true, ...options }
 
     editor.setValue(input)
@@ -158,4 +160,9 @@ describe('slate', () => {
 
     assert.deepEqual(actual, expected)
   })
+
+  fixtures(__dirname, 'utils/path-utils', ({ module }) => {
+    const fn = module.default
+    fn()
+  })
 })

packages/slate/test/utils/path-utils/transform/move-node/from-before-not-younger.js

@@ -0,0 +1,30 @@
+import assert from 'assert'
+import { PathUtils, Operation } from 'slate'
+
+const assertEqualPaths = (p1, p2) =>
+  assert.deepEqual(p1.toArray(), p2.toArray())
+
+export default () => {
+  const path = PathUtils.create([0, 1, 0, 1])
+  const newPath = PathUtils.create([0, 1, 1, 0, 1])
+  const op = Operation.create({ path, newPath, type: 'move_node' })
+
+  const result_eq = PathUtils.transform(path, op).first()
+  assertEqualPaths(result_eq, newPath)
+
+  const before_old = PathUtils.create([0, 1, 0, 0])
+  const result_before_old = PathUtils.transform(before_old, op).first()
+  assertEqualPaths(result_before_old, before_old)
+
+  const after_old = PathUtils.create([0, 1, 0, 2])
+  const result_after_old = PathUtils.transform(after_old, op).first()
+  assertEqualPaths(result_after_old, PathUtils.decrement(after_old))
+
+  const before_new = PathUtils.create([0, 1, 1, 0, 0])
+  const result_before_new = PathUtils.transform(before_new, op).first()
+  assertEqualPaths(result_before_new, before_new)
+
+  const after_new = PathUtils.create([0, 1, 1, 0, 2])
+  const result_after_new = PathUtils.transform(after_new, op).first()
+  assertEqualPaths(result_after_new, PathUtils.increment(after_new))
+}

packages/slate/test/utils/path-utils/transform/move-node/from-younger.js

@@ -0,0 +1,30 @@
+import assert from 'assert'
+import { PathUtils, Operation } from 'slate'
+
+const assertEqualPaths = (p1, p2) =>
+  assert.deepEqual(p1.toArray(), p2.toArray())
+
+export default () => {
+  const path = PathUtils.create([0, 1, 1])
+  const newPath = PathUtils.create([0, 1, 2, 0, 1])
+  const op = Operation.create({ path, newPath, type: 'move_node' })
+
+  const result_eq = PathUtils.transform(path, op).first()
+  assertEqualPaths(result_eq, PathUtils.create([0, 1, 1, 0, 1]))
+
+  const before_old = PathUtils.create([0, 1, 0])
+  const result_before_old = PathUtils.transform(before_old, op).first()
+  assertEqualPaths(result_before_old, before_old)
+
+  const after_old = PathUtils.create([0, 1, 2])
+  const result_after_old = PathUtils.transform(after_old, op).first()
+  assertEqualPaths(result_after_old, PathUtils.decrement(after_old))
+
+  const before_new = PathUtils.create([0, 1, 2, 0, 0])
+  const result_before_new = PathUtils.transform(before_new, op).first()
+  assertEqualPaths(result_before_new, PathUtils.create([0, 1, 1, 0, 0]))
+
+  const after_new = PathUtils.create([0, 1, 2, 0, 2])
+  const result_after_new = PathUtils.transform(after_new, op).first()
+  assertEqualPaths(result_after_new, PathUtils.create([0, 1, 1, 0, 2]))
+}

packages/slate/test/utils/path-utils/transform/move-node/siblings-backward.js

@@ -0,0 +1,17 @@
+import assert from 'assert'
+import { PathUtils, Operation } from 'slate'
+
+const assertEqualPaths = (p1, p2) =>
+  assert.deepEqual(p1.toArray(), p2.toArray())
+
+export default () => {
+  const path = PathUtils.create([1])
+  const newPath = PathUtils.create([0])
+  const op = Operation.create({ path, newPath, type: 'move_node' })
+
+  const moved_node_result = PathUtils.transform(path, op).first()
+  assertEqualPaths(moved_node_result, newPath)
+
+  const sibling_result = PathUtils.transform(newPath, op).first()
+  assertEqualPaths(sibling_result, path)
+}

packages/slate/test/utils/path-utils/transform/move-node/siblings-forward.js

@@ -0,0 +1,17 @@
+import assert from 'assert'
+import { PathUtils, Operation } from 'slate'
+
+const assertEqualPaths = (p1, p2) =>
+  assert.deepEqual(p1.toArray(), p2.toArray())
+
+export default () => {
+  const path = PathUtils.create([0])
+  const newPath = PathUtils.create([1])
+  const op = Operation.create({ path, newPath, type: 'move_node' })
+
+  const moved_node_result = PathUtils.transform(path, op).first()
+  assertEqualPaths(moved_node_result, newPath)
+
+  const sibling_result = PathUtils.transform(newPath, op).first()
+  assertEqualPaths(sibling_result, path)
+}