From 88e3d7af9f93eeac5253c023ebc0d6deb9064a1d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matias=20Niemel=C3=A4?= Date: Fri, 10 Feb 2017 14:22:34 -0800 Subject: [PATCH] refactor(animations): do not inherit past styles into sub timelines --- .../src/dsl/animation_timeline_visitor.ts | 93 ++++++------------- .../web_animations/web_animations_player.ts | 10 +- .../animation/test/dsl/animation_spec.ts | 71 ++++++++++++-- 3 files changed, 96 insertions(+), 78 deletions(-) diff --git a/modules/@angular/animation/src/dsl/animation_timeline_visitor.ts b/modules/@angular/animation/src/dsl/animation_timeline_visitor.ts index 32f4f3103b..70a24df24f 100644 --- a/modules/@angular/animation/src/dsl/animation_timeline_visitor.ts +++ b/modules/@angular/animation/src/dsl/animation_timeline_visitor.ts @@ -51,17 +51,10 @@ import {AnimationTimelineInstruction, createTimelineInstruction} from './animati * * [TimelineBuilder] * This class is responsible for tracking the styles and building a series of keyframe objects for a - * timeline between a start and end time. There is always one top-level timeline and sub timelines - * are forked in two specific cases: - * - * 1. When keyframes() is used it will create a sub timeline. Upon creation, ALL OF THE COLLECTED - * STYLES from the parent timeline up until this point will be inherited into the keyframes - * timeline. - * - * 2. When group() is used it will create a sub timeline. Upon creation, NONE OF THE COLLECTED - * STYLES from the parent timeline will be inherited. Although, if the sub timeline does reference a - * style that was previously used within the parent then it will be copied over into the sub - * timeline. + * timeline between a start and end time. The builder starts off with an initial timeline and each + * time the AST comes across a `group()`, `keyframes()` or a combination of the two wihtin a + * `sequence()` then it will generate a sub timeline for each step as well as a new one after + * they are complete. * * As the AST is traversed, the timing state on each of the timelines will be incremented. If a sub * timeline was created (based on one of the cases above) then the parent timeline will attempt to @@ -96,22 +89,14 @@ import {AnimationTimelineInstruction, createTimelineInstruction} from './animati * keyframes. Therefore the missing `height` value will be properly filled into the already * processed keyframes. * + * When a sub-timeline is created it will have its own backFill property. This is done so that + * styles present within the sub-timeline do not accidentally seep into the previous/future timeline + * keyframes + * * (For prototypically-inherited contents to be detected a `for(i in obj)` loop must be used.) * - * Based on whether the styles are inherited into a sub timeline (depending on the two cases - * mentioned above), the functionality of the backFill will behave differently: - * - * 1. If the styles are inherited from the parent then the backFill property will also be inherited - * and therefore any newly added styles to the backFill will be propagated to the parent timeline - * and its already processed keyframes. - * - * 2. If the styles are not inherited from the parent then the sub timeline will have its own - * backFill. Then if the sub timeline comes across a property that was not defined already then it - * will read that from the parent's styles and pass that into its own backFill (which will then - * propagate the missing styles across the sub timeline only). - * * [Validation] - * The code in this file is not responsible for validation. That functionaliy happens with within + * The code in this file is not responsible for validation. That functionality happens with within * the `AnimationValidatorVisitor` code. */ export function buildAnimationKeyframes( @@ -139,9 +124,9 @@ export class AnimationTimelineContext { timelines.push(this.currentTimeline); } - createSubContext(inherit: boolean = false): AnimationTimelineContext { - const context = new AnimationTimelineContext( - this.errors, this.timelines, this.currentTimeline.fork(inherit)); + createSubContext(): AnimationTimelineContext { + const context = + new AnimationTimelineContext(this.errors, this.timelines, this.currentTimeline.fork()); context.previousNode = this.previousNode; context.currentAnimateTimings = this.currentAnimateTimings; this.subContextCount++; @@ -154,7 +139,7 @@ export class AnimationTimelineContext { if (newTime > 0) { oldTimeline.time = newTime; } - this.currentTimeline = oldTimeline.fork(true); + this.currentTimeline = oldTimeline.fork(); oldTimeline.time = oldTime; this.timelines.push(this.currentTimeline); return this.currentTimeline; @@ -217,35 +202,33 @@ export class AnimationTimelineVisitor implements AnimationDslVisitor { context.currentTimeline.snapshotCurrentStyles(); } ast.steps.map(s => visitAnimationNode(this, s, context)); - context.previousNode = ast; + + // this means that some animation function within the sequence + // ended up creating a sub timeline (which means the current + // timeline cannot overlap with the contents of the sequence) if (context.subContextCount > subContextCount) { context.transformIntoNewTimeline(); - context.currentTimeline.snapshotCurrentStyles(); } + + context.previousNode = ast; } visitGroup(ast: meta.AnimationGroupMetadata, context: AnimationTimelineContext) { const innerTimelines: TimelineBuilder[] = []; let furthestTime = context.currentTimeline.currentTime; ast.steps.map(s => { - const innerContext = context.createSubContext(false); - innerContext.currentTimeline.snapshotCurrentStyles(); + const innerContext = context.createSubContext(); visitAnimationNode(this, s, innerContext); furthestTime = Math.max(furthestTime, innerContext.currentTimeline.currentTime); innerTimelines.push(innerContext.currentTimeline); }); - context.transformIntoNewTimeline(furthestTime); - // this operation is run after the AST loop because otherwise // if the parent timeline's collected styles were updated then // it would pass in invalid data into the new-to-be forked items innerTimelines.forEach( timeline => context.currentTimeline.mergeTimelineCollectedStyles(timeline)); - - // we do this because the window between this timeline and the sub timeline - // should ensure that the styles within are exactly the same as they were before - context.currentTimeline.snapshotCurrentStyles(); + context.transformIntoNewTimeline(furthestTime); context.previousNode = ast; } @@ -307,16 +290,10 @@ export class AnimationTimelineVisitor implements AnimationDslVisitor { } const keyframeDuration = context.currentAnimateTimings.duration; - const innerContext = context.createSubContext(true); + const innerContext = context.createSubContext(); const innerTimeline = innerContext.currentTimeline; innerTimeline.easing = context.currentAnimateTimings.easing; - // this will ensure that all collected styles so far - // are populated into the first keyframe of the keyframes() - // timeline (even if there exists a starting keyframe then - // it will override the contents of the first frame later) - innerTimeline.snapshotCurrentStyles(); - ast.steps.map((step: meta.AnimationStyleMetadata, i: number) => { const normalizedStyles = normalizeStyles(new AnimationStyles(step.styles)); const offset = containsOffsets ? normalizedStyles['offset'] : @@ -332,7 +309,6 @@ export class AnimationTimelineVisitor implements AnimationDslVisitor { // we do this because the window between this timeline and the sub timeline // should ensure that the styles within are exactly the same as they were before context.transformIntoNewTimeline(context.currentTimeline.time + keyframeDuration); - context.currentTimeline.snapshotCurrentStyles(); context.previousNode = ast; } } @@ -346,18 +322,8 @@ export class TimelineBuilder { private _localTimelineStyles: StyleData; private _backFill: StyleData = {}; - constructor( - public startTime: number, private _globalTimelineStyles: StyleData = null, - inheritedBackFill: StyleData = null, inheritedStyles: StyleData = null) { - if (inheritedBackFill) { - this._backFill = inheritedBackFill; - } - + constructor(public startTime: number, private _globalTimelineStyles: StyleData = null) { this._localTimelineStyles = Object.create(this._backFill, {}); - if (inheritedStyles) { - this._localTimelineStyles = copyStyles(inheritedStyles, false, this._localTimelineStyles); - } - if (!this._globalTimelineStyles) { this._globalTimelineStyles = this._localTimelineStyles; } @@ -368,11 +334,8 @@ export class TimelineBuilder { get currentTime() { return this.startTime + this.time; } - fork(inherit: boolean = false): TimelineBuilder { - let inheritedBackFill = inherit ? this._backFill : null; - let inheritedStyles = inherit ? this._localTimelineStyles : null; - return new TimelineBuilder( - this.currentTime, this._globalTimelineStyles, inheritedBackFill, inheritedStyles); + fork(): TimelineBuilder { + return new TimelineBuilder(this.currentTime, this._globalTimelineStyles); } private _loadKeyframe() { @@ -395,9 +358,6 @@ export class TimelineBuilder { private _updateStyle(prop: string, value: string|number) { if (prop != 'easing') { - if (!this._localTimelineStyles[prop]) { - this._backFill[prop] = this._globalTimelineStyles[prop] || meta.AUTO_STYLE; - } this._localTimelineStyles[prop] = value; this._globalTimelineStyles[prop] = value; this._styleSummary[prop] = {time: this.currentTime, value}; @@ -409,6 +369,9 @@ export class TimelineBuilder { if (prop !== 'offset') { const val = styles[prop]; this._currentKeyframe[prop] = val; + if (prop !== 'easing' && !this._localTimelineStyles[prop]) { + this._backFill[prop] = this._globalTimelineStyles[prop] || meta.AUTO_STYLE; + } this._updateStyle(prop, val); } }); diff --git a/modules/@angular/animation/src/engine/web_animations/web_animations_player.ts b/modules/@angular/animation/src/engine/web_animations/web_animations_player.ts index 9c6fb63f13..28deb7a51e 100644 --- a/modules/@angular/animation/src/engine/web_animations/web_animations_player.ts +++ b/modules/@angular/animation/src/engine/web_animations/web_animations_player.ts @@ -16,11 +16,13 @@ export class WebAnimationsPlayer implements AnimationPlayer { private _onDestroyFns: Function[] = []; private _player: DOMAnimation; private _duration: number; + private _delay: number; private _initialized = false; private _finished = false; private _started = false; private _destroyed = false; private _finalKeyframe: {[key: string]: string | number}; + public time = 0; public parentPlayer: AnimationPlayer = null; public previousStyles: {[styleName: string]: string | number}; @@ -30,6 +32,8 @@ export class WebAnimationsPlayer implements AnimationPlayer { public options: {[key: string]: string | number}, previousPlayers: WebAnimationsPlayer[] = []) { this._duration = options['duration']; + this._delay = options['delay'] || 0; + this.time = this._duration + this._delay; this.previousStyles = {}; previousPlayers.forEach(player => { @@ -157,11 +161,9 @@ export class WebAnimationsPlayer implements AnimationPlayer { } } - get totalTime(): number { return this._duration; } + setPosition(p: number): void { this._player.currentTime = p * this.time; } - setPosition(p: number): void { this._player.currentTime = p * this.totalTime; } - - getPosition(): number { return this._player.currentTime / this.totalTime; } + getPosition(): number { return this._player.currentTime / this.time; } private _captureStyles(): {[prop: string]: string | number} { const styles: {[key: string]: string | number} = {}; diff --git a/modules/@angular/animation/test/dsl/animation_spec.ts b/modules/@angular/animation/test/dsl/animation_spec.ts index 33bd62e281..e120e2b89e 100644 --- a/modules/@angular/animation/test/dsl/animation_spec.ts +++ b/modules/@angular/animation/test/dsl/animation_spec.ts @@ -227,8 +227,8 @@ export function main() { const steps = [ animate(1000, style({opacity: .5})), animate(1000, style({opacity: 1})), animate( - 1000, keyframes([style({height: 0}), style({height: 100}), style({height: 0})])), - animate(1000, style({opacity: 0})) + 1000, keyframes([style({height: 0}), style({height: 100}), style({height: 50})])), + animate(1000, style({height: 0, opacity: 0})) ]; const players = invokeAnimationSequence(steps); @@ -237,23 +237,23 @@ export function main() { const player0 = players[0]; expect(player0.delay).toEqual(0); expect(player0.keyframes).toEqual([ - {opacity: AUTO_STYLE, height: AUTO_STYLE, offset: 0}, - {opacity: .5, height: AUTO_STYLE, offset: .5}, - {opacity: 1, height: AUTO_STYLE, offset: 1}, + {opacity: AUTO_STYLE, offset: 0}, + {opacity: .5, offset: .5}, + {opacity: 1, offset: 1}, ]); const subPlayer = players[1]; expect(subPlayer.delay).toEqual(2000); expect(subPlayer.keyframes).toEqual([ - {opacity: 1, height: 0, offset: 0}, - {opacity: 1, height: 100, offset: .5}, - {opacity: 1, height: 0, offset: 1}, + {height: 0, offset: 0}, + {height: 100, offset: .5}, + {height: 50, offset: 1}, ]); const player1 = players[2]; expect(player1.delay).toEqual(3000); expect(player1.keyframes).toEqual([ - {opacity: 1, height: 0, offset: 0}, {opacity: 0, height: 0, offset: 1} + {opacity: 1, height: 50, offset: 0}, {opacity: 0, height: 0, offset: 1} ]); }); @@ -322,6 +322,59 @@ export function main() { const player = invokeAnimationSequence(steps)[1]; expect(player.delay).toEqual(2500); }); + + it('should not leak in additional styles used later on after keyframe styles have already been declared', + () => { + const steps = [ + animate(1000, style({height: '50px'})), + animate( + 2000, keyframes([ + style({left: '0', transform: 'rotate(0deg)', offset: 0}), + style({ + left: '40%', + transform: 'rotate(250deg) translateY(-200px)', + offset: .33 + }), + style( + {left: '60%', transform: 'rotate(180deg) translateY(200px)', offset: .66}), + style({left: 'calc(100% - 100px)', transform: 'rotate(0deg)', offset: 1}), + ])), + group([animate('2s', style({width: '200px'}))]), + animate('2s', style({height: '300px'})), + group([animate('2s', style({height: '500px', width: '500px'}))]) + ]; + + const players = invokeAnimationSequence(steps); + expect(players.length).toEqual(5); + + const firstPlayerKeyframes = players[0].keyframes; + expect(firstPlayerKeyframes[0]['width']).toBeFalsy(); + expect(firstPlayerKeyframes[1]['width']).toBeFalsy(); + expect(firstPlayerKeyframes[0]['height']).toEqual(AUTO_STYLE); + expect(firstPlayerKeyframes[1]['height']).toEqual('50px'); + + const keyframePlayerKeyframes = players[1].keyframes; + expect(keyframePlayerKeyframes[0]['width']).toBeFalsy(); + expect(keyframePlayerKeyframes[0]['height']).toBeFalsy(); + + const groupPlayerKeyframes = players[2].keyframes; + expect(groupPlayerKeyframes[0]['width']).toEqual(AUTO_STYLE); + expect(groupPlayerKeyframes[1]['width']).toEqual('200px'); + expect(groupPlayerKeyframes[0]['height']).toBeFalsy(); + expect(groupPlayerKeyframes[1]['height']).toBeFalsy(); + + const secondToFinalAnimatePlayerKeyframes = players[3].keyframes; + expect(secondToFinalAnimatePlayerKeyframes[0]['width']).toBeFalsy(); + expect(secondToFinalAnimatePlayerKeyframes[1]['width']).toBeFalsy(); + expect(secondToFinalAnimatePlayerKeyframes[0]['height']).toEqual('50px'); + expect(secondToFinalAnimatePlayerKeyframes[1]['height']).toEqual('300px'); + + const finalAnimatePlayerKeyframes = players[4].keyframes; + expect(finalAnimatePlayerKeyframes[0]['width']).toEqual('200px'); + expect(finalAnimatePlayerKeyframes[1]['width']).toEqual('500px'); + expect(finalAnimatePlayerKeyframes[0]['height']).toEqual('300px'); + expect(finalAnimatePlayerKeyframes[1]['height']).toEqual('500px'); + }); }); describe('group()', () => {