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

[skogsmaskin]

EDIT: previously this was only an issue when the last block ended in a inline void node. Now it is always an issue.

Do you want to request a _feature_ or report a _bug_?

Bug

What's the current behavior?

If you have three blocks or more, where the last block ends with an inline void node, and you select everything and hit backspace (delete), it will fail with error:

Uncaught Error: `Node.assertNode` could not find node with path or key: List [ 1 ]

Reproduction:

1) Goto the emoji-example
2) Add an emoji at the end of the last block.
3) Select everything (command/ctrl-A)
3) Hit backspace

What's the expected behavior?

That everything will be deleted without error.

I have noticed that when deleting all (and you have three or more blocks) the change will include an operation with type move_node where the path property is a block path (i.ie [2] with the same value as the newPath prop [2]. I think Slate will regenerate keys for this operation and it may lead to issues.

[Slapbox]

I've narrowed down where this error occurs to at least here: https://github.com/ianstormtaylor/slate/blob/master/packages/slate/src/commands/at-range.js#L238-L242

The error itself seems likely to be in mergeNodeByKey

[Slapbox]

More specifically it fails on a second pass through here:

https://github.com/ianstormtaylor/slate/blob/master/packages/slate/src/commands/by-path.js#L170-L183

Error is: `Node.assertNode could not find node with path or key: List [ 1 ]`

Here's what the editor looks like when producing this:

[skogsmaskin]

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

[skogsmaskin]

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).

[Slapbox]

Almost definitely is not #2290 because I implemented the inverse back in August. It's likely related to the normalization changes. Poking around now.

[Slapbox]

@skogsmaskin, can you change the issue title to match the new symptoms?

---

@ianstormtaylor @ericedem - Unfortunately this is due to https://github.com/ianstormtaylor/slate/pull/2316

Stepping back to one commit prior, the issue vanishes.

---

Full list of known symptoms:

This issue affects selections of 3 or more blocks, which include the last block in the document AND the last block contains at least one character.

• Cannot delete or backspace
• Cannot cut text
• Cannot paste over text

Any number of nodes can be selected/modified if they do not include the final block of the document.

[skogsmaskin]

I'm not quite sure if it is due to #2316 or we just see the changes in #2316 make an undiscovered bug (like in .deleteAtRange or in withoutNormalization) play out differently.

[skogsmaskin]

Found the issue I think, in mergeNodeByKeythe endBlock.key is not right. It is the key of the endBlock before we started the whole delete process. It should be the key of what is now the endBlock.

As I said first in this issue:

I have noticed that when deleting all (and you have three or more blocks) the change will include an operation with type move_node where the path property is a block path (i.ie [2] with the same value as the newPath prop [2]. I think Slate will regenerate keys for this operation and it may lead to issues.

EDIT: updated the comment, got it wrong written out the first time.
EDIT2: Meh, sorry. Ignore what I said. I read my JSON.stringify wrong.

[Slapbox]

Following @skogsmaskin lead, the furthest I could trace this was to https://github.com/ianstormtaylor/slate/blob/master/packages/slate/src/commands/at-range.js#L241 a couple lines later. It fails after this point, but where is mergeNodeByKey defined?

This all occurs after the changes from #2316, so I'm also inclined to think that fixing the schema uncovered some downstream issue.

The only function definition I could find was in a test:

[ericedem]

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

[Slapbox]

@ericedem thanks! I know in at least one function I saw there was an explicit conversion from path to key, so I figured that was done everywhere. Aliasing makes a lot more sense though.

[Slapbox]

I traced it to here, but I'm too burned out to figure out any more than that right now https://github.com/ianstormtaylor/slate/blob/master/packages/slate/src/models/node.js#L89

On one call of this it gets no data and no type which I imagine is causing it, but maybe not.

[skogsmaskin]

If we look at the old code, before the withoutNormalizing helper, it did some changes without normalizing and some with:

https://github.com/ianstormtaylor/slate/blob/57138de130b1e9ca7fdada35dda76aef90777543/packages/slate/src/changes/at-range.js#L73

Now everything is done withoutNormalizing, but no changes to the code elsewhere as far as I can tell.

[ericedem]

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.

[ianstormtaylor]

What order are the dirty operations being added in? The transform logic should kick in to remove dirty paths implicated in a remove_node operation, but maybe there's a bug in that part of the code.

[ericedem]

@ianstormtaylor It looks like the top level blocks are being removed first, then it iterates through the text nodes and removes those, adding the parents that have been removed to the list of dirty paths.

[Slapbox]

Could fixing this be as simple as reversing the order of the items in the array in the changes made yesterday?

If there is some logic that should manage nodes implicated by remove_node, could it be as simple as changing:

return [...ancestors, path, ...paths]

to

return [path, ...paths, ...ancestors]

?

[ericedem]

Ok tracing through a bit more, looks like this is happening:

1. The middle nodes get deleted: This adds top level normalization for the document, this marks [] as dirty.
2. The "overlapping" text of the start block and end block gets removed: This marks [ 0 ] and [ 1 ] as dirty.
3. The nodes are merged together.

[ianstormtaylor]

@ericedem why would it remove the parent node and then iterate the text nodes to remove them too? Wouldn't removing the parent imply the latter?

[ericedem]

Sorry I was misunderstanding the code, the middle blocks are removed, then some of the text from the first and last blocks are removed, then those blocks are merged together.

[Slapbox]

The answer to my question is no.

[Slapbox]

@ericedem @ianstormtaylor

On the surface, changing to the following fixes it. Unsure of potential unforeseen consequences.

    case 'remove_text':
      return [path]

    case 'add_mark':
    case 'insert_text':
    case 'remove_mark':
    case 'set_mark':
    case 'set_node': {
      const ancestors = PathUtils.getAncestors(path).toArray()
      return [...ancestors, path]
    }

Changing that case brings us back to the original behavior of only documents ending in inline nodes failing to delete, from what I can see.

[ericedem]

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.

[skogsmaskin]

Besides this normalization issue here, is am wondering if Slate should do anything at all when we get a moveNode operation where the path equals the newPath like I mentioned in the OP. I have noticed that slate-react will regenerate keys for the block on such an operation, and for us that leads to issues futher down the line. I have worked around this by intercepting those operations with a plugin though (this is because we rely on predictable keys in our custom implementation). And this operation seems more to be a side-effect of normalization than something that was actually commanded / needed.

Or maybe everything is this issue all together and that kind of operation is never produced anywhere else in the code.

[ericedem]

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.

[skogsmaskin]

Seems like this will fix it:

const npEqual = isEqual(np, path)
if (npEqual) {
  return List([path])
}

All my tests pass now (also the added failing tests).

[skogsmaskin]

Thanks for tracking that line down @ericedem!

Took the liberty to create a PR as I had a test and suggested code ready. Besides, I'm falling behind on my PR-contributions here ;)

Please review.

[ianstormtaylor]

@ericedem @skogsmaskin I should have made the variables there more clear, but can you comment on whether you are talking about when...

operation.path === operation.newPath

Or when...

dirtyPath === operation.newPath

Because I'd understand not ever creating operations in the former case. But for the latter case, won't that be a reasonable operation that should indeed increment the dirty path?

[skogsmaskin]

@ianstormtaylor

https://github.com/ianstormtaylor/slate/pull/2324/files#diff-7e5c3b562666e6718badd20baadba246L344

[ianstormtaylor]

@skogsmaskin yeah I see that, but without extra information I don't understand why that change is the way it is. Although I do see that it makes that test pass.

[skogsmaskin]

@ianstormtaylor - when trying to move a block with path [1] to new path [1], the resulting path should be the same no? It should be concidered a NOOP?

[ianstormtaylor]

But I don't think that's what's happening at that line. In the codebase...

dirtyPath: path
operation.path: p
operation.newPath: np

So that line is saying:

if (isEqual(dirtyPath, operation.newPath))

And not:

if (isEqual(operation.path, operation.newPath))

Unless I'm misunderstanding something.

[ianstormtaylor]

Could one of you print out the specific dirtyPath and operation we're talking about? That might help. My guess is that it is an edge-case that occurs only when npEqual === true, but not that should be done for all cases? (Based on my current understanding at least.)

[skogsmaskin]

@ianstormtaylor I was talking about operation.path === operation.newPath. Can't see where the dirtyPath comes in at this point in the code, other than the code dealing with dirty paths would get the wrong path from the applied operation?

[ianstormtaylor]

The PathUtils.transform function is used to transform any existing "dirty" paths that are marked for normalization as new operations come in. That way we can keep the path reference correct, even if new nodes are inserted/removed/moved that would make it invalid.

https://github.com/ianstormtaylor/slate/blob/4e906b3be2c9c2cd2512cee063940a039ef994c0/packages/slate/src/utils/path-utils.js#L283-L284

So in this case, apparently there is a previous "dirty" path that was waiting to be normalized. But a new move_node operation is received, which happens to have an operation.newPath === dirtyPath.

Usually I'd think that that means we should increment dirtyPath, since it would have had another node moved directly in front of it. But it sounds like there's an edge case there that we aren't accounting for.

(However, separately... I completely agree with you that move_node operations that have path === newPath should never be created in the first place. We should check for this in moveNodeByPath I think, and abort if they're not actually moving it.)

[Slapbox]

I've gotten too busy to follow along closely today, but my question is, do we expect the potential issues/changes identified to also fix the issue of being unable to delete when the last node is an inline? (which was what this issue was originally about until last night when it expanded in scope.)

[skogsmaskin]

@ianstormtaylor: this is how it looks without the modified code:

{ object: 'operation',
  type: 'move_node',
  path: [ 1 ],
  newPath: [ 1 ] }
Operation path List [ 1 ]
Operation new path List [ 1 ]
Transformed path: List [ List [ 2 ] ]

(Not sure if this is what you asked for)

[ianstormtaylor]

@skogsmaskin thank you! That's super helpful. Is that Transformed path: the output?

If so, it sounds like it is failing on the case where path === newPath === dirtyPath, which is partly because the move_node operation should never have been created in the first place.

But it does sound like a good idea to handle the edge case that the move_node operation occurs, so we'd need to do:

if (isEqual(p, np)) {
  return List([path])
}

And then keep the other logic as it was I believe.

(And then separately we can make a fix to not be generating those unnecessary move operations in the first place, to improve performance.)

[skogsmaskin]

Yes Transformed path:is the output.

I'm getting the same results with this btw:
if (pEqual && npEqual && isEqual(p, np)) { return List([p]) }
It's even more specific, but if you're sure it's only the isEqual(p, np) we are looking for?

[ianstormtaylor]

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

[skogsmaskin]

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! <3

[dminkovsky]

@skogsmaskin, can you change the issue title to match the new symptoms?

[ ... ]

Full list of known symptoms:

This issue affects selections of 3 or more blocks, which include the last block in the document AND the last block contains at least one character.

• Cannot delete or backspace
• Cannot cut text
• Cannot paste over text

Any number of nodes can be selected/modified if they do not include the final block of the document.

I am experiencing exactly this behavior with slate 0.47.8, slate-react 0.22.9 and react 16.9.0. I was experiencing it on 0.47.7/0.22.8/16.8.0 as well. Oddly I am not able to replicate the behavior using the example site.

---

The error is

slate.es.js?d1a0:7195 Uncaught Error: Unable to merge node with path "List [ 0 ]", because it has no previous sibling.
    at Commands$2.mergeNodeByPath (slate.es.js?d1a0:7195)
    at Editor.command (slate.es.js?d1a0:10924)
    at Editor$$1.command (slate-react.es.js?028a:5886)
    at Editor$$1.command (react-hot-loader.development.js?c2cb:667)
    at onCommand (slate.es.js?d1a0:5219)
    at next (slate.es.js?d1a0:11125)
    at onCommand (slate.es.js?d1a0:5218)
    at next (slate.es.js?d1a0:11125)
    at onCommand (slate.es.js?d1a0:5218)
    at next (slate.es.js?d1a0:11125)

---

-    "immutable": "^4.0.0-rc.12",
+    "immutable": "^3.8.1",

fixed this issue, and other issues I was having. Should I open a new ticket?