Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
mvkampen opened this issue Sep 12, 2024 · 5 comments

Comments

@mvkampen
Copy link

mvkampen commented Sep 12, 2024

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

@adumesny
Copy link
Member

adumesny commented Sep 12, 2024

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

@mvkampen
Copy link
Author

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/

@adumesny
Copy link
Member

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

@mvkampen
Copy link
Author

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/

@adumesny
Copy link
Member

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants