Sortable Display Grid not working in 1.3

[jwgda]

https://jqueryui.com/sortable/#display-grid

No longer drops into place.

[mgol]

Thanks for the report. That's a pretty substantial bug, indeed.

[mgol]

I bisected it to c866e45. PR that introduced the regression: #1793.

[borgboyone]

I can take a look this evening. My suspect is that it's something very trivial.

[borgboyone]

FYI: If you change the CSS on the ul in the demo to {display;flex;flex-flow;row wrap;} it works just fine. Seems to have something to do with the floating perhaps.

[mgol]

Thanks, @borgboyone! Here's what I've already found: this line in _contactContainers:
https://github.com/jquery/jquery-ui/blob/1.13.0/ui/widgets/sortable.js#L1017
always evaluates to false in the demo; this is because the ul with no extra styles has zero height and so it doesn't encompass its children. Due to that, we never reach the line that sets innermostContainer:
https://github.com/jquery/jquery-ui/blob/1.13.0/ui/widgets/sortable.js#L1027
which in turn makes _mouseDrag skip a lot of its logic:
https://github.com/jquery/jquery-ui/blob/1.13.0/ui/widgets/sortable.js#L427

Since there was no requirement in 1.12 to have the parent encompass its children, it'd be good to preserve that.

Note that in 1.12 _contactContainers behaved in the same way, it's just that the dragging logic didn't depend on innermostContainer being non-null.

Hopefully you can find a solution out of that info. Thanks!

[borgboyone]

I've looked over the issue. Obviously containers that have zero height prevent the block encompassing L427 to kick in since the evaluation of _contactContainers indicates the dragged element is outside of containers. This is true for both 1.12.1 and 1.13.0. However, the logic starting at L247 runs regardless in 1.12.1. The intent was to prevent unnecessary processing of the intersection logic when outside of containers. The zero height for containers wasn't expected as a feasible option. Attempting to get the actual visible client height of such containers is highly problematic in itself (not to mention code cluttering).

In order to maintain backward compatibility, my recommendation is to remove L427 and L495 and move L424 and L425 to L496.

Regardless, be aware that container specific events will not fire when the dragged element is interacting with zero height containers.

Thoughts?
Anthony

[mgol]

@borgboyone Thanks for your analysis. I understand the case is a bit specific here but I expect this to be a common problem considering that even the jQuery UI website used a pattern that broke and it did so in one of the few demos it has for the Sortable widget. So maintaining backwards compatibility would be great here.

Would you like to submit a PR with a fix?

[borgboyone]

@mgol - Thank you, but I'm going to pass. I'm sorry that my contribution caused this issue.
Anthony

[mgol]

No problem. Thank you for your input into this issue!