Opened 6 years ago

Closed 6 years ago

Last modified 5 years ago

#9727 closed bug (notabug)

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:

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:

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!

Change History (9)

comment:1 Changed 6 years ago by dmethvin

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?

comment:2 Changed 6 years ago by tj.vantoll

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

comment:3 Changed 6 years ago by tj.vantoll

Owner: set to egrajeda
Status: newpending

comment:4 Changed 6 years ago by egrajeda

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!

comment:5 Changed 6 years ago by Scott González

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.

comment:6 Changed 6 years ago by 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/

comment:7 Changed 6 years ago by 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 :).

comment:8 in reply to:  7 Changed 6 years ago by tj.vantoll

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

comment:9 Changed 5 years ago by Scott González

#10800 is a duplicate of this ticket.

Note: See TracTickets for help on using tickets.