ianstormtaylor / slate

A completely customizable framework for building rich text editors. (Currently in beta.)
http://slatejs.org
MIT License
29.71k stars 3.24k forks source link

moveNodes `to` wrong for later siblings? #5557

Open beorn opened 10 months ago

beorn commented 10 months ago

Description For moveNodes the to seems to be off when moving to a later sibling. (Probably issue with move_node too.)

Steps The given test passes, but it doesn't make sense.

/** @jsx jsx */
import { Transforms } from 'slate'
import { jsx } from '../../..'

export const input = (
  <editor>
    <block>1</block>
    <block>2</block>
    <block>3</block>
    <block>4</block>
  </editor>
)
const blockText = (n) => n.children && n.children[0].text || undefined
const match = (n, p) => ["1", "2"].includes(blockText(n))
export const run = editor => {
  Transforms.moveNodes(editor, { at: [], to: [2], match })
}
export const output = (
  <editor>
    <block>3</block>
    <block>1</block>
    <block>2</block>
    <block>4</block>
  </editor>
)

Expectation The path specified by to should presumably reference the position you want a node to be inserted at based on the slatejs node tree before the operation — so in this case nodes "1" and "2" should end up at path [2], which is right before "3". In other words, this should be a no-op operation, but instead the nodes are inserted after "3".

If you move to an earlier sibling then it works as expected — if you try to move "4" to [2] it ends up before "3".

Am I misunderstanding something?

Environment

RavenColEvol commented 2 weeks ago

@beorn in code i can see move_nodes for match are performed one by one. So since you are trying to move 1,2 at [2]

i can see once 1 move to [2] output will be 2,3,1,4 and same done for 2 at [2] will be 3, 1, 2, 4.

I don't know if it's expected but i'm just letting you know what's happening in code.

beorn commented 1 week ago

Yes, I understand this is the current logic, but I just think it is impossible to reason about except in the case where you're moving only one node. I think semantically it makes much more sense if moveNodes move all of the nodes to the position as it is before the operation starts; not where the position is for after each individual move, nor where the position happen to be at the end when all moves are complete — those are just impossible to reason about.

It should be a simple matter of moving 1 & 2 to position [2], which is between 2 & 3, so it ends up being a no-op.

In my case I'm doing drag & drop with multi-select, so 1 & 2 are selected, and when dropped right after 2 then 3 suddenly jumps in front of 1 & 2. It doesn't make sense.