From 796389c7d3d9cead1493abcba6c678cb9dfa979f Mon Sep 17 00:00:00 2001 From: Andrew Herron Date: Thu, 6 May 2021 09:31:13 +1000 Subject: [PATCH] Added insert_node path checking to match how other operations are applied (#4230) * Added try/finally to editor.apply and Editor.withoutNormalizing for state cleanup * Added insert_node path checking to match how other operations are applied --- .changeset/bright-pants-grin.md | 5 + .changeset/sixty-glasses-beg.md | 5 + packages/slate/src/interfaces/editor.ts | 7 +- packages/slate/src/transforms/general.ts | 467 +++++++++--------- .../general/invalid-insert_node.tsx | 30 ++ 5 files changed, 286 insertions(+), 228 deletions(-) create mode 100644 .changeset/bright-pants-grin.md create mode 100644 .changeset/sixty-glasses-beg.md create mode 100644 packages/slate/test/transforms/general/invalid-insert_node.tsx diff --git a/.changeset/bright-pants-grin.md b/.changeset/bright-pants-grin.md new file mode 100644 index 0000000000..a51f665935 --- /dev/null +++ b/.changeset/bright-pants-grin.md @@ -0,0 +1,5 @@ +--- +'slate': minor +--- + +Applying invalid `insert_node` operations will now throw an exception for all invalid paths, not just invalid parent paths. diff --git a/.changeset/sixty-glasses-beg.md b/.changeset/sixty-glasses-beg.md new file mode 100644 index 0000000000..14ea67f59b --- /dev/null +++ b/.changeset/sixty-glasses-beg.md @@ -0,0 +1,5 @@ +--- +'slate': patch +--- + +Exceptions in `editor.apply()` and `Editor.withoutNormalizing()` will no longer leave the editor in an invalid state diff --git a/packages/slate/src/interfaces/editor.ts b/packages/slate/src/interfaces/editor.ts index 68394123b9..d6b4eb4409 100644 --- a/packages/slate/src/interfaces/editor.ts +++ b/packages/slate/src/interfaces/editor.ts @@ -1648,8 +1648,11 @@ export const Editor: EditorInterface = { withoutNormalizing(editor: Editor, fn: () => void): void { const value = Editor.isNormalizing(editor) NORMALIZING.set(editor, false) - fn() - NORMALIZING.set(editor, value) + try { + fn() + } finally { + NORMALIZING.set(editor, value) + } Editor.normalize(editor) }, } diff --git a/packages/slate/src/transforms/general.ts b/packages/slate/src/transforms/general.ts index 6ccbb96461..4c05d305b6 100644 --- a/packages/slate/src/transforms/general.ts +++ b/packages/slate/src/transforms/general.ts @@ -2,6 +2,7 @@ import { createDraft, finishDraft, isDraft } from 'immer' import { Node, Editor, + Selection, Range, Point, Text, @@ -17,294 +18,308 @@ export interface GeneralTransforms { transform: (editor: Editor, op: Operation) => void } -export const GeneralTransforms: GeneralTransforms = { - /** - * Transform the editor by an operation. - */ - - transform(editor: Editor, op: Operation): void { - editor.children = createDraft(editor.children) - let selection = editor.selection && createDraft(editor.selection) +const applyToDraft = (editor: Editor, selection: Selection, op: Operation) => { + switch (op.type) { + case 'insert_node': { + const { path, node } = op + const parent = Node.parent(editor, path) + const index = path[path.length - 1] + + if (index > parent.children.length) { + throw new Error( + `Cannot apply an "insert_node" operation at path [${path}] because the destination is past the end of the node.` + ) + } - switch (op.type) { - case 'insert_node': { - const { path, node } = op - const parent = Node.parent(editor, path) - const index = path[path.length - 1] - parent.children.splice(index, 0, node) + parent.children.splice(index, 0, node) - if (selection) { - for (const [point, key] of Range.points(selection)) { - selection[key] = Point.transform(point, op)! - } + if (selection) { + for (const [point, key] of Range.points(selection)) { + selection[key] = Point.transform(point, op)! } - - break } - case 'insert_text': { - const { path, offset, text } = op - if (text.length === 0) break - const node = Node.leaf(editor, path) - const before = node.text.slice(0, offset) - const after = node.text.slice(offset) - node.text = before + text + after - - if (selection) { - for (const [point, key] of Range.points(selection)) { - selection[key] = Point.transform(point, op)! - } - } + break + } - break + case 'insert_text': { + const { path, offset, text } = op + if (text.length === 0) break + const node = Node.leaf(editor, path) + const before = node.text.slice(0, offset) + const after = node.text.slice(offset) + node.text = before + text + after + + if (selection) { + for (const [point, key] of Range.points(selection)) { + selection[key] = Point.transform(point, op)! + } } - case 'merge_node': { - const { path } = op - const node = Node.get(editor, path) - const prevPath = Path.previous(path) - const prev = Node.get(editor, prevPath) - const parent = Node.parent(editor, path) - const index = path[path.length - 1] - - if (Text.isText(node) && Text.isText(prev)) { - prev.text += node.text - } else if (!Text.isText(node) && !Text.isText(prev)) { - prev.children.push(...node.children) - } else { - throw new Error( - `Cannot apply a "merge_node" operation at path [${path}] to nodes of different interfaces: ${node} ${prev}` - ) - } + break + } - parent.children.splice(index, 1) + case 'merge_node': { + const { path } = op + const node = Node.get(editor, path) + const prevPath = Path.previous(path) + const prev = Node.get(editor, prevPath) + const parent = Node.parent(editor, path) + const index = path[path.length - 1] + + if (Text.isText(node) && Text.isText(prev)) { + prev.text += node.text + } else if (!Text.isText(node) && !Text.isText(prev)) { + prev.children.push(...node.children) + } else { + throw new Error( + `Cannot apply a "merge_node" operation at path [${path}] to nodes of different interfaces: ${node} ${prev}` + ) + } - if (selection) { - for (const [point, key] of Range.points(selection)) { - selection[key] = Point.transform(point, op)! - } - } + parent.children.splice(index, 1) - break + if (selection) { + for (const [point, key] of Range.points(selection)) { + selection[key] = Point.transform(point, op)! + } } - case 'move_node': { - const { path, newPath } = op + break + } - if (Path.isAncestor(path, newPath)) { - throw new Error( - `Cannot move a path [${path}] to new path [${newPath}] because the destination is inside itself.` - ) - } + case 'move_node': { + const { path, newPath } = op - const node = Node.get(editor, path) - const parent = Node.parent(editor, path) - const index = path[path.length - 1] - - // This is tricky, but since the `path` and `newPath` both refer to - // the same snapshot in time, there's a mismatch. After either - // removing the original position, the second step's path can be out - // of date. So instead of using the `op.newPath` directly, we - // transform `op.path` to ascertain what the `newPath` would be after - // the operation was applied. - parent.children.splice(index, 1) - const truePath = Path.transform(path, op)! - const newParent = Node.get(editor, Path.parent(truePath)) as Ancestor - const newIndex = truePath[truePath.length - 1] - - newParent.children.splice(newIndex, 0, node) - - if (selection) { - for (const [point, key] of Range.points(selection)) { - selection[key] = Point.transform(point, op)! - } - } + if (Path.isAncestor(path, newPath)) { + throw new Error( + `Cannot move a path [${path}] to new path [${newPath}] because the destination is inside itself.` + ) + } - break + const node = Node.get(editor, path) + const parent = Node.parent(editor, path) + const index = path[path.length - 1] + + // This is tricky, but since the `path` and `newPath` both refer to + // the same snapshot in time, there's a mismatch. After either + // removing the original position, the second step's path can be out + // of date. So instead of using the `op.newPath` directly, we + // transform `op.path` to ascertain what the `newPath` would be after + // the operation was applied. + parent.children.splice(index, 1) + const truePath = Path.transform(path, op)! + const newParent = Node.get(editor, Path.parent(truePath)) as Ancestor + const newIndex = truePath[truePath.length - 1] + + newParent.children.splice(newIndex, 0, node) + + if (selection) { + for (const [point, key] of Range.points(selection)) { + selection[key] = Point.transform(point, op)! + } } - case 'remove_node': { - const { path } = op - const index = path[path.length - 1] - const parent = Node.parent(editor, path) - parent.children.splice(index, 1) + break + } - // Transform all of the points in the value, but if the point was in the - // node that was removed we need to update the range or remove it. - if (selection) { - for (const [point, key] of Range.points(selection)) { - const result = Point.transform(point, op) + case 'remove_node': { + const { path } = op + const index = path[path.length - 1] + const parent = Node.parent(editor, path) + parent.children.splice(index, 1) - if (selection != null && result != null) { - selection[key] = result - } else { - let prev: NodeEntry | undefined - let next: NodeEntry | undefined - - for (const [n, p] of Node.texts(editor)) { - if (Path.compare(p, path) === -1) { - prev = [n, p] - } else { - next = [n, p] - break - } - } + // Transform all of the points in the value, but if the point was in the + // node that was removed we need to update the range or remove it. + if (selection) { + for (const [point, key] of Range.points(selection)) { + const result = Point.transform(point, op) - if (prev) { - point.path = prev[1] - point.offset = prev[0].text.length - } else if (next) { - point.path = next[1] - point.offset = 0 + if (selection != null && result != null) { + selection[key] = result + } else { + let prev: NodeEntry | undefined + let next: NodeEntry | undefined + + for (const [n, p] of Node.texts(editor)) { + if (Path.compare(p, path) === -1) { + prev = [n, p] } else { - selection = null + next = [n, p] + break } } - } - } - - break - } - case 'remove_text': { - const { path, offset, text } = op - if (text.length === 0) break - const node = Node.leaf(editor, path) - const before = node.text.slice(0, offset) - const after = node.text.slice(offset + text.length) - node.text = before + after - - if (selection) { - for (const [point, key] of Range.points(selection)) { - selection[key] = Point.transform(point, op)! + if (prev) { + point.path = prev[1] + point.offset = prev[0].text.length + } else if (next) { + point.path = next[1] + point.offset = 0 + } else { + selection = null + } } } - - break } - case 'set_node': { - const { path, properties, newProperties } = op + break + } - if (path.length === 0) { - throw new Error(`Cannot set properties on the root node!`) + case 'remove_text': { + const { path, offset, text } = op + if (text.length === 0) break + const node = Node.leaf(editor, path) + const before = node.text.slice(0, offset) + const after = node.text.slice(offset + text.length) + node.text = before + after + + if (selection) { + for (const [point, key] of Range.points(selection)) { + selection[key] = Point.transform(point, op)! } + } - const node = Node.get(editor, path) + break + } - for (const key in newProperties) { - if (key === 'children' || key === 'text') { - throw new Error(`Cannot set the "${key}" property of nodes!`) - } + case 'set_node': { + const { path, properties, newProperties } = op - const value = newProperties[key] + if (path.length === 0) { + throw new Error(`Cannot set properties on the root node!`) + } - if (value == null) { - delete node[key] - } else { - node[key] = value - } - } + const node = Node.get(editor, path) - // properties that were previously defined, but are now missing, must be deleted - for (const key in properties) { - if (!newProperties.hasOwnProperty(key)) { - delete node[key] - } + for (const key in newProperties) { + if (key === 'children' || key === 'text') { + throw new Error(`Cannot set the "${key}" property of nodes!`) } - break + const value = newProperties[key] + + if (value == null) { + delete node[key] + } else { + node[key] = value + } } - case 'set_selection': { - const { newProperties } = op + // properties that were previously defined, but are now missing, must be deleted + for (const key in properties) { + if (!newProperties.hasOwnProperty(key)) { + delete node[key] + } + } - if (newProperties == null) { - selection = newProperties - } else { - if (selection == null) { - if (!Range.isRange(newProperties)) { - throw new Error( - `Cannot apply an incomplete "set_selection" operation properties ${JSON.stringify( - newProperties - )} when there is no current selection.` - ) - } + break + } - selection = { ...newProperties } + case 'set_selection': { + const { newProperties } = op + + if (newProperties == null) { + selection = newProperties + } else { + if (selection == null) { + if (!Range.isRange(newProperties)) { + throw new Error( + `Cannot apply an incomplete "set_selection" operation properties ${JSON.stringify( + newProperties + )} when there is no current selection.` + ) } - for (const key in newProperties) { - const value = newProperties[key] + selection = { ...newProperties } + } - if (value == null) { - if (key === 'anchor' || key === 'focus') { - throw new Error(`Cannot remove the "${key}" selection property`) - } + for (const key in newProperties) { + const value = newProperties[key] - delete selection[key] - } else { - selection[key] = value + if (value == null) { + if (key === 'anchor' || key === 'focus') { + throw new Error(`Cannot remove the "${key}" selection property`) } + + delete selection[key] + } else { + selection[key] = value } } - - break } - case 'split_node': { - const { path, position, properties } = op + break + } - if (path.length === 0) { - throw new Error( - `Cannot apply a "split_node" operation at path [${path}] because the root node cannot be split.` - ) - } + case 'split_node': { + const { path, position, properties } = op - const node = Node.get(editor, path) - const parent = Node.parent(editor, path) - const index = path[path.length - 1] - let newNode: Descendant - - if (Text.isText(node)) { - const before = node.text.slice(0, position) - const after = node.text.slice(position) - node.text = before - newNode = { - ...(properties as Partial), - text: after, - } - } else { - const before = node.children.slice(0, position) - const after = node.children.slice(position) - node.children = before + if (path.length === 0) { + throw new Error( + `Cannot apply a "split_node" operation at path [${path}] because the root node cannot be split.` + ) + } - newNode = { - ...(properties as Partial), - children: after, - } + const node = Node.get(editor, path) + const parent = Node.parent(editor, path) + const index = path[path.length - 1] + let newNode: Descendant + + if (Text.isText(node)) { + const before = node.text.slice(0, position) + const after = node.text.slice(position) + node.text = before + newNode = { + ...(properties as Partial), + text: after, + } + } else { + const before = node.children.slice(0, position) + const after = node.children.slice(position) + node.children = before + + newNode = { + ...(properties as Partial), + children: after, } + } - parent.children.splice(index + 1, 0, newNode) + parent.children.splice(index + 1, 0, newNode) - if (selection) { - for (const [point, key] of Range.points(selection)) { - selection[key] = Point.transform(point, op)! - } + if (selection) { + for (const [point, key] of Range.points(selection)) { + selection[key] = Point.transform(point, op)! } - - break } + + break } + } + return selection +} - editor.children = finishDraft(editor.children) +export const GeneralTransforms: GeneralTransforms = { + /** + * Transform the editor by an operation. + */ - if (selection) { - editor.selection = isDraft(selection) - ? (finishDraft(selection) as Range) - : selection - } else { - editor.selection = null + transform(editor: Editor, op: Operation): void { + editor.children = createDraft(editor.children) + let selection = editor.selection && createDraft(editor.selection) + + try { + selection = applyToDraft(editor, selection, op) + } finally { + editor.children = finishDraft(editor.children) + + if (selection) { + editor.selection = isDraft(selection) + ? (finishDraft(selection) as Range) + : selection + } else { + editor.selection = null + } } }, } diff --git a/packages/slate/test/transforms/general/invalid-insert_node.tsx b/packages/slate/test/transforms/general/invalid-insert_node.tsx new file mode 100644 index 0000000000..052813326b --- /dev/null +++ b/packages/slate/test/transforms/general/invalid-insert_node.tsx @@ -0,0 +1,30 @@ +/** @jsx jsx */ +import assert from 'assert' +import { Transforms } from 'slate' +import { jsx } from '../..' + +export const input = ( + + + word + + + +) +export const run = editor => { + // position 2 is past the end of the block children + assert.throws(() => { + Transforms.insertNodes(editor, another, { at: [0, 2] }) + }, 'Inserting a node after the end of a block should fail') + // 1 is _at_ the end, so it's still valid + Transforms.insertNodes(editor, another, { at: [0, 1] }) +} +export const output = ( + + + word + + another + + +)