Skip to content

Commit

Permalink
fix(animations): report correct totalTime value even during noOp anim…
Browse files Browse the repository at this point in the history
…ations (#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
  • Loading branch information
matsko authored and vicb committed Feb 17, 2018
1 parent 884de18 commit e1bf067
Show file tree
Hide file tree
Showing 12 changed files with 108 additions and 37 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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 => {
Expand All @@ -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);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ export interface AnimationTransitionInstruction extends AnimationEngineInstructi
queriedElements: any[];
preStyleProps: Map<any, {[prop: string]: boolean}>;
postStyleProps: Map<any, {[prop: string]: boolean}>;
totalTime: number;
errors?: any[];
}

Expand All @@ -29,7 +30,7 @@ export function createTransitionInstruction(
isRemovalTransition: boolean, fromStyles: ɵStyleData, toStyles: ɵStyleData,
timelines: AnimationTimelineInstruction[], queriedElements: any[],
preStyleProps: Map<any, {[prop: string]: boolean}>,
postStyleProps: Map<any, {[prop: string]: boolean}>,
postStyleProps: Map<any, {[prop: string]: boolean}>, totalTime: number,
errors?: any[]): AnimationTransitionInstruction {
return {
type: AnimationTransitionInstructionType.TransitionAnimation,
Expand All @@ -44,6 +45,7 @@ export function createTransitionInstruction(
queriedElements,
preStyleProps,
postStyleProps,
totalTime,
errors
};
}
2 changes: 1 addition & 1 deletion packages/animations/browser/src/render/animation_driver.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}

Expand Down
17 changes: 9 additions & 8 deletions packages/animations/browser/src/render/shared.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down Expand Up @@ -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 =
Expand Down Expand Up @@ -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);
}
}

Expand All @@ -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) {}

Expand All @@ -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());
Expand Down Expand Up @@ -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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -299,7 +299,8 @@ const DEFAULT_NAMESPACE_ID = 'id';
phaseName: 'start',
fromState: '123',
toState: '456',
totalTime: 1234
totalTime: 1234,
disabled: false
});

capture = null !;
Expand All @@ -313,7 +314,8 @@ const DEFAULT_NAMESPACE_ID = 'id';
phaseName: 'done',
fromState: '123',
toState: '456',
totalTime: 1234
totalTime: 1234,
disabled: false
});
});
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 => {
Expand All @@ -68,8 +68,6 @@ export class MockAnimationPlayer extends NoopAnimationPlayer {
}
});
}

this.totalTime = delay + duration;
}

/* @internal */
Expand Down
1 change: 1 addition & 0 deletions packages/animations/src/animation_event.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,4 +44,5 @@ export interface AnimationEvent {
phaseName: string;
element: any;
triggerName: string;
disabled: boolean;
}
8 changes: 8 additions & 0 deletions packages/animations/src/animation_metadata.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*/
Expand Down
8 changes: 5 additions & 3 deletions packages/animations/src/players/animation_player.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@ export interface AnimationPlayer {
beforeDestroy?: () => any;
/* @internal */
triggerCallback?: (phaseName: string) => void;
/* @internal */
disabled?: boolean;
}

/**
Expand All @@ -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;
Expand Down Expand Up @@ -100,4 +102,4 @@ export class NoopAnimationPlayer implements AnimationPlayer {
methods.forEach(fn => fn());
methods.length = 0;
}
}
}
67 changes: 57 additions & 10 deletions packages/core/test/animation/animation_integration_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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: `
<div [@myAnimation]="exp" (@myAnimation.start)="cb($event)" (@myAnimation.done)="cb($event)"></div>
`,
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', () => {
Expand Down Expand Up @@ -166,7 +210,7 @@ const DEFAULT_COMPONENT_ID = '1';
}

TestBed.configureTestingModule({
providers: [{provide: AnimationDriver, useClass: ɵNoopAnimationDriver}],
providers: [{provide: AnimationDriver, useClass: NoopAnimationDriver}],
declarations: [Cmp]
});

Expand Down Expand Up @@ -2461,7 +2505,7 @@ const DEFAULT_COMPONENT_ID = '1';
}

TestBed.configureTestingModule({
providers: [{provide: AnimationDriver, useClass: ɵNoopAnimationDriver}],
providers: [{provide: AnimationDriver, useClass: NoopAnimationDriver}],
declarations: [Cmp]
});

Expand Down Expand Up @@ -2500,7 +2544,7 @@ const DEFAULT_COMPONENT_ID = '1';
}

TestBed.configureTestingModule({
providers: [{provide: AnimationDriver, useClass: ɵNoopAnimationDriver}],
providers: [{provide: AnimationDriver, useClass: NoopAnimationDriver}],
declarations: [Cmp]
});

Expand Down Expand Up @@ -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]});
Expand All @@ -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
}));

Expand Down Expand Up @@ -3428,7 +3475,7 @@ const DEFAULT_COMPONENT_ID = '1';
});
});
});
});
})();

function assertHasParent(element: any, yes: boolean) {
const parent = getDOM().parentElement(element);
Expand Down
Loading

0 comments on commit e1bf067

Please sign in to comment.