Fix move_node behaviour in PathUtils.transform

[themithy]

Hi,

We have a fix for two bugs in move_node in path transform. The problem is that in some cases the path does not get transformed correctly, which leads to assertions being thrown.

The first bug exists because the check against op.newPath is done after the transformation against op.path. This leads to unexpected behaviour in scenario that the path is transformed in such a way that the second check matches, but should not.

The second problem is the check against path size which should be <= and not < in our opinion. After this fix, some test cases begin to throw, but this is because what we believe there is a problem in those testcases. If we have two siblings [0] and [1], and move the first to the path [1], the item should not move at all, because path [1] is before the second element. That is how we understand this, but however please confirm this finding.

Thanks in advance for reviewing it.

[ianstormtaylor]

@themithy thanks for this.

Can you remove the <= change? I believe that one is working as expected, since the paths are both framed in the context of the "before" state. (If not, we can fix it in a separate pull request.) Thanks.

[pitercl]

Hi @ianstormtaylor!

First of all, thanks for the awesome tool!

As for the <= vs <, I agree that we're working on the "before" state, but this is still a correct fix in my opinion.

Let's take the @themithy's example.

We have 2 nodes at paths [0] (let it contain a) and [1] (let it contain b). So, simplifying a bit, we have ab. On such tree, let's apply a move_node(p=[0], np=[2]) operation to get ba. Hopefully we agree that this is the correct behaviour - I will get back to this in a bit [*].

So if we were to transform the path [0] by such operation, it should be [1] (not [2] - we only have 2 nodes here).

Now, how will the transformPath behave?
We'll enter the first if: if (pAbove || pEqual), but will not get into the second: if (isYounger(p, np) && p.size < np.size) due to the < inequality. And we'll end up with [2] as a result, which is wrong (again - we only have 2 nodes in our tree).

[*] - getting back. I want to explain my thinking here in case you disagree. When working with Slate's operations I always assume that when inserting at a path [X], I'm actually inserting before the node that [X] points to - in particular the node can be non-existent.

Sorry for the lengthy explanation, hope this clarifies the fix a bit.

[ianstormtaylor]

@justinweiss could you double check this one? I can't remember what we decided on move_node operations and the before/after consideration for sibling nodes. I can't remember if the operation @pitercl is talking about should be [0] -> [2] or [0] -> [1]. Thanks!

[justinweiss]

Here's the discussion we had before: #2192

Like I said in there, I'm on a much older version, so this may not still be accurate. But the way I understood it, if you have:

transform(move_node [0] => [2], move_node [1] => [2])

The 0 => 2 will turn into 0 => 1. That is, [1] => [2] will swap the nodes at position 1 and 2, rather than being a no-op, as you might expect. A move to a sibling position is different than how a move works between trees.

After this fix, some test cases begin to throw, but this is because what we believe there is a problem in those testcases. If we have two siblings [0] and [1], and move the first to the path [1], the item should not move at all, because path [1] is before the second element.

So I think this is what you might expect, but doesn't match the way that Slate actually performs move_node operations, at least in version 0.33.

[jtadmor]

This is very interesting, I did not consider that interpretation of how a move_node should work when I made some of these changes.

If I have a at [0] and b at [1], and I say "move [0] to [1]", my gut tells me that I should have "ba". It also seems foreign to say that I need to "move [0] to [2]" in order to achieve that effect, since there is presently no [2] and there will be no [2] after the operation either. I am also a little uncomfortable with the notion that "move p to np" should be a no-op where p != np.

However, I do see that there are advantages to having a hard rule that a node is always inserted before a node that exists at new_path. My one concern would be that this is potentially a breaking change for any code that is manually moving nodes.

[jtadmor]

If I am reading this correctly, the way the old code functioned was that [0] -> [1] represented a swap:

https://github.com/ianstormtaylor/slate/blame/4d0ccc8e81e23f024c05e1af4b8cbdde721fbe38/packages/slate/src/interfaces/element.js#L1770

[ianstormtaylor]

Fixed by #3093.