multidrag select across sortables

[maxbol]

Solves #2173

[jjeff]

This is great! And I've updated the CodePen example with your patch.

A couple of things though… and I'm not sure if they're existing bugs or issues with your patch:

1. In my primary implementation of MultiDrag, I'm selecting/deselecting elements programmatically using Sortable.utils.select() and Sortable.utils.deselect(). I've added links below the example to select list items programmatically. It appears that the new select-across-lists functionality works fine the first time, but on subsequent attempts to select programmatically, Sortable falls back to its old you-can't-select-across-lists functionality.
2. The multiDragKey option doesn't appear to have any effect. Ah ha! Okay, here's an issue about that.. I guess this is an existing bug. People in that thread seem to think this is a non-issue. So maybe it's a new bug introduced by this patch? I've added multiDragKey: 'Meta' to the example and it seems to have no effect (commented out for now). But I'll comment on the other issue.
3. MultiDrag has functionality that allows you to hold down the shift key (evt.shiftKey) and it will select consecutive elements between the current click and previous. This still works, but only within a single list. Could we get this to work between lists? I guess we would honor the HTML source order.

[maxbol]

Thanks for the feedback @jjeff , I'll have a look at it tomorrow!

[maxbol]

I've added a new commit that should take care of 1 and 2.

It turns out I was making a mistake when entering the sortables into the global group map. I falsely expected the option parsing that happens in the constructor of Sortable to already have happened, so did not take in account that options.group could be a string at this point. An easy fix.

To (hopefully) make things somewhat more legible, I separated the "global" part of the _deselectMultiDrag method out into a utility function called clear.

I also made a small helper function called itemElIsInSortableGroup, that returns true if the sortable instance that holds a specific list item belongs to the same group as some other sortable. Had to add this check to select() to make sure items weren't deselected simply for belonging to another sortable than the one you were currently selecting in (this is what made cross-group programmatic selection impossible once multiDragSortable was set (after first selection)).

As for 3, I'll give it a shot!

[jjeff]

Updated the Code Pen again.

The multiDragKey functionality is still not acting like I'd expect. According to the docs: The multiDragKey is "a custom keyboard key may be designated as having to be down for the user to be able to select multi-drag elements." In my mind, that means you can't select anything unless you hold down the key. With the current PR, single items can be selected. But selecting a second will deselect the first. But since single items can be dragged without "selecting" them first (just grab and drag), this functionality doesn't really make sense. The behavior that I would expect is: if the key isn't down, nothing will get selected.

I would probably change those docs to say "A custom keyboard key may be designated as having to be down for the user to be able to select multi-drag-enabled elements."

Also FWIW, the MultiDrag Options Example shows multiDragKey: null. But the null option actually produces an error: Uncaught TypeError: Cannot read properties of null (reading 'toLowerCase'). Apparently, the code is trying to set null to lowercase. 🙂

[maxbol]

Aha! This is good to know. I'll see what I can do.

[maxbol]

That's strange, this behavior (multidrag not implementing multiDragKey correctly, as stated above) seems to be identical to me on the current master branch. Can you please have a look and see if this is the same for you?

EDIT: Wrt the null option, this seems to be the result of a number of contingencies:

• The multiDragKey option listener presupposes its value property to be a string.
• Option listeners are triggered by the modifyOption method in the plugin manager, called from initializePlugin. So an explicit null value on this option makes it throw the TypeError you mentioned.
• At this point defaults have not been projected into options, which explains why the default null value doesn't trigger the error (defaults don't trigger the option listeners!). This may or may not be considered a bug depending on your expectations.

Commiting a small fix that circumvents the issue of the option listener expecting the value to be a string.

EDIT2: Added a commit to make multiDragKey to behave in line with what you detailed above. Again, I do not believe this is how it actually worked before my changes, so if you merge this, be prepared for some confusion from users who might have gotten used to the prior behavior :)

With regards to shift-selecting across lists, I believe this is going to be quite tricky to implement, and I have already spent a bit more time on this than the bounty amount really permits. If someone wants to raise another $50 or so in a separate issue I'd be happy to take a look at it. :)

[jjeff]

I've been testing and I'm sometimes getting an error saying:

Unhandled Exception TypeError: Cannot read properties of null (reading 'Sortable1660768120063') at t.dragOver

…which appears to be happening in the expando line of the dragOver method (in the new MultiDrag.js).

And it looks like adding this return; solves the problem:

dragOver({ target, completed, cancel, originalEvent }) {
  if (folding && ~multiDragElements.indexOf(target)) {
    completed(false);
    cancel();
    return; // <-- added this
  }

  const toSortable = target.parentNode[expando]; // <-- this is where the error is happening
  // etc

I'm not sure I understand the code enough to know if I might be breaking something else. But in my testing, it looks like this solves the problem and (I think) everything is working.

EDIT: Actually, I don't think the return line is solving the problem for me. I'm still getting the error.

[jjeff]

Okay another approach:

dragOver(…) {
  …

  const toSortable = target.parentNode && target.parentNode[expando];
  //                 ^^^^^^^^^^^^^^^^^^^^

This seems to solve the problem.

[maxbol]

Thanks @jjeff , I copied your fixes and reverted to the old behavior with multiDragKey. I've also squashed the branch history to hopefully make merging cleaner. :)

[jjeff]

Great! Thanks, @maxbol. This makes multiDrag a lot more useful for me.
I hope this gets into Sortable core soon so others can benefit.

[jjeff]

I've updated the CodePen example with the latest version of this PR. Works great!

Closes #2173

[jjeff]

Nudge to @owen-m1 and @RubaXa. It would be great to get this into the distribution! (along with #2193) 🙂

[Oles-R]

Any news about when it possible merge?

[ArianJM]

@owen-m1 @RubaXa is this moving forward? I am running into a bug that this PR helps address.
Thanks!