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

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?

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

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!

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.

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/

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 :).

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

#10800 is a duplicate of this ticket.