fix(cdk/drag-drop): don't stop event propagation unless nested

[crisbeto]

Some time ago we added logic to stop event propagation so that nested drag items don't trigger multiple sequences. Stopping propagation for all events seems to interfere multiple other use cases so these changes add some logic so that we only stop propagation when items are nested.

There was something similar in #19334, but I decided not to move forward with it, because it required consumers to know the internals of the drag-drop module, whereas this approach can do it automatically.

Fixes #19333.

[Achilles1515]

this._config.parentDragRef will leak memory if the "nested" child outlives the parent (dynamic component creation/transplanted views).

Also, stopPropagation doesn't need to be called if the parent drag ref is disabled, unless a grandparent drag ref exists that is enabled.

On top of walking up the drag ref chain, even if a parent drag ref is enabled, its handles need to be checked to be enabled and to actually be encapsulating the click/touch, otherwise there is no need to stop propagation.

If stopPropagation is not called, is native HTML5 dragging of parent elements still being disabled?

[crisbeto]

I think the parent leaking is unlikely, because usually the child components will be removed together with the parent. It technically can happen if the DragRef was created manually, but that's not the most common case.

Regarding checking whether the parent is disabled/the handles are disabled, I'm not sure it's worth the extra effort at the moment, because even if we were to stop propagation, the parent is disabled so you can't start dragging it anyway.

As for the HTML5 dragging, stopPropagation doesn't have an effect on it. There's a preventDefault call a bit further down that disables it.

[jelbourn]

LGTM

[lincolnthree]

VERY NICE. This is a great solution! Thank you @annieyw.

[luastoned]

How do I reverse the effects of this merge request? This completely breaks nested / multi-level drag-drop.
When I try to drag a nested child-element / cdkDrag inside an existing container that's also a cdkDrag both get dragged.

[crisbeto]

The idea with this PR was that it would still stop propagation when it's nested. Can you post an example where it breaks?

[luastoned]

The rough outlines are:

<div
  cdkDropList
  [cdkDropListData]="headerRows"
  (cdkDropListDropped)="elementDrop($event)"
  [cdkDropListConnectedTo]="layoutContainerList"
>
  <div cdkDrag *ngFor="let obj of headerRows; let idx = index; trackBy: trackByDrag">
    <ng-container *ngTemplateOutlet="dragContainer; context: { obj: obj, idx: idx }"></ng-container>
  </div>
</div>

<ng-template #dragContainer let-obj="obj" let-idx="idx" let-position="position" let-subrow="subrow">
  <div class="flex">
    <div
      [id]="obj.id"
      cdkDropList
      [cdkDropListData]="obj.children"
      (cdkDropListDropped)="elementDrop($event)"
      [cdkDropListOrientation]="getOrientation(obj.style)"
      [cdkDropListConnectedTo]="connectedElementList"
    >
      <ng-container *ngFor="let child of obj.children; let idx = index">
        <div cdkDrag *ngIf="child.type !== 'container'">
          <ng-container *ngTemplateOutlet="dragElement; context: { child: child }"></ng-container>
        </div>

        <div cdkDrag *ngIf="child.type === 'container'">
          <ng-container *ngTemplateOutlet="dragContainer; context: { obj: child, idx: idx, subrow: true }"></ng-container>
        </div>
      </ng-container>
    </div>
  </div>
</ng-template>

Here is a temporary fix:

@ViewChildren(CdkDrag) set getDragElements(cdkDragElements: QueryList<CdkDrag>) {
  for (const dragRef of cdkDragElements.toArray()) {
    // @ts-ignore
    dragRef._dragRef?._config?.parentDragRef = true;
  }
}

[Achilles1515]

I'm also confused as to how and why this got merged. It can clearly leak memory.
Why not just make it configurable by the user as a:

CdkDrag input property > CdkDropList input property > DragRefConfig property

[crisbeto]

As I mentioned above, the configuration option is sub-optimal because it's coupled to the current implementation and people would have to know about the internals of the CDK. The option can become useless if we changed the internals. As for the memory leak, it's unlikely to have a child which outlives the parent.

I'll look into a solution for handling the case from @luastoned

[lincolnthree]

A configurable property would be ideal -- that was my original feature request & PR, but the angular team decided that was "too much control" for users to have about internals of the library. Encapsulating technical issues is great, but lack of fine-grained control in low level features of CDK will continue to cause challenges and ongoing issues like this, IMO.

I can see situations where you'd even want nested containers to propagate events to an intermediate element. So... it's just a matter of what the library maintainers want to support or not. That's just my perspective.

[Achilles1515]

As for the memory leak, it's unlikely to have a child which outlives the parent.

This could maybe be an acceptable answer if the code was for a specific application, but I don't understand how this is acceptable for a general purpose library of a framework that supports dynamic components. If the decision is made to not cleanup this implementation, then can you at least make DragRef._config public, or make another public API, so that users can clear the hanging CdkDrag reference?

[shervin01]

This change has broken dragging on mobile devices. On mobile dragging will also trigger scroll. To reproduce go here on a mobile device or use chrome's toggle device tool bar: https://material.angular.io/cdk/drag-drop/examples then scroll down a bit and then try to drag one of the boxes. You can see that screen also scrolls.

Same issue is not present in: https://v10.material.angular.io/cdk/drag-drop/examples

[angular-automatic-lock-bot]

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}