BUGFIX: Insert copied or moved nodes in order of selection

[Sebobo]

This reverses the order of nodes for insertAfter (copy & move) operations to prevent the reverse order of insertion due to the way the atomic inserts are done by the change operations.

Also turns insertInto internally into a insertAfter if the selected target parent node already has children. Here the reversion is not necessary as always the last child node is picked to "insert after" and therefore keeping the order of the users selection (clipboard).

Resolves: #3040

[Sebobo]

I keep this for now as draft, as I have to check the move operations too.

[neos-bot]

🎥 End-to-End Test Recordings

• Recording 1
• Recording 2

These videos demonstrate the end-to-end tests for the changes in this pull request.

[Sebobo]

moveInto doesn't have this issue, as it internally always sets the inserted node to POSITION_LAST

Now debatable why copyInto doesn't do this 🤔

[Sebobo]

This is not an elegant change, but I'm out of ideas right now how to do it better without a larger code change that would introduce "batch changes" which carry more information about the ordering of elements.

[mhsdesign]

Okay i tested now without this fix first and that were my results: https://github.com/neos/neos-ui/issues/3040#issuecomment-1931468038

interaction	8.3	paste-in-order
selection top -> bottom + insert after	❌(reversed)	✅
selection top -> bottom + insert before	✅	✅
selection bottom -> top + insert after	✅	❌ (random?)
selection bottom -> top + insert before	❌ (reversed)	❌ (random?)
selection random + insert after	❌ (order reversed)	🟠 (order)
selection random + insert before	🟠 (order)	🟠 (order)

✅ = expected order like the nodes arranged in the ui 🟠 = order like nodes selected in the neos ui, marked as orange as its not expected that the application will actually remember this.

So the problem with this fix is, the workaround that is currently used to select the nodes from bottom to top will be now reserved which is also mindblowing?

[Sebobo]

@mhsdesign thx for the thorough testing! Will check the two random cases again. I thought I had verified them too.

I wouldn't do anything about the clipboard order issue as this would be a larger change we can check separately if we can implement this as a fix or feature for future Neos or keep it like it is.

[mhsdesign]

We found out that my testing uncovered another problem. Shift click and command click behave different

[Sebobo]

OK after a screen share session we found out that there is a difference behaviour on how items are added to the clipboard depending whether one uses Command+Select or Shift+Select. The later seems to add them in the incorrect order to the clipboard and therefore the resulting result of the insert operation seems again random.

[Sebobo]

@mhsdesign my latest change fixes that RangeSelect behaviour

[grebaldi]

EDIT: The descriptions in this comment are based on false assumptions about the requirements in this PR. For clarification see: https://github.com/neos/neos-ui/pull/3708#issuecomment-1943468486

Hi @Sebobo,

I have tried to compile some E2E tests for this problem. The original issue #3040 focuses on move and copy operations for content nodes. I based my tests on the existing tests for the document tree, but I expect this to translate well to the content tree. This way I was able to to write up a comprehensive set of cases.

Since this problem space has a couple of dimensions, I ended up with 45 different scenarios. Those test cases alone run for quite a while (~8min on my machine), which is - amongst some other, minor problems - why I didn't push the test code right away.

Nevertheless, I still wanted to share my findings. Since there are lots of parameters, I'll start by describing some background assumptions for the tests:

Let's suppose, we have the following simplified document tree:

🗋 Parent
├─ 🗋 Page A
├─ 🗋 Page B
├─ 🗋 Page C
└─ 🗋 Page D

For the first part of the test matrix, we need to distinguish between two methods of selecting multiple nodes (CMD-Click and SHIFT-Click), both of which allow for different orders of selection (as identified by @mhsdesign: top-down, bottom-up, random):

• CMD-Click - We select nodes by clicking on each node individually while holding the CMD(or Ctrl)-key
  • top-down - We select first 🗋 Page A, then 🗋 Page B, then 🗋 Page C
  • bottom-up - We select first 🗋 Page C, then 🗋 Page B, then 🗋 Page A
  • random - We select first 🗋 Page B, then 🗋 Page C, then 🗋 Page A
• SHIFT-Click - We select a range of nodes by clicking on a start node and then, while holding the SHIFT-key, we click an end node
  • top-down - We click first on 🗋 Page A, then 🗋 Page C
  • bottom-up - We click first on 🗋 Page C, then 🗋 Page A

The second part of the test matrix concerns the operations a user can perform with a selection of multiple nodes. Those are basically Move and Copy. Both operations happen in an "insert mode" (AFTER, BEFORE, INTO), which results in a total of 6 possible operations.

The following tables illustrate what the resulting page tree (given the tree from above) is expected to look like after each operation:

Scenario: Move

AFTER	BEFORE	INTO
🗋 Parent ├─ 🗋 Page D ├─ ❙🗋 Page A ├─ ❙🗋 Page B └─ ❙🗋 Page C	🗋 Parent ├─ ❙🗋 Page B ├─ ❙🗋 Page C ├─ ❙🗋 Page D └─ 🗋 Page A	🗋 Parent └─ ❙🗋 Page D ├─ ❙🗋 Page A ├─ ❙🗋 Page B └─ ❙🗋 Page C
We have moved 🗋 Page A..C AFTER 🗋 Page D	We have moved 🗋 Page B..D BEFORE 🗋 Page A	We have moved 🗋 Page A..C INTO 🗋 Page D

Scenario: Copy

AFTER	BEFORE	INTO
🗋 Parent ├─ 🗋 Page A ├─ 🗋 Page B ├─ 🗋 Page C ├─ 🗋 Page D ├─ ❙🗋 Page A ├─ ❙🗋 Page B └─ ❙🗋 Page C	🗋 Parent ├─ ❙🗋 Page B ├─ ❙🗋 Page C ├─ ❙🗋 Page D ├─ 🗋 Page A ├─ 🗋 Page B ├─ 🗋 Page C └─ 🗋 Page D	🗋 Parent ├─ 🗋 Page A ├─ 🗋 Page B ├─ 🗋 Page C └─ ❙🗋 Page D ├─ ❙🗋 Page A ├─ ❙🗋 Page B └─ ❙🗋 Page C
We have copied 🗋 Page A..C AFTER 🗋 Page D	We have copied 🗋 Page B..D BEFORE 🗋 Page A	We have copied 🗋 Page A..C INTO 🗋 Page D

Based on these assumptions, I ran my 45 test cases on both the 8.3 branch and the PR-branch bugfix/3040-paste-in-order. The following tables show the respective test results. ✅ means that the resulting page tree meets the expectation from above, ❌ means that it doesn't.

Test results on branch 8.3 (before the fix)

		Drag & Drop			Cut & Paste			Copy & Paste
		AFTER	BEFORE	INTO	AFTER	BEFORE	INTO	AFTER	BEFORE	INTO
CMD-Click	top-down	❌	✅	✅	❌	✅	✅	❌	✅	✅
	bottom-up	✅	❌	❌	✅	❌	❌	✅	❌	✅
	random	❌	❌	❌	❌	❌	❌	❌	❌	✅
SHIFT-Click	top-down	❌	✅	✅	❌	✅	✅	❌	✅	✅
	bottom-up	❌	❌	❌	❌	❌	❌	❌	❌	✅

Test results on branch bugfix/3040-paste-in-order (after the fix)

		Drag & Drop			Cut & Paste			Copy & Paste
		AFTER	BEFORE	INTO	AFTER	BEFORE	INTO	AFTER	BEFORE	INTO
CMD-Click	top-down	❌	✅	✅	✅	✅	✅	✅	✅	✅
	bottom-up	✅	❌	❌	❌	❌	❌	❌	❌	❌
	random	❌	❌	❌	❌	❌	❌	❌	❌	❌
SHIFT-Click	top-down	❌	✅	✅	✅	✅	✅	✅	✅	✅
	bottom-up	✅	❌	❌	❌	❌	❌	❌	❌	❌

---

Both test runs result in 27 cases failing (though different cases fail).

I'm not sure whether it would be feasible to fix all those cases. If so, I'm also not sure whether it would be wise to add all cases to our test suite - because that's +8min on all CI runs :/

But I hope that I could give some insight :sweat_smile:

(I will share the test code as well of course - I'll probably open a separate PR, so it doesn't clutter up this one)

[Sebobo]

Hi @grebaldi,

wow, thx for the thorough test!

The intention of the PR is actually for the bottom-to-top case to also reflect the bottom-to-top order and not invert it to top-to-bottom. Of course this is debatable, but I somehow preferred keeping the order of selection.

This should make most cases green except the random one. We. An only make the whole thing more sane if we introduce a new batch operation instead of the atomic operations which simply cannot produce something plausible as the UI needs to know the quirks of the backend right now. When the nodes are not on the same level, we are also out of luck to find any useful order except the order of selection.

[markusguenther]

wow, thx for the thorough test!

That was exactly the same thought when I saw the e-mail yesterday in the evening. Just WOW, and thank you @grebaldi for all your detailed descriptions and comments. You are so valuable and awesome :)

[mhsdesign]

Yes hats of to your testings @grebaldi :D Its very unfortunate that the test runs take so long and force us to strip them down. But thank you for writing them as we currently have no tests for batch moving/copy i assume? It would be great to have the happy cases (top-down) running in ci i think.

Also it seems your tests uncovered that Drag & Drop after does not behave as expected, so it would be great to have that corrected, that all the happy cases work?

[grebaldi]

@Sebobo:

The intention of the PR is actually for the bottom-to-top case to also reflect the bottom-to-top order and not invert it to top-to-bottom. Of course this is debatable, but I somehow preferred keeping the order of selection.

Whoops, how did I miss that? Sorry for the confusion, I should have guessed the intention from the title of this PR :facepalm:

Well, my intuition went exactly the other way :sweat_smile:, but I understand the technical limitations of course.

So, just to clearify the expected behavior given the tree:

🗋 Parent
├─ 🗋 Page A
├─ 🗋 Page B
├─ 🗋 Page C
└─ 🗋 Page D

If we CMD-Click-select first 🗋 Page B, then 🗋 Page C, then 🗋 Page A, and then drag all three nodes into 🗋 Page D, the resulting tree is expected to look like this?:

🗋 Parent
└─ ❙🗋 Page D
    ├─ ❙🗋 Page B
    ├─ ❙🗋 Page C
    └─ ❙🗋 Page A

(With all other cases resulting in an analogous order?)

If so, I will adjust my test cases accordingly and report back :)

@mhsdesign:

[...] we currently have no tests for batch moving/copy i assume?

Well, there's treeMultiselect.e2e.js, but it doesn't assert the resulting order of nodes.

Its very unfortunate that the test runs take so long and force us to strip them down.

It may be possible to run some of the tests conditionally. I believe we can tell Circle CI / Github Actions to filter the tests based on whether a specific label is set on the PR. Or maybe we could run specific tests by prompting some bot? Don't know exactly how to set up something like that, but it would allow us to keep the average push/PR test-runs small and only run more thourough tests when they're actually needed. For the release branches, we could run the entire, much more comprehensive test suite which would greatly increase our certainty.

[Sebobo]

If we CMD-Click-select first 🗋 Page B, then 🗋 Page C, then 🗋 Page A, and then drag all three nodes into 🗋 Page D, the resulting tree is expected to look like this?:

Yes

[grebaldi]

Here are the updated test results:

Test results on branch 8.3 (before the fix)

		Drag & Drop			Cut & Paste			Copy & Paste
		AFTER	BEFORE	INTO	AFTER	BEFORE	INTO	AFTER	BEFORE	INTO
CMD-Click	top-down	❌	✅	✅	❌	✅	✅	❌	✅	✅
	bottom-up	❌	✅	✅	❌	✅	✅	❌	✅	❌
	random	❌	✅	❌	❌	✅	❌	❌	✅	❌
SHIFT-Click	top-down	❌	✅	✅	❌	✅	✅	❌	✅	✅
	bottom-up	❌	❌	❌	❌	❌	❌	❌	❌	❌

Test results on branch bugfix/3040-paste-in-order (after the fix)

		Drag & Drop			Cut & Paste			Copy & Paste
		AFTER	BEFORE	INTO	AFTER	BEFORE	INTO	AFTER	BEFORE	INTO
CMD-Click	top-down	❌	❌	✅	✅	✅	✅	✅	✅	✅
	bottom-up	❌	❌	✅	✅	✅	✅	✅	✅	✅
	random	❌	❌	❌	✅	✅	❌	✅	✅	✅
SHIFT-Click	top-down	❌	❌	✅	✅	✅	✅	✅	✅	✅
	bottom-up	❌	❌	✅	✅	✅	✅	✅	✅	✅

---

There are only 12 remaining failures. Almost the entire Drag & Drop - BEFORE-column appears to consist of regressions.

[Sebobo]

@grebaldi thx for the update :)

I will check on the drag & drop, I didn't even expect it to behave different so I didn't test it before 😔

[Sebobo]

@grebaldi I finally got to make the adjustment for the drag & drop part. With that The remaining red crosses are checks for me. Though I couldn't reproduce the issue you had with the "before" and "into" column which both worked fine for me, of course under consideration that the nodes should be inserted in the order of selection.