Fix handling of editor.isSelectable in Editor.positions (#5929)

* Fix handling of `editor.isSelectable` in `Editor.positions`

* Clean-up

.changeset/eleven-clocks-begin.md

@@ -0,0 +1,7 @@
+---
+'slate': patch
+---
+
+- Fix error when a non-selectable node has no next or previous node
+- Do not return points from `Editor.positions` that are inside non-selectable nodes
+  - Previously, `editor.isSelectable` was handled incorrectly inside `Editor.positions`. When encountering a non-selectable node, it would immediately return the point before or after it (depending on `reverse`), but it would not skip returning points inside the non-selectable node if more than one point was consumed from `Editor.positions`.

packages/slate/src/editor/positions.ts

@@ -51,6 +51,7 @@ export function* positions(
   let distance = 0 // Distance for leafText to catch up to blockText.
   let leafTextRemaining = 0
   let leafTextOffset = 0
+  const skippedPaths: Path[] = []
 
   // Iterate through all nodes in range, grabbing entire textual content
   // of block nodes in blockText, and text nodes in leafText.
@@ -63,19 +64,35 @@ export function* positions(
     reverse,
     voids,
   })) {
+    // If the node is inside a skipped ancestor, do not return any points, but
+    // still process its content so that the iteration state remains correct.
+    const hasSkippedAncestor = skippedPaths.some(p => Path.isAncestor(p, path))
+
+    function* maybeYield(point: Point) {
+      if (!hasSkippedAncestor) {
+        yield point
+      }
+    }
+
     /*
      * ELEMENT NODE - Yield position(s) for voids, collect blockText for blocks
      */
     if (Element.isElement(node)) {
       if (!editor.isSelectable(node)) {
         /**
-         * If the node is not selectable, skip it
+         * If the node is not selectable, skip it and its descendants
          */
+        skippedPaths.push(path)
         if (reverse) {
-          yield Editor.end(editor, Path.previous(path))
+          if (Path.hasPrevious(path)) {
+            yield* maybeYield(Editor.end(editor, Path.previous(path)))
+          }
           continue
         } else {
-          yield Editor.start(editor, Path.next(path))
+          const nextPath = Path.next(path)
+          if (Editor.hasPath(editor, nextPath)) {
+            yield* maybeYield(Editor.start(editor, nextPath))
+          }
           continue
         }
       }
@@ -84,7 +101,7 @@ export function* positions(
       // yield their first point. If the `voids` option is set to true,
       // then we will iterate over their content.
       if (!voids && (editor.isVoid(node) || editor.isElementReadOnly(node))) {
-        yield Editor.start(editor, path)
+        yield* maybeYield(Editor.start(editor, path))
         continue
       }
 
@@ -143,7 +160,7 @@ export function* positions(
 
       // Yield position at the start of node (potentially).
       if (isFirst || isNewBlock || unit === 'offset') {
-        yield { path, offset: leafTextOffset }
+        yield* maybeYield({ path, offset: leafTextOffset })
         isNewBlock = false
       }
 
@@ -178,7 +195,7 @@ export function* positions(
         // to catch up with `blockText`, so we can reset `distance`
         // and yield this position in this node.
         distance = 0
-        yield { path, offset: leafTextOffset }
+        yield* maybeYield({ path, offset: leafTextOffset })
       }
     }
   }

packages/slate/test/interfaces/Editor/after/non-selectable-block-last.tsx

@@ -0,0 +1,17 @@
+/** @jsx jsx */
+
+import { Editor } from 'slate'
+import { jsx } from '../../..'
+
+export const input = (
+  <editor>
+    <block>one</block>
+    <block nonSelectable>two</block>
+  </editor>
+)
+
+export const test = editor => {
+  return Editor.after(editor, { path: [0, 0], offset: 3 })
+}
+
+export const output = undefined

packages/slate/test/interfaces/Editor/after/non-selectable-block.tsx

@@ -0,0 +1,18 @@
+/** @jsx jsx */
+
+import { Editor } from 'slate'
+import { jsx } from '../../..'
+
+export const input = (
+  <editor>
+    <block>one</block>
+    <block nonSelectable>two</block>
+    <block>three</block>
+  </editor>
+)
+
+export const test = editor => {
+  return Editor.after(editor, { path: [0, 0], offset: 3 })
+}
+
+export const output = { path: [2, 0], offset: 0 }

packages/slate/test/interfaces/Editor/after/non-selectable-inline-last.tsx

@@ -0,0 +1,20 @@
+/** @jsx jsx */
+
+import { Editor } from 'slate'
+import { jsx } from '../../..'
+
+// This is invalid due to the lack of a text node after the inline, but this
+// case can arise prior to normalization so it needs to be handled anyway.
+export const input = (
+  <editor>
+    <block>
+      one<inline nonSelectable>two</inline>
+    </block>
+  </editor>
+)
+
+export const test = editor => {
+  return Editor.after(editor, { path: [0, 0], offset: 3 })
+}
+
+export const output = undefined

packages/slate/test/interfaces/Editor/before/non-selectable-block-first.tsx

@@ -0,0 +1,17 @@
+/** @jsx jsx */
+
+import { Editor } from 'slate'
+import { jsx } from '../../..'
+
+export const input = (
+  <editor>
+    <block nonSelectable>two</block>
+    <block>three</block>
+  </editor>
+)
+
+export const test = editor => {
+  return Editor.before(editor, { path: [1, 0], offset: 0 })
+}
+
+export const output = undefined

packages/slate/test/interfaces/Editor/before/non-selectable-block.tsx

@@ -0,0 +1,18 @@
+/** @jsx jsx */
+
+import { Editor } from 'slate'
+import { jsx } from '../../..'
+
+export const input = (
+  <editor>
+    <block>one</block>
+    <block nonSelectable>two</block>
+    <block>three</block>
+  </editor>
+)
+
+export const test = editor => {
+  return Editor.before(editor, { path: [2, 0], offset: 0 })
+}
+
+export const output = { path: [0, 0], offset: 3 }

packages/slate/test/interfaces/Editor/before/non-selectable-inline-first.tsx

@@ -0,0 +1,20 @@
+/** @jsx jsx */
+
+import { Editor } from 'slate'
+import { jsx } from '../../..'
+
+// This is invalid due to the lack of a text node before the inline, but this
+// case can arise prior to normalization so it needs to be handled anyway.
+export const input = (
+  <editor>
+    <block>
+      <inline nonSelectable>two</inline>three
+    </block>
+  </editor>
+)
+
+export const test = editor => {
+  return Editor.before(editor, { path: [0, 1], offset: 0 })
+}
+
+export const output = undefined