Slate: Error when trying to delete selection of list item to paragraph

[ExplicitlyImplicit]

Do you want to request a feature or report a bug?

Report a bug.

What's the current behavior?

1. Make a selection from or to the end of a list item (except the last list item) to a paragraph
2. Hit delete/backspace
3. An error is thrown (Could not find a node with key X.) and the selection is not removed

Observed on the Rich Text Slate Example:

Tested with Slate 0.20.3 in latest Chrome (59.0.3071.115) on Windows 7 and 10 (64-bit).

What's the expected behavior?

The selected text should be deleted.

[ms88privat]

Yes, easily reproducible. Thanks for pointing this one out. We found quite a lot of these lately :(

[ExplicitlyImplicit]

It _looks_ like the bug originates from splitting nested blocks where focus is at the end of a block. When I split a regular paragraph it results in a new empty paragraph below but when i split a list-item it results in an empty list-item below that contains the key of the next list-item (the next list-item, which was not part of the split, gets a new key).

[ExplicitlyImplicit]

For what it's worth, I'll publish my findings here just in case.

Deleting a selection consists of three steps:

1. Splitting the start and end nodes (of the selection).
2. Removing all nodes between the start and end node, including the second part of the start node and the first part of the end node.
3. Moving all of the nodes in the second part of the end node into the first part of the start node.

Now, the split function (simplified) takes the following input:

• A node which should be split
• An offset that denotes where the split should take place inside the node

Consider these two examples presented in HTML and using [] to denote the cursor/selection.

Example 1.

<ul>
  <li>item1[]</li>
  <li>item2</li>
</ul>

Example 2.

<ul>
  <li>item1</li>
  <li>[]item2</li>
</ul>

Both these examples, when splitting on the ul, will have an offset of 5. When we try to split a list like the one above and using the offset 5, slate will perform a split as in example 2. This means that, after the split, the resulting state will look like the following:

<ul>
  <li>item1</li>
  <li></li>
</ul>
<ul>
  <li>item2</li>
</ul>

So item2 was split at the beginning of its text resulting in an empty list item in the first list.

Lets say we are performing a delete on a similar document, adding a paragraph above the list and denoting [ and ] as the beginning and end of the selection:

<p>te[xt</p>
<ul>
  <li>item1]</li>
  <li>item2</li>
</ul>

Performing step 1 (splitting start and end node) gets us:

<p>te</p>
<p>xt</p>
<ul>
  <li>item1</li>
  <li></li>
</ul>
<ul>
  <li>item2</li>
</ul>

I'll call the first list item (item1) _part 1_ of the split end node and the empty list item _part 2_ of the split end node.

Performing step 2 (removing all nodes between start and end node) we notice one issue with _how_ the delete is performed.

The delete assumes we can remove everything after the start node up to (and including) the end node (since we just split them and its the second block in a split that gets a new key) so it deletes the second paragraph and the first list, including the empty list item (which would be the second part of the split end node).

When we later want to perform step 3 (moving all of the nodes in the second part of the end node into the first part of the start node) we will get an error since we've already deleted the second part of the end node (the empty list item) in step 2.

Feel free to comment if I've misunderstood something or need to clarify anything.

[ExplicitlyImplicit]

This is a bit larger problem than just the issue described above. It boils down to how offsets are used to resolve node operations:

<ul>
  <li>item1</li>
  <li>item2</li>
</ul>

What if I invoke node.getTextAtOffset on the ul-node and specify an offset of 5? I will get the text node inside the second list-item. If I wanted the first list-item I'm out of luck since we cannot express the end of a node using an offset from a parent node (that has more nodes inside it).

I have solved this problem by adding a parameter denoting how node resolution should work when offsets are 'between' nodes but I don't feel like it is a good approach since it is such a big step away from how it currently works with paths and offsets.

If anyone have any input or experience with this issue I'll be happy to discuss it.

[ExplicitlyImplicit]

This problem is also causing #883 and probably more since offsets are a big part of the inner and outer workings of slate.

[ianstormtaylor]

@ExplicitlyImplicit thank you very much for writing that up. The way offsets are used right now is clearly a problem. Do you see this as only a real problem in the way the split/join operations are using the offsets?

I have been thinking that the current split/join operations aren't ideal. Instead of trying to create a single operation that knows how to traverse down or up the tree, we should instead just have the transforms create all of the subsequent operations that are needed. Since the transforms know the exact position that needs to be split, then we won't have this arbitrary place where we lose information, since they'll just create n number of split_node transforms that traverse up the tree, splitting all of the necessary nodes.

Does that make sense?

[ExplicitlyImplicit]

@ianstormtaylor thank you for the response! Yes, that would solve the bugs in the split function and fixing it at a higher level might be enough to keep this issue contained. I'm still worried though that it will introduce/still exist more bugs as developers might easily miss this caveat.

The main problem is that an offset is not enough to express a position in a document/fragment with nested nodes. So there is quite a big gotcha when using node.getTextAtOffset since you might not get the text node you are thinking about. If the current split operation got the correct node from getTextAtOffset it would actually split correctly.

I have not been able to assess the extent of this problem though (I would have to go through everywhere offsets are used and see _how_ it is used) so I don't know if it is worth doing anything more.

[ianstormtaylor]

Very true, the getTextAtOffset is inherently buggy. As long as we restricted offset use to only be used when relative to a Text node we should be fine.

[ExplicitlyImplicit]

Agreed, from what I've assessed, I see two necessary steps:

• Remove getTextAtOffset
• Remove special cases where a selections anchor and/or focus node is not a text node

It would probably be a good idea to also validate a selection beforehand or at least document it more so developers don't accidentally use offsets outside of a text node (since it might look like it works but bugs might creep up on them eventually).

[ianstormtaylor]

We should already be only allowing selections to reference text nodes, but there may be gaps I don't know about for sure. Your plan sounds good!

[oyeanuj]

@ExplicitlyImplicit @ianstormtaylor Have you guys used a workaround here until the fixes are made? Running into what seems like a related issue.

[ianstormtaylor]

On the #836 branch, I've updated the split_node and merge_node (previously join_node) transforms/operations to not actually use deep offsets and deep splitting. The transforms now directly reference the text node in question, and the operations are recursive. This probably helps things a lot, but I haven't looked again in the context of this issue.