From f8658cdc38284efe1afe90173d2999a1cab7611d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mi=C5=A1ko=20Hevery?= Date: Wed, 15 Nov 2017 17:04:22 -0600 Subject: [PATCH] Revert "fix(animations): always fire inner trigger callbacks even if blocked by parent animations (#19753)" This reverts commit d47b2a6f706c42c049bf0faf1112c97ecf681c0a. --- .../src/render/transition_animation_engine.ts | 68 +++------- .../web_animations/web_animations_player.ts | 7 - .../web_animations_player_spec.ts | 20 --- .../src/players/animation_group_player.ts | 7 - .../src/players/animation_player.ts | 11 +- .../animations/test/animation_player_spec.ts | 24 ---- .../animation_query_integration_spec.ts | 123 ++---------------- 7 files changed, 32 insertions(+), 228 deletions(-) diff --git a/packages/animations/browser/src/render/transition_animation_engine.ts b/packages/animations/browser/src/render/transition_animation_engine.ts index 0e65e9753d..0d3f06f820 100644 --- a/packages/animations/browser/src/render/transition_animation_engine.ts +++ b/packages/animations/browser/src/render/transition_animation_engine.ts @@ -986,15 +986,11 @@ export class TransitionAnimationEngine { } const allPreviousPlayersMap = new Map(); - // this map works to tell which element in the DOM tree is contained by - // which animation. Further down below this map will get populated once - // the players are built and in doing so it can efficiently figure out - // if a sub player is skipped due to a parent player having priority. - const animationElementMap = new Map(); + let sortedParentElements: any[] = []; queuedInstructions.forEach(entry => { const element = entry.element; if (subTimelines.has(element)) { - animationElementMap.set(element, element); + sortedParentElements.unshift(element); this._beforeAnimationBuild( entry.player.namespaceId, entry.instruction, allPreviousPlayersMap); } @@ -1045,7 +1041,6 @@ export class TransitionAnimationEngine { const rootPlayers: TransitionAnimationPlayer[] = []; const subPlayers: TransitionAnimationPlayer[] = []; - const NO_PARENT_ANIMATION_ELEMENT_DETECTED = {}; queuedInstructions.forEach(entry => { const {element, player, instruction} = entry; // this means that it was never consumed by a parent animation which @@ -1057,41 +1052,29 @@ export class TransitionAnimationEngine { return; } - // this will flow up the DOM and query the map to figure out - // if a parent animation has priority over it. In the situation - // that a parent is detected then it will cancel the loop. If - // nothing is detected, or it takes a few hops to find a parent, - // then it will fill in the missing nodes and signal them as having - // a detected parent (or a NO_PARENT value via a special constant). - let parentWithAnimation: any = NO_PARENT_ANIMATION_ELEMENT_DETECTED; - if (animationElementMap.size > 1) { - let elm = element; - const parentsToAdd: any[] = []; - while (elm = elm.parentNode) { - const detectedParent = animationElementMap.get(elm); - if (detectedParent) { - parentWithAnimation = detectedParent; - break; - } - parentsToAdd.push(elm); - } - parentsToAdd.forEach(parent => animationElementMap.set(parent, parentWithAnimation)); - } - const innerPlayer = this._buildAnimation( player.namespaceId, instruction, allPreviousPlayersMap, skippedPlayersMap, preStylesMap, postStylesMap); - player.setRealPlayer(innerPlayer); - if (parentWithAnimation === NO_PARENT_ANIMATION_ELEMENT_DETECTED) { - rootPlayers.push(player); - } else { - const parentPlayers = this.playersByElement.get(parentWithAnimation); + let parentHasPriority: any = null; + for (let i = 0; i < sortedParentElements.length; i++) { + const parent = sortedParentElements[i]; + if (parent === element) break; + if (this.driver.containsElement(parent, element)) { + parentHasPriority = parent; + break; + } + } + + if (parentHasPriority) { + const parentPlayers = this.playersByElement.get(parentHasPriority); if (parentPlayers && parentPlayers.length) { player.parentPlayer = optimizeGroupPlayer(parentPlayers); } skippedPlayers.push(player); + } else { + rootPlayers.push(player); } } else { eraseStyles(element, instruction.fromStyles); @@ -1122,7 +1105,7 @@ export class TransitionAnimationEngine { // fire the start/done transition callback events skippedPlayers.forEach(player => { if (player.parentPlayer) { - player.syncPlayerEvents(player.parentPlayer); + player.parentPlayer.onDestroy(() => player.destroy()); } else { player.destroy(); } @@ -1383,15 +1366,6 @@ export class TransitionAnimationPlayer implements AnimationPlayer { getRealPlayer() { return this._player; } - syncPlayerEvents(player: AnimationPlayer) { - const p = this._player as any; - if (p.triggerCallback) { - player.onStart(() => p.triggerCallback('start')); - } - player.onDone(() => this.finish()); - player.onDestroy(() => this.destroy()); - } - private _queueEvent(name: string, callback: (event: any) => any): void { getOrSetAsInMap(this._queuedCallbacks, name, []).push(callback); } @@ -1445,14 +1419,6 @@ export class TransitionAnimationPlayer implements AnimationPlayer { getPosition(): number { return this.queued ? 0 : this._player.getPosition(); } get totalTime(): number { return this._player.totalTime; } - - /* @internal */ - triggerCallback(phaseName: string): void { - const p = this._player as any; - if (p.triggerCallback) { - p.triggerCallback(phaseName); - } - } } function deleteOrUnsetInMap(map: Map| {[key: string]: any}, key: any, value: any) { 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 879bcf3f1d..dc7396f4f9 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 @@ -184,13 +184,6 @@ export class WebAnimationsPlayer implements AnimationPlayer { } this.currentSnapshot = styles; } - - /* @internal */ - triggerCallback(phaseName: string): void { - const methods = phaseName == 'start' ? this._onStartFns : this._onDoneFns; - methods.forEach(fn => fn()); - methods.length = 0; - } } function _computeStyle(element: any, prop: string): string { diff --git a/packages/animations/browser/test/render/web_animations/web_animations_player_spec.ts b/packages/animations/browser/test/render/web_animations/web_animations_player_spec.ts index a258ca03a0..d7dc7aefe7 100644 --- a/packages/animations/browser/test/render/web_animations/web_animations_player_spec.ts +++ b/packages/animations/browser/test/render/web_animations/web_animations_player_spec.ts @@ -45,26 +45,6 @@ export function main() { const p = innerPlayer !; expect(p.log).toEqual(['play']); }); - - it('should fire start/done callbacks manually when called directly', () => { - const log: string[] = []; - - const player = new WebAnimationsPlayer(element, [], {duration: 1000}); - player.onStart(() => log.push('started')); - player.onDone(() => log.push('done')); - - player.triggerCallback('start'); - expect(log).toEqual(['started']); - - player.play(); - expect(log).toEqual(['started']); - - player.triggerCallback('done'); - expect(log).toEqual(['started', 'done']); - - player.finish(); - expect(log).toEqual(['started', 'done']); - }); }); } diff --git a/packages/animations/src/players/animation_group_player.ts b/packages/animations/src/players/animation_group_player.ts index a73ebb2975..b67c4ef82f 100644 --- a/packages/animations/src/players/animation_group_player.ts +++ b/packages/animations/src/players/animation_group_player.ts @@ -140,11 +140,4 @@ export class AnimationGroupPlayer implements AnimationPlayer { } }); } - - /* @internal */ - triggerCallback(phaseName: string): void { - const methods = phaseName == 'start' ? this._onStartFns : this._onDoneFns; - methods.forEach(fn => fn()); - methods.length = 0; - } } diff --git a/packages/animations/src/players/animation_player.ts b/packages/animations/src/players/animation_player.ts index e8b5392be6..14d14f35f6 100644 --- a/packages/animations/src/players/animation_player.ts +++ b/packages/animations/src/players/animation_player.ts @@ -31,8 +31,6 @@ export interface AnimationPlayer { parentPlayer: AnimationPlayer|null; readonly totalTime: number; beforeDestroy?: () => any; - /* @internal */ - triggerCallback?: (phaseName: string) => void; } /** @@ -93,11 +91,4 @@ export class NoopAnimationPlayer implements AnimationPlayer { reset(): void {} setPosition(p: number): void {} getPosition(): number { return 0; } - - /* @internal */ - triggerCallback(phaseName: string): void { - const methods = phaseName == 'start' ? this._onStartFns : this._onDoneFns; - methods.forEach(fn => fn()); - methods.length = 0; - } -} \ No newline at end of file +} diff --git a/packages/animations/test/animation_player_spec.ts b/packages/animations/test/animation_player_spec.ts index aef8460d49..bf86c8f8aa 100644 --- a/packages/animations/test/animation_player_spec.ts +++ b/packages/animations/test/animation_player_spec.ts @@ -40,29 +40,5 @@ export function main() { player.destroy(); expect(log).toEqual(['started', 'done', 'destroy']); }); - - it('should fire start/done callbacks manually when called directly', fakeAsync(() => { - const log: string[] = []; - - const player = new NoopAnimationPlayer(); - player.onStart(() => log.push('started')); - player.onDone(() => log.push('done')); - flushMicrotasks(); - - player.triggerCallback('start'); - expect(log).toEqual(['started']); - - player.play(); - expect(log).toEqual(['started']); - - player.triggerCallback('done'); - expect(log).toEqual(['started', 'done']); - - player.finish(); - expect(log).toEqual(['started', 'done']); - - flushMicrotasks(); - expect(log).toEqual(['started', 'done']); - })); }); } diff --git a/packages/core/test/animation/animation_query_integration_spec.ts b/packages/core/test/animation/animation_query_integration_spec.ts index 736e2dc12b..c694c2afe5 100644 --- a/packages/core/test/animation/animation_query_integration_spec.ts +++ b/packages/core/test/animation/animation_query_integration_spec.ts @@ -409,39 +409,39 @@ export function main() { const fixture = TestBed.createComponent(Cmp); const cmp = fixture.componentInstance; + cmp.exp0 = 0; + cmp.exp1 = 0; cmp.exp2 = 0; cmp.exp3 = 0; cmp.exp4 = 0; cmp.exp5 = 0; fixture.detectChanges(); - - cmp.exp0 = 0; - fixture.detectChanges(); + engine.flush(); let players = engine.players; cancelAllPlayers(players); + cmp.exp0 = 1; + cmp.exp2 = 1; cmp.exp4 = 1; fixture.detectChanges(); - - cmp.exp0 = 1; - fixture.detectChanges(); + engine.flush(); players = engine.players; cancelAllPlayers(players); expect(players.length).toEqual(3); + cmp.exp0 = 2; + cmp.exp1 = 2; cmp.exp2 = 2; cmp.exp3 = 2; cmp.exp4 = 2; cmp.exp5 = 2; fixture.detectChanges(); - - cmp.exp0 = 2; - fixture.detectChanges(); + engine.flush(); players = engine.players; cancelAllPlayers(players); @@ -449,6 +449,7 @@ export function main() { cmp.exp0 = 3; fixture.detectChanges(); + engine.flush(); players = engine.players; cancelAllPlayers(players); @@ -2287,15 +2288,19 @@ export function main() { } TestBed.configureTestingModule({declarations: [ParentCmp, ChildCmp]}); + + const engine = TestBed.get(ɵAnimationEngine); const fixture = TestBed.createComponent(ParentCmp); const cmp = fixture.componentInstance; fixture.detectChanges(); + engine.flush(); const childCmp = cmp.childElm; cmp.exp = false; fixture.detectChanges(); + engine.flush(); flushMicrotasks(); expect(cmp.childEvent.toState).toEqual('void'); @@ -2723,106 +2728,6 @@ export function main() { expect(engine.players[0].getRealPlayer()).toBe(players[1]); }); - it('should fire and synchronize the start/done callbacks on sub triggers even if they are not allowed to animate within the animation', - fakeAsync(() => { - @Component({ - selector: 'parent-cmp', - animations: [ - trigger( - 'parent', - [ - transition( - '* => go', - [ - style({height: '0px'}), - animate(1000, style({height: '100px'})), - ]), - ]), - ], - template: ` -
- -
- ` - }) - class ParentCmp { - @ViewChild('child') public childCmp: any; - - public exp: any; - public log: string[] = []; - public remove = false; - - track(event: any) { this.log.push(`${event.triggerName}-${event.phaseName}`); } - } - - @Component({ - selector: 'child-cmp', - animations: [ - trigger( - 'child', - [ - transition( - '* => go', - [ - style({width: '0px'}), - animate(1000, style({width: '100px'})), - ]), - ]), - ], - template: ` -
- ` - }) - class ChildCmp { - public exp: any; - public log: string[] = []; - track(event: any) { this.log.push(`${event.triggerName}-${event.phaseName}`); } - } - - TestBed.configureTestingModule({declarations: [ParentCmp, ChildCmp]}); - const engine = TestBed.get(ɵAnimationEngine); - const fixture = TestBed.createComponent(ParentCmp); - fixture.detectChanges(); - flushMicrotasks(); - - const cmp = fixture.componentInstance; - const child = cmp.childCmp; - - expect(cmp.log).toEqual(['parent-start', 'parent-done']); - expect(child.log).toEqual(['child-start', 'child-done']); - - cmp.log = []; - child.log = []; - cmp.exp = 'go'; - cmp.childCmp.exp = 'go'; - fixture.detectChanges(); - flushMicrotasks(); - - expect(cmp.log).toEqual(['parent-start']); - expect(child.log).toEqual(['child-start']); - - const players = engine.players; - expect(players.length).toEqual(1); - players[0].finish(); - - expect(cmp.log).toEqual(['parent-start', 'parent-done']); - expect(child.log).toEqual(['child-start', 'child-done']); - - cmp.log = []; - child.log = []; - cmp.remove = true; - fixture.detectChanges(); - flushMicrotasks(); - - expect(cmp.log).toEqual(['parent-start', 'parent-done']); - expect(child.log).toEqual(['child-start', 'child-done']); - })); - it('should stretch the starting keyframe of a child animation queries are issued by the parent', () => { @Component({