diff --git a/packages/slate/src/controllers/editor.js b/packages/slate/src/controllers/editor.js index 5732a47bf..9a4904562 100644 --- a/packages/slate/src/controllers/editor.js +++ b/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': { diff --git a/packages/slate/src/interfaces/element.js b/packages/slate/src/interfaces/element.js index 4aac9c292..1e635cc23 100644 --- a/packages/slate/src/interfaces/element.js +++ b/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) diff --git a/packages/slate/src/operations/invert.js b/packages/slate/src/operations/invert.js index 1c1c34a20..c530ad101 100644 --- a/packages/slate/src/operations/invert.js +++ b/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 } diff --git a/packages/slate/src/utils/path-utils.js b/packages/slate/src/utils/path-utils.js index abbea1b3b..749cbc858 100644 --- a/packages/slate/src/utils/path-utils.js +++ b/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) } } } diff --git a/packages/slate/test/commands/at-current-range/delete-backward/with-plugin-normalization.js b/packages/slate/test/commands/at-current-range/delete-backward/with-plugin-normalization.js new file mode 100644 index 000000000..6684f3854 --- /dev/null +++ b/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 = ( + + + + 1 + + + 1.1 + + + 1.1.1 + + + + + +) + +export const output = ( + + + + + 1 1.1 + + + + + 1.1.1 + + + + + +) diff --git a/packages/slate/test/commands/at-current-range/delete/all-with-nested-blocks.js b/packages/slate/test/commands/at-current-range/delete/all-with-nested-blocks.js new file mode 100644 index 000000000..eaa79a48e --- /dev/null +++ b/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 = ( + + + + + one + + + two + three + + four + + + {' '} + five + + + + + + + +) + +export const output = ( + + + + + + + + + +) diff --git a/packages/slate/test/history/undo/insert-fragment.js b/packages/slate/test/history/undo/insert-fragment.js new file mode 100644 index 000000000..f0e6f3f80 --- /dev/null +++ b/packages/slate/test/history/undo/insert-fragment.js @@ -0,0 +1,47 @@ +/** @jsx h */ + +import h from '../../helpers/h' + +const fragment = ( + + + A + + + B + + + C + + + + + D + + + + +) + +export default function(editor) { + editor + .insertFragment(fragment) + .flush() + .undo() +} + +export const input = ( + + + + + + + + + + + +) + +export const output = input diff --git a/packages/slate/test/index.js b/packages/slate/test/index.js index 7fd2fabaa..b1fb3fcf8 100644 --- a/packages/slate/test/index.js +++ b/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() + }) }) diff --git a/packages/slate/test/utils/path-utils/transform/move-node/from-before-not-younger.js b/packages/slate/test/utils/path-utils/transform/move-node/from-before-not-younger.js new file mode 100644 index 000000000..295ee8514 --- /dev/null +++ b/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)) +} diff --git a/packages/slate/test/utils/path-utils/transform/move-node/from-younger.js b/packages/slate/test/utils/path-utils/transform/move-node/from-younger.js new file mode 100644 index 000000000..3a5ce45ba --- /dev/null +++ b/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])) +} diff --git a/packages/slate/test/utils/path-utils/transform/move-node/siblings-backward.js b/packages/slate/test/utils/path-utils/transform/move-node/siblings-backward.js new file mode 100644 index 000000000..1ec6eab5b --- /dev/null +++ b/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) +} diff --git a/packages/slate/test/utils/path-utils/transform/move-node/siblings-forward.js b/packages/slate/test/utils/path-utils/transform/move-node/siblings-forward.js new file mode 100644 index 000000000..69f5e4600 --- /dev/null +++ b/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) +}