Opened 2 years ago

Closed 11 months ago

Last modified 11 months ago

#10727 closed bug (fixed)

Sortable: placeholder offset incorrect

Reported by: dstrohl Owned by: dstrohl
Priority: minor Milestone: 1.12.0
Component: ui.sortable Version: 1.11.2
Keywords: Cc:
Blocked by: Blocking:

Description

In trying to troubleshoot a bug in a page I may have found a bug in jQueryUI.Sortable. When I try to drop an object onto a sortable object, it will often drop higher in the stack than it should. (I am holding the item under all of the items in the stack, but the placeholder is showing up at the top of the stack.

In troubleshooting this, I added some console output lines to the jqueryui code and found that it seems like when it is comparing the position of the event to the position of the item (around line 14330 or so in my copy) it is using the event[clientx/y] position compared to the item.offset() position. if I am reading the docs correctly, the clientX/Y position is based on the offset from the current window, and the item.offset() is based on the offset of the document (is that correct?) This seems like it might be valid as by moving the window up and down relative to the document, the placement of the placeholder changes.

Since I am not an expert in JS or JQuery, it is entirely possible that I am missing something here, or have mis-read the code or docs...

This seems like it might be a dupe of several other reported bugs, but none of them seemed to indicate that the problem had to do with the offset position.

I also verified that this happens with versions 1.10.3, 1.10.4, 1.11.1, and 1.11.2

I apologize for not creating a jfiddle for this, hopefully the description is good enough.. if not, I can see what I can hack together.

Change History (9)

comment:1 Changed 2 years ago by dstrohl

Darn it, I created it in the wrong place, I tried to change it to ui.sortable but dont see where I can do that. another thing to apologize for.

comment:2 Changed 2 years ago by tj.vantoll

  • Owner set to dstrohl
  • Status changed from new to pending

Hi dstrohl,

Unfortunately we do need a reduced test case in order to look into this. You can use this as a starting point: http://jsfiddle.net/tj_vantoll/fwk8R/. If you need support please try our forums or Stack Overflow.

comment:3 Changed 2 years ago by dstrohl

  • Status changed from pending to new

No problem, I understand (I was hoping... but I get it <grin>);

so, here is a jsfiddle of the problem: http://jsfiddle.net/dstrohl/17n55z5u/7/

Most of this is based on one fo the sortable examples from the main site.

So, after going to the example, the top sortable works as expected, but the bottom one (you may have to scroll down) does not. *(Make sure to have the window SMALL enough to require scrolling, it works as expected when you can see both).

I also put some basic instrumentation in to show my theory, based on what I could get from the jQueryUI code, it is comparing the clientX/Y to the item offset, so I am showing client X/Y and offset top/left in a box on the side.

You can see that the client x/y numbers are generally similar for both sortables, but the targetloc (from the ui.item.offset) is significantly different between the two.

Does this help?

comment:4 Changed 2 years ago by dstrohl

(by the main site, I meant the jQuery main demo site)

comment:5 Changed 2 years ago by dstrohl

Per the instructions, I tried creating a fix for this after forking the repo. (https://github.com/dstrohl/jquery-ui/blob/master/ui/sortable.js). Basically I just changed:

axix = floating ? "clientX" : "clientY"; 

to

axix = floating ? "pageX" : "pageY"; 

This worked on my test page, but what I dont know is if there was a reason for this being clientX/Y in the first place, and if therefore I would have just broken something else. I don't know how to run the tests in the system or I would have run those to see if it passed the tests, and I don't know how to submit my changes back to the core repo (or even if I should). Thoughts?

comment:6 Changed 2 years ago by scottgonzalez

  • Component changed from ui.core to ui.sortable

comment:7 Changed 23 months ago by tj.vantoll

  • Status changed from new to open
  • Summary changed from Sortable placeholder offset incorrect to Sortable: placeholder offset incorrect

Verified this in master and verified that the fix corrects the issue: http://jsfiddle.net/9a1Lk6bd/

comment:8 Changed 11 months ago by arschmitz

  • Resolution set to fixed
  • Status changed from open to closed

In 9aca706:

Sortable: Update _contactContainers to use page clientX/Y values

The sortable was using the window position compared with page position to
determine where to drop objects. this was only a problem for sortables far
enough down to require scrolling.

Fixes #10727
Fixes #5039
Closes gh-1475
Closes gh-1585

comment:9 Changed 11 months ago by scottgonzalez

  • Milestone changed from none to 1.12.0
Note: See TracTickets for help on using tickets.