From e1bf067090c20ba783197bec298ac35ac385e391 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matias=20Niemel=C3=A4?= Date: Wed, 14 Feb 2018 10:12:10 -0800 Subject: [PATCH] fix(animations): report correct totalTime value even during noOp animations (#22225) This patch ensures that if the NoopAnimationsModule is used then it will correctly report the associated `totalTime` property within the emitted AnimationEvent instance when an animation event trigger is fired. BREAKING CHANGE: When animation is trigged within a disabled zone, the associated event (which an instance of AnimationEvent) will no longer report the totalTime as 0 (it will emit the actual time of the animation). To detect if an animation event is reporting a disabled animation then the `event.disabled` property can be used instead. PR Close #22225 --- .../src/dsl/animation_transition_factory.ts | 7 +- .../dsl/animation_transition_instruction.ts | 4 +- .../browser/src/render/animation_driver.ts | 2 +- .../animations/browser/src/render/shared.ts | 17 ++--- .../src/render/transition_animation_engine.ts | 16 +++-- .../transition_animation_engine_spec.ts | 6 +- .../testing/src/mock_animation_driver.ts | 4 +- packages/animations/src/animation_event.ts | 1 + packages/animations/src/animation_metadata.ts | 8 +++ .../src/players/animation_player.ts | 8 ++- .../animation/animation_integration_spec.ts | 67 ++++++++++++++++--- .../animations/animations.d.ts | 5 +- 12 files changed, 108 insertions(+), 37 deletions(-) diff --git a/packages/animations/browser/src/dsl/animation_transition_factory.ts b/packages/animations/browser/src/dsl/animation_transition_factory.ts index cd45922eb8..d407da046f 100644 --- a/packages/animations/browser/src/dsl/animation_transition_factory.ts +++ b/packages/animations/browser/src/dsl/animation_transition_factory.ts @@ -59,10 +59,13 @@ export class AnimationTransitionFactory { driver, element, this.ast.animation, enterClassName, leaveClassName, currentStateStyles, nextStateStyles, animationOptions, subInstructions, errors); + let totalTime = 0; + timelines.forEach(tl => { totalTime = Math.max(tl.duration + tl.delay, totalTime); }); + if (errors.length) { return createTransitionInstruction( element, this._triggerName, currentState, nextState, isRemoval, currentStateStyles, - nextStateStyles, [], [], preStyleMap, postStyleMap, errors); + nextStateStyles, [], [], preStyleMap, postStyleMap, totalTime, errors); } timelines.forEach(tl => { @@ -81,7 +84,7 @@ export class AnimationTransitionFactory { const queriedElementsList = iteratorToArray(queriedElements.values()); return createTransitionInstruction( element, this._triggerName, currentState, nextState, isRemoval, currentStateStyles, - nextStateStyles, timelines, queriedElementsList, preStyleMap, postStyleMap); + nextStateStyles, timelines, queriedElementsList, preStyleMap, postStyleMap, totalTime); } } diff --git a/packages/animations/browser/src/dsl/animation_transition_instruction.ts b/packages/animations/browser/src/dsl/animation_transition_instruction.ts index f1062b322b..a09194faec 100644 --- a/packages/animations/browser/src/dsl/animation_transition_instruction.ts +++ b/packages/animations/browser/src/dsl/animation_transition_instruction.ts @@ -21,6 +21,7 @@ export interface AnimationTransitionInstruction extends AnimationEngineInstructi queriedElements: any[]; preStyleProps: Map; postStyleProps: Map; + totalTime: number; errors?: any[]; } @@ -29,7 +30,7 @@ export function createTransitionInstruction( isRemovalTransition: boolean, fromStyles: ɵStyleData, toStyles: ɵStyleData, timelines: AnimationTimelineInstruction[], queriedElements: any[], preStyleProps: Map, - postStyleProps: Map, + postStyleProps: Map, totalTime: number, errors?: any[]): AnimationTransitionInstruction { return { type: AnimationTransitionInstructionType.TransitionAnimation, @@ -44,6 +45,7 @@ export function createTransitionInstruction( queriedElements, preStyleProps, postStyleProps, + totalTime, errors }; } diff --git a/packages/animations/browser/src/render/animation_driver.ts b/packages/animations/browser/src/render/animation_driver.ts index 0fb38de9be..462c6afdcb 100644 --- a/packages/animations/browser/src/render/animation_driver.ts +++ b/packages/animations/browser/src/render/animation_driver.ts @@ -34,7 +34,7 @@ export class NoopAnimationDriver implements AnimationDriver { animate( element: any, keyframes: {[key: string]: string | number}[], duration: number, delay: number, easing: string, previousPlayers: any[] = []): AnimationPlayer { - return new NoopAnimationPlayer(); + return new NoopAnimationPlayer(duration, delay); } } diff --git a/packages/animations/browser/src/render/shared.ts b/packages/animations/browser/src/render/shared.ts index 034a091ade..916fce6746 100644 --- a/packages/animations/browser/src/render/shared.ts +++ b/packages/animations/browser/src/render/shared.ts @@ -75,23 +75,24 @@ export function listenOnPlayer( callback: (event: any) => any) { switch (eventName) { case 'start': - player.onStart(() => callback(event && copyAnimationEvent(event, 'start', player.totalTime))); + player.onStart(() => callback(event && copyAnimationEvent(event, 'start', player))); break; case 'done': - player.onDone(() => callback(event && copyAnimationEvent(event, 'done', player.totalTime))); + player.onDone(() => callback(event && copyAnimationEvent(event, 'done', player))); break; case 'destroy': - player.onDestroy( - () => callback(event && copyAnimationEvent(event, 'destroy', player.totalTime))); + player.onDestroy(() => callback(event && copyAnimationEvent(event, 'destroy', player))); break; } } export function copyAnimationEvent( - e: AnimationEvent, phaseName?: string, totalTime?: number): AnimationEvent { + e: AnimationEvent, phaseName: string, player: AnimationPlayer): AnimationEvent { + const totalTime = player.totalTime; + const disabled = (player as any).disabled ? true : false; const event = makeAnimationEvent( e.element, e.triggerName, e.fromState, e.toState, phaseName || e.phaseName, - totalTime == undefined ? e.totalTime : totalTime); + totalTime == undefined ? e.totalTime : totalTime, disabled); const data = (e as any)['_data']; if (data != null) { (event as any)['_data'] = data; @@ -101,8 +102,8 @@ export function copyAnimationEvent( export function makeAnimationEvent( element: any, triggerName: string, fromState: string, toState: string, phaseName: string = '', - totalTime: number = 0): AnimationEvent { - return {element, triggerName, fromState, toState, phaseName, totalTime}; + totalTime: number = 0, disabled?: boolean): AnimationEvent { + return {element, triggerName, fromState, toState, phaseName, totalTime, disabled: !!disabled}; } export function getOrSetAsInMap( diff --git a/packages/animations/browser/src/render/transition_animation_engine.ts b/packages/animations/browser/src/render/transition_animation_engine.ts index b483cbc219..e06fc3221b 100644 --- a/packages/animations/browser/src/render/transition_animation_engine.ts +++ b/packages/animations/browser/src/render/transition_animation_engine.ts @@ -1081,6 +1081,8 @@ export class TransitionAnimationEngine { if (subTimelines.has(element)) { if (disabledElementsSet.has(element)) { player.onDestroy(() => setStyles(element, instruction.toStyles)); + player.disabled = true; + player.overrideTotalTime(instruction.totalTime); skippedPlayers.push(player); return; } @@ -1311,7 +1313,8 @@ export class TransitionAnimationEngine { // FIXME (matsko): make sure to-be-removed animations are removed properly const details = element[REMOVAL_FLAG]; - if (details && details.removedBeforeQueried) return new NoopAnimationPlayer(); + if (details && details.removedBeforeQueried) + return new NoopAnimationPlayer(timelineInstruction.duration, timelineInstruction.delay); const isQueriedElement = element !== rootElement; const previousPlayers = @@ -1379,7 +1382,7 @@ export class TransitionAnimationEngine { // special case for when an empty transition|definition is provided // ... there is no point in rendering an empty animation - return new NoopAnimationPlayer(); + return new NoopAnimationPlayer(instruction.duration, instruction.delay); } } @@ -1392,8 +1395,10 @@ export class TransitionAnimationPlayer implements AnimationPlayer { public parentPlayer: AnimationPlayer; public markedForDestroy: boolean = false; + public disabled = false; readonly queued: boolean = true; + public readonly totalTime: number = 0; constructor(public namespaceId: string, public triggerName: string, public element: any) {} @@ -1407,15 +1412,18 @@ export class TransitionAnimationPlayer implements AnimationPlayer { }); this._queuedCallbacks = {}; this._containsRealPlayer = true; + this.overrideTotalTime(player.totalTime); (this as{queued: boolean}).queued = false; } getRealPlayer() { return this._player; } + overrideTotalTime(totalTime: number) { (this as any).totalTime = totalTime; } + syncPlayerEvents(player: AnimationPlayer) { const p = this._player as any; if (p.triggerCallback) { - player.onStart(() => p.triggerCallback('start')); + player.onStart(() => p.triggerCallback !('start')); } player.onDone(() => this.finish()); player.onDestroy(() => this.destroy()); @@ -1473,8 +1481,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; diff --git a/packages/animations/browser/test/render/transition_animation_engine_spec.ts b/packages/animations/browser/test/render/transition_animation_engine_spec.ts index c1e25e0dd3..314b262780 100644 --- a/packages/animations/browser/test/render/transition_animation_engine_spec.ts +++ b/packages/animations/browser/test/render/transition_animation_engine_spec.ts @@ -299,7 +299,8 @@ const DEFAULT_NAMESPACE_ID = 'id'; phaseName: 'start', fromState: '123', toState: '456', - totalTime: 1234 + totalTime: 1234, + disabled: false }); capture = null !; @@ -313,7 +314,8 @@ const DEFAULT_NAMESPACE_ID = 'id'; phaseName: 'done', fromState: '123', toState: '456', - totalTime: 1234 + totalTime: 1234, + disabled: false }); }); }); diff --git a/packages/animations/browser/testing/src/mock_animation_driver.ts b/packages/animations/browser/testing/src/mock_animation_driver.ts index fc48070ba0..9e05ab0476 100644 --- a/packages/animations/browser/testing/src/mock_animation_driver.ts +++ b/packages/animations/browser/testing/src/mock_animation_driver.ts @@ -58,7 +58,7 @@ export class MockAnimationPlayer extends NoopAnimationPlayer { public element: any, public keyframes: {[key: string]: string | number}[], public duration: number, public delay: number, public easing: string, public previousPlayers: any[]) { - super(); + super(duration, delay); if (allowPreviousPlayerStylesMerge(duration, delay)) { previousPlayers.forEach(player => { @@ -68,8 +68,6 @@ export class MockAnimationPlayer extends NoopAnimationPlayer { } }); } - - this.totalTime = delay + duration; } /* @internal */ diff --git a/packages/animations/src/animation_event.ts b/packages/animations/src/animation_event.ts index cfe5ab46e9..d4df068925 100644 --- a/packages/animations/src/animation_event.ts +++ b/packages/animations/src/animation_event.ts @@ -44,4 +44,5 @@ export interface AnimationEvent { phaseName: string; element: any; triggerName: string; + disabled: boolean; } diff --git a/packages/animations/src/animation_metadata.ts b/packages/animations/src/animation_metadata.ts index 2522ed8cfd..72bfee2d00 100755 --- a/packages/animations/src/animation_metadata.ts +++ b/packages/animations/src/animation_metadata.ts @@ -352,6 +352,14 @@ export interface AnimationStaggerMetadata extends AnimationMetadata { elements located in disabled areas of the template and still animate them as it sees fit. This is also the case for when a sub animation is queried by a parent and then later animated using {@link animateChild animateChild}. + + * ### Detecting when an animation is disabled + * If a region of the DOM (or the entire application) has its animations disabled, then animation + * trigger callbacks will still fire just as normal (only for zero seconds). + * + * When a trigger callback fires it will provide an instance of an {@link AnimationEvent}. If + animations + * are disabled then the `.disabled` flag on the event will be true. * * @experimental Animation support is experimental. */ diff --git a/packages/animations/src/players/animation_player.ts b/packages/animations/src/players/animation_player.ts index c789d420a9..de3dfb4ffa 100644 --- a/packages/animations/src/players/animation_player.ts +++ b/packages/animations/src/players/animation_player.ts @@ -33,6 +33,8 @@ export interface AnimationPlayer { beforeDestroy?: () => any; /* @internal */ triggerCallback?: (phaseName: string) => void; + /* @internal */ + disabled?: boolean; } /** @@ -46,8 +48,8 @@ export class NoopAnimationPlayer implements AnimationPlayer { private _destroyed = false; private _finished = false; public parentPlayer: AnimationPlayer|null = null; - public totalTime = 0; - constructor() {} + public readonly totalTime: number; + constructor(duration: number = 0, delay: number = 0) { this.totalTime = duration + delay; } private _onFinish() { if (!this._finished) { this._finished = true; @@ -100,4 +102,4 @@ export class NoopAnimationPlayer implements AnimationPlayer { methods.forEach(fn => fn()); methods.length = 0; } -} \ No newline at end of file +} diff --git a/packages/core/test/animation/animation_integration_spec.ts b/packages/core/test/animation/animation_integration_spec.ts index 1bd7c3b752..a5c92395a3 100644 --- a/packages/core/test/animation/animation_integration_spec.ts +++ b/packages/core/test/animation/animation_integration_spec.ts @@ -5,8 +5,8 @@ * 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 {AUTO_STYLE, AnimationEvent, AnimationOptions, animate, animateChild, group, keyframes, query, state, style, transition, trigger, ɵPRE_STYLE as PRE_STYLE} from '@angular/animations'; -import {AnimationDriver, ɵAnimationEngine, ɵNoopAnimationDriver} from '@angular/animations/browser'; +import {AUTO_STYLE, AnimationEvent, AnimationOptions, AnimationPlayer, NoopAnimationPlayer, animate, animateChild, group, keyframes, query, state, style, transition, trigger, ɵPRE_STYLE as PRE_STYLE} from '@angular/animations'; +import {AnimationDriver, ɵAnimationEngine, ɵNoopAnimationDriver as NoopAnimationDriver} from '@angular/animations/browser'; import {MockAnimationDriver, MockAnimationPlayer} from '@angular/animations/browser/testing'; import {ChangeDetectorRef, Component, HostBinding, HostListener, RendererFactory2, ViewChild} from '@angular/core'; import {ɵDomRendererFactory2} from '@angular/platform-browser'; @@ -112,6 +112,50 @@ const DEFAULT_COMPONENT_ID = '1'; flushMicrotasks(); expect(cmp.log).toEqual(['start', 'done']); })); + + it('should emit the correct totalTime value for a noop-animation', fakeAsync(() => { + @Component({ + selector: 'cmp', + template: ` +
+ `, + animations: [ + trigger( + 'myAnimation', + [ + transition( + '* => go', + [ + animate('1s', style({opacity: 0})), + ]), + ]), + ] + }) + class Cmp { + exp: any = false; + log: AnimationEvent[] = []; + cb(event: AnimationEvent) { this.log.push(event); } + } + + TestBed.configureTestingModule({ + declarations: [Cmp], + providers: [ + {provide: AnimationDriver, useClass: NoopAnimationDriver}, + ], + }); + + const fixture = TestBed.createComponent(Cmp); + const cmp = fixture.componentInstance; + cmp.exp = 'go'; + fixture.detectChanges(); + expect(cmp.log).toEqual([]); + + flushMicrotasks(); + expect(cmp.log.length).toEqual(2); + const [start, end] = cmp.log; + expect(start.totalTime).toEqual(1000); + expect(end.totalTime).toEqual(1000); + })); }); describe('component fixture integration', () => { @@ -166,7 +210,7 @@ const DEFAULT_COMPONENT_ID = '1'; } TestBed.configureTestingModule({ - providers: [{provide: AnimationDriver, useClass: ɵNoopAnimationDriver}], + providers: [{provide: AnimationDriver, useClass: NoopAnimationDriver}], declarations: [Cmp] }); @@ -2461,7 +2505,7 @@ const DEFAULT_COMPONENT_ID = '1'; } TestBed.configureTestingModule({ - providers: [{provide: AnimationDriver, useClass: ɵNoopAnimationDriver}], + providers: [{provide: AnimationDriver, useClass: NoopAnimationDriver}], declarations: [Cmp] }); @@ -2500,7 +2544,7 @@ const DEFAULT_COMPONENT_ID = '1'; } TestBed.configureTestingModule({ - providers: [{provide: AnimationDriver, useClass: ɵNoopAnimationDriver}], + providers: [{provide: AnimationDriver, useClass: NoopAnimationDriver}], declarations: [Cmp] }); @@ -2971,8 +3015,8 @@ const DEFAULT_COMPONENT_ID = '1'; class Cmp { disableExp = false; exp = ''; - startEvent: any; - doneEvent: any; + startEvent: AnimationEvent; + doneEvent: AnimationEvent; } TestBed.configureTestingModule({declarations: [Cmp]}); @@ -2988,14 +3032,17 @@ const DEFAULT_COMPONENT_ID = '1'; cmp.exp = '1'; fixture.detectChanges(); flushMicrotasks(); - expect(cmp.startEvent.totalTime).toEqual(0); - expect(cmp.doneEvent.totalTime).toEqual(0); + expect(cmp.startEvent.totalTime).toEqual(9876); + expect(cmp.startEvent.disabled).toBeTruthy(); + expect(cmp.doneEvent.totalTime).toEqual(9876); + expect(cmp.doneEvent.disabled).toBeTruthy(); cmp.exp = '2'; cmp.disableExp = false; fixture.detectChanges(); flushMicrotasks(); expect(cmp.startEvent.totalTime).toEqual(9876); + expect(cmp.startEvent.disabled).toBeFalsy(); // the done event isn't fired because it's an actual animation })); @@ -3428,7 +3475,7 @@ const DEFAULT_COMPONENT_ID = '1'; }); }); }); -}); +})(); function assertHasParent(element: any, yes: boolean) { const parent = getDOM().parentElement(element); diff --git a/tools/public_api_guard/animations/animations.d.ts b/tools/public_api_guard/animations/animations.d.ts index a302384103..156e002969 100644 --- a/tools/public_api_guard/animations/animations.d.ts +++ b/tools/public_api_guard/animations/animations.d.ts @@ -43,6 +43,7 @@ export declare abstract class AnimationBuilder { /** @experimental */ export interface AnimationEvent { + disabled: boolean; element: any; fromState: string; phaseName: string; @@ -199,8 +200,8 @@ export declare function keyframes(steps: AnimationStyleMetadata[]): AnimationKey /** @experimental */ export declare class NoopAnimationPlayer implements AnimationPlayer { parentPlayer: AnimationPlayer | null; - totalTime: number; - constructor(); + readonly totalTime: number; + constructor(duration?: number, delay?: number); destroy(): void; finish(): void; getPosition(): number;