From 951add921d17f4c693395dd4b4b9cd9290375ef7 Mon Sep 17 00:00:00 2001 From: Kristiyan Kostadinov Date: Tue, 11 Sep 2018 19:38:51 +0200 Subject: [PATCH] fix(drag-drop): incorrectly picking up transitions on non-transform properties (#12966) Currently we wait for the transition on a dropped item to end before completing the sequence or timing it out. Our current approach works for a single property transition (e.g. `transition: transform 300ms`), but will break down for multi-property ones (`transition: opacity 300ms, transform: 600ms`) or ones that don't target `transform` at all. The following changes rework the logic to handle the extra cases. --- src/cdk/drag-drop/drag.spec.ts | 50 ++++++++++++++++++++++++++++++++++ src/cdk/drag-drop/drag.ts | 32 +++++++++++++++++----- 2 files changed, 75 insertions(+), 7 deletions(-) diff --git a/src/cdk/drag-drop/drag.spec.ts b/src/cdk/drag-drop/drag.spec.ts index 25caeac081d..c2949ddc0dd 100644 --- a/src/cdk/drag-drop/drag.spec.ts +++ b/src/cdk/drag-drop/drag.spec.ts @@ -683,6 +683,56 @@ describe('CdkDrag', () => { .toBeFalsy('Expected preview to be removed from the DOM if the transition timed out'); })); + it('should not wait for transition that are not on the `transform` property', fakeAsync(() => { + const fixture = createComponent(DraggableInDropZone); + fixture.detectChanges(); + const item = fixture.componentInstance.dragItems.toArray()[1].element.nativeElement; + + dispatchMouseEvent(item, 'mousedown'); + fixture.detectChanges(); + + const preview = document.querySelector('.cdk-drag-preview')! as HTMLElement; + preview.style.transition = 'opacity 500ms ease'; + + dispatchTouchEvent(document, 'mousemove', 50, 50); + fixture.detectChanges(); + + dispatchMouseEvent(document, 'mouseup'); + fixture.detectChanges(); + tick(0); + + expect(preview.parentNode) + .toBeFalsy('Expected preview to be removed from the DOM immediately'); + })); + + it('should pick out the `transform` duration if multiple properties are being transitioned', + fakeAsync(() => { + const fixture = createComponent(DraggableInDropZone); + fixture.detectChanges(); + const item = fixture.componentInstance.dragItems.toArray()[1].element.nativeElement; + + dispatchMouseEvent(item, 'mousedown'); + fixture.detectChanges(); + + const preview = document.querySelector('.cdk-drag-preview')! as HTMLElement; + preview.style.transition = 'opacity 500ms ease, transform 1000ms ease'; + + dispatchTouchEvent(document, 'mousemove', 50, 50); + fixture.detectChanges(); + + dispatchMouseEvent(document, 'mouseup'); + fixture.detectChanges(); + tick(500); + + expect(preview.parentNode) + .toBeTruthy('Expected preview to be in the DOM at the end of the opacity transition'); + + tick(1000); + + expect(preview.parentNode).toBeFalsy( + 'Expected preview to be removed from the DOM at the end of the transform transition'); + })); + it('should create a placeholder element while the item is dragged', fakeAsync(() => { const fixture = createComponent(DraggableInDropZone); fixture.detectChanges(); diff --git a/src/cdk/drag-drop/drag.ts b/src/cdk/drag-drop/drag.ts index e0715703aeb..f61cf090e45 100644 --- a/src/cdk/drag-drop/drag.ts +++ b/src/cdk/drag-drop/drag.ts @@ -536,7 +536,7 @@ export class CdkDrag implements AfterViewInit, OnDestroy { // we need to trigger a style recalculation in order for the `cdk-drag-animating` class to // apply its style, we take advantage of the available info to figure out whether we need to // bind the event in the first place. - const duration = getTransitionDurationInMs(this._preview); + const duration = getTransformTransitionDurationInMs(this._preview); if (duration === 0) { return Promise.resolve(); @@ -545,7 +545,7 @@ export class CdkDrag implements AfterViewInit, OnDestroy { return this._ngZone.runOutsideAngular(() => { return new Promise(resolve => { const handler = ((event: TransitionEvent) => { - if (!event || event.target === this._preview) { + if (!event || (event.target === this._preview && event.propertyName === 'transform')) { this._preview.removeEventListener('transitionend', handler); resolve(); clearTimeout(timeout); @@ -686,13 +686,31 @@ function parseCssTimeUnitsToMs(value: string): number { return parseFloat(value) * multiplier; } -/** Gets the transition duration, including the delay, of an element in milliseconds. */ -function getTransitionDurationInMs(element: HTMLElement): number { +/** Gets the transform transition duration, including the delay, of an element in milliseconds. */ +function getTransformTransitionDurationInMs(element: HTMLElement): number { const computedStyle = getComputedStyle(element); - const rawDuration = computedStyle.getPropertyValue('transition-duration'); - const rawDelay = computedStyle.getPropertyValue('transition-delay'); + const transitionedProperties = parseCssPropertyValue(computedStyle, 'transition-property'); + const property = transitionedProperties.find(prop => prop === 'transform' || prop === 'all'); - return parseCssTimeUnitsToMs(rawDuration) + parseCssTimeUnitsToMs(rawDelay); + // If there's no transition for `all` or `transform`, we shouldn't do anything. + if (!property) { + return 0; + } + + // Get the index of the property that we're interested in and match + // it up to the same index in `transition-delay` and `transition-duration`. + const propertyIndex = transitionedProperties.indexOf(property); + const rawDurations = parseCssPropertyValue(computedStyle, 'transition-duration'); + const rawDelays = parseCssPropertyValue(computedStyle, 'transition-delay'); + + return parseCssTimeUnitsToMs(rawDurations[propertyIndex]) + + parseCssTimeUnitsToMs(rawDelays[propertyIndex]); +} + +/** Parses out multiple values from a computed style into an array. */ +function parseCssPropertyValue(computedStyle: CSSStyleDeclaration, name: string): string[] { + const value = computedStyle.getPropertyValue(name); + return value.split(',').map(part => part.trim()); } /** Point on the page or within an element. */