fix(cdk/drag-drop): support for nested drop containers (#16671) #21526

soren-petersen · 2021-01-09T14:33:45Z

Fixes a two issues with nested drop containers. The first fixed issue is that the elements of
cdkDropListConnectedTo previously had to be ordered correctly to support moving items into an
inner drop container. Sepcifically, inner drop containers had to be listed before their outer
ancestors. The second fixed issue is that items of an inner container could not be re-ordered
without moving the item out of the container and then back in.

Fixes #16671

Fixes a two issues with nested drop containers. The first fixed issue is that the elements of cdkDropListConnectedTo previously had to be ordered correctly to support moving items into an inner drop container. Sepcifically, inner drop containers had to be listed before their outer ancestors. The second fixed issue is that items of an inner container could not be re-ordered without moving the item out of the container and then back in. Fixes angular#16671

google-cla · 2021-01-09T14:33:51Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

soren-petersen · 2021-01-09T14:42:37Z

@googlebot I signed it!

…r#16671)

crisbeto

On top of the other comments, we definitely need some unit tests to ensure that things work as expected. It would be nice to have an example in the dev app as well so nested dragging can be tested manually.

crisbeto · 2021-01-14T18:11:17Z

src/cdk/drag-drop/drop-list-ref.ts

+    // Only consider targets where the drag postition is within the client rect
+    // (this avoids calling enterPredicate on each possible target)
+    let matchingTargets = targets.filter(ref => {
+      return ref._clientRect && isInsideClientRect(ref._clientRect, x, y);


I don't think that we can depend purely on the ClientRect, because it doesn't account for overlapping containers or ones that have been scrolled out of view. That's why we have the elementFromPoint call inside _canReceive.

The code does actually not depend solely on ClientRect. There is a call to _canRecieve on line 849 that should handle situations were the drop container is scrolled out of view or when there are elements overlapping the target drop container.

There is, however, an implicit assumption that the ClientRect of an inner drop container will only overlap with siblings that are its own ancestors in the drop container hierarchy. Although this is likely to be the most common case, it may not always be true. Let me see if I can find an elegant way to do away with this assumption.

crisbeto · 2021-01-14T18:13:26Z

src/cdk/drag-drop/drop-list-ref.ts

+   * outer drop container appear before the inner drop container.
+   * @param refs List of DropListRefs.
+   */
+  private _orderByHierarchy(refs: DropListRef[]): DropListRef[] {


This whole method is a bit concerning in terms of performance, because we call it for every pixel that the user has dragged. The DOM actually has an API that returns the relative position between two nodes: https://developer.mozilla.org/en-US/docs/Web/API/Node/compareDocumentPosition. That being said, I don't know what the performance implications of the native API are either.

Alternatively, we could cache the tree structure after the first check, because the DOM is unlikely to change while the user is dragging.

I share your concerns, although I might not become a problem in practice. The number of references ordred in each call to _orderByHierarchy is limited by the depth of the drop container hierarchy. Hence, if the hierarchy is 10 levels deep (a rather massive tree), the method will be called with a list of at most 10 drop containers to sort. This is not a huge effort for a modern computer / device.

Anyway, I think caching would be a good idea.

It might also make sense to re-implement the function using "compareDocumentPosition" rather than traversing the hierarchy manually.

I'll have a go at it.

soren-petersen · 2021-01-14T20:12:31Z

First of all, thank you for constructive feedback!

On top of the other comments, we definitely need some unit tests to ensure that things work as expected. It would be nice to have an example in the dev app as well so nested dragging can be tested manually.

With respect to unit tests and an example in the dev app, I'm sure I would be able to put something together. Unfortunately, I would need to spend quite a bit of time to figure out how it all works. At the moment, I have no idea where the dev-app is located, let alone how to add an example. Nor am I familiar with the unit test framework. Is there, perhaps, someone with more experience as a contributor who could support me with this?

crisbeto · 2021-01-14T20:20:36Z

I wrote most of the drag-drop module so I can help with it. To run the dev app, you have to install the dependencies by running yarn and then yarn dev-app will start the server. As for the unit tests, you have to run yarn test drag-drop. The dev app for the drag-drop module is in src/dev-app/drag-drop/drag-drop-demo.ts and the unit tests are in src/cdk/drag-drop/directives/drag.spec.ts.

peternixey · 2021-08-20T12:12:02Z

Really excited to see this go in guys as it relates directly to a big requirement in my current project. Thank you for your work @soren-petersen and @crisbeto. Any ideas on what version of Material it might be expected in? It would be brilliant to start using it.

henrikdahl8240 · 2021-10-15T10:35:44Z

@crisbeto So still nothing has been done to fix the issue "The second fixed issue is that items of an inner container could not be re-ordered without moving the item out of the container and then back in.". There is no workaround for this.
I would like to suggest an alternative approach, as you wrote "I wrote most of the drag-drop module so I can help with it.".
My suggestion is, that you just define a unit test yourself which verifies that the re-ordering can be done. This unit test will fail because it doesn't work and then you just correct the drag-drop module and afterwards the unit test will succeed.
This problem has existed for years, wouldn't it be an idea to finally get it fixed?
I don't know how your drag-drop module works, but I use it and it works fine in many regards, however not in this regard! Isn't it quite simple, if the user is dragging it over it's own droplist, and sorting is not turned off, it should be reordered, otherwise it should work as usual?
By the way, I think it should rather be named re-ordering than sorting.

diego-rapoport · 2023-06-19T14:14:25Z

Is there any hope that this will ever be the solution? Or that there will be any kind of solution for this kind of feature? Should I try another approach? Any ideas?

ankit9015 · 2024-06-21T13:33:57Z

@crisbeto Can we please get this issue fixed, its been 3 years since people have been encountering this issue.

Would really appreciate if you could spare a little bit of your time to resolve this.

squelix · 2024-09-16T08:39:57Z

We have this issue too, can you resolve this issue ?

andrewseguin · 2025-01-22T19:42:36Z

Closing this particular PR since it has gotten out of sync with the repo and still needs tests added. We encourage anyone to reopen this PR with tests so we can get this merged in

soren-petersen requested a review from crisbeto as a code owner January 9, 2021 14:33

google-cla bot added the cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla label Jan 9, 2021

google-cla bot added cla: yes PR author has agreed to Google's Contributor License Agreement and removed cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla labels Jan 9, 2021

soren-petersen added 2 commits January 9, 2021 20:26

fixup! fix(cdk/drag-drop): support for nested drop containers (angula…

b4c714d

…r#16671)

fixup! fix(cdk/drag-drop): support for nested drop containers (angula…

d3690c3

…r#16671)

soren-petersen force-pushed the nested-drop-container-fix branch from d6db608 to d3690c3 Compare January 9, 2021 20:27

crisbeto reviewed Jan 14, 2021

View reviewed changes

andrewseguin removed the cla: yes PR author has agreed to Google's Contributor License Agreement label Dec 28, 2021

googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Dec 29, 2021

wawyed mentioned this pull request Mar 31, 2022

CDK Drag Drop issues with nested lists #16671

Open

josephperrott requested a review from a team as a code owner December 18, 2024 17:40

josephperrott requested review from crisbeto and wagnermaciel and removed request for a team December 18, 2024 17:40

andrewseguin closed this Jan 22, 2025

andrewseguin self-assigned this Jan 22, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(cdk/drag-drop): support for nested drop containers (#16671) #21526

fix(cdk/drag-drop): support for nested drop containers (#16671) #21526

soren-petersen commented Jan 9, 2021

google-cla bot commented Jan 9, 2021

soren-petersen commented Jan 9, 2021

crisbeto left a comment

crisbeto Jan 14, 2021

soren-petersen Jan 14, 2021

crisbeto Jan 14, 2021

soren-petersen Jan 14, 2021

soren-petersen commented Jan 14, 2021

crisbeto commented Jan 14, 2021

peternixey commented Aug 20, 2021

henrikdahl8240 commented Oct 15, 2021

diego-rapoport commented Jun 19, 2023

ankit9015 commented Jun 21, 2024

squelix commented Sep 16, 2024

andrewseguin commented Jan 22, 2025

fix(cdk/drag-drop): support for nested drop containers (#16671) #21526

fix(cdk/drag-drop): support for nested drop containers (#16671) #21526

Conversation

soren-petersen commented Jan 9, 2021

google-cla bot commented Jan 9, 2021

What to do if you already signed the CLA

Individual signers

Corporate signers

soren-petersen commented Jan 9, 2021

crisbeto left a comment

Choose a reason for hiding this comment

crisbeto Jan 14, 2021

Choose a reason for hiding this comment

soren-petersen Jan 14, 2021

Choose a reason for hiding this comment

crisbeto Jan 14, 2021

Choose a reason for hiding this comment

soren-petersen Jan 14, 2021

Choose a reason for hiding this comment

soren-petersen commented Jan 14, 2021

crisbeto commented Jan 14, 2021

peternixey commented Aug 20, 2021

henrikdahl8240 commented Oct 15, 2021

diego-rapoport commented Jun 19, 2023

ankit9015 commented Jun 21, 2024

squelix commented Sep 16, 2024

andrewseguin commented Jan 22, 2025