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

crisbeto · 2020-12-06T11:16:25Z

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.

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 angular#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 angular#19333.

Achilles1515 · 2020-12-10T14:43:49Z

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 · 2020-12-10T16:10:20Z

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

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. (cherry picked from commit e42d502)

lincolnthree · 2021-01-06T17:16:16Z

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

…ar#21227) 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 angular#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 angular#19333.

luastoned · 2021-01-21T15:56:39Z

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 · 2021-01-21T16:23:10Z

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 · 2021-01-21T16:34:04Z

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 · 2021-01-21T16:41:46Z

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 · 2021-01-21T16:51:17Z

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 · 2021-01-21T16:51:18Z

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 · 2021-01-21T18:06:09Z

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?

…ateOutlet Some logic was added in angular#21227 which would only `stopPropagation` when dragging if a parent drag item is detected. The problem is that we resolve the parent using DI which won't work if the item is projected through something like `ngTemplateOutlet`. These changes resolve the issue by falling back to resolving the parent through the DOM.

…ateOutlet (#21668) Some logic was added in #21227 which would only `stopPropagation` when dragging if a parent drag item is detected. The problem is that we resolve the parent using DI which won't work if the item is projected through something like `ngTemplateOutlet`. These changes resolve the issue by falling back to resolving the parent through the DOM.

…ateOutlet (#21668) Some logic was added in #21227 which would only `stopPropagation` when dragging if a parent drag item is detected. The problem is that we resolve the parent using DI which won't work if the item is projected through something like `ngTemplateOutlet`. These changes resolve the issue by falling back to resolving the parent through the DOM. (cherry picked from commit 92cb67e)

…ateOutlet (angular#21668) Some logic was added in angular#21227 which would only `stopPropagation` when dragging if a parent drag item is detected. The problem is that we resolve the parent using DI which won't work if the item is projected through something like `ngTemplateOutlet`. These changes resolve the issue by falling back to resolving the parent through the DOM.

shervin01 · 2021-01-30T03:46:30Z

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

…ateOutlet (angular#21668) Some logic was added in angular#21227 which would only `stopPropagation` when dragging if a parent drag item is detected. The problem is that we resolve the parent using DI which won't work if the item is projected through something like `ngTemplateOutlet`. These changes resolve the issue by falling back to resolving the parent through the DOM.

angular-automatic-lock-bot · 2021-03-02T00:09:11Z

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.}

crisbeto added P2 The issue is important to a large percentage of users, with a workaround target: patch This PR is targeted for the next patch release labels Dec 6, 2020

crisbeto assigned mmalerba Dec 6, 2020

google-cla bot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Dec 6, 2020

crisbeto assigned jelbourn Dec 15, 2020

jelbourn approved these changes Jan 5, 2021

View reviewed changes

jelbourn added the action: merge The PR is ready for merge by the caretaker label Jan 5, 2021

annieyw merged commit e42d502 into angular:master Jan 6, 2021

This was referenced Jan 6, 2021

feat(cdk/drag-drop): support configurable propagation of mousedown an… #19334

Closed

ngbDropdown not working with cdkDrag Angular 11.0.3 #21518

Closed

crisbeto mentioned this pull request Jan 21, 2021

fix(cdk/drag-drop): not detecting parent when projected using ngTemplateOutlet #21668

Merged

shervin01 mentioned this pull request Jan 30, 2021

bug(cdk/drag-drop): Cannot drag without scrolling on mobile devices. #21749

Closed

angular-automatic-lock-bot bot locked and limited conversation to collaborators Mar 2, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

crisbeto commented Dec 6, 2020

Achilles1515 commented Dec 10, 2020

crisbeto commented Dec 10, 2020 •

edited

Loading

jelbourn left a comment

lincolnthree commented Jan 6, 2021

luastoned commented Jan 21, 2021 •

edited

Loading

crisbeto commented Jan 21, 2021

luastoned commented Jan 21, 2021 •

edited

Loading

Achilles1515 commented Jan 21, 2021

crisbeto commented Jan 21, 2021

lincolnthree commented Jan 21, 2021 •

edited

Loading

Achilles1515 commented Jan 21, 2021

shervin01 commented Jan 30, 2021

angular-automatic-lock-bot bot commented Mar 2, 2021

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

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

Conversation

crisbeto commented Dec 6, 2020

Achilles1515 commented Dec 10, 2020

crisbeto commented Dec 10, 2020 • edited Loading

jelbourn left a comment

Choose a reason for hiding this comment

lincolnthree commented Jan 6, 2021

luastoned commented Jan 21, 2021 • edited Loading

crisbeto commented Jan 21, 2021

luastoned commented Jan 21, 2021 • edited Loading

Achilles1515 commented Jan 21, 2021

crisbeto commented Jan 21, 2021

lincolnthree commented Jan 21, 2021 • edited Loading

Achilles1515 commented Jan 21, 2021

shervin01 commented Jan 30, 2021

angular-automatic-lock-bot bot commented Mar 2, 2021

crisbeto commented Dec 10, 2020 •

edited

Loading

luastoned commented Jan 21, 2021 •

edited

Loading

luastoned commented Jan 21, 2021 •

edited

Loading

lincolnthree commented Jan 21, 2021 •

edited

Loading