Fix normalizing dirty paths (#2222)

#### Is this adding or improving a _feature_ or fixing a _bug_?

Fix.

#### What's the new behavior?

The dirty paths in a change are now transformed against incoming operations, such that they don't get out of sync as normalizations occur. This is a rough pass to get correctness and the bug fixed, and we can later optimize lots of the little details for performance.

#### Have you checked that...?

<!-- 
Please run through this checklist for your pull request: 
-->

* [x] The new code matches the existing patterns and styles.
* [x] The tests pass with `yarn test`.
* [x] The linter passes with `yarn lint`. (Fix errors with `yarn prettier`.)
* [x] The relevant examples still work. (Run examples with `yarn watch`.)

#### Does this fix any issues or need any specific reviewers?

Fixes: #2211
Fixes: #2215
Fixes: #2194

packages/slate-hyperscript/test/fixtures/cursor-across-blocks-and-inlines.js

@@ -39,7 +39,7 @@ export const output = {
         nodes: [
           {
             object: 'text',
-            key: '11',
+            key: '13',
             leaves: [
               {
                 object: 'leaf',
@@ -69,7 +69,7 @@ export const output = {
           },
           {
             object: 'text',
-            key: '12',
+            key: '14',
             leaves: [
               {
                 object: 'leaf',
@@ -88,7 +88,7 @@ export const output = {
         nodes: [
           {
             object: 'text',
-            key: '13',
+            key: '11',
             leaves: [
               {
                 object: 'leaf',
@@ -118,7 +118,7 @@ export const output = {
           },
           {
             object: 'text',
-            key: '14',
+            key: '12',
             leaves: [
               {
                 object: 'leaf',

packages/slate/src/models/change.js

@@ -1,7 +1,7 @@
 import Debug from 'debug'
 import isPlainObject from 'is-plain-object'
 import warning from 'slate-dev-warning'
-import { List, Map } from 'immutable'
+import { List } from 'immutable'
 
 import Changes from '../changes'
 import Operation from './operation'
@@ -55,7 +55,6 @@ class Change {
     const { operations } = this
     let { value } = this
     let { history } = value
-    const oldValue = value
 
     // Add in the current `value` in case the operation was serialized.
     if (isPlainObject(operation)) {
@@ -84,9 +83,16 @@ class Change {
       value = value.set('history', history)
     }
 
-    // Get the keys of the affected nodes, and mark them as dirty.
+    // Get the paths of the affected nodes, and mark them as dirty.
-    const keys = getDirtyKeys(operation, value, oldValue)
+    const newDirtyPaths = getDirtyPaths(operation)
-    this.tmp.dirty = this.tmp.dirty.concat(keys)
+    const dirty = this.tmp.dirty.reduce((memo, path) => {
+      path = PathUtils.create(path)
+      const transformed = PathUtils.transform(path, operation)
+      memo = memo.concat(transformed.toArray())
+      return memo
+    }, newDirtyPaths)
+
+    this.tmp.dirty = dirty
 
     // Update the mutable change object.
     this.value = value
@@ -117,7 +123,7 @@ class Change {
 
   call(fn, ...args) {
     fn(this, ...args)
-    this.normalizeDirtyOperations()
+    this.normalizeDirtyPaths()
     return this
   }
 
@@ -130,75 +136,29 @@ class Change {
   normalize() {
     const { value } = this
     const { document } = value
-    const keys = Object.keys(document.getKeysToPathsTable())
-    this.normalizeKeys(keys)
-    return this
-  }
-
-  /**
-   * Normalize any new "dirty" operations that have been added to the change.
-   *
-   * @return {Change}
-   */
-
-  normalizeDirtyOperations() {
-    const { normalize, dirty } = this.tmp
-    if (!normalize) return this
-    if (!dirty.length) return this
-    this.tmp.dirty = []
-    this.normalizeKeys(dirty)
-    return this
-  }
-
-  /**
-   * Normalize a set of nodes by their `keys`.
-   *
-   * @param {Array} keys
-   * @return {Change}
-   */
-
-  normalizeKeys(keys) {
-    const { value } = this
-    const { document } = value
-
-    // TODO: if we had an `Operations.tranform` method, we could optimize this
-    // to not use keys, and instead used transformed operation paths.
     const table = document.getKeysToPathsTable()
-    let map = Map()
+    const paths = Object.values(table).map(PathUtils.create)
-
+    this.tmp.dirty = this.tmp.dirty.concat(paths)
-    // TODO: this could be optimized to not need the nested map, and instead use
+    this.normalizeDirtyPaths()
-    // clever sorting to arrive at the proper depth-first normalizing.
-    keys.forEach(key => {
-      const path = table[key]
-      if (!path) return
-      if (!path.length) return
-      if (!map.hasIn(path)) map = map.setIn(path, Map())
-    })
-
-    // To avoid infinite loops, we need to defer normalization until the end.
-    this.withoutNormalizing(() => {
-      this.normalizeMapAndPath(map)
-    })
-
     return this
   }
 
   /**
-   * Normalize all of the nodes in a normalization `map`, depth-first. An
+   * Normalize any new "dirty" paths that have been added to the change.
-   * additional `path` argument specifics the current depth/location.
    *
-   * @param {Map} map
-   * @param {Array} path (optional)
    * @return {Change}
    */
 
-  normalizeMapAndPath(map, path = []) {
+  normalizeDirtyPaths() {
-    map.forEach((m, k) => {
+    if (!this.tmp.normalize) {
-      const p = [...path, k]
+      return this
-      this.normalizeMapAndPath(m, p)
+    }
-    })
+
+    while (this.tmp.dirty.length) {
+      const path = this.tmp.dirty.pop()
+      this.normalizeNodeByPath(path)
+    }
 
-    this.normalizePath(path)
     return this
   }
 
@@ -210,7 +170,7 @@ class Change {
    * @return {Change}
    */
 
-  normalizePath(path) {
+  normalizeNodeByPath(path) {
     const { value } = this
     let { document, schema } = value
     let node = document.assertNode(path)
@@ -264,7 +224,10 @@ class Change {
       iterate()
     }
 
-    iterate()
+    this.withoutNormalizing(() => {
+      iterate()
+    })
+
     return this
   }
 
@@ -281,11 +244,7 @@ class Change {
     this.tmp.normalize = false
     fn(this)
     this.tmp.normalize = value
-
+    this.normalizeDirtyPaths()
-    if (this.tmp.normalize) {
-      this.normalizeDirtyOperations()
-    }
-
     return this
   }
 
@@ -373,18 +332,14 @@ class Change {
 }
 
 /**
- * Get the "dirty" nodes's keys for a given `operation` and values.
+ * Get the "dirty" paths for a given `operation`.
  *
  * @param {Operation} operation
- * @param {Value} newValue
- * @param {Value} oldValue
  * @return {Array}
  */
 
-function getDirtyKeys(operation, newValue, oldValue) {
+function getDirtyPaths(operation) {
   const { type, node, path, newPath } = operation
-  const newDocument = newValue.document
-  const oldDocument = oldValue.document
 
   switch (type) {
     case 'add_mark':
@@ -393,46 +348,50 @@ function getDirtyKeys(operation, newValue, oldValue) {
     case 'remove_text':
     case 'set_mark':
     case 'set_node': {
-      const target = newDocument.assertNode(path)
+      return [path]
-      const keys = [target.key]
-      return keys
     }
 
     case 'insert_node': {
       const table = node.getKeysToPathsTable()
-      const keys = Object.keys(table)
+      const paths = Object.values(table).map(p => path.concat(p))
-      return keys
+      const parentPath = PathUtils.lift(path)
+      return [parentPath, path, ...paths]
     }
 
     case 'split_node': {
+      const parentPath = PathUtils.lift(path)
       const nextPath = PathUtils.increment(path)
-      const target = newDocument.assertNode(path)
+      return [parentPath, path, nextPath]
-      const split = newDocument.assertNode(nextPath)
-      const keys = [target.key, split.key]
-      return keys
     }
 
     case 'merge_node': {
+      const parentPath = PathUtils.lift(path)
       const previousPath = PathUtils.decrement(path)
-      const merged = newDocument.assertNode(previousPath)
+      return [parentPath, previousPath]
-      const keys = [merged.key]
-      return keys
     }
 
     case 'move_node': {
-      const parentPath = PathUtils.lift(path)
+      let parentPath = PathUtils.lift(path)
-      const newParentPath = PathUtils.lift(newPath)
+      let newParentPath = PathUtils.lift(newPath)
-      const oldParent = oldDocument.assertNode(parentPath)
+
-      const newParent = oldDocument.assertNode(newParentPath)
+      // HACK: this clause only exists because the `move_path` logic isn't
-      const keys = [oldParent.key, newParent.key]
+      // consistent when it deals with siblings.
-      return keys
+      if (!PathUtils.isSibling(path, newPath)) {
+        if (newParentPath.size && PathUtils.isYounger(path, newPath)) {
+          newParentPath = PathUtils.decrement(newParentPath, 1, path.size - 1)
+        }
+
+        if (parentPath.size && PathUtils.isYounger(newPath, path)) {
+          parentPath = PathUtils.increment(parentPath, 1, newPath.size - 1)
+        }
+      }
+
+      return [parentPath, newParentPath]
     }
 
     case 'remove_node': {
       const parentPath = PathUtils.lift(path)
-      const parent = newDocument.assertNode(parentPath)
+      return [parentPath]
-      const keys = [parent.key]
-      return keys
     }
 
     default: {

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

@@ -143,6 +143,23 @@ function isEqual(path, target) {
   return path.equals(target)
 }
 
+/**
+ * Is a `path` older than a `target` path? Meaning that it ends as an older
+ * sibling of one of the indexes in the target.
+ *
+ * @param {List} path
+ * @param {List} target
+ * @return {Boolean}
+ */
+
+function isOlder(path, target) {
+  const index = path.size - 1
+  const [p, t] = crop(path, target, index)
+  const pl = path.get(index)
+  const tl = target.get(index)
+  return isEqual(p, t) && pl > tl
+}
+
 /**
  * Is a `path` a sibling of a `target` path?
  *
@@ -158,6 +175,23 @@ function isSibling(path, target) {
   return p.equals(t)
 }
 
+/**
+ * Is a `path` younger than a `target` path? Meaning that it ends as a younger
+ * sibling of one of the indexes in the target.
+ *
+ * @param {List} path
+ * @param {List} target
+ * @return {Boolean}
+ */
+
+function isYounger(path, target) {
+  const index = path.size - 1
+  const [p, t] = crop(path, target, index)
+  const pl = path.get(index)
+  const tl = target.get(index)
+  return isEqual(p, t) && pl < tl
+}
+
 /**
  * Lift a `path` to refer to its parent.
  *
@@ -222,6 +256,98 @@ function relate(a, b) {
   return path
 }
 
+/**
+ * Transform a `path` by an `operation`, adjusting it to stay current.
+ *
+ * @param {List} path
+ * @param {Operation} operation
+ * @return {List<List>}
+ */
+
+function transform(path, operation) {
+  const { type, position, path: p } = operation
+
+  if (
+    type === 'add_mark' ||
+    type === 'insert_text' ||
+    type === 'remove_mark' ||
+    type === 'remove_text' ||
+    type === 'set_mark' ||
+    type === 'set_node' ||
+    type === 'set_selection' ||
+    type === 'set_value' ||
+    path.size === 0
+  ) {
+    return List([path])
+  }
+
+  const pIndex = p.size - 1
+  const pEqual = isEqual(p, path)
+  const pYounger = isYounger(p, path)
+  const pAbove = isAbove(p, path)
+
+  if (type === 'insert_node') {
+    if (pEqual || pYounger || pAbove) {
+      path = increment(path, 1, pIndex)
+    }
+  }
+
+  if (type === 'remove_node') {
+    if (pYounger) {
+      path = decrement(path, 1, pIndex)
+    } else if (pEqual || pAbove) {
+      path = []
+    }
+  }
+
+  if (type === 'merge_node') {
+    if (pEqual || pYounger) {
+      path = decrement(path, 1, pIndex)
+    } else if (pAbove) {
+      path = decrement(path, 1, pIndex)
+      path = increment(path, position, pIndex + 1)
+    }
+  }
+
+  if (type === 'split_node') {
+    if (pEqual) {
+      path = [path, increment(path)]
+    } else if (pYounger) {
+      path = increment(path, 1, pIndex)
+    } else if (pAbove) {
+      if (path.get(pIndex + 1) >= position) {
+        path = increment(path, 1, pIndex)
+        path = decrement(path, position, pIndex + 1)
+      }
+    }
+  }
+
+  if (type === 'move_node') {
+    const { newPath: np } = operation
+    const npIndex = np.size - 1
+    const npEqual = isEqual(np, path)
+    const npYounger = isYounger(np, path)
+    const npAbove = isAbove(np, path)
+
+    if (pAbove) {
+      path = np.concat(path.slice(p.size))
+    } else {
+      if (pEqual) {
+        path = np
+      } else if (pYounger) {
+        path = decrement(path, 1, pIndex)
+      }
+
+      if (npEqual || npYounger || npAbove) {
+        path = increment(path, 1, npIndex)
+      }
+    }
+  }
+
+  const paths = Array.isArray(path) ? path : [path]
+  return List(paths)
+}
+
 /**
  * Export.
  *
@@ -238,9 +364,12 @@ export default {
   isAfter,
   isBefore,
   isEqual,
+  isOlder,
   isSibling,
+  isYounger,
   lift,
   max,
   min,
   relate,
+  transform,
 }