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.
  • Loading branch information
crisbeto committed Apr 30, 2024
1 parent 0309adf commit c356f65
Show file tree
Hide file tree
Showing 9 changed files with 117 additions and 214 deletions.
9 changes: 9 additions & 0 deletions src/cdk/drag-drop/BUILD.bazel
Original file line number Diff line number Diff line change
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
1 change: 0 additions & 1 deletion src/cdk/drag-drop/directives/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,5 +44,4 @@ export interface DragDropConfig extends Partial<DragRefConfig> {
listOrientation?: DropListOrientation;
zIndex?: number;
previewContainer?: 'global' | 'parent';
disablePreviewPopover?: boolean;
}
145 changes: 32 additions & 113 deletions src/cdk/drag-drop/directives/drag.spec.ts
Original file line number Diff line number Diff line change
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 @@ -3176,25 +3115,9 @@ describe('CdkDrag', () => {
fixture.componentInstance.previewContainer = previewContainer;
fixture.detectChanges();

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

it('should not create a popover wrapper if disablePreviewPopover is enabled', fakeAsync(() => {
const fixture = createComponent(DraggableInDropZone);
fixture.componentInstance.previewContainer = 'global';
fixture.componentInstance.disablePreviewPopover = true;
fixture.detectChanges();
const item = fixture.componentInstance.dragItems.toArray()[1].element.nativeElement;

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

it('should remove the id from the placeholder', fakeAsync(() => {
Expand Down Expand Up @@ -3706,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 Expand Up @@ -6950,7 +6871,6 @@ const DROP_ZONE_FIXTURE_TEMPLATE = `
[cdkDragBoundary]="boundarySelector"
[cdkDragPreviewClass]="previewClass"
[cdkDragPreviewContainer]="previewContainer"
[cdkDragDisablePreviewPopover]="disablePreviewPopover"
[style.height.px]="item.height"
[style.margin-bottom.px]="item.margin"
(cdkDragStarted)="startedSpy($event)"
Expand Down Expand Up @@ -6980,7 +6900,6 @@ class DraggableInDropZone implements AfterViewInit {
});
startedSpy = jasmine.createSpy('started spy');
previewContainer: PreviewContainer = 'global';
disablePreviewPopover = false;

constructor(protected _elementRef: ElementRef) {}

Expand Down
18 changes: 1 addition & 17 deletions src/cdk/drag-drop/directives/drag.ts
Original file line number Diff line number Diff line change
Expand Up @@ -159,20 +159,6 @@ export class CdkDrag<T = any> implements AfterViewInit, OnChanges, OnDestroy {
*/
@Input('cdkDragPreviewContainer') previewContainer: PreviewContainer;

/**
* By default the preview element is wrapped in a native popover in order to be compatible
* with other native popovers and to avoid issues with `overflow: hidden`. In some edge cases
* this can interfere with styling (e.g. CSS selectors targeting direct descendants). Enable
* this option to remove the wrapper around the preview, but note that it can cause the following
* issues when used with `cdkDragPreviewContainer` set to `parent` or a specific DOM node:
* - The preview may be clipped by a parent with `overflow: hidden`.
* - The preview isn't guaranteed to be on top of other elements, despite its `z-index`.
* - Transforms on the parent of the preview can affect its positioning.
* - The preview may be positioned under native `<dialog>` or popover elements.
*/
@Input({alias: 'cdkDragDisablePreviewPopover', transform: booleanAttribute})
disablePreviewPopover: boolean;

/** Emits when the user starts dragging the item. */
@Output('cdkDragStarted') readonly started: EventEmitter<CdkDragStart> =
new EventEmitter<CdkDragStart>();
Expand Down Expand Up @@ -472,7 +458,7 @@ export class CdkDrag<T = any> implements AfterViewInit, OnChanges, OnDestroy {
.withBoundaryElement(this._getBoundaryElement())
.withPlaceholderTemplate(placeholder)
.withPreviewTemplate(preview)
.withPreviewContainer(this.previewContainer || 'global', this.disablePreviewPopover);
.withPreviewContainer(this.previewContainer || 'global');

if (dir) {
ref.withDirection(dir.value);
Expand Down Expand Up @@ -573,12 +559,10 @@ export class CdkDrag<T = any> implements AfterViewInit, OnChanges, OnDestroy {
draggingDisabled,
rootElementSelector,
previewContainer,
disablePreviewPopover,
} = config;

this.disabled = draggingDisabled == null ? false : draggingDisabled;
this.dragStartDelay = dragStartDelay || 0;
this.disablePreviewPopover = disablePreviewPopover || false;

if (lockAxis) {
this.lockAxis = lockAxis;
Expand Down

0 comments on commit c356f65

Please sign in to comment.