Skip to main content

Search and Top Navigation

#9335 closed bug (fixed)

Opened May 28, 2013 03:40PM UTC

Closed August 14, 2014 12:40AM UTC

Last modified August 18, 2014 02:40PM UTC

Sortable: over & out events does not consistently fire

Reported by: saluce65 Owned by:
Priority: minor Milestone: 1.11.2
Component: ui.sortable Version: 1.10.2
Keywords: regression Cc:
Blocked by: Blocking:
Description

Dragging draggables over a sortable that is further connected to another sortable is not firing the oversortable event as expected.

In the attached demo, I have a single list of draggable items (in the "Available Column" box), which is then connected to three different sortables. When the sortable is connected to another sortable (besides itself), the oversortable event does not fire when dragging the draggable over the sortable. The event does, however, fire when dragging from one sortable to another.

Steps to reproduce:

1. Open the demo code.

2. Open the dev tools console.

3. Drag from the "Available Columns" list to the "Display Columns" list.

3. Observe that the oversortable event does not fire.

4. Drag from the "Available Columns" list to the "Group/Sort" list.

5. Observe that the oversortable event does fire (group/sort is not connected to another sortable).

6. Drag from the "Available Columns" list to the "Filter" list.

7. Observe that the oversortable event does fire (filter is connected to itself).

In jQueryUI 1.10.1, when the oversortable event was firing on every pixel movement of the mouse, the event fired perfectly over all three sortables. However, when this bug was fixed in 1.10.2, it introduced a new bug (which actually existed in 1.8.24) where the oversortable event wasn't firing over sortables that connected to other sortables.

Demo of code: http://jsbin.com/ahetev/5/edit

Attachments (0)
Change History (23)

Changed May 29, 2013 02:01AM UTC by tj.vantoll comment:1

status: newopen
summary: Draggable connected to Sortable that further connects to another sortable doesn't fire the oversortable eventSortable: oversortable event does not consistently fire

Confirmed. This appears to have been introduced by the conditional added here: https://github.com/jquery/jquery-ui/commit/07ce771a13504b851bb9f74c8ce8e960d207384a.

saluce65 could you reduce your test case so that it has the minimum amount of code to show this problem? There's a lot going on in your examples and it'd be easier to debug this if it were simplified.

Changed May 29, 2013 03:55PM UTC by saluce65 comment:2

I have reduced the code down to a bare minimum to show the issue.

http://jsbin.com/ahetev/9/edit

Changed May 29, 2013 04:14PM UTC by tj.vantoll comment:3

Replying to [comment:2 saluce65]:

I have reduced the code down to a bare minimum to show the issue. http://jsbin.com/ahetev/9/edit

Perfect, thank you.

Changed July 22, 2013 06:18PM UTC by tj.vantoll comment:4

#9445 is a duplicate of this ticket.

Changed July 30, 2013 08:05PM UTC by tj.vantoll comment:5

#9462 is a duplicate of this ticket.

Changed July 30, 2013 08:08PM UTC by tj.vantoll comment:6

Per #9462 this affects more than just draggables connected to sortables. If you drag a sortable item outside of a list then back in the over event will not fire.

Test case from Skaffen http://jsfiddle.net/3X2bL/2/.

Changed July 31, 2013 08:54AM UTC by Skaffen comment:7

My patch from #9462 seems to sort the issue with this ticket too. Am looking at doing a pull request in due course.

Changed July 31, 2013 09:37AM UTC by Skaffen comment:8

Done. First time I've used github for doing anything, so hopefully I did it all right :). https://github.com/matt-hoskins/jquery-ui/commit/885b90e753ed2bea49de8d62bf693d8b924e5f77

Changed December 23, 2013 03:18PM UTC by tj.vantoll comment:9

#9717 is a duplicate of this ticket.

Changed December 23, 2013 03:19PM UTC by tj.vantoll comment:10

summary: Sortable: oversortable event does not consistently fireSortable: over & out events does not consistently fire

The same commit in 1.10.2 also broke the

out
event. See #9717 and http://jsfiddle.net/tj_vantoll/8he2r/.

Changed January 08, 2014 07:34PM UTC by tj.vantoll comment:11

#9732 is a duplicate of this ticket.

Changed January 14, 2014 09:51AM UTC by NiGhTTraX comment:12

Submitted a pull request https://github.com/jquery/jquery-ui/pull/1170 that includes a fix and additional tests.

Changed January 28, 2014 11:24AM UTC by Skaffen comment:13

I'd submitted a pull request with decent test coverage (as requested by tj.vantoll) 5-6 months ago:

https://github.com/jquery/jquery-ui/pull/1038

Nothing seemed to have happened on it though.

Changed January 28, 2014 01:43PM UTC by tj.vantoll comment:14

Replying to [comment:13 Skaffen]:

I'd submitted a pull request with decent test coverage (as requested by tj.vantoll) 5-6 months ago: https://github.com/jquery/jquery-ui/pull/1038 Nothing seemed to have happened on it though.

Apologies Skaffen. These sortable issues are time consuming to look through, and they often end up causing more problems than they fix. We have a rewrite of all of our interactions that's going on as well.

I will say that this issue in particular is something we should address in some fashion because it has come up numerous times. We'll try to get around to this.

Changed January 28, 2014 02:41PM UTC by Skaffen comment:15

That's ok - I figured it was just down to a lack of time. As an aside - I've noticed there were jshint issues with the new tests I'd written and I've just quickly fixed those with another commit on my branch (I think I may have inadvertently reverted a couple of information text corrections I'd made to a couple of the original tests while doing that, but that's very minor).

I've checked that my changes should still apply ok against the current master with only one conflict (which is down to the renaming of jquery.ui.xxxx.js to just xxxx.js so the TestHelpers.loadResources block changed) and that the tests still pass against the current master.

NiGhTTraX's patch takes a similar approach to mine but NiGhTTraX's patch will not trigger the "over" event when "(this.currentContainer === this.containers[innermostIndex])" is false whereas mine will - as it's been 6 months since I looked at that area so everything I'd figured out about sortables has fallen out of my brain so I'm not sure if that difference is important. Note that tests also cover interactions with draggables as this bug affected those as well as just sortable ones.

Changed January 29, 2014 01:14PM UTC by NiGhTTraX comment:16

Replying to [comment:15 Skaffen]:

NiGhTTraX's patch takes a similar approach to mine but NiGhTTraX's patch will not trigger the "over" event when "(this.currentContainer === this.containers[innermostIndex])" is false whereas mine will - as it's been 6 months since I looked at that area so everything I'd figured out about sortables has fallen out of my brain so I'm not sure if that difference is important. Note that tests also cover interactions with draggables as this bug affected those as well as just sortable ones.

The triggering in that case is done further down the code, specifically at line 925. I also tested that it works with draggables as well.

Both fixes seem equivalent to me.

Changed January 30, 2014 09:57AM UTC by Skaffen comment:17

You're quite right NiGhTTrAX - my mistake for just looking at the patch and not the code that follows.

Actually where our patches differ, I think, is that in the case where "!(this.currentContainer === this.containers[innermostIndex])" and "this.containers[innermostIndex].containerCache.over" your patched version will still trigger "over". I think that's why I rearranged the code how I did in my patch. I can't remember if it's possible for those two things to be true though (i.e. I can't recall if I was seeing redundant "over" events in my testing) or was just erring on the side of caution.

Changed February 07, 2014 09:13PM UTC by NiGhTTraX comment:18

Replying to [comment:17 Skaffen]:

Actually where our patches differ, I think, is that in the case where "!(this.currentContainer === this.containers[innermostIndex])" and "this.containers[innermostIndex].containerCache.over" your patched version will still trigger "over".

You're right, but that was the original logic so I didn't want to mess with that. I did test that the over event is fired only once in my test cases, so either those 2 things can't ever happen at the same time, or more tests need to be written.

Changed August 12, 2014 06:36PM UTC by tj.vantoll comment:19

#10541 is a duplicate of this ticket.

Changed August 12, 2014 06:38PM UTC by tj.vantoll comment:20

keywords: → regression

Changed August 14, 2014 12:40AM UTC by Andrei Picus comment:21

resolution: → fixed
status: openclosed

Sortable: fire "over" and "out" even when a connectWith hasn't changed

Fixes #9335

Changeset: 1cfebf803beedef05e8dcdd54e34d10c412a9a2b

Changed August 14, 2014 12:41AM UTC by mikesherov comment:22

milestone: none1.11.2

Changed August 18, 2014 02:40PM UTC by tj.vantoll comment:23

#10558 is a duplicate of this ticket.