Skip to content

fix(drag-drop): avoid generating elements with duplicate ids #13489

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

Merged
merged 1 commit into from
Nov 10, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion src/cdk/drag-drop/drag-drop.md
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,10 @@ restrict the user to only be able to do so using a handle element, you can do it
### Customizing the drag preview
When a `cdkDrag` element is picked up, it will create a preview element visible while dragging.
By default, this will be a clone of the original element positioned next to the user's cursor.
This preview can be customized, though, by providing a custom template via `*cdkDragPreview`:
This preview can be customized, though, by providing a custom template via `*cdkDragPreview`.
Note that the cloned element will remove its `id` attribute in order to avoid having multiple
elements with the same `id` on the page. This will cause any CSS that targets that `id` not
to be applied.

<!-- example(cdk-drag-drop-custom-preview) -->

Expand Down
27 changes: 27 additions & 0 deletions src/cdk/drag-drop/drag.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -831,6 +831,19 @@ describe('CdkDrag', () => {
expect(preview.parentNode).toBeFalsy('Expected preview to be removed from the DOM');
}));

it('should clear the id from the preview', fakeAsync(() => {
const fixture = createComponent(DraggableInDropZone);
fixture.detectChanges();
const item = fixture.componentInstance.dragItems.toArray()[1].element.nativeElement;
item.id = 'custom-id';

startDraggingViaMouse(fixture, item);

const preview = document.querySelector('.cdk-drag-preview')! as HTMLElement;

expect(preview.getAttribute('id')).toBeFalsy();
}));

it('should not create a preview if the element was not dragged far enough', fakeAsync(() => {
const fixture = createComponent(DraggableInDropZone, [], 5);
fixture.detectChanges();
Expand Down Expand Up @@ -988,6 +1001,20 @@ describe('CdkDrag', () => {
expect(placeholder.parentNode).toBeFalsy('Expected placeholder to be removed from the DOM');
}));

it('should remove the id from the placeholder', fakeAsync(() => {
const fixture = createComponent(DraggableInDropZone);
fixture.detectChanges();
const item = fixture.componentInstance.dragItems.toArray()[1].element.nativeElement;

item.id = 'custom-id';

startDraggingViaMouse(fixture, item);

const placeholder = document.querySelector('.cdk-drag-placeholder')! as HTMLElement;

expect(placeholder.getAttribute('id')).toBeFalsy();
}));

it('should not create placeholder if the element was not dragged far enough', fakeAsync(() => {
const fixture = createComponent(DraggableInDropZone, [], 5);
fixture.detectChanges();
Expand Down
12 changes: 10 additions & 2 deletions src/cdk/drag-drop/drag.ts
Original file line number Diff line number Diff line change
Expand Up @@ -566,7 +566,7 @@ export class CdkDrag<T = any> implements AfterViewInit, OnDestroy {
const element = this._rootElement;
const elementRect = element.getBoundingClientRect();

preview = element.cloneNode(true) as HTMLElement;
preview = deepCloneNode(element);
preview.style.width = `${elementRect.width}px`;
preview.style.height = `${elementRect.height}px`;
preview.style.transform = getTransform(elementRect.left, elementRect.top);
Expand Down Expand Up @@ -596,7 +596,7 @@ export class CdkDrag<T = any> implements AfterViewInit, OnDestroy {
);
placeholder = this._placeholderRef.rootNodes[0];
} else {
placeholder = this._rootElement.cloneNode(true) as HTMLElement;
placeholder = deepCloneNode(this._rootElement);
}

placeholder.classList.add('cdk-drag-placeholder');
Expand Down Expand Up @@ -803,3 +803,11 @@ interface Point {
function getTransform(x: number, y: number): string {
return `translate3d(${x}px, ${y}px, 0)`;
}

/** Creates a deep clone of an element. */
function deepCloneNode(node: HTMLElement): HTMLElement {
const clone = node.cloneNode(true) as HTMLElement;
// Remove the `id` to avoid having multiple elements with the same id on the page.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this cause issues if the element is styled based on the id?

Should we at least document this in the md file for dragdrop?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll add a note about it, but we've been doing this in the MatIconRegistry for a long time now and it hasn't come up.

clone.removeAttribute('id');
return clone;
}