Draggable: [BUG] Incorrect oldIndex when there are nested draggable containers

[davidaik]

I'm using the library to build a layout creator.

There is a main Sortable container which contains draggable items. Some of these draggable items can themselves contain a nested Sortable container.

When I drag an item from the main Sortable container and drop it into a nested Sortable container, I'm getting an incorrect value for oldIndex. This happens only if I'm dragging an item which is located after the nested Sortable container in the DOM.

______________________________________________________________________________________________
| Main Sortable                                                                               |
|     ___________________________________________________________________________________     |
|    | Draggable item A (direct child to main Sortable)                                  |    |
|    |     _________________________________________________________________________     |    |
|    |    | Nested Sortable                                                         |    |    |
|    |    |    ________________________________________________________________     |    |    |
|    |    |    | Nested Draggable item B                                       |    |    |    |
|    |    |    |_______________________________________________________________|    |    |    |
|    |    |_________________________________________________________________________|    |    |
|    |___________________________________________________________________________________|    |
|                                                                                             |
|     ___________________________________________________________________________________     |
|    | Draggable item C (direct child to main Sortable)                                  |    |
|    |___________________________________________________________________________________|    |
|_____________________________________________________________________________________________|

When I drag Draggable item C into Nested Sortable just next to Nested Draggable item B, the value of oldIndex for Draggable item C is 2, instead of 1.

I'm expecting 1 because inside Main Sortable, Draggable item C is the second child and should have 1 as its index.

I found the issue at this line in Draggable.js

getDraggableElementsForContainer(container) {
>   const allDraggableElements = container.querySelectorAll(this.options.draggable);

    return [...allDraggableElements].filter((childElement) => {
        return childElement !== this.originalSource && childElement !== this.mirror;
    });
}

We are querying ALL draggable items inside the container of Draggable item C, which includes the nested draggable items too. This makes the oldIndex for Draggable item C much higher than expected.

A quick fix could be to update the getDraggableElementsForContainer() function to this:

getDraggableElementsForContainer(container)  {
    const allDraggableElements = container.querySelectorAll(this.options.draggable);

    return [...allDraggableElements].filter((childElement) => {
    >   return childElement !== this.originalSource && childElement !== this.mirror && childElement.parentNode === container;
    });
}

I'm returning only the direct children of the parent by using childElement.parentNode === container.

Please let me know if it makes sense to file a pull request including this fix. I'd love to contribute, even though it's a tiny contribution :)

[bahung1221]

Thanks, you are welcome 😄

Currently, we have some issues with nested sortable containers (#129 ).
Not just the index issue which you described above, so can you please provide an example code that makes your nested containers worked? Do you have any else issues except the wrong index issue?

[davidaik]

Everything is working perfectly for me (so far) with nested sortable containers after the small fix that I mentioned earlier.

I went through the issue that you've mentioned and it seems like there could be issues if you're not using a handle. I'll try with and without a handle and post here if I manage to come up with working examples.

[davidaik]

@bahung1221 Here is a Codepen with a working nested sortable. It has the fix applied, so the value of oldIndex is correct. https://codepen.io/davidaik/pen/gOrpeMO

[bahung1221]

Hi,

Sorry for late response, It worked but we still have some problems that need to be resolved (the errors logged on console), please see image below.

I think your fix can be merged with some test cases but it still can't be released until we find out the error. Currently, I have an urgent deadline so I can't fix it soon, maybe a few weeks later 😢

May you help to fix this error? It will be our pleasure.

BTW, Hi @zjffun , sorry for bothering you, just to says that if you have rest time, may you help to check this issue? I really appreciate your help

Thank you so much 😄

[davidaik]

I'll be happy to look into that error message. I'll update here if I manage to come up with a solution.

[zjffun]

Emm.. I think we shouldn't change getDraggableElementsForContainer but add a getSortableElementsForContainer method in sortable.

Maybe because of network problems I got Refused to execute script from 'https://srv-file4.gofile.io/download/RFpGbt/draggable.js' because its MIME type ('text/html') is not executable, and strict MIME type checking is enabled. error when opened https://codepen.io/davidaik/pen/gOrpeMO . There are tests based on the discussion above https://github.com/zjffun/draggable/commit/f2e020d7f2c8c7965ec2f6cf378551716c17724a. It works fine except Failed to execute 'insertBefore' on 'Node' error.

[bahung1221]

Yep, seems that it worked fine except the console error, don't know does it will cause any side effect.

Emm.. I think we shouldn't change getDraggableElementsForContainer but add a getSortableElementsForContainer method in sortable.

I absolutely agree with you 😄

[davidaik]

@bahung1221 I think the console error issue is fixed by updating Sortable.js at line 282 from this:

if (oldIndex < newIndex) {
    source.parentNode.insertBefore(source, over.nextElementSibling);
>} else {
    source.parentNode.insertBefore(source, over);
}

To this:

if (oldIndex < newIndex) {
    source.parentNode.insertBefore(source, over.nextElementSibling);
>} else if (source.parentNode === over.parentNode) {
    source.parentNode.insertBefore(source, over);
}

I've updated the Codepen with the fix applied. Please check to see if you're still seeing errors. https://codepen.io/davidaik/pen/gOrpeMO

[davidaik]

@zjffun I think the Codepen should work for you now. I updated the script urls.

[davidaik]

Hmm... I found a somewhat rare occasion where the error was still showing. I think updating the moveWithinContainer function in Sortable.js at line 276 from this:

function moveWithinContainer(source, over) {
  const oldIndex = index(source);
  const newIndex = index(over);

  if (oldIndex < newIndex) {
    source.parentNode.insertBefore(source, over.nextElementSibling);
  } else {
    source.parentNode.insertBefore(source, over);
  }

  return {oldContainer: source.parentNode, newContainer: source.parentNode};
}

To this:

function moveWithinContainer(source, over) {
  const oldIndex = index(source);
  const newIndex = index(over);

  >if (source.parentNode === over.parentNode) {
    if (oldIndex < newIndex) {
      source.parentNode.insertBefore(source, over.nextElementSibling);
    } else {
      source.parentNode.insertBefore(source, over);
    }
  }

  return {oldContainer: source.parentNode, newContainer: source.parentNode};
}

is better.

Please confirm that it works for you too :)

[bahung1221]

Amazing, it worked now! 🎉

But it means the check sameContainer was wrong, so I think we should fix this check instead of change moveWithinContainer function.

https://github.com/Shopify/draggable/blob/03012df4ceed26b6e3fec2016c4f9e11d42fe73c/src/Sortable/Sortable.js#L255

How about it? 😄

[davidaik]

It seems to work when I change that line to this:

const sameContainer = over && source.parentNode === over.parentNode;

What do you think?

[bahung1221]

Yep, it seems the value of overContainer was wrong, I will try to verify it later.

For now, I think your fix should be merged for the next release (include the fix of sameContainer check). Please create a PR for it 😄
I really appreciate a test suite as well as the fixes.

Emm.. I think we shouldn't change getDraggableElementsForContainer but add a getSortableElementsForContainer method in sortable.

BTW, like @zjffun commented above, we should create a new method instead change the behaviour of getDraggableElementsForContainer method to avoid a breaking change.

Thank you so much for your contribute ❤️

[davidaik]

I'm glad I was able to help make this great library better. :)

And yeah, I agree that we should create a new method instead of updating the existing one. I'll create the PR and the tests after a bit. Hopefully it gets merged for the next release.

[zjffun]

It seems to work when I change that line to this:
const sameContainer = over && source.parentNode === over.parentNode;

This fix works very well.

[davidaik]

I have created a PR with the changes that we discussed. As for the test suite, I just updated the same Codepen. Since only the Sortable.js file is changed, I thought the same Codepen with updated sortable.js would do.

[bahung1221]

Thank you so much, I will check it tonight 🎉