Skip to content

Commit

Permalink
fix(cdk/drag-drop): remove preview wrapper
Browse files Browse the repository at this point in the history 
Switches back to positioning the preview directly instead of using a wrapper which can be breaking for existing apps. Instead we load a stylesheet dynamically with the necessary resets.

(cherry picked from commit 9729a53)
  • Loading branch information
@crisbeto
crisbeto committed May 1, 2024
1 parent b86c8c8 commit f13b909
Show file tree
Hide file tree
Showing 5 changed files with 112 additions and 138 deletions.
9 changes: 9 additions & 0 deletions src/cdk/drag-drop/BUILD.bazel
View file
Expand Up @@ -4,6 +4,7 @@ load(
"ng_module",
"ng_test_library",
"ng_web_test_suite",
"sass_binary",
)

package(default_visibility = ["//visibility:public"])
Expand All @@ -14,6 +15,9 @@ ng_module(
["**/*.ts"],
exclude = ["**/*.spec.ts"],
),
assets = [
":resets_scss",
],
deps = [
"//src:dev_mode_types",
"//src/cdk/a11y",
Expand Down Expand Up @@ -44,6 +48,11 @@ ng_test_library(
],
)

sass_binary(
name = "resets_scss",
src = "resets.scss",
)

ng_web_test_suite(
name = "unit_tests",
deps = [":unit_test_sources"],
Expand Down
131 changes: 33 additions & 98 deletions src/cdk/drag-drop/directives/drag.spec.ts
View file
Expand Up @@ -23,7 +23,6 @@ import {
ViewEncapsulation,
} from '@angular/core';
import {TestBed, ComponentFixture, fakeAsync, flush, tick} from '@angular/core/testing';
import {DOCUMENT} from '@angular/common';
import {ViewportRuler, CdkScrollableModule} from '@angular/cdk/scrolling';
import {_supportsShadowDom} from '@angular/cdk/platform';
import {of as observableOf} from 'rxjs';
Expand Down Expand Up @@ -2490,7 +2489,6 @@ describe('CdkDrag', () => {
startDraggingViaMouse(fixture, item);

const preview = document.querySelector('.cdk-drag-preview') as HTMLElement;
const previewContainer = document.querySelector('.cdk-drag-preview-container') as HTMLElement;
const previewRect = preview.getBoundingClientRect();
const zeroPxRegex = /^0(px)?$/;

Expand All @@ -2512,23 +2510,18 @@ describe('CdkDrag', () => {
.withContext('Expected element to be removed from layout')
.toBe('-999em');
expect(item.style.opacity).withContext('Expected element to be invisible').toBe('0');
expect(previewContainer)
.withContext('Expected preview container to be in the DOM')
.toBeTruthy();
expect(previewContainer.style.color)
.withContext('Expected preview container to reset user agent color')
.toBe('inherit');
expect(previewContainer.style.margin)
.withContext('Expected preview container to reset user agent margin')
.toMatch(zeroPxRegex);
expect(previewContainer.style.padding)
.withContext('Expected preview container to reset user agent padding')
expect(preview).withContext('Expected preview to be in the DOM').toBeTruthy();
expect(preview.getAttribute('popover'))
.withContext('Expected preview to be a popover')
.toBe('manual');
expect(preview.style.margin)
.withContext('Expected preview to reset the margin')
.toMatch(zeroPxRegex);
expect(preview.textContent!.trim())
.withContext('Expected preview content to match element')
.toContain('One');
expect(previewContainer.getAttribute('dir'))
.withContext('Expected preview container element to inherit the directionality.')
expect(preview.getAttribute('dir'))
.withContext('Expected preview element to inherit the directionality.')
.toBe('ltr');
expect(previewRect.width)
.withContext('Expected preview width to match element')
Expand All @@ -2539,8 +2532,8 @@ describe('CdkDrag', () => {
expect(preview.style.pointerEvents)
.withContext('Expected pointer events to be disabled on the preview')
.toBe('none');
expect(previewContainer.style.zIndex)
.withContext('Expected preview container to have a high default zIndex.')
expect(preview.style.zIndex)
.withContext('Expected preview to have a high default zIndex.')
.toBe('1000');
// Use a regex here since some browsers normalize 0 to 0px, but others don't.
// Use a regex here since some browsers normalize 0 to 0px, but others don't.
Expand All @@ -2561,8 +2554,8 @@ describe('CdkDrag', () => {
expect(item.style.top).withContext('Expected element to be within the layout').toBeFalsy();
expect(item.style.left).withContext('Expected element to be within the layout').toBeFalsy();
expect(item.style.opacity).withContext('Expected element to be visible').toBeFalsy();
expect(previewContainer.parentNode)
.withContext('Expected preview container to be removed from the DOM')
expect(preview.parentNode)
.withContext('Expected preview to be removed from the DOM')
.toBeFalsy();
}));

Expand All @@ -2580,59 +2573,10 @@ describe('CdkDrag', () => {
const item = fixture.componentInstance.dragItems.toArray()[1].element.nativeElement;
startDraggingViaMouse(fixture, item);

const preview = document.querySelector('.cdk-drag-preview-container')! as HTMLElement;
const preview = document.querySelector('.cdk-drag-preview')! as HTMLElement;
expect(preview.style.zIndex).toBe('3000');
}));

it('should create the preview inside the fullscreen element when in fullscreen mode', fakeAsync(() => {
// Provide a limited stub of the document since we can't trigger fullscreen
// mode in unit tests and there are some issues with doing it in e2e tests.
const fakeDocument = {
body: document.body,
documentElement: document.documentElement,
fullscreenElement: document.createElement('div'),
ELEMENT_NODE: Node.ELEMENT_NODE,
querySelectorAll: (...args: [string]) => document.querySelectorAll(...args),
querySelector: (...args: [string]) => document.querySelector(...args),
createElement: (...args: [string]) => document.createElement(...args),
createTextNode: (...args: [string]) => document.createTextNode(...args),
addEventListener: (
...args: [
string,
EventListenerOrEventListenerObject,
(boolean | AddEventListenerOptions | undefined)?,
]
) => document.addEventListener(...args),
removeEventListener: (
...args: [
string,
EventListenerOrEventListenerObject,
(boolean | AddEventListenerOptions | undefined)?,
]
) => document.addEventListener(...args),
createComment: (text: string) => document.createComment(text),
};
const fixture = createComponent(DraggableInDropZone, [
{
provide: DOCUMENT,
useFactory: () => fakeDocument,
},
]);
fixture.detectChanges();
const item = fixture.componentInstance.dragItems.toArray()[1].element.nativeElement;

document.body.appendChild(fakeDocument.fullscreenElement);
startDraggingViaMouse(fixture, item);
flush();

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

expect(previewContainer.parentNode).toBe(fakeDocument.fullscreenElement);
fakeDocument.fullscreenElement.remove();
}));

it('should be able to constrain the preview position', fakeAsync(() => {
const fixture = createComponent(DraggableInDropZone);
fixture.componentInstance.boundarySelector = '.cdk-drop-list';
Expand Down Expand Up @@ -2928,8 +2872,8 @@ describe('CdkDrag', () => {
const item = fixture.componentInstance.dragItems.toArray()[1].element.nativeElement;
startDraggingViaMouse(fixture, item);

expect(document.querySelector('.cdk-drag-preview-container')!.getAttribute('dir'))
.withContext('Expected preview container to inherit the directionality.')
expect(document.querySelector('.cdk-drag-preview')!.getAttribute('dir'))
.withContext('Expected preview to inherit the directionality.')
.toBe('rtl');
}));

Expand All @@ -2941,7 +2885,6 @@ describe('CdkDrag', () => {
startDraggingViaMouse(fixture, item);

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

// Add a duration since the tests won't include one.
preview.style.transitionDuration = '500ms';
Expand All @@ -2954,13 +2897,13 @@ describe('CdkDrag', () => {
fixture.detectChanges();
tick(250);

expect(previewContainer.parentNode)
expect(preview.parentNode)
.withContext('Expected preview to be in the DOM mid-way through the transition')
.toBeTruthy();

tick(500);

expect(previewContainer.parentNode)
expect(preview.parentNode)
.withContext('Expected preview to be removed from the DOM if the transition timed out')
.toBeFalsy();
}));
Expand Down Expand Up @@ -3064,7 +3007,6 @@ describe('CdkDrag', () => {
startDraggingViaMouse(fixture, item);

const preview = document.querySelector('.cdk-drag-preview')! as HTMLElement;
const previewContainer = document.querySelector('.cdk-drag-preview-container') as HTMLElement;
preview.style.transition = 'opacity 500ms ease';

dispatchMouseEvent(document, 'mousemove', 50, 50);
Expand All @@ -3074,8 +3016,8 @@ describe('CdkDrag', () => {
fixture.detectChanges();
tick(0);

expect(previewContainer.parentNode)
.withContext('Expected preview container to be removed from the DOM immediately')
expect(preview.parentNode)
.withContext('Expected preview to be removed from the DOM immediately')
.toBeFalsy();
}));

Expand All @@ -3087,7 +3029,6 @@ describe('CdkDrag', () => {
startDraggingViaMouse(fixture, item);

const preview = document.querySelector('.cdk-drag-preview')! as HTMLElement;
const previewContainer = document.querySelector('.cdk-drag-preview-container') as HTMLElement;
preview.style.transition = 'opacity 500ms ease, transform 1000ms ease';

dispatchMouseEvent(document, 'mousemove', 50, 50);
Expand All @@ -3097,17 +3038,15 @@ describe('CdkDrag', () => {
fixture.detectChanges();
tick(500);

expect(previewContainer.parentNode)
.withContext(
'Expected preview container to be in the DOM at the end of the opacity transition',
)
expect(preview.parentNode)
.withContext('Expected preview to be in the DOM at the end of the opacity transition')
.toBeTruthy();

tick(1000);

expect(previewContainer.parentNode)
expect(preview.parentNode)
.withContext(
'Expected preview container to be removed from the DOM at the end of the transform transition',
'Expected preview to be removed from the DOM at the end of the transform transition',
)
.toBeFalsy();
}));
Expand Down Expand Up @@ -3149,8 +3088,8 @@ describe('CdkDrag', () => {
const item = fixture.componentInstance.dragItems.toArray()[1].element.nativeElement;

startDraggingViaMouse(fixture, item);
const previewContainer = document.querySelector('.cdk-drag-preview-container') as HTMLElement;
expect(previewContainer.parentNode).toBe(document.body);
const preview = document.querySelector('.cdk-drag-preview') as HTMLElement;
expect(preview.parentNode).toBe(document.body);
}));

it('should insert the preview into the parent node if previewContainer is set to `parent`', fakeAsync(() => {
Expand All @@ -3161,9 +3100,9 @@ describe('CdkDrag', () => {
const list = fixture.nativeElement.querySelector('.drop-list');

startDraggingViaMouse(fixture, item);
const previewContainer = document.querySelector('.cdk-drag-preview-container') as HTMLElement;
const preview = document.querySelector('.cdk-drag-preview') as HTMLElement;
expect(list).toBeTruthy();
expect(previewContainer.parentNode).toBe(list);
expect(preview.parentNode).toBe(list);
}));

it('should insert the preview into a particular element, if specified', fakeAsync(() => {
Expand All @@ -3177,10 +3116,8 @@ describe('CdkDrag', () => {
fixture.detectChanges();

startDraggingViaMouse(fixture, item);
const previewContainerElement = document.querySelector(
'.cdk-drag-preview-container',
) as HTMLElement;
expect(previewContainerElement.parentNode).toBe(previewContainer.nativeElement);
const preview = document.querySelector('.cdk-drag-preview') as HTMLElement;
expect(preview.parentNode).toBe(previewContainer.nativeElement);
}));

it('should remove the id from the placeholder', fakeAsync(() => {
Expand Down Expand Up @@ -3692,17 +3629,15 @@ describe('CdkDrag', () => {

startDraggingViaMouse(fixture, item);

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

expect(previewContainer.parentNode)
.withContext('Expected preview container to be in the DOM')
.toBeTruthy();
expect(preview.parentNode).withContext('Expected preview to be in the DOM').toBeTruthy();
expect(item.parentNode).withContext('Expected drag item to be in the DOM').toBeTruthy();

fixture.destroy();

expect(previewContainer.parentNode)
.withContext('Expected preview container to be removed from the DOM')
expect(preview.parentNode)
.withContext('Expected preview to be removed from the DOM')
.toBeFalsy();
expect(item.parentNode)
.withContext('Expected drag item to be removed from the DOM')
Expand Down
54 changes: 53 additions & 1 deletion src/cdk/drag-drop/drag-drop.ts
View file
Expand Up @@ -6,7 +6,19 @@
* found in the LICENSE file at https://angular.io/license
*/

import {Injectable, Inject, NgZone, ElementRef} from '@angular/core';
import {
Injectable,
Inject,
NgZone,
ElementRef,
Component,
ViewEncapsulation,
ChangeDetectionStrategy,
ApplicationRef,
inject,
createComponent,
EnvironmentInjector,
} from '@angular/core';
import {DOCUMENT} from '@angular/common';
import {ViewportRuler} from '@angular/cdk/scrolling';
import {DragRef, DragRefConfig} from './drag-ref';
Expand All @@ -19,11 +31,31 @@ const DEFAULT_CONFIG = {
pointerDirectionChangeThreshold: 5,
};

/** Keeps track of the apps currently containing badges. */
const activeApps = new Set<ApplicationRef>();

/**
* Component used to load the drag&drop reset styles.
* @docs-private
*/
@Component({
standalone: true,
styleUrl: 'resets.css',
encapsulation: ViewEncapsulation.None,
template: '',
changeDetection: ChangeDetectionStrategy.OnPush,
host: {'cdk-drag-resets-container': ''},
})
export class _ResetsLoader {}

/**
* Service that allows for drag-and-drop functionality to be attached to DOM elements.
*/
@Injectable({providedIn: 'root'})
export class DragDrop {
private _appRef = inject(ApplicationRef);
private _environmentInjector = inject(EnvironmentInjector);

constructor(
@Inject(DOCUMENT) private _document: any,
private _ngZone: NgZone,
Expand All @@ -40,6 +72,7 @@ export class DragDrop {
element: ElementRef<HTMLElement> | HTMLElement,
config: DragRefConfig = DEFAULT_CONFIG,
): DragRef<T> {
this._loadResets();
return new DragRef<T>(
element,
config,
Expand All @@ -63,4 +96,23 @@ export class DragDrop {
this._viewportRuler,
);
}

// TODO(crisbeto): abstract this away into something reusable.
/** Loads the CSS resets needed for the module to work correctly. */
private _loadResets() {
if (!activeApps.has(this._appRef)) {
activeApps.add(this._appRef);

const componentRef = createComponent(_ResetsLoader, {
environmentInjector: this._environmentInjector,
});

this._appRef.onDestroy(() => {
activeApps.delete(this._appRef);
if (activeApps.size === 0) {
componentRef.destroy();
}
});
}
}
}

0 comments on commit f13b909

Please sign in to comment.