Sortable Fix-Up

[borgboyone]

Component: ui/widgets/sortable.js

• Created _scroll extension point and migrated scroll code from _mouseDrag (350-399, 411)
• Cleaned up logic for scrolled (412-417)
• Fixed appendTo functionality to match documentation (198-202, 333-338, 1108)
• Remove unnecessary function calls
• Move set-up position functions to appropriate place (340-344)
• Base scrollParent on placeholder and not helper (263-264, 1090-1091)
• Update scrollParent when switching containers (1090-1091)

[jsf-clabot]

All committers have signed the CLA.

[borgboyone]

Woo Hoo!

[scottgonzalez]

Can you please link to the bugs you're fixing?

[borgboyone]

Scott,
I just wanted you to be aware that I'm not ignoring your request. I was in the process of creating a jsFiddle last night for a bug report specific to the appendTo functionality (if it's in the list, I couldn't find it even though its mentioned a few times on StackOverflow) when it became clear that I can't ignore another issue related to scrolling (which does have a ticket btw) if i want to get this project out the door. If it looks like I'll be unable to get back to this by the end of the weekend, I'll close the pull request and resubmit at a later date.
Anthony

[scottgonzalez]

No problem. Don't worry about closing it if you don't get to it by this weekend. If it sits for a long time with no responses, we'll close it, but a few days is not a problem at all :-)

[borgboyone]

Scott,
From my stand point, I have just committed my last change to sortable.js and all seems well on my end in terms of scroll based functionality. I have made some rearrangement of the code to address issues related to performance (all fairly common sense) but you may find some of these rearrangements disconcerting given the age of this code.
I will leave this pull request open and will attempt to free up some time next week to create jsFiddle's related to functionality differences so that you can visually access what has changed.
Anthony

[Daijobou]

Fix it this bug too? https://bugs.jqueryui.com/ticket/15097

[borgboyone]

@Daijobou I can't say for certain that I have addressed issue(s) specific to #15097 but I did make some rearrangements and changes that most certainly affect performance. Beyond what I have committed here, I have further changes that target other scrollable related issues as well as addressing autoscrolling, but I did not provide these here as they are fairly far a field from what I would consider the jQuery ui standard. (They did make the client fairly happy, however.) You are welcome to try the changes presented on your own and see how they impact performance. Any feedback you may be able to provide is always appreciated.

[scottgonzalez]

@borgboyone If you don't see any existing tickets for bugs that this fixes, can you please file a new ticket so that this will show up in the changelog?

You're right that these changes are disconcerting due to the age of the code (and the lack of anyone willing to own this code). However, if you're willing to own just this change and deal with any regressions that are reported as a result of this change, we can merge this.

[borgboyone]

@scottgonzalez I had completely let this get away from me. I have made time tomorrow evening to create the tickets necessary for the changelog and post them here once completed. Gotta dig up those notes.

That's fine on the regression. I don't foresee that these changes could generate new issues. The only thing I could potentially see an issue with is if someone has attempted to fix them via callbacks previous and then introduce these changes leading to conflict. Something to consider given what I've seen others attempt on StackOverflow.

I'm assuming you have looked over these changes? Did you see anything that concerned you?

If you're so inclined, check out: https://github.com/borgboyone/jQuery-UI-ScrollPane/blob/master/src/jquery-ui-scrollsortable-autoscroll.js It's not a version I would recommend for the main stream, but it has some tidbits you might find interesting and it essentially "fixes" the scrollbar movement issue associated with the non atomic operation of absolute positioning (popping out) of the helper and dropping in the placeholder. I just wish I had been able to find a proper solution to that problem.
Anthony

[scottgonzalez]

I've looked over the changes and I didn't see anything that concerned me. We've just had a lot of regressions in the interaction widgets over time, due to their age and smaller test coverage.

[scottgonzalez]

Thanks for filing all of those tickets. I see that a lot of them have placeholders for fiddles and SO discussions. Are you planning on filling those in or should they just be removed?

[borgboyone]

@scottgonzalez I do plan on filling those in and listing them here with corresponding line numbers this evening.

[borgboyone]

@scottgonzalez
Line numbers reference the incoming version

#3173: (1106-1114)
The majority of solutions presented on SO only implement 1106-1107 but the full solution needs the additional 1109-1114.

#15165: (353-401, 426)
Self-explanatory

#15166: (429, 852-877, 887)
This issue is treated mainly as an annoyance and not a functional show stopper. Unfortunately, I didn't have that luxury. An attempt was made to switch the comparison reference so that the item offsets were absolute to the scrollparent and the mouse position was translated to that reference but this required to much of a rewrite so I didn't pursue this route further. The easiest thing to do was just to update the item positions if a scroll occurred. You'll notice that a slight discrepancy in the overlap detection the height of an item (y-axis dragging) may still occur. This is due to #15175. "Fixing" #15175 removes all issues associated with the helper/item intersection that I could see.

#15167: (198-202, 333-339, 1130)
This issue was almost a show stopper for me in using ui.sortable in a specific project. It was either fix it or move on to a from scratch solution or find something else. There is a lot of SO traffic on this issue as well.

#15168: (259-265, 1107)
Self-explanatory

#15169: (422, 1036)
It's very odd behavior to have scrolling of the last container occur when the helper is outside of any sortable container. This isn't a necessary fix but it helps with the user experience.

#15170: (419-420, 422, 1036)
This was in an attempt to improve performance especially where large number of sortable items were involved and multiple connected sortables were involved.

(437-440, 694-695, 709-710)
I left this in mainly as a precursor to future enhancements. I take advantage of these values for the autoscroll feature (where you simply hold the mouse at the edge(s)) and scrolling occurs with out the need for mouse movement. I also limited scrolling to mouse direction, so if you're coming into a container it doesn't autoscroll. I did not include these changes in the pull request but I thought I'd mention them.

Having written this out, it doesn't feel like a lot of changes. However, they do inherently change some internal processing that has been around for a while. Personally, I'd like to see more testing of a few of these changes in remarkably different usage situations...something outside of my working frame of reference.

With that said, I leave it to you to decide what to include, if anything, and what to exclude, if nothing.
Anthony

[scottgonzalez]

I'm going to merge the whole thing. It seems like you've got a good understanding of this code and if any regressions do pop up, you should be able to address them. So even though I'm a little nervous about this, I'm going to land it since overall it will be an improvement.

[mgol]

Changes from this PR broke sortable in the very simple case, see #1998. I'll try to understand what may be happening here but if by any chance you're available, @borgboyone (I know, it's been a while) then I'd appreciate any help you could provide.

[mgol]

Another regression caused by this PR: #2001. In this one I don't see any CSS issues in the test case as in #1998 so it might be a real bug, not just a compatibility issue with UI 1.12.1.