From 105e920b6934a19a8de27cbd54514e57b9e54311 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matias=20Niemel=C3=A4?= Date: Thu, 29 Jun 2017 17:05:52 -0700 Subject: [PATCH] fix(animations): properly handle cancelled animation style application --- .../src/render/transition_animation_engine.ts | 29 ++-- .../web_animations/web_animations_player.ts | 15 +- packages/animations/browser/src/util.ts | 4 + .../testing/src/mock_animation_driver.ts | 17 ++- .../animation/animation_integration_spec.ts | 2 +- .../animation_query_integration_spec.ts | 65 ++++----- ...ns_with_web_animations_integration_spec.ts | 136 ++++++++++++++++-- 7 files changed, 194 insertions(+), 74 deletions(-) diff --git a/packages/animations/browser/src/render/transition_animation_engine.ts b/packages/animations/browser/src/render/transition_animation_engine.ts index 0d2bd44d6f..9e654a94e8 100644 --- a/packages/animations/browser/src/render/transition_animation_engine.ts +++ b/packages/animations/browser/src/render/transition_animation_engine.ts @@ -18,7 +18,7 @@ import {ENTER_CLASSNAME, LEAVE_CLASSNAME, NG_ANIMATING_CLASSNAME, NG_ANIMATING_S import {AnimationDriver} from './animation_driver'; import {getOrSetAsInMap, listenOnPlayer, makeAnimationEvent, normalizeKeyframes, optimizeGroupPlayer} from './shared'; -const EMPTY_PLAYER_ARRAY: AnimationPlayer[] = []; +const EMPTY_PLAYER_ARRAY: TransitionAnimationPlayer[] = []; const NULL_REMOVAL_STATE: ElementAnimationState = { namespaceId: '', setForRemoval: null, @@ -708,7 +708,14 @@ export class TransitionAnimationEngine { if (this._namespaceList.length && (this.totalQueuedPlayers || this.collectedLeaveElements.length)) { - players = this._flushAnimations(microtaskId); + const cleanupFns: Function[] = []; + try { + players = this._flushAnimations(cleanupFns, microtaskId); + } finally { + for (let i = 0; i < cleanupFns.length; i++) { + cleanupFns[i](); + } + } } else { for (let i = 0; i < this.collectedLeaveElements.length; i++) { const element = this.collectedLeaveElements[i]; @@ -737,7 +744,8 @@ export class TransitionAnimationEngine { } } - private _flushAnimations(microtaskId: number): TransitionAnimationPlayer[] { + private _flushAnimations(cleanupFns: Function[], microtaskId: number): + TransitionAnimationPlayer[] { const subTimelines = new ElementInstructionMap(); const skippedPlayers: TransitionAnimationPlayer[] = []; const skippedPlayersMap = new Map(); @@ -745,7 +753,6 @@ export class TransitionAnimationEngine { const queriedElements = new Map(); const allPreStyleElements = new Map>(); const allPostStyleElements = new Map>(); - const cleanupFns: Function[] = []; const bodyNode = getBodyNode(); const allEnterNodes: any[] = this.collectedEnterElements.length ? @@ -855,7 +862,6 @@ export class TransitionAnimationEngine { instruction.errors !.forEach(error => { msg += `- ${error}\n`; }); }); - cleanupFns.forEach(fn => fn()); allPlayers.forEach(player => player.destroy()); throw new Error(msg); } @@ -1011,7 +1017,6 @@ export class TransitionAnimationEngine { player.play(); }); - cleanupFns.forEach(fn => fn()); return rootPlayers; } @@ -1107,20 +1112,16 @@ export class TransitionAnimationEngine { const allSubElements = new Set(); const allNewPlayers = instruction.timelines.map(timelineInstruction => { const element = timelineInstruction.element; + allConsumedElements.add(element); // FIXME (matsko): make sure to-be-removed animations are removed properly const details = element[REMOVAL_FLAG]; if (details && details.removedBeforeQueried) return new NoopAnimationPlayer(); const isQueriedElement = element !== rootElement; - let previousPlayers: AnimationPlayer[] = EMPTY_PLAYER_ARRAY; - if (!allConsumedElements.has(element)) { - allConsumedElements.add(element); - const _previousPlayers = allPreviousPlayersMap.get(element); - if (_previousPlayers) { - previousPlayers = _previousPlayers.map(p => p.getRealPlayer()); - } - } + const previousPlayers = + (allPreviousPlayersMap.get(element) || EMPTY_PLAYER_ARRAY).map(p => p.getRealPlayer()); + const preStyles = preStylesMap.get(element); const postStyles = postStylesMap.get(element); const keyframes = normalizeKeyframes( diff --git a/packages/animations/browser/src/render/web_animations/web_animations_player.ts b/packages/animations/browser/src/render/web_animations/web_animations_player.ts index 3e0490340f..580ebb5042 100644 --- a/packages/animations/browser/src/render/web_animations/web_animations_player.ts +++ b/packages/animations/browser/src/render/web_animations/web_animations_player.ts @@ -7,7 +7,7 @@ */ import {AnimationPlayer} from '@angular/animations'; -import {copyStyles, eraseStyles, setStyles} from '../../util'; +import {allowPreviousPlayerStylesMerge, copyStyles} from '../../util'; import {DOMAnimation} from './dom_animation'; @@ -26,7 +26,7 @@ export class WebAnimationsPlayer implements AnimationPlayer { public time = 0; public parentPlayer: AnimationPlayer|null = null; - public previousStyles: {[styleName: string]: string | number}; + public previousStyles: {[styleName: string]: string | number} = {}; public currentSnapshot: {[styleName: string]: string | number} = {}; constructor( @@ -37,11 +37,12 @@ export class WebAnimationsPlayer implements AnimationPlayer { this._delay = options['delay'] || 0; this.time = this._duration + this._delay; - this.previousStyles = {}; - previousPlayers.forEach(player => { - let styles = player.currentSnapshot; - Object.keys(styles).forEach(prop => this.previousStyles[prop] = styles[prop]); - }); + if (allowPreviousPlayerStylesMerge(this._duration, this._delay)) { + previousPlayers.forEach(player => { + let styles = player.currentSnapshot; + Object.keys(styles).forEach(prop => this.previousStyles[prop] = styles[prop]); + }); + } } private _onFinish() { diff --git a/packages/animations/browser/src/util.ts b/packages/animations/browser/src/util.ts index 62e4c81f19..a6dbb75f56 100644 --- a/packages/animations/browser/src/util.ts +++ b/packages/animations/browser/src/util.ts @@ -213,3 +213,7 @@ const DASH_CASE_REGEXP = /-+([a-z0-9])/g; export function dashCaseToCamelCase(input: string): string { return input.replace(DASH_CASE_REGEXP, (...m: any[]) => m[1].toUpperCase()); } + +export function allowPreviousPlayerStylesMerge(duration: number, delay: number) { + return duration === 0 || delay === 0; +} diff --git a/packages/animations/browser/testing/src/mock_animation_driver.ts b/packages/animations/browser/testing/src/mock_animation_driver.ts index 0bc3501631..a04e069808 100644 --- a/packages/animations/browser/testing/src/mock_animation_driver.ts +++ b/packages/animations/browser/testing/src/mock_animation_driver.ts @@ -9,7 +9,7 @@ import {AUTO_STYLE, AnimationPlayer, NoopAnimationPlayer, ɵStyleData} from '@an import {AnimationDriver} from '../../src/render/animation_driver'; import {containsElement, invokeQuery, matchesElement} from '../../src/render/shared'; - +import {allowPreviousPlayerStylesMerge} from '../../src/util'; /** * @experimental Animation support is experimental. @@ -56,12 +56,15 @@ export class MockAnimationPlayer extends NoopAnimationPlayer { public duration: number, public delay: number, public easing: string, public previousPlayers: any[]) { super(); - previousPlayers.forEach(player => { - if (player instanceof MockAnimationPlayer) { - const styles = player.currentSnapshot; - Object.keys(styles).forEach(prop => this.previousStyles[prop] = styles[prop]); - } - }); + + if (allowPreviousPlayerStylesMerge(duration, delay)) { + previousPlayers.forEach(player => { + if (player instanceof MockAnimationPlayer) { + const styles = player.currentSnapshot; + Object.keys(styles).forEach(prop => this.previousStyles[prop] = styles[prop]); + } + }); + } this.totalTime = delay + duration; } diff --git a/packages/core/test/animation/animation_integration_spec.ts b/packages/core/test/animation/animation_integration_spec.ts index d8fe04024a..34284a940a 100644 --- a/packages/core/test/animation/animation_integration_spec.ts +++ b/packages/core/test/animation/animation_integration_spec.ts @@ -782,7 +782,7 @@ export function main() { expect(players.length).toEqual(3); const [p1, p2, p3] = players; expect(p1.previousStyles).toEqual({opacity: AUTO_STYLE}); - expect(p2.previousStyles).toEqual({}); + expect(p2.previousStyles).toEqual({opacity: AUTO_STYLE}); expect(p3.previousStyles).toEqual({}); }); diff --git a/packages/core/test/animation/animation_query_integration_spec.ts b/packages/core/test/animation/animation_query_integration_spec.ts index 4577a51cf5..aa358d972f 100644 --- a/packages/core/test/animation/animation_query_integration_spec.ts +++ b/packages/core/test/animation/animation_query_integration_spec.ts @@ -1000,8 +1000,9 @@ export function main() { } }); - it('should properly cancel items that were queried into a former animation', () => { - @Component({ + it('should properly cancel items that were queried into a former animation and pass in the associated styles into the follow-up players per element', + () => { + @Component({ selector: 'ani-cmp', template: `
@@ -1024,45 +1025,45 @@ export function main() { ])] }) class Cmp { - public exp: any; - public items: any[]; - } + public exp: any; + public items: any[]; + } - TestBed.configureTestingModule({declarations: [Cmp]}); + TestBed.configureTestingModule({declarations: [Cmp]}); - const engine = TestBed.get(ɵAnimationEngine); - const fixture = TestBed.createComponent(Cmp); - const cmp = fixture.componentInstance; + const engine = TestBed.get(ɵAnimationEngine); + const fixture = TestBed.createComponent(Cmp); + const cmp = fixture.componentInstance; - cmp.exp = 'on'; - cmp.items = [0, 1, 2, 3, 4]; - fixture.detectChanges(); - engine.flush(); + cmp.exp = 'on'; + cmp.items = [0, 1, 2, 3, 4]; + fixture.detectChanges(); + engine.flush(); - const previousPlayers = getLog(); - expect(previousPlayers.length).toEqual(10); - resetLog(); + const previousPlayers = getLog(); + expect(previousPlayers.length).toEqual(10); + resetLog(); - cmp.exp = 'off'; - cmp.items = [0, 1, 2]; - fixture.detectChanges(); - engine.flush(); + cmp.exp = 'off'; + cmp.items = [0, 1, 2]; + fixture.detectChanges(); + engine.flush(); - const players = getLog(); - expect(players.length).toEqual(4); + const players = getLog(); + expect(players.length).toEqual(4); - const [p1, p2, p3, p4] = players; - const [p1p1, p1p2] = p1.previousPlayers; - const [p2p1, p2p2] = p2.previousPlayers; + const [p1, p2, p3, p4] = players; - expect(p1p1).toBe(previousPlayers[3]); - expect(p1p2).toBe(previousPlayers[8]); - expect(p2p1).toBe(previousPlayers[4]); - expect(p2p2).toBe(previousPlayers[9]); + // p1 && p2 are the starting players for item3 and item4 + expect(p1.previousStyles) + .toEqual({opacity: AUTO_STYLE, width: AUTO_STYLE, height: AUTO_STYLE}); + expect(p2.previousStyles) + .toEqual({opacity: AUTO_STYLE, width: AUTO_STYLE, height: AUTO_STYLE}); - expect(p3.previousPlayers).toEqual([]); - expect(p4.previousPlayers).toEqual([]); - }); + // p3 && p4 are the following players for item3 and item4 + expect(p3.previousStyles).toEqual({}); + expect(p4.previousStyles).toEqual({}); + }); it('should not remove a parent container if its contents are queried into by an ancestor element', () => { diff --git a/packages/core/test/animation/animations_with_web_animations_integration_spec.ts b/packages/core/test/animation/animations_with_web_animations_integration_spec.ts index 1e03a142dd..4664815a95 100644 --- a/packages/core/test/animation/animations_with_web_animations_integration_spec.ts +++ b/packages/core/test/animation/animations_with_web_animations_integration_spec.ts @@ -5,10 +5,10 @@ * Use of this source code is governed by an MIT-style license that can be * found in the LICENSE file at https://angular.io/license */ -import {animate, state, style, transition, trigger} from '@angular/animations'; -import {AnimationDriver, ɵAnimationEngine} from '@angular/animations/browser'; -import {ɵWebAnimationsDriver, ɵWebAnimationsPlayer, ɵsupportsWebAnimations} from '@angular/animations/browser' -import {Component, ViewChild} from '@angular/core'; +import {animate, query, state, style, transition, trigger} from '@angular/animations'; +import {AnimationDriver, ɵAnimationEngine, ɵWebAnimationsDriver, ɵWebAnimationsPlayer, ɵsupportsWebAnimations} from '@angular/animations/browser'; +import {AnimationGroupPlayer} from '@angular/animations/src/players/animation_group_player'; +import {Component} from '@angular/core'; import {BrowserAnimationsModule} from '@angular/platform-browser/animations'; import {TestBed} from '../../testing'; @@ -172,15 +172,125 @@ export function main() { {height: '100px', offset: 0}, {height: '80px', offset: 1} ]); }); + + it('should compute intermediate styles properly when an animation is cancelled', () => { + @Component({ + selector: 'ani-cmp', + template: ` +
...
+ `, + animations: [ + trigger( + 'myAnimation', + [ + transition( + '* => a', + [ + style({width: 0, height: 0}), + animate('1s', style({width: '300px', height: '600px'})), + ]), + transition('* => b', [animate('1s', style({opacity: 0}))]), + ]), + ] + }) + class Cmp { + public exp: string; + } + + TestBed.configureTestingModule({declarations: [Cmp]}); + + const engine = TestBed.get(ɵAnimationEngine); + const fixture = TestBed.createComponent(Cmp); + const cmp = fixture.componentInstance; + + cmp.exp = 'a'; + fixture.detectChanges(); + + let player = engine.players[0] !; + let webPlayer = player.getRealPlayer() as ɵWebAnimationsPlayer; + webPlayer.setPosition(0.5); + + cmp.exp = 'b'; + fixture.detectChanges(); + + player = engine.players[0] !; + webPlayer = player.getRealPlayer() as ɵWebAnimationsPlayer; + expect(approximate(parseFloat(webPlayer.previousStyles['width'] as string), 150)) + .toBeLessThan(0.05); + expect(approximate(parseFloat(webPlayer.previousStyles['height'] as string), 300)) + .toBeLessThan(0.05); + }); + + it('should compute intermediate styles properly for multiple queried elements when an animation is cancelled', + () => { + @Component({ + selector: 'ani-cmp', + template: ` +
+
+
+ `, + animations: [ + trigger( + 'myAnimation', + [ + transition( + '* => full', [query( + '.target', + [ + style({width: 0, height: 0}), + animate('1s', style({width: '500px', height: '1000px'})), + ])]), + transition( + '* => empty', [query('.target', [animate('1s', style({opacity: 0}))])]), + ]), + ] + }) + class Cmp { + public exp: string; + public items: any[] = []; + } + + TestBed.configureTestingModule({declarations: [Cmp]}); + + const engine = TestBed.get(ɵAnimationEngine); + const fixture = TestBed.createComponent(Cmp); + const cmp = fixture.componentInstance; + + cmp.exp = 'full'; + cmp.items = [0, 1, 2, 3, 4]; + fixture.detectChanges(); + + let player = engine.players[0] !; + let groupPlayer = player.getRealPlayer() as AnimationGroupPlayer; + let players = groupPlayer.players; + expect(players.length).toEqual(5); + + for (let i = 0; i < players.length; i++) { + const p = players[i] as ɵWebAnimationsPlayer; + p.setPosition(0.5); + } + + cmp.exp = 'empty'; + cmp.items = []; + fixture.detectChanges(); + + player = engine.players[0]; + groupPlayer = player.getRealPlayer() as AnimationGroupPlayer; + players = groupPlayer.players; + + expect(players.length).toEqual(5); + for (let i = 0; i < players.length; i++) { + const p = players[i] as ɵWebAnimationsPlayer; + expect(approximate(parseFloat(p.previousStyles['width'] as string), 250)) + .toBeLessThan(0.05); + expect(approximate(parseFloat(p.previousStyles['height'] as string), 500)) + .toBeLessThan(0.05); + } + }); }); } -function assertStyleBetween( - element: any, prop: string, start: string | number, end: string | number) { - const style = (window.getComputedStyle(element) as any)[prop] as string; - if (typeof start == 'number' && typeof end == 'number') { - const value = parseFloat(style); - expect(value).toBeGreaterThan(start); - expect(value).toBeLessThan(end); - } -} +function approximate(value: number, target: number) { + return Math.abs(target - value) / value; +} \ No newline at end of file