Opened 4 years ago

Closed 3 years ago

Last modified 3 years ago

#9335 closed bug (fixed)

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.
  4. Observe that the oversortable event does not fire.
  5. Drag from the "Available Columns" list to the "Group/Sort" list.
  6. Observe that the oversortable event does fire (group/sort is not connected to another sortable).
  7. Drag from the "Available Columns" list to the "Filter" list.
  8. 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

Change History (23)

comment:1 Changed 4 years ago by tj.vantoll

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.

comment:2 Changed 4 years ago by saluce65

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

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

comment:3 in reply to:  2 Changed 4 years ago by tj.vantoll

Replying to saluce65:

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

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

Perfect, thank you.

comment:4 Changed 4 years ago by tj.vantoll

#9445 is a duplicate of this ticket.

comment:5 Changed 4 years ago by tj.vantoll

#9462 is a duplicate of this ticket.

comment:6 Changed 4 years ago by tj.vantoll

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/.

comment:7 Changed 4 years ago by Skaffen

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

comment:8 Changed 4 years ago by Skaffen

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

comment:9 Changed 4 years ago by tj.vantoll

#9717 is a duplicate of this ticket.

comment:10 Changed 4 years ago by tj.vantoll

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/.

comment:11 Changed 4 years ago by tj.vantoll

#9732 is a duplicate of this ticket.

comment:12 Changed 4 years ago by NiGhTTraX

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

comment:13 Changed 4 years ago by 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.

comment:14 in reply to:  13 Changed 4 years ago by tj.vantoll

Replying to 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.

comment:15 Changed 4 years ago by Skaffen

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.

comment:16 in reply to:  15 Changed 4 years ago by NiGhTTraX

Replying to 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.

comment:17 Changed 4 years ago by Skaffen

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.

comment:18 in reply to:  17 Changed 4 years ago by NiGhTTraX

Replying to 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.

comment:19 Changed 3 years ago by tj.vantoll

#10541 is a duplicate of this ticket.

comment:20 Changed 3 years ago by tj.vantoll

Keywords: regression added

comment:21 Changed 3 years ago by Andrei Picus

Resolution: fixed
Status: openclosed

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

Fixes #9335

Changeset: 1cfebf803beedef05e8dcdd54e34d10c412a9a2b

comment:22 Changed 3 years ago by mikesherov

Milestone: none1.11.2

comment:23 Changed 3 years ago by tj.vantoll

#10558 is a duplicate of this ticket.

Note: See TracTickets for help on using tickets.