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

[CdkDrag] Dragging the last item will break the structure of the list. #14148

Open
shlomiassaf opened this issue Nov 15, 2018 · 7 comments
Open
Labels
area: cdk/drag-drop P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent

Comments

@shlomiassaf
Copy link
Contributor

shlomiassaf commented Nov 15, 2018

Bug, feature request, or proposal: Bug

What is the expected behavior?

List structure remains identical after drag/drop

What is the current behavior?

List structure breaks when dragging the last item in a list where the
drop container is not the direct father (and when not using NGFOR)

What are the steps to reproduce?

https://stackblitz.com/edit/angular-9xpwbj

Which versions of Angular, Material, OS, TypeScript, browsers are affected?

7.0.4

Is there anything else we should know?

This bug is in the cleanup routine of the CdkDrag compoment, if there is no "nextSibling", i.e. last item in drag container, and the drop container is not the father (direct parent) of the list it will append the dragged item to the container which is not the direct list owner.

This will not happend when using *ngFor because *ngFor will add a comment element after each list item which causes CdkDrag to think it's not the last sibling.


I also think that the fact it doesn't happen in*ngFor is a bug, the nextSibling should reflect the next draggable item but the code queries for any node... instead of element.nextSibling it should probably be element.nextElementSibling


@shlomiassaf
Copy link
Contributor Author

shlomiassaf commented Nov 15, 2018

Proposed Solution

The first goal of the cleanup routine is to restore the DOM structure to its state before doing the drag & drop, letting the user handle the dropped event and decide if to move or not to move the item. (in the demo it's through ngFor and CD)

    this._rootElement.style.display = '';

    if (this._nextSibling) {
      this._nextSibling.parentNode!.insertBefore(this._rootElement, this._nextSibling);
    } else {
      this._initialContainer.element.nativeElement.appendChild(this._rootElement);
    }

In a single drop container scenario the solution should be simple, just don't do anything if there is no last sibling. We don't remove the drag element from the DOM anyway, so it should stay as is.

Which means, removing this line of code:

this._initialContainer.element.nativeElement.appendChild(this._rootElement);

In a multi drop-container scenario it's a bit more complex because a DOM element moves between drop containers while dragged and we need to return it to the original position it was in.

With both scenarios in mind, I think the solution should be adding an additional reference to the initial direct container and using it in the cleanup routine.

When starting the drag sequence (_initializeDragSequence()) we save a reference to the drop container (this._initialContainer = this.dropContainer;) because the container might change, we use that for appending in the cleanup... which is wrong. We need to reference the direct DOM element container as well: this._initialRootElementParent = this._rootElement.parentNode and the cleanup routine:

    this._rootElement.style.display = '';

    if (this._nextSibling) {
      this._nextSibling.parentNode!.insertBefore(this._rootElement, this._nextSibling);
    } else {
      this._initialRootElementParent.appendChild(this._rootElement);
    }

@crisbeto crisbeto self-assigned this Nov 15, 2018
@shlomiassaf
Copy link
Contributor Author

@crisbeto I have a PR, would you like me to post it?

@crisbeto
Copy link
Member

Sure.

@shlomiassaf
Copy link
Contributor Author

shlomiassaf commented Nov 15, 2018

Delayed a bit, found another issue.

With connected containers, when both containers are not direct parents of the dragged elements, we need to be able to detect the direct element that is the parent of the dragged items.

For example:

  <div cdkDropListGroup>
    <div cdkDropList>
      <div class="inner-drop-list">
        <div [cdkDragData]="item" cdkDrag>Zero</div>
        <div [cdkDragData]="item" cdkDrag>One</div>
        <div [cdkDragData]="item" cdkDrag>Two</div>
        <div [cdkDragData]="item" cdkDrag>Three</div>
      </div>
    </div>

    <div cdkDropList>
      <div class="inner-drop-list">
        <div [cdkDragData]="item" cdkDrag>Four</div>
        <div [cdkDragData]="item" cdkDrag>Five</div>
        <div [cdkDragData]="item" cdkDrag>Six</div>
      </div>
    </div>
  </div>

Now when moving between containers the enter() method will use:

this.element.nativeElement.appendChild(placeholder);

When this.element is the cdkDropList host, not good.

I will use the same approach taken by cdkDrag using cdkDragRootElement.

  /**
   * Selector that will be used to determine the direct container element, starting from
   * the `cdkDropList` element and going down the DOM. Passing an alternate direct container element
   * is useful when the `cdkDropList` is not the direct parent (i.e. ancestor but not father)
   * of the draggable elements.
   */
  @Input('cdkDropListDirectContainerElement') directContainerElement: string;

It will do the same as cdkDragRootElement but instead of going up it will search down and with querySelector.

      let element = this.element.nativeElement;
      if (this.directContainerElement) {
        element = element.querySelector(this.directContainerElement) || element;
      }
      element.appendChild(placeholder);

@crisbeto
Copy link
Member

Let's take these two issues one at a time. Usually we try to avoid having PRs that span multiple issues since it makes it harder to diagnose presubmit issues.

shlomiassaf added a commit to shlomiassaf/material2 that referenced this issue Nov 15, 2018
Adds the `cdkDropListDirectContainerElement` input which allows consumers to pass in a selector that will be used to determine which elements is going to be the direct parent (father) of the draggable items, starting from the `cdkDropList` element and going down the DOM.

This will also fix angular#14148, propely handling cleanup in `cdkDrag`.

closes angular#14148
shlomiassaf added a commit to shlomiassaf/material2 that referenced this issue Nov 15, 2018
Adds the `cdkDropListDirectContainerElement` input which allows consumers to pass in a selector that will be used to determine which elements is going to be the direct parent (father) of the draggable items, starting from the `cdkDropList` element and going down the DOM.

This will also fix angular#14148, propely handling cleanup in `cdkDrag`.

closes angular#14148
@shlomiassaf
Copy link
Contributor Author

@crisbeto Sorry, I already posted the PR.

I can remove it, but it's quite a small change in code overall. Most code is in the tests

@shlomiassaf
Copy link
Contributor Author

For anyone facing this issue, until resolved, the following gist contains a workaround for all issues in this PR + the ability to lazy bind a CdkDrag and a CdkDropList (i.e. out of scope binding between CdkDrag and CdkDropList as described in #14099)

@andrewseguin andrewseguin added the P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent label May 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: cdk/drag-drop P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent
Projects
None yet
3 participants