Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CDK Drag Drop issues with nested lists #16671

Open
shesse opened this issue Aug 2, 2019 · 52 comments
Open

CDK Drag Drop issues with nested lists #16671

shesse opened this issue Aug 2, 2019 · 52 comments
Labels
area: cdk/drag-drop P2 The issue is important to a large percentage of users, with a workaround

Comments

@shesse
Copy link

shesse commented Aug 2, 2019

Reproduction

https://stackblitz.com/edit/angular-drag-and-drop-child-groups-fxzjja?file=app%2Fcdk-drag-drop-connected-sorting-group-example.html

Steps to reproduce:

  1. Try to drag element "Brush teeth" to before "Drive to office" or before "Walk dog"
  2. this will not be possible

Expected Behavior

Drag / Drop to all lists contained in cdkDropListConnectedTo

Actual Behavior

Items from inner Lists may only be dropped to outer lists, not to sibling or their own lists

Environment

  • Angular: 8.1.2
  • CDK/Material: 8.1.2
  • Browser(s): Firefox / Chrome
  • Operating System (e.g. Windows, macOS, Ubuntu): Debian Linux, Windows

Additional Info

When using CDK drag / drop (8.0.0 up to and including 8.1.2) with nested
lists, two issues arise that prevent an easy use of the feature:

a) dragging does not work from inner list to inner list

b) dragging does not work in the drop item's source list

Explanation and proposed solution.

For above case a) mentioning it in the documentation may be sufficient -
it can be influenced from the outside. However, a more intelligent
solution (e.g. identifying the toplevel sublist) would be nice.

For case b), filtering out the source list in
src/cdk/drag-drop/directives/drop-list.ts seems to cause the issue:
ref.connectedTo(siblings.filter(drop => drop && drop !==
this).map(list => list._dropListRef))
My hack replaces this by an unfiltered list - in quite an ugly manner.

@shesse shesse changed the title in src/cdk/drag-drop/directives/drop-list.ts seems to CDK Drag Drop issues with nested lists Aug 2, 2019
@mmalerba mmalerba added the needs triage This issue needs to be triaged by the team label May 20, 2020
@denkerny
Copy link

it doesnt work well, cant get needed element index in nested array.

@james-logan
Copy link

+1

@crisbeto crisbeto added P2 The issue is important to a large percentage of users, with a workaround and removed needs triage This issue needs to be triaged by the team labels Jul 12, 2020
@crisbeto
Copy link
Member

Setting as a P2 based on the number of duplicate issue we get about this.

@crisbeto
Copy link
Member

From @matthewerwin in #19085:

Reproduction

Use StackBlitz to reproduce your issue:

https://stackblitz.com/edit/angular-rf6gg6

Steps to reproduce:

  1. Look at the stack blitz.
  2. Markup has an effect on ability to even start a drag without any console or compile indicator that cdkDrag and cdkDropList cannot be on the same drag element (they could be in v8 --v9 allows them on the same container for the "drop element")
  3. Exit/Enter aren't reliably detected based on the client-rect

Expected Behavior

What behavior were you expecting to see?

  • Drop zone container should always be detected as soon as the mouse goes over the element.
  • If use of cdkDrag/cdkDropList combo only works in certain situations (e.g. codebase looks for this._container on a dragref in v9...which appears to only get set if cdkDropList is on a parent element). That should either be documented and also the compiler should write an error to the console...or the code should be adjusted to set this._container to the element itself to handle the directive combo on the same element.

Actual Behavior

What behavior did you actually see?

  • No errors to console or at compile time with directives appended in any fashion.
  • v9 does not detect a drop-zone accurately at all for entry or exit. Shaking the mouse a bunch over drop zones or redragging repeatedly will randomly then detect it correctly...though continuing to drag around shows the same inability to detect the drop-zones.

@f1nality
Copy link

+1

@admosity
Copy link
Contributor

admosity commented Oct 21, 2020

So it seems like there is some internal priority here. I've gotten pretty decent results with sorting the lists by dom depth as a workaround:

    this.connectedDroplists$ = this._connectedDroplists$.pipe(
      debounceTime(16),
      tap((connectedDropLists) => {
        // sort the lists by depth to prefer deeper lists
        connectedDropLists.sort((a, b) => {
          return (
            this._getDepth(b.element.nativeElement) -
            this._getDepth(a.element.nativeElement)
          );
        });
      }),
      shareReplay(1)
    ) as BehaviorSubject<CdkDropList[]>;

It seems that no matter what I do, I can't seem to get it to re-order within the source droplist where I grabbed an item from.

Sorting the droplists by element depth and using the enforce hack that @shesse has provided, I have gotten an excellent result even with completely overlapping nested drop lists (looks like I had a gap that allowed for me to add to the parent). Thank you @shesse!

I suppose the reversing makes sense because the deeper the list is the later it would be registered (resulting it to be at the end).

Oct-21-2020 14-21-08

@soren-petersen
Copy link

Hi everyone,

I ran into this issue a few days ago and decided to make an attempt at fixing it :-) I have tried to summarise my "analysis" of the issue and a proposal for how to solve it below. I apologise for - to some extent - repeating what has already been said above. I have also attached "Monkey Patch" that seems to fix the issue for Angular Material v11.0.3. I will try to transform it into a pull request shortly.

The main issue seems to be with the implementation of _getSiblingContainerFromPosition in DropListRef which is used by _updateActiveDropContainer in DragRef to identify whether a draggable item is being moved over a new (and connected) drop container and, if it is, which drop container the item is being moved over.

Based on the code and comments in _updateActiveDropContainer (which is the only place where _getSiblingContainerFromPosition is called), _getSiblingContainerFromPosition is expected to return the DropListRef for the drop container that the item is being moved over, provided that it is not the drop container that it currently belongs to. The method is expected to return 'undefined' if the item is being moved over the current drop container or if it is being moved over something that is not a drop container.

The current implementation of _getSiblingContainerFromPosition does this by returning the first connected drop container ("sibling") in the list of siblings that has a client rect that contains the current position of the item being dragged and for which enterPredicate returns true (I know this is somewhat simplified, but the still argument holds). This approach works perfectly well as long the client rects of the drop containers are not overlapping.

However, when drop containers are nested (and therefore overlapping), this causes two problems:

  1. If an outer drop container is listed before an inner drop container in the list of siblings, _getSiblingContainerFromPosition will return the outer drop container when the item is moved over the inner container. Hence, the list of siblings must be ordered to list inner drop containers before outer drop containers (as mentioned above)
  2. As the list of siblings does not include the current drop container ("this"), _getSiblingContainerFromPosition will always return the outer drop container when an item of the inner drop container is dragged over the inner container. As a result, it is not possible to re-order items in an inner drop container.

The first issue can be addressed by ordering the items of 'cdkDropListConnectedTo' (as mentioned above). The current drop container can, however, not be added to siblings due to the code in _setupInputSyncSubscription of DropList that actively removes 'this' from the list (as pointed out by @shesse). The hack described by @shesse above addresses this issue by forcing the 'this' entry back into the siblings list. This approach does, however, bring the risk of breaking other code as the siblings list is clearly not intended to include 'this'.

My proposal would be to fix the issue by rewriting _getSiblingContainerFromPosition such that it correctly handles nested drop containers without any assumptions on the order of the siblings list. This could be done in the following way:

  • Create a list containing all siblings and 'this'
  • Filter the list so that it only contains those drop containers that has a client rect that contains the current item position
  • Sort the remaining items based on the DOM hierarchy so that outer drop containers appear before the inner drop containers that they contain
  • Extract the last item of the sorted list
  • Check whether the extracted item is indeed a match using _canReceive
  • if _canReceive returns true, return the extracted item otherwise return 'undefined'

I have created a "Monkey Patch" that seems to work with Angular Material version 11.0.3 (attached). It should be straight forward to transform this into a pull request. I will gladly do so (but never having done a pull request before, I'll have to read up on how to do it)

nested-drag-drop-patch.ts.zip

I suspect thet the code breaks more than one Angular Material coding guideline. It is also a quick write-up so I would not be surprised if there are some issues that need to be resolved. A proper code review might be in order ... :-)

@FritzHerbers
Copy link

3 years ago I tried to use drag-drop, but got stuck, some issues were closed due to inactivity. I might have a need in drag-drop in half a year, maybe I will immediately look for another implementation. So I am far off, what is currently implemented or which issues are open and solved, and if this is the correct place for:

  • programmatic callback if it is allowed to drop

soren-petersen added a commit to soren-petersen/angular-material-components that referenced this issue Jan 9, 2021
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
@soren-petersen
Copy link

@FritzHerbers

I propose you start by browsing through the excellent documentation for CDK Drag and Drop. I think you will find that the drag-drop feature has improved significantly over the last couple of years. You will likely also find the answer to your callback question there. Have a look at cdkDropListEnterPredicate and cdkDropListSortPredicate.

The issue tracker and this particular issue is, however, not the right place for a general discussion about the state of cdk/drag-drop nor your specific question of support for a programatic callback. If you have further questions, community support requests can be submitted at StackOverflow or in the Angular Material gitter channel.

Please refer to the official instruction for support questions for more details.

soren-petersen added a commit to soren-petersen/angular-material-components that referenced this issue Jan 9, 2021
soren-petersen added a commit to soren-petersen/angular-material-components that referenced this issue Jan 9, 2021
soren-petersen added a commit to soren-petersen/angular-material-components that referenced this issue Jan 9, 2021
@Akhil-Myadarapu
Copy link

Akhil-Myadarapu commented Jan 12, 2021

hi @soren-petersen. can you attach stackblitz link for the same.
It will be very helpful. thanks

@LeahPike
Copy link

Just run into this myself, would be great to have please :)

@slavede
Copy link

slavede commented Oct 20, 2022

I have solution. So I have multiple, dynamic nested lists (I really don't know in ahead how many there will be). In the drag&drop code I've noticed they are removing "itself" from siblings list wherever they can :) so I did this:

  public ngAfterViewInit() {
    const dropListRef = this.cdkDropList._dropListRef;
    const originalConnectedTo: any = dropListRef.connectedTo;

    dropListRef['connectedTo'] = ((connectedTo) => {
      originalConnectedTo.apply(dropListRef, [connectedTo]);

      const targetIndex = this.allDropListsIds.findIndex(id => id === this.listId);
      dropListRef['_siblings'].splice(targetIndex, 0, dropListRef);
      return dropListRef;
    }) as any;
  }

Where allDropListsIds is the list of ids for every list we want to be connected
Works fine for now :)

@BenRacicot
Copy link

@slavede that's great! Could you put together a Stackblitz with that for everyone?

@SkoZombie
Copy link

I think @slavede 's work is awesome, but we really need to pressure the CDK team to merge a fix for this. It's been over 3 years now and it's still broken.

Does anyone have any idea who we need to bribe to actually get them to pay attention to closing out bugs rather than focusing on new features?

@wupaz
Copy link

wupaz commented Oct 21, 2022

Maybe create a gofundme for "bribe dev team" :)

@Zodiark1020
Copy link

Zodiark1020 commented Jan 27, 2023

I'm using the patch to resolve the "nesting" issue but it has some issues on recognizing the target drop area, in some cases.
I'm trying to debug the patch as long as no other solutions come up.

Is there any news about this?

@haroldcampbell
Copy link

I have solution. So I have multiple, dynamic nested lists (I really don't know in ahead how many there will be). In the drag&drop code I've noticed they are removing "itself" from siblings list wherever they can :) so I did this:

  public ngAfterViewInit() {
    const dropListRef = this.cdkDropList._dropListRef;
    const originalConnectedTo: any = dropListRef.connectedTo;

    dropListRef['connectedTo'] = ((connectedTo) => {
      originalConnectedTo.apply(dropListRef, [connectedTo]);

      const targetIndex = this.allDropListsIds.findIndex(id => id === this.listId);
      dropListRef['_siblings'].splice(targetIndex, 0, dropListRef);
      return dropListRef;
    }) as any;
  }

Where allDropListsIds is the list of ids for every list we want to be connected Works fine for now :)

This helped me tremendously. Thank you! @slavede

@fclmman
Copy link

fclmman commented Mar 1, 2023

@slavede solution does not work for horizontal dropLists for some reason

@KingR1
Copy link

KingR1 commented Mar 30, 2023

I have solution. So I have multiple, dynamic nested lists (I really don't know in ahead how many there will be). In the drag&drop code I've noticed they are removing "itself" from siblings list wherever they can :) so I did this:

  public ngAfterViewInit() {
    const dropListRef = this.cdkDropList._dropListRef;
    const originalConnectedTo: any = dropListRef.connectedTo;

    dropListRef['connectedTo'] = ((connectedTo) => {
      originalConnectedTo.apply(dropListRef, [connectedTo]);

      const targetIndex = this.allDropListsIds.findIndex(id => id === this.listId);
      dropListRef['_siblings'].splice(targetIndex, 0, dropListRef);
      return dropListRef;
    }) as any;
  }

Where allDropListsIds is the list of ids for every list we want to be connected Works fine for now :)

Hi @slavede :) I know quite a lot time was gone... but what are this.cdkDropList and this.listId?

@slavede
Copy link

slavede commented Mar 30, 2023

this.cdkDropList is a drop list used inside this component and listId is recursively passed list of every cdk drop list used.

Where component is a wrapper around cdkDropList. Now every inner component receives listIds of every list id used before and then it adds itself to that list. And this listId is passed as reference so every wrapped component gets that

@diego-rapoport
Copy link

Any news on this? Couldn't get any workarounds to really work on my case and examples are always at older versions of angular.

@FritzHerbers
Copy link

FritzHerbers commented Jun 16, 2023

I am not a big user of components, I only needed the drag, stepper and tabs and quickly noticed they fail to have some functionality.
#16671
#9733
and some others

I opened #15958 with the hope that with github labels a developer could easily see which issues exist on a specific component and fix them when they are anyway updating the component. I have seen many updates on the stepper and drag component, but none of the issues of interest were fixed.

This issue has an open pull request (January 2021): #21526
Needs a review: better don't have a fix, as a fix which might not work 100% or break something.
Also the unassignment of crisbeto 2 weeks ago, the original developer of this component, is questionable.

Some of the issues I had were from 2018 and still open or closed/unresolved. I have seen several new components added and work was done on performance, but none of my issues of interest were fixed. (Independent of improvements made in those components)

I tried to fix some of the issues, and had a hard time, the code is very cryptic, fails any inline documentation, and you need to know and understand the eco-system of components in depth. This might also be the cause, what i could watch, that development is only crafted by a few developers, compared to the number of users (contributors:700, users:700'000, don't know the accuracy/measuring method of these numbers). Also guidance of a core developer on how to fix an issue, which could evolve to a pull request is somehow missing. Besides that, a lot of pull requests don't even make it to merge, not that they might not be feasible, but of age (closed/unmerged flag and with no reason, when not otherwise solved this is a slap in the face for the PR author).

I appreciate all work done, but lost hope that these issues will ever get fixed and I think only that someone occasionally asks, keeps some of these issues open and not automatically closed by the infamous "closing" bot. I have seen, in my eyes important issues been closed, and have seen some issues being reopened by other authors.

@SkoZombie
Copy link

I'm really disappointed that the NG team have ignored long standing bugs like this. I've never looked at the code, but from what @FritzHerbers says, it seems like they have HUGE technical debt they're totally ignoring which is really concerning as someone who sells software that uses Angular.

Hopefully the team can address it. I would encourage anyone who knows people in the angular team, to reach out to them or at least try to get some attention on this and similar issues. If we all try to contact them, they might actually put some focus on some of these old issues.

@BenRacicot
Copy link

Well it's not Angular per-say, it's Angular material components. Which has always been very poorly maintained. The CDK is like a 6 out of 10 but Material is like a 2 out of 10 IMO. Use Angular because it's super-pro but seek alternatives to Material for pre-built components.

@BenRacicot
Copy link

We've got a working version of the drag-drop CDK with nesting into github.com/Recruitler/drag-drop main.
Would love some review and critique/improvement.

@MatviiStelmakh
Copy link

@shesse thanks for the workaround, you saved my day

@xXNickznXx
Copy link

For anyone using the monkey-patch from #16671 (comment): 17.0.5 introduced a breaking change by renaming _clientRect to _domRect, so you'll need to do that in the patch too

@t0m-4
Copy link

t0m-4 commented Feb 27, 2024

Thanks @xXNickznXx and @soren-petersen

I update the "Monkey Patch" from @soren-petersen to work with angular@17

nested-drag-drop-patch.zip

@MeMeMax
Copy link

MeMeMax commented Apr 24, 2024

I would appreciate an official fix from the angular team.

@anthime2010
Copy link

anthime2010 commented May 28, 2024

Hello,

With the idea of @slavede , and this example on stackblitz, I made a Mix of the two and I obtained

 this.dragAndDropService
      .getCdkDropListObservable()
      .pipe(takeUntil(this.ngUnsubscribe))
      .subscribe((cdkListId: CdkDropList[]) => {
        this.cdkDropLists = cdkListId;
        this.cdkDropLists.forEach((dropList) => {
          let dropListRef = dropList._dropListRef;
          let siblings = this.cdkDropLists.map((dl) => dl?._dropListRef);
          siblings = siblings.sort( (a,b) => a.data.id.localeCompare(b.data.id))
          dropListRef._getSiblingContainerFromPosition = (item, x, y) =>
                siblings.find((sibling) => sibling._canReceive(item, x, y));
            });

        });

I ve got Dnd service, which provide an observable to react on every update, then I overload '_getSiblingContainerFromPosition' to select the correct DropList. The only constraint is to be sure that the parent DropList is at the last place in the siblings list. If it'is not the case , it may provide the top parent (if before whished child drop list). Put the parent at the end of the list (with specific naming of the top parent drop list for example xxx_parent_drop_list)

Thanks to all for your help

@shyamal890
Copy link

shyamal890 commented Jun 21, 2024

Thanks @xXNickznXx and @soren-petersen

I update the "Monkey Patch" from @soren-petersen to work with angular@17

nested-drag-drop-patch.zip

MonkeyPatch not working when the nested element is dynamically rendered with a delay. In such cases .domRef and .container are undefined

@BenRacicot
Copy link

Hi @crisbeto is there anything we, as the community, can do to get this as an official feature?

Currently I'm loading my own (forked) drag-drop for nesting.

@john-melin
Copy link

We just migrated to https://sortablejs.github.io/Sortable/#nested because of this.

@crisbeto
Copy link
Member

crisbeto commented Jan 9, 2025

@BenRacicot if anybody is looking to send a PR, I can help move it along. There's #21526 which stalled out a while ago and didn't have any tests.

@BenRacicot
Copy link

@crisbeto Thank you for responding, I've been through it on this one :) please allow me to explain.
I built a prototype with sortablejs a long time ago but that lib is an enormous mess. I spent about 80 hours trying to build it into SortableTS but gave up, it needed way too much time, basically we could have started from scratch easier.

So I settled on forking the Angular CDK's drag-drop package and built-in nested drag and drop. It works very well actually.

Perhaps your team would want to review it and merge it in? I'm about to launch my project and I'm not sure I have the time to create a proper PR at this time but the fork is here https://github.com/Recruitler/drag-drop

Would love to get this feature in the official CDK so I can commit my work to the official repo and not my fork.

@andrewseguin
Copy link
Contributor

An attempt at adding this feature was made in #21526, but has since fallen out of sync with the repo and lacked tests. We'll close the PR but we encourage anyone in the community to re-open a rebased version with tests to get this merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: cdk/drag-drop P2 The issue is important to a large percentage of users, with a workaround
Projects
None yet