#9036 closed bug (notabug)

Opened January 31, 2013 01:24PM UTC

Closed March 10, 2013 11:24PM UTC

Sortable: $.contains() give the wrong arguments.

Reported by: nyanhan Owned by: nyanhan
Priority: minor Milestone: none
Component: ui.sortable Version: 1.10.0
Keywords: haspatch Cc:
Blocked by: Blocking:
Description

fix a bug. $.contains() give the wrong arguments, on link 798

this bug case, connect parent error, and can't sort well in one container.

Attachments (0)
Change History (6)

Changed January 31, 2013 02:52PM UTC by tj.vantoll comment:1

owner: → nyanhan
status: new → pending

Hi nyanhan, thanks for taking the time to contribute to the jQuery UI project. The arguments do appear to be backwards on that line (https://github.com/jquery/jquery-ui/blob/master/ui/jquery.ui.sortable.js#L797).

But I'm unable to recreate the problem you're describing as nested connected sortables do appear to be ignored correctly - http://jsfiddle.net/tj_vantoll/gXNxq/.

Can you alter my test case to show the problem you're experiencing?

Thanks.

Changed February 01, 2013 02:35AM UTC by nyanhan comment:2

status: pending → new

Replying to [comment:1 tj.vantoll]:

Hi nyanhan, thanks for taking the time to contribute to the jQuery UI project. The arguments do appear to be backwards on that line (https://github.com/jquery/jquery-ui/blob/master/ui/jquery.ui.sortable.js#L797). But I'm unable to recreate the problem you're describing as nested connected sortables do appear to be ignored correctly - http://jsfiddle.net/tj_vantoll/gXNxq/. Can you alter my test case to show the problem you're experiencing? Thanks.

It will appear in this case. http://jsfiddle.net/nmnJC/

Thank you for your help.

Changed February 01, 2013 03:11AM UTC by tj.vantoll comment:3

status: new → open
summary: $.contains() give the wrong arguments. → Sortable: $.contains() give the wrong arguments.

Thanks I see the issue now. If 

connectWith
is removed in your test case the problem goes away.

Changed March 04, 2013 03:06PM UTC by tj.vantoll comment:4

keywords: → haspatch

Pending pull request https://github.com/jquery/jquery-ui/pull/904. This also might be addressed by the fix for #8792 (https://github.com/jquery/jquery-ui/pull/916).

Changed March 10, 2013 11:06PM UTC by mikesherov comment:5

http://jsfiddle.net/nmnJC/1/ show that the connectWith issue is fixed by https://github.com/jquery/jquery-ui/pull/916

Please submit a test case showing how $.contains argument order causes a problem. Thanks!

Changed March 10, 2013 11:24PM UTC by mikesherov comment:6

resolution: → notabug
status: open → closed

If you read the comment on https://github.com/jquery/jquery-ui/blob/83cbf979788f22ba3bd1668507623c0dd6b57041/ui/jquery.ui.sortable.js#L796, you'll notice that the contains statement is there to prevent a nonsensical edge case: the item contains its container.

For now, I'm going to close this as not a bug. If you have a test case that shows otherwise, I'll reopen. Thanks again!