slate: Delete all will fail when there are three blocks or more

Pushed an update to the PR. Now for a go at nuking that move_node(X,X) operation. Thanks a lot @ericedem, @Slapbox and @ianstormtaylor! ❤️

Ok I think I may have found the culprit, or at least something close. This bit of code: https://github.com/ianstormtaylor/slate/blob/master/packages/slate/src/commands/at-range.js#L228-L234

Which is why this is specifically failing with 3 or more blocks, because it is looking at the original start and end blocks which will never be next to each other. What happens in the case of 3 blocks is that move seems to be inadvertently adding or transforming one of the existing dirty paths to [ 2 ] (even though there isn’t a node at [ 2 ] at this point), which will eventually get changed to [ 1 ] by the merge (which both of these seem a little odd). Then when normalization happens [ 1 ] is invalid.

Yup, it should just be the isEqual(p, np) that we need. The others should cause bugs I think.

Ok yep tracked it down to this line:

https://github.com/ianstormtaylor/slate/blob/master/packages/slate/src/utils/path-utils.js#L358

When moving and newPath === path the path gets incremented to the path that no longer exists.

The answer to my question is no.

Playing around with it locally, this change seems to work:

@@ -195,7 +196,11 @@ class Change {
   normalizeNodeByPath(path) {
     const { editor, value } = this
     let { document } = value
-    let node = document.assertNode(path)
+
+    let node = document.getNode(path)
+    // This node has since been removed, so just skip this normalization.
+    if (!node) return
+
     let iterations = 0

The problem is that while withoutNormalizing is on, a whole bunch of dirty paths will get queued up, but some will no longer be valid by the time normalization actually happens.

I haven’t dug super deep, but it looks like at some point children of path [ 1 ] are being removed / modified which queues up [ 1 ] as a dirty path since it is an ancestor, but that node is obviously deleted along with everything else. When normalization finally runs, the node at [ 1 ] no longer exists, but the path is still considered dirty.

@slapbox All the “key” methods are aliases for “path” methods now.

Umm…what?

After pulling the latest code, we can’t delete anything (select/deleteAll) anymore if we have three or more blocks (ending in inline void or not). Fails with the same error. Seems related to what was done here:

• fix: use ancestors for dirt paths (#2316) …
• Fixes deleteWordForward at line ending (#2290)

I suspect the difference is due to #2316, which makes me believe there is something funny with the withoutNormalization wrapping (that something is getting normalized when it shoudn’t).

Here are some findings, all seems related to the algorithm inside .deleteAtRange

• If we remove change.withoutNormalizing this case will work as intended but breaks other tests.
• If we remove moveNodeByKey this case will work as intended but breaks other tests.

I suspect there is something missing in the algorithm that doesn’t take into account that voids can be inlines too when it’s trying to do the void checks and the related texts test. That, or there is something going on with the (without) normalization somehow.

I have added a failing test here: https://github.com/skogsmaskin/slate/commit/17429b5b5c6e0dc5de088adfec4c80a67972e031