From add5953aa1df7d302de0a6167f415ddbf9ecca92 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mi=C5=A1ko=20Hevery?= Date: Tue, 28 Nov 2017 15:08:44 -0600 Subject: [PATCH] Revert "fix(animations): ensure multi-level enter animations work (#19455)" This reverts commit dd6237ecd97123e8122dd30d6ae1d546fed599b8. --- .../animations/browser/src/dsl/animation.ts | 5 +- .../browser/src/dsl/animation_ast_builder.ts | 5 +- .../src/dsl/animation_timeline_builder.ts | 22 ++- .../src/dsl/animation_transition_factory.ts | 6 +- .../src/render/timeline_animation_engine.ts | 4 +- .../src/render/transition_animation_engine.ts | 128 ++++++++---------- .../test/dsl/animation_trigger_spec.ts | 4 +- .../animation_query_integration_spec.ts | 67 +-------- 8 files changed, 76 insertions(+), 165 deletions(-) diff --git a/packages/animations/browser/src/dsl/animation.ts b/packages/animations/browser/src/dsl/animation.ts index c31ffb4eb8..f0948a1c4d 100644 --- a/packages/animations/browser/src/dsl/animation.ts +++ b/packages/animations/browser/src/dsl/animation.ts @@ -8,7 +8,7 @@ import {AnimationMetadata, AnimationMetadataType, AnimationOptions, ɵStyleData} from '@angular/animations'; import {AnimationDriver} from '../render/animation_driver'; -import {ENTER_CLASSNAME, normalizeStyles} from '../util'; +import {normalizeStyles} from '../util'; import {Ast} from './animation_ast'; import {buildAnimationAst} from './animation_ast_builder'; @@ -39,8 +39,7 @@ export class Animation { const errors: any = []; subInstructions = subInstructions || new ElementInstructionMap(); const result = buildAnimationTimelines( - this._driver, element, this._animationAst, ENTER_CLASSNAME, start, dest, options, - subInstructions, errors); + this._driver, element, this._animationAst, start, dest, options, subInstructions, errors); if (errors.length) { const errorMessage = `animation building failed:\n${errors.join("\n")}`; throw new Error(errorMessage); diff --git a/packages/animations/browser/src/dsl/animation_ast_builder.ts b/packages/animations/browser/src/dsl/animation_ast_builder.ts index 20f490c999..c6ecbef22f 100644 --- a/packages/animations/browser/src/dsl/animation_ast_builder.ts +++ b/packages/animations/browser/src/dsl/animation_ast_builder.ts @@ -62,6 +62,8 @@ export function buildAnimationAst( const LEAVE_TOKEN = ':leave'; const LEAVE_TOKEN_REGEX = new RegExp(LEAVE_TOKEN, 'g'); +const ENTER_TOKEN = ':enter'; +const ENTER_TOKEN_REGEX = new RegExp(ENTER_TOKEN, 'g'); const ROOT_SELECTOR = ''; export class AnimationAstBuilderVisitor implements AnimationDslVisitor { @@ -476,7 +478,8 @@ function normalizeSelector(selector: string): [string, boolean] { selector = selector.replace(SELF_TOKEN_REGEX, ''); } - selector = selector.replace(LEAVE_TOKEN_REGEX, LEAVE_SELECTOR) + selector = selector.replace(ENTER_TOKEN_REGEX, ENTER_SELECTOR) + .replace(LEAVE_TOKEN_REGEX, LEAVE_SELECTOR) .replace(/@\*/g, NG_TRIGGER_SELECTOR) .replace(/@\w+/g, match => NG_TRIGGER_SELECTOR + '-' + match.substr(1)) .replace(/:animating/g, NG_ANIMATING_SELECTOR); diff --git a/packages/animations/browser/src/dsl/animation_timeline_builder.ts b/packages/animations/browser/src/dsl/animation_timeline_builder.ts index df46177a7e..5bfedae0f5 100644 --- a/packages/animations/browser/src/dsl/animation_timeline_builder.ts +++ b/packages/animations/browser/src/dsl/animation_timeline_builder.ts @@ -15,8 +15,6 @@ import {AnimationTimelineInstruction, createTimelineInstruction} from './animati import {ElementInstructionMap} from './element_instruction_map'; const ONE_FRAME_IN_MILLISECONDS = 1; -const ENTER_TOKEN = ':enter'; -const ENTER_TOKEN_REGEX = new RegExp(ENTER_TOKEN, 'g'); /* * The code within this file aims to generate web-animations-compatible keyframes from Angular's @@ -103,22 +101,20 @@ const ENTER_TOKEN_REGEX = new RegExp(ENTER_TOKEN, 'g'); * the `AnimationValidatorVisitor` code. */ export function buildAnimationTimelines( - driver: AnimationDriver, rootElement: any, ast: Ast, enterClassName: string, + driver: AnimationDriver, rootElement: any, ast: Ast, startingStyles: ɵStyleData = {}, finalStyles: ɵStyleData = {}, options: AnimationOptions, subInstructions?: ElementInstructionMap, errors: any[] = []): AnimationTimelineInstruction[] { return new AnimationTimelineBuilderVisitor().buildKeyframes( - driver, rootElement, ast, enterClassName, startingStyles, finalStyles, options, - subInstructions, errors); + driver, rootElement, ast, startingStyles, finalStyles, options, subInstructions, errors); } export class AnimationTimelineBuilderVisitor implements AstVisitor { buildKeyframes( - driver: AnimationDriver, rootElement: any, ast: Ast, enterClassName: string, + driver: AnimationDriver, rootElement: any, ast: Ast, startingStyles: ɵStyleData, finalStyles: ɵStyleData, options: AnimationOptions, subInstructions?: ElementInstructionMap, errors: any[] = []): AnimationTimelineInstruction[] { subInstructions = subInstructions || new ElementInstructionMap(); - const context = new AnimationTimelineContext( - driver, rootElement, subInstructions, enterClassName, errors, []); + const context = new AnimationTimelineContext(driver, rootElement, subInstructions, errors, []); context.options = options; context.currentTimeline.setStyles([startingStyles], null, context.errors, options); @@ -449,9 +445,8 @@ export class AnimationTimelineContext { constructor( private _driver: AnimationDriver, public element: any, - public subInstructions: ElementInstructionMap, private _enterClassName: string, - public errors: any[], public timelines: TimelineBuilder[], - initialTimeline?: TimelineBuilder) { + public subInstructions: ElementInstructionMap, public errors: any[], + public timelines: TimelineBuilder[], initialTimeline?: TimelineBuilder) { this.currentTimeline = initialTimeline || new TimelineBuilder(this._driver, element, 0); timelines.push(this.currentTimeline); } @@ -504,8 +499,8 @@ export class AnimationTimelineContext { AnimationTimelineContext { const target = element || this.element; const context = new AnimationTimelineContext( - this._driver, target, this.subInstructions, this._enterClassName, this.errors, - this.timelines, this.currentTimeline.fork(target, newTime || 0)); + this._driver, target, this.subInstructions, this.errors, this.timelines, + this.currentTimeline.fork(target, newTime || 0)); context.previousNode = this.previousNode; context.currentAnimateTimings = this.currentAnimateTimings; @@ -560,7 +555,6 @@ export class AnimationTimelineContext { results.push(this.element); } if (selector.length > 0) { // if :self is only used then the selector is empty - selector = selector.replace(ENTER_TOKEN_REGEX, '.' + this._enterClassName); const multi = limit != 1; let elements = this._driver.query(this.element, selector, multi); if (limit !== 0) { diff --git a/packages/animations/browser/src/dsl/animation_transition_factory.ts b/packages/animations/browser/src/dsl/animation_transition_factory.ts index 7937f02730..4eea313a8b 100644 --- a/packages/animations/browser/src/dsl/animation_transition_factory.ts +++ b/packages/animations/browser/src/dsl/animation_transition_factory.ts @@ -37,7 +37,7 @@ export class AnimationTransitionFactory { build( driver: AnimationDriver, element: any, currentState: any, nextState: any, - enterClassName: string, currentOptions?: AnimationOptions, nextOptions?: AnimationOptions, + currentOptions?: AnimationOptions, nextOptions?: AnimationOptions, subInstructions?: ElementInstructionMap): AnimationTransitionInstruction { const errors: any[] = []; @@ -55,8 +55,8 @@ export class AnimationTransitionFactory { const animationOptions = {params: {...transitionAnimationParams, ...nextAnimationParams}}; const timelines = buildAnimationTimelines( - driver, element, this.ast.animation, enterClassName, currentStateStyles, nextStateStyles, - animationOptions, subInstructions, errors); + driver, element, this.ast.animation, currentStateStyles, nextStateStyles, animationOptions, + subInstructions, errors); if (errors.length) { return createTransitionInstruction( diff --git a/packages/animations/browser/src/render/timeline_animation_engine.ts b/packages/animations/browser/src/render/timeline_animation_engine.ts index cbb6c9a55a..49fa5bc39c 100644 --- a/packages/animations/browser/src/render/timeline_animation_engine.ts +++ b/packages/animations/browser/src/render/timeline_animation_engine.ts @@ -13,7 +13,6 @@ import {buildAnimationTimelines} from '../dsl/animation_timeline_builder'; import {AnimationTimelineInstruction} from '../dsl/animation_timeline_instruction'; import {ElementInstructionMap} from '../dsl/element_instruction_map'; import {AnimationStyleNormalizer} from '../dsl/style_normalization/animation_style_normalizer'; -import {ENTER_CLASSNAME} from '../util'; import {AnimationDriver} from './animation_driver'; import {getOrSetAsInMap, listenOnPlayer, makeAnimationEvent, normalizeKeyframes, optimizeGroupPlayer} from './shared'; @@ -56,8 +55,7 @@ export class TimelineAnimationEngine { if (ast) { instructions = buildAnimationTimelines( - this._driver, element, ast, ENTER_CLASSNAME, {}, {}, options, EMPTY_INSTRUCTION_MAP, - errors); + this._driver, element, ast, {}, {}, options, EMPTY_INSTRUCTION_MAP, errors); instructions.forEach(inst => { const styles = getOrSetAsInMap(autoStylesMap, inst.element, {}); inst.postStyleProps.forEach(prop => styles[prop] = null); diff --git a/packages/animations/browser/src/render/transition_animation_engine.ts b/packages/animations/browser/src/render/transition_animation_engine.ts index 0e4e6c5001..0e65e9753d 100644 --- a/packages/animations/browser/src/render/transition_animation_engine.ts +++ b/packages/animations/browser/src/render/transition_animation_engine.ts @@ -13,7 +13,7 @@ import {AnimationTransitionInstruction} from '../dsl/animation_transition_instru import {AnimationTrigger} from '../dsl/animation_trigger'; import {ElementInstructionMap} from '../dsl/element_instruction_map'; import {AnimationStyleNormalizer} from '../dsl/style_normalization/animation_style_normalizer'; -import {ENTER_CLASSNAME, LEAVE_CLASSNAME, NG_ANIMATING_CLASSNAME, NG_ANIMATING_SELECTOR, NG_TRIGGER_CLASSNAME, NG_TRIGGER_SELECTOR, copyObj, eraseStyles, iteratorToArray, setStyles} from '../util'; +import {ENTER_CLASSNAME, LEAVE_CLASSNAME, NG_ANIMATING_CLASSNAME, NG_ANIMATING_SELECTOR, NG_TRIGGER_CLASSNAME, NG_TRIGGER_SELECTOR, copyObj, eraseStyles, setStyles} from '../util'; import {AnimationDriver} from './animation_driver'; import {getBodyNode, getOrSetAsInMap, listenOnPlayer, makeAnimationEvent, normalizeKeyframes, optimizeGroupPlayer} from './shared'; @@ -714,10 +714,9 @@ export class TransitionAnimationEngine { return () => {}; } - private _buildInstruction( - entry: QueueInstruction, subTimelines: ElementInstructionMap, enterClassName: string) { + private _buildInstruction(entry: QueueInstruction, subTimelines: ElementInstructionMap) { return entry.transition.build( - this.driver, entry.element, entry.fromState.value, entry.toState.value, enterClassName, + this.driver, entry.element, entry.fromState.value, entry.toState.value, entry.fromState.options, entry.toState.options, subTimelines); } @@ -863,19 +862,16 @@ export class TransitionAnimationEngine { }); const bodyNode = getBodyNode(); - const enterNodeMap = - buildRootMap(Array.from(this.statesByElement.keys()), this.collectedEnterElements); + const allEnterNodes: any[] = this.collectedEnterElements.length ? + this.collectedEnterElements.filter(createIsRootFilterFn(this.collectedEnterElements)) : + []; // this must occur before the instructions are built below such that // the :enter queries match the elements (since the timeline queries // are fired during instruction building). - const enterNodeMapIds = new Map(); - let i = 0; - enterNodeMap.forEach((nodes, root) => { - const className = ENTER_CLASSNAME + i++; - enterNodeMapIds.set(root, className); - nodes.forEach(node => addClass(node, className)); - }); + for (let i = 0; i < allEnterNodes.length; i++) { + addClass(allEnterNodes[i], ENTER_CLASSNAME); + } const allLeaveNodes: any[] = []; const leaveNodesWithoutAnimations = new Set(); @@ -892,11 +888,7 @@ export class TransitionAnimationEngine { } cleanupFns.push(() => { - enterNodeMap.forEach((nodes, root) => { - const className = enterNodeMapIds.get(root) !; - nodes.forEach(node => removeClass(node, className)); - }); - + allEnterNodes.forEach(element => removeClass(element, ENTER_CLASSNAME)); allLeaveNodes.forEach(element => { removeClass(element, LEAVE_CLASSNAME); this.processLeaveNode(element); @@ -917,8 +909,7 @@ export class TransitionAnimationEngine { return; } - const enterClassName = enterNodeMapIds.get(element) !; - const instruction = this._buildInstruction(entry, subTimelines, enterClassName) !; + const instruction = this._buildInstruction(entry, subTimelines) !; if (instruction.errors && instruction.errors.length) { erroneousTransitions.push(instruction); return; @@ -982,6 +973,18 @@ export class TransitionAnimationEngine { this.reportError(errors); } + // these can only be detected here since we have a map of all the elements + // that have animations attached to them... We use a set here in the event + // multiple enter captures on the same element were caught in different + // renderer namespaces (e.g. when a @trigger was on a host binding that had *ngIf) + const enterNodesWithoutAnimations = new Set(); + for (let i = 0; i < allEnterNodes.length; i++) { + const element = allEnterNodes[i]; + if (!subTimelines.has(element)) { + enterNodesWithoutAnimations.add(element); + } + } + 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 @@ -1019,9 +1022,8 @@ export class TransitionAnimationEngine { }); // POST STAGE: fill the * styles - const postStylesMap = new Map(); - const allLeaveQueriedNodes = cloakAndComputeStyles( - postStylesMap, this.driver, leaveNodesWithoutAnimations, allPostStyleElements, AUTO_STYLE); + const [postStylesMap, allLeaveQueriedNodes] = cloakAndComputeStyles( + this.driver, leaveNodesWithoutAnimations, allPostStyleElements, AUTO_STYLE); allLeaveQueriedNodes.forEach(node => { if (replacePostStylesAsPre(node, allPreStyleElements, allPostStyleElements)) { @@ -1030,11 +1032,10 @@ export class TransitionAnimationEngine { }); // PRE STAGE: fill the ! styles - const preStylesMap = new Map(); - enterNodeMap.forEach((nodes, root) => { - cloakAndComputeStyles( - preStylesMap, this.driver, new Set(nodes), allPreStyleElements, PRE_STYLE); - }); + const [preStylesMap] = allPreStyleElements.size ? + cloakAndComputeStyles( + this.driver, enterNodesWithoutAnimations, allPreStyleElements, PRE_STYLE) : + [new Map()]; replaceNodes.forEach(node => { const post = postStylesMap.get(node); @@ -1504,11 +1505,12 @@ function cloakElement(element: any, value?: string) { } function cloakAndComputeStyles( - valuesMap: Map, driver: AnimationDriver, elements: Set, - elementPropsMap: Map>, defaultStyle: string): any[] { + driver: AnimationDriver, elements: Set, elementPropsMap: Map>, + defaultStyle: string): [Map, any[]] { const cloakVals: string[] = []; elements.forEach(element => cloakVals.push(cloakElement(element))); + const valuesMap = new Map(); const failedElements: any[] = []; elementPropsMap.forEach((props: Set, element: any) => { @@ -1530,57 +1532,39 @@ function cloakAndComputeStyles( // an index value for the closure (but instead just the value) let i = 0; elements.forEach(element => cloakElement(element, cloakVals[i++])); - - return failedElements; + return [valuesMap, failedElements]; } /* Since the Angular renderer code will return a collection of inserted nodes in all areas of a DOM tree, it's up to this algorithm to figure -out which nodes are roots for each animation @trigger. +out which nodes are roots. -By placing each inserted node into a Set and traversing upwards, it -is possible to find the @trigger elements and well any direct *star -insertion nodes, if a @trigger root is found then the enter element -is placed into the Map[@trigger] spot. +By placing all nodes into a set and traversing upwards to the edge, +the recursive code can figure out if a clean path from the DOM node +to the edge container is clear. If no other node is detected in the +set then it is a root element. + +This algorithm also keeps track of all nodes along the path so that +if other sibling nodes are also tracked then the lookup process can +skip a lot of steps in between and avoid traversing the entire tree +multiple times to the edge. */ -function buildRootMap(roots: any[], nodes: any[]): Map { - const rootMap = new Map(); - roots.forEach(root => rootMap.set(root, [])); - - if (nodes.length == 0) return rootMap; - - const NULL_NODE = 1; +function createIsRootFilterFn(nodes: any): (node: any) => boolean { const nodeSet = new Set(nodes); - const localRootMap = new Map(); - - function getRoot(node: any): any { - if (!node) return NULL_NODE; - - let root = localRootMap.get(node); - if (root) return root; - - const parent = node.parentNode; - if (rootMap.has(parent)) { // ngIf inside @trigger - root = parent; - } else if (nodeSet.has(parent)) { // ngIf inside ngIf - root = NULL_NODE; - } else { // recurse upwards - root = getRoot(parent); + const knownRootContainer = new Set(); + let isRoot: (node: any) => boolean; + isRoot = node => { + if (!node) return true; + if (nodeSet.has(node.parentNode)) return false; + if (knownRootContainer.has(node.parentNode)) return true; + if (isRoot(node.parentNode)) { + knownRootContainer.add(node); + return true; } - - localRootMap.set(node, root); - return root; - } - - nodes.forEach(node => { - const root = getRoot(node); - if (root !== NULL_NODE) { - rootMap.get(root) !.push(node); - } - }); - - return rootMap; + return false; + }; + return isRoot; } const CLASSES_CACHE_KEY = '$$classes'; diff --git a/packages/animations/browser/test/dsl/animation_trigger_spec.ts b/packages/animations/browser/test/dsl/animation_trigger_spec.ts index 9e885e63f3..9db8e2404b 100644 --- a/packages/animations/browser/test/dsl/animation_trigger_spec.ts +++ b/packages/animations/browser/test/dsl/animation_trigger_spec.ts @@ -10,7 +10,6 @@ import {AnimationOptions, animate, state, style, transition} from '@angular/anim import {AnimationTransitionInstruction} from '@angular/animations/browser/src/dsl/animation_transition_instruction'; import {AnimationTrigger} from '@angular/animations/browser/src/dsl/animation_trigger'; -import {ENTER_CLASSNAME} from '../../src/util'; import {MockAnimationDriver} from '../../testing'; import {makeTrigger} from '../shared'; @@ -229,8 +228,7 @@ function buildTransition( const trans = trigger.matchTransition(fromState, toState) !; if (trans) { const driver = new MockAnimationDriver(); - return trans.build( - driver, element, fromState, toState, ENTER_CLASSNAME, fromOptions, toOptions) !; + return trans.build(driver, element, fromState, toState, fromOptions, toOptions) !; } return null; } diff --git a/packages/core/test/animation/animation_query_integration_spec.ts b/packages/core/test/animation/animation_query_integration_spec.ts index 2ca24be857..736e2dc12b 100644 --- a/packages/core/test/animation/animation_query_integration_spec.ts +++ b/packages/core/test/animation/animation_query_integration_spec.ts @@ -5,7 +5,7 @@ * 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, AnimationPlayer, animate, animateChild, group, query, sequence, stagger, state, style, transition, trigger, ɵAnimationGroupPlayer as AnimationGroupPlayer} from '@angular/animations'; +import {AUTO_STYLE, AnimationPlayer, animate, animateChild, query, stagger, state, style, transition, trigger, ɵAnimationGroupPlayer as AnimationGroupPlayer} from '@angular/animations'; import {AnimationDriver, ɵAnimationEngine} from '@angular/animations/browser'; import {matchesElement} from '@angular/animations/browser/src/render/shared'; import {ENTER_CLASSNAME, LEAVE_CLASSNAME} from '@angular/animations/browser/src/util'; @@ -2968,71 +2968,6 @@ export function main() { {offset: 0, width: '0px'}, {offset: .67, width: '0px'}, {offset: 1, width: '200px'} ]); }); - - it('should scope :enter queries between sub animations', () => { - @Component({ - selector: 'cmp', - animations: [ - trigger( - 'parent', - [ - transition(':enter', group([ - sequence([ - style({opacity: 0}), - animate(1000, style({opacity: 1})), - ]), - query(':enter @child', animateChild()), - ])), - ]), - trigger( - 'child', - [ - transition( - ':enter', - [ - query( - ':enter .item', - [style({opacity: 0}), animate(1000, style({opacity: 1}))]), - ]), - ]), - ], - template: ` -
-
-
-
-
-
-
-
-
- ` - }) - class Cmp { - public exp1: any; - public exp2: any; - public exp3: any; - } - - TestBed.configureTestingModule({declarations: [Cmp]}); - - const fixture = TestBed.createComponent(Cmp); - fixture.detectChanges(); - resetLog(); - - const cmp = fixture.componentInstance; - cmp.exp1 = true; - cmp.exp2 = true; - cmp.exp3 = true; - fixture.detectChanges(); - - const players = getLog(); - expect(players.length).toEqual(2); - - const [p1, p2] = players; - expect(p1.element.classList.contains('container')).toBeTruthy(); - expect(p2.element.classList.contains('item')).toBeTruthy(); - }); }); describe('animation control flags', () => {