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

bug(drag-drop): cdkDragPreview matchSize does not always match size due to view container insertion #19060

Closed
Achilles1515 opened this issue Apr 13, 2020 · 2 comments · Fixed by #19062 or lingounet/testage#29
Assignees
Labels
P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent

Comments

@Achilles1515
Copy link

The below StackBlitz reproduction is a fork of the horizontal list official example. All that was added was a cdkDragPreview with the matchSize input property.

<div cdkDropList cdkDropListOrientation="horizontal" class="example-list" (cdkDropListDropped)="drop($event)">
  <div class="example-box" *ngFor="let timePeriod of timePeriods" cdkDrag>
    {{timePeriod}}
    <ng-template cdkDragPreview matchSize>
      <div class="example-box">
        {{timePeriod}}
      </div>
    </ng-template>
  </div>
</div>

(In this case, the preview element is basically the same thing as the cdkDrag element, which doesn't make too much sense, but it is just for demonstration purposes and could contain different elements/styles)

Upon dragging an item, the preview does not match the same size as the original item/placeholder.

GIF
(Notice the "Bronze Age" preview is not as wide causing text wrap)

The cause of this problem is the flexbox droplist container (where the items grow/shrink to fill the available space) combined with how the preview element is created.

The preview template is currently being stamped out in the cdkDrag injected view container.

private _createPreviewElement(): HTMLElement {
    const previewConfig = this._previewTemplate;
    const previewClass = this.previewClass;
    const previewTemplate = previewConfig ? previewConfig.template : null;
    let preview: HTMLElement;

    if (previewTemplate) {
      // <-- HERE -->
      const viewRef = previewConfig!.viewContainer.createEmbeddedView(previewTemplate,
                                                                       previewConfig!.context);
      viewRef.detectChanges();
      preview = getRootNode(viewRef, this._document);
      this._previewRef = viewRef;

      if (previewConfig!.matchSize) {
        matchElementSize(preview, this._rootElement);
      } else {
        //...
      }
    } else {
      //...
    }
//...
}

If you put a breakpoint immediately after the viewRef.detectChanges() line, you will see the preview becoming another item in the list, as creating and inserting the embedded view in the view container essentially just tacks on another sibling.

image
("Bronze Age" was started to be dragged)

Since the preview item is added as a sibling, flexbox recalculates the width of all the items. The matchSize functionality is then executed with these modified (smaller, in this case) dimensions.


Possible Solution

Is a view container even necessary here?
Changing the above code to the following appears to work just fine.

 if (previewTemplate) {
      // <-- HERE -->
      //const viewRef = previewConfig!.viewContainer.createEmbeddedView(previewTemplate,
      //                                                                 previewConfig!.context);
      const viewRef = previewTemplate.createEmbeddedView(previewConfig!.context);
      viewRef.detectChanges();

This creates the same drag preview viewRef, except it is not immediately attached to the DOM.

The reproduction link contains some copied source code to preview this change - just need to comment/uncomment those lines.

Alternatively, the dragged item could be measured before the preview is created. But if there is no reason to use a view container, the above change seems like the more optimal approach.

Reproduction

StackBlitz

Expected Behavior

Preview to match size.

Actual Behavior

Preview does not match size.

Environment

  • Angular:
  • CDK/Material: 9.2
  • Browser(s):
  • Operating System (e.g. Windows, macOS, Ubuntu):
@Achilles1515 Achilles1515 added the needs triage This issue needs to be triaged by the team label Apr 13, 2020
@crisbeto crisbeto self-assigned this Apr 13, 2020
@crisbeto crisbeto added has pr P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent and removed needs triage This issue needs to be triaged by the team labels Apr 13, 2020
crisbeto added a commit to crisbeto/material2 that referenced this issue Apr 13, 2020
…tainer

We create a custom preview through `ViewContainerRef.createEmbeddedView` and we move it to the correct place in the DOM. The problem is that for the brief period that it's in the DOM, we also measure the host node if the `matchSize` option is set. In some cases (e.g. a flex container), this can throw off the size. These changes switch to measuring the size ahead of time when necessary.

Fixes angular#19060.
@crisbeto
Copy link
Member

Thank you for the detailed report 👍 . The idea behind inserting the v thiewrough the ViewContainerRef is to ensure that it's inside the same view/DI hierarchy as drag item. I agree that ideally we'd just create the element outside the DOM, but an alternate solution is to measure the element before the preview has been created.

jelbourn pushed a commit that referenced this issue Apr 20, 2020
…tainer (#19062)

We create a custom preview through `ViewContainerRef.createEmbeddedView` and we move it to the correct place in the DOM. The problem is that for the brief period that it's in the DOM, we also measure the host node if the `matchSize` option is set. In some cases (e.g. a flex container), this can throw off the size. These changes switch to measuring the size ahead of time when necessary.

Fixes #19060.
jelbourn pushed a commit that referenced this issue Apr 20, 2020
…tainer (#19062)

We create a custom preview through `ViewContainerRef.createEmbeddedView` and we move it to the correct place in the DOM. The problem is that for the brief period that it's in the DOM, we also measure the host node if the `matchSize` option is set. In some cases (e.g. a flex container), this can throw off the size. These changes switch to measuring the size ahead of time when necessary.

Fixes #19060.

(cherry picked from commit cd75979)
soro-google pushed a commit to soro-google/components that referenced this issue Apr 24, 2020
…tainer (angular#19062)

We create a custom preview through `ViewContainerRef.createEmbeddedView` and we move it to the correct place in the DOM. The problem is that for the brief period that it's in the DOM, we also measure the host node if the `matchSize` option is set. In some cases (e.g. a flex container), this can throw off the size. These changes switch to measuring the size ahead of time when necessary.

Fixes angular#19060.
@angular-automatic-lock-bot
Copy link

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators May 21, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent
Projects
None yet
2 participants