Skip to main content

Search and Top Navigation

#9727 closed bug (notabug)

Opened January 04, 2014 07:07AM UTC

Closed January 04, 2014 07:30PM UTC

Last modified January 22, 2015 03:15PM UTC

Sortable: .cancel() ignores comment nodes (causes problems with AngularJS)

Reported by: egrajeda Owned by: egrajeda
Priority: minor Milestone: none
Component: ui.sortable Version: 1.10.3
Keywords: Cc:
Blocked by: Blocking:
Description

The version(s) of jQuery affected

jQuery 1.10.2+ and jQuery UI 1.10.3+

The browser (or browsers) that you are able to reproduce the bug in, including version numbers

Any browser. Personally I tested it on Chromium Version 31.0.1650.63 Ubuntu 13.04

The operating system (or operating systems) you experienced the bug on.

Ubuntu 13.04

Description

Hello,

AngularJS uses comment nodes to store information. The sortable component (probably other components aswell) uses jQuery .prev()/.next() to traverse the DOM. The problem is that these functions don't recognize comment nodes, so for instance when I use sortable.cancel(), the dragged element should be inserted back in the same spot it originally was; but this is not happening because the comment nodes are being ignored.

I have setup a basic demo here:

http://plnkr.co/edit/kLeMRjPqEVZ1NZtCyrZH?p=preview

In its initial state, the sortable-related elements looks like this:

[[Image(http://i.imgur.com/0m466Sj.jpg)]]

If you try to drag "Item A", after you drop it the code will automatically call sortable.cancel() for you. The elements will now look like this:

[[Image(http://i.imgur.com/gPULUxy.jpg)]]

If you see, the "Item A" <li> tag should be after the first comment, but it is inserted back before it.

This might seem like a small issue, but it can make a difference between a broken sortable:

http://plnkr.co/edit/BVpHl8iZyKfI4snvrafR?p=preview

and a working one:

http://plnkr.co/edit/i6lAv57wW0J7UtIAZcbT?p=preview

This last demo is using a modified version of the sortable code; instead of using .prev() I'm using .previousSibling.

I can submit a PR, but I wanted to add this here first to know if there is anything against this.

Thanks!

Attachments (0)
Change History (9)

Changed January 04, 2014 03:28PM UTC by dmethvin comment:1

Angular is using HTML comment nodes to denote important document structure, which isn't what they are meant for. jQuery's Core methods like .next() have always either skipped or removed comment nodes, so we can't change those semantics without breaking a lot of code.

So that leaves the option of having jQuery UI and others skip the jQuery Core APIs to make this work properly in Angular. Essentially, UI would need to say "our components guarantee that comment nodes won't be ignored or removed" which is a pretty big guarantee to make. You've found one case, but as you noted this probably affects other components as well.

This just seems like the wrong direction to me. Longer term there will be a non-hacky standard way to build components via ShadowRoot, and as I understood it the Angular team was committed to migrating to that. I understand the feeling behind this patch, "it's just a small change" and "it fixes the problem I'm seeing" but having us commit to the support it implies could open up a big can of worms.

Angular proponents seem to be pretty down on jQuery in general because they feel like it's the wrong way to do things. What would be the "Angular way" to implement Sortable?

Changed January 04, 2014 03:31PM UTC by tj.vantoll comment:2

+1 dmethvin

egrajeda: Could you provide the specific patch you used in a gist or a pull request? I'm curious if it's possible to build a lightweight extension for Angular users.

Changed January 04, 2014 03:38PM UTC by tj.vantoll comment:3

owner: → egrajeda
status: newpending

Changed January 04, 2014 07:15PM UTC by egrajeda comment:4

status: pendingnew

dmethvin: I agree with most of what you say; in fact, I've opened an issue in AngularJS aswell (https://github.com/angular/angular.js/issues/5619) to see what is to be done with issues like these, which arose after they used some extra comments in their ngRepeat directive. I asked there the same question as you, which would be the "Angular way" to do this?.

I think the issue with .cancel() shouldn't be seen as related to Angular; I ran accross it while trying to find an alternate way to make sortable and Angular work together, but the fact is that the purpose of .cancel() is to revert the sortable to the state prior to when the current sort was started, and currently, because of the way it is implemented, it isn't doing that when comments are involved.

tj.vantoll: I've uploaded a patch here:

https://gist.github.com/egrajeda/8259186

I could also create a PR if you guys needed it.

Thanks!

Changed January 04, 2014 07:30PM UTC by scottgonzalez comment:5

resolution: → notabug
status: newclosed

Sortable is doing The Right Thing™ according to the rules of jQuery. We've never cared about comments, and I doubt we ever will. On a related note, The cancel() method won't exist in the rewrite. Canceling a user's interaction in the middle of the interaction is pretty bad UX.

I'm going to close this as not a bug, since comments are never supported in jQuery's API. But feel free to continue the discussion here with tj.vantoll.

Changed January 04, 2014 11:04PM UTC by tj.vantoll comment:6

egrajeda: How about this?

$.widget( "ui.sortable", $.ui.sortable, {
    _mouseStart: function() {
        this._superApply( arguments );
        this.domPosition = { prev: this.currentItem[ 0 ].previousSibling, parent: this.currentItem.parent()[ 0 ] };
    }
});

Example: http://jsfiddle.net/tj_vantoll/7kmJ6/

Changed January 05, 2014 07:16AM UTC by egrajeda comment:7

tj.vantoll: Thanks! I was trying with http://www.paulirish.com/2010/duck-punching-with-jquery/ but I like your code better, I'll be using it in my app :).

Changed January 05, 2014 07:55AM UTC by tj.vantoll comment:8

Replying to [comment:7 egrajeda]:

tj.vantoll: Thanks! I was trying with http://www.paulirish.com/2010/duck-punching-with-jquery/ but I like your code better, I'll be using it in my app :).

Awesome! It looks like we can shorten the solution a little as well.

$.widget( "ui.sortable", $.ui.sortable, {
    _mouseStart: function() {
        this._superApply( arguments );
        this.domPosition.prev = this.currentItem[ 0 ].previousSibling;
    }
});

And just fyi, for more information on extending widgets see http://learn.jquery.com/jquery-ui/widget-factory/extending-widgets/.

Changed January 22, 2015 03:15PM UTC by scottgonzalez comment:9

#10800 is a duplicate of this ticket.