gridstack.js icon indicating copy to clipboard operation
gridstack.js copied to clipboard

Vertical order is not respected when packing nodes, if space has become available after a swap.

Open mvkampen opened this issue 1 year ago • 5 comments

Subject of the issue

When attempting to move node 0 underneath node 3 in column 0. It seems to collide with both node 3 AND 4. When packing the nodes, it seems to fit node 4 first not respecting the current vertical order of the nodes. Resulting in an unexpected layout.

Your environment

  • Gridstack 10.1.2
  • MacOS Sonoma 14.6.1 - Firefox 129.0.2

Steps to reproduce

Drag node 0 down underneath node 3, see how node 4 jumps over node 3. This is because node 1 is not as tall as node 0 and 2. This indicates that a node may take up this space once the dragged node 0 is swapped with node 3 (snapped underneath) You will see that node 4 will jump unexpectedly ahead of node 3

https://jsfiddle.net/9ruhoabm/5/

Expected behavior

I would expect that the order of the nodes top to bottom would remain the same. As the vertical space between node 1 and node 3 is not large enough to fit widget 4. As node 0 is swapped, space has become available. When packing nodes we see that there is space to move node 3 up to the bottom of node 1. Node 4 should retain its position relative to node 3. (underneath, and remain in column 1)

Start situation Screenshot 2024-09-12 at 16 33 35

Resulting situation Screenshot 2024-09-12 at 16 34 52

Expected situation Screenshot 2024-09-12 at 16 34 34

mvkampen avatar Sep 12 '24 14:09 mvkampen

yep. collision (which I reworked in v4 a lot) is incredibly difficult turns out... not looking forward to figure this one out.

adumesny avatar Sep 12 '24 22:09 adumesny

I get it, after looking into the engine. Seeing 3 functions working recursively with intermittent mouse events is hard debugging. 😅 Appreciate your and others effort in the migration out of jQuery and moving to Typescript.

I just keep adding information to narrow it down:

Currently we are using gridstackjs 7.3.0 in production, where I cannot reproduce this issue. I was trying to debug some other issue there. So we thought to migrate to a newer version, to see if that would have an impact. Then I found this issue.

Seems to be introduced in version 8.

Can reproduce with: 9.5.1 https://jsfiddle.net/qak4t9rz/ 8.4.0 https://jsfiddle.net/qak4t9rz/1/

mvkampen avatar Sep 13 '24 10:09 mvkampen

Narrowing it down to an exact rev would help diff and see what might be the cause.

adumesny avatar Sep 17 '24 02:09 adumesny

While I tried to reproduce this with version 7.3.0 as we currently use in production. I noticed i could not reproduce the production behavior with the current example. Comparing with production situation, i noticed that over there node 4 has the same size as node 0 from the example.

When I increased the height of node 4 in the example, I did get the expected behavior. The height of node 4 affects where node 4 will be placed by the algorithm. Even when node 4 is slightly smaller then node 0, it would still place it as expected. As I reduce node 4 height to be equal as node 1 above, then you see it will compact node 4 between node 1 and 3 vertically, even tho node 4 does not fit the space between 1 and 3.

Expected behavior would be that node 3 will move up against node 1, and node 4 remains relative to node 3. I would not expect that the height of node 4 relative to the height of node 1, would have an impact on where node 3 and node 4 positions are compacted to, other then that node 3 is positioned vertically against node 1.

Since you mentioned V4 I also tested that, but the behavior seems consistent among the versions. 7.3.0 https://jsfiddle.net/rs1bpm75/ 4.4.1 https://jsfiddle.net/rs1bpm75/2/

I added console.log information on changed nodes, to see if it gave unexpected node information, but does not seem the case. (We use this to update node positions in the database) https://jsfiddle.net/0mt8d14q/12/

mvkampen avatar Oct 15 '24 14:10 mvkampen

to be clear, finding the exact rev when that started breaking would help in me deciding to take a look. Make sure it still na issue with the latest first (no point in wasting time if fixed now).

adumesny avatar Oct 15 '24 17:10 adumesny

closing since no update on specific rev that introduced issue to narrow it down...

adumesny avatar Aug 09 '25 16:08 adumesny