From 409688fe174164b060d95eebda471d74f18fe243 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matias=20Niemel=C3=A4?= Date: Tue, 15 Aug 2017 16:11:11 -0700 Subject: [PATCH] feat(animations): report errors when invalid CSS properties are detected (#18718) Closes #18701 PR Close #18718 --- .../animations/browser/src/dsl/animation.ts | 2 +- .../browser/src/dsl/animation_ast_builder.ts | 14 +++++- .../src/dsl/animation_timeline_builder.ts | 14 +++--- .../browser/src/render/animation_driver.ts | 7 ++- .../src/render/animation_engine_next.ts | 9 ++-- .../animations/browser/src/render/shared.ts | 15 ++++++ .../src/render/timeline_animation_engine.ts | 2 +- .../src/render/transition_animation_engine.ts | 9 +--- .../web_animations/web_animations_driver.ts | 4 +- .../browser/test/dsl/animation_spec.ts | 48 ++++++++++--------- .../transition_animation_engine_spec.ts | 3 +- packages/animations/browser/test/shared.ts | 4 +- .../testing/src/mock_animation_driver.ts | 5 +- .../animation/animation_integration_spec.ts | 2 +- .../public_api_guard/animations/browser.d.ts | 1 + .../animations/browser/testing.d.ts | 1 + 16 files changed, 88 insertions(+), 52 deletions(-) diff --git a/packages/animations/browser/src/dsl/animation.ts b/packages/animations/browser/src/dsl/animation.ts index a76bac35cb..f03f9c697c 100644 --- a/packages/animations/browser/src/dsl/animation.ts +++ b/packages/animations/browser/src/dsl/animation.ts @@ -20,7 +20,7 @@ export class Animation { private _animationAst: Ast; constructor(private _driver: AnimationDriver, input: AnimationMetadata|AnimationMetadata[]) { const errors: any[] = []; - const ast = buildAnimationAst(input, errors); + const ast = buildAnimationAst(_driver, input, errors); if (errors.length) { const errorMessage = `animation validation 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 3fc8d50d65..0a6fd2fd6e 100644 --- a/packages/animations/browser/src/dsl/animation_ast_builder.ts +++ b/packages/animations/browser/src/dsl/animation_ast_builder.ts @@ -7,6 +7,7 @@ */ import {AUTO_STYLE, AnimateTimings, AnimationAnimateChildMetadata, AnimationAnimateMetadata, AnimationAnimateRefMetadata, AnimationGroupMetadata, AnimationKeyframesSequenceMetadata, AnimationMetadata, AnimationMetadataType, AnimationOptions, AnimationQueryMetadata, AnimationQueryOptions, AnimationReferenceMetadata, AnimationSequenceMetadata, AnimationStaggerMetadata, AnimationStateMetadata, AnimationStyleMetadata, AnimationTransitionMetadata, AnimationTriggerMetadata, style, ɵStyleData} from '@angular/animations'; +import {AnimationDriver} from '../render/animation_driver'; import {getOrSetAsInMap} from '../render/shared'; import {ENTER_SELECTOR, LEAVE_SELECTOR, NG_ANIMATING_SELECTOR, NG_TRIGGER_SELECTOR, SUBSTITUTION_EXPR_START, copyObj, extractStyleParams, iteratorToArray, normalizeAnimationEntry, resolveTiming, validateStyleParams} from '../util'; @@ -54,8 +55,9 @@ const SELF_TOKEN_REGEX = new RegExp(`\s*${SELF_TOKEN}\s*,?`, 'g'); * Otherwise an error will be thrown. */ export function buildAnimationAst( - metadata: AnimationMetadata | AnimationMetadata[], errors: any[]): Ast { - return new AnimationAstBuilderVisitor().build(metadata, errors); + driver: AnimationDriver, metadata: AnimationMetadata | AnimationMetadata[], + errors: any[]): Ast { + return new AnimationAstBuilderVisitor(driver).build(metadata, errors); } const LEAVE_TOKEN = ':leave'; @@ -65,6 +67,8 @@ const ENTER_TOKEN_REGEX = new RegExp(ENTER_TOKEN, 'g'); const ROOT_SELECTOR = ''; export class AnimationAstBuilderVisitor implements AnimationDslVisitor { + constructor(private _driver: AnimationDriver) {} + build(metadata: AnimationMetadata|AnimationMetadata[], errors: any[]): Ast { const context = new AnimationAstBuilderContext(errors); this._resetContextStyleTimingState(context); @@ -273,6 +277,12 @@ export class AnimationAstBuilderVisitor implements AnimationDslVisitor { if (typeof tuple == 'string') return; Object.keys(tuple).forEach(prop => { + if (!this._driver.validateStyleProperty(prop)) { + context.errors.push( + `The provided animation property "${prop}" is not a supported CSS property for animations`); + return; + } + const collectedStyles = context.collectedStyles[context.currentQuerySelector !]; const collectedEntry = collectedStyles[prop]; let updateCollectedStyle = true; diff --git a/packages/animations/browser/src/dsl/animation_timeline_builder.ts b/packages/animations/browser/src/dsl/animation_timeline_builder.ts index 85750cbc53..78f70b84f6 100644 --- a/packages/animations/browser/src/dsl/animation_timeline_builder.ts +++ b/packages/animations/browser/src/dsl/animation_timeline_builder.ts @@ -447,7 +447,7 @@ export class AnimationTimelineContext { private _driver: AnimationDriver, public element: any, public subInstructions: ElementInstructionMap, public errors: any[], public timelines: TimelineBuilder[], initialTimeline?: TimelineBuilder) { - this.currentTimeline = initialTimeline || new TimelineBuilder(element, 0); + this.currentTimeline = initialTimeline || new TimelineBuilder(this._driver, element, 0); timelines.push(this.currentTimeline); } @@ -530,7 +530,7 @@ export class AnimationTimelineContext { easing: '' }; const builder = new SubTimelineBuilder( - instruction.element, instruction.keyframes, instruction.preStyleProps, + this._driver, instruction.element, instruction.keyframes, instruction.preStyleProps, instruction.postStyleProps, updatedTimings, instruction.stretchStartingKeyframe); this.timelines.push(builder); return updatedTimings; @@ -582,7 +582,7 @@ export class TimelineBuilder { private _currentEmptyStepKeyframe: ɵStyleData|null = null; constructor( - public element: any, public startTime: number, + private _driver: AnimationDriver, public element: any, public startTime: number, private _elementTimelineStylesLookup?: Map) { if (!this._elementTimelineStylesLookup) { this._elementTimelineStylesLookup = new Map(); @@ -632,7 +632,7 @@ export class TimelineBuilder { fork(element: any, currentTime?: number): TimelineBuilder { this.applyStylesToKeyframe(); return new TimelineBuilder( - element, currentTime || this.currentTime, this._elementTimelineStylesLookup); + this._driver, element, currentTime || this.currentTime, this._elementTimelineStylesLookup); } private _loadKeyframe() { @@ -796,10 +796,10 @@ class SubTimelineBuilder extends TimelineBuilder { public timings: AnimateTimings; constructor( - public element: any, public keyframes: ɵStyleData[], public preStyleProps: string[], - public postStyleProps: string[], timings: AnimateTimings, + driver: AnimationDriver, public element: any, public keyframes: ɵStyleData[], + public preStyleProps: string[], public postStyleProps: string[], timings: AnimateTimings, private _stretchStartingKeyframe: boolean = false) { - super(element, timings.delay); + super(driver, element, timings.delay); this.timings = {duration: timings.duration, delay: timings.delay, easing: timings.easing}; } diff --git a/packages/animations/browser/src/render/animation_driver.ts b/packages/animations/browser/src/render/animation_driver.ts index 6fe7e732d3..986b4d1e03 100644 --- a/packages/animations/browser/src/render/animation_driver.ts +++ b/packages/animations/browser/src/render/animation_driver.ts @@ -7,13 +7,14 @@ */ import {AnimationPlayer, NoopAnimationPlayer} from '@angular/animations'; -import {containsElement, invokeQuery, matchesElement} from './shared'; - +import {containsElement, invokeQuery, matchesElement, validateStyleProperty} from './shared'; /** * @experimental */ export class NoopAnimationDriver implements AnimationDriver { + validateStyleProperty(prop: string): boolean { return validateStyleProperty(prop); } + matchesElement(element: any, selector: string): boolean { return matchesElement(element, selector); } @@ -41,6 +42,8 @@ export class NoopAnimationDriver implements AnimationDriver { export abstract class AnimationDriver { static NOOP: AnimationDriver = new NoopAnimationDriver(); + abstract validateStyleProperty(prop: string): boolean; + abstract matchesElement(element: any, selector: string): boolean; abstract containsElement(elm1: any, elm2: any): boolean; diff --git a/packages/animations/browser/src/render/animation_engine_next.ts b/packages/animations/browser/src/render/animation_engine_next.ts index d7d598cbe1..abfdd91764 100644 --- a/packages/animations/browser/src/render/animation_engine_next.ts +++ b/packages/animations/browser/src/render/animation_engine_next.ts @@ -25,9 +25,9 @@ export class AnimationEngine { // this method is designed to be overridden by the code that uses this engine public onRemovalComplete = (element: any, context: any) => {}; - constructor(driver: AnimationDriver, normalizer: AnimationStyleNormalizer) { - this._transitionEngine = new TransitionAnimationEngine(driver, normalizer); - this._timelineEngine = new TimelineAnimationEngine(driver, normalizer); + constructor(private _driver: AnimationDriver, normalizer: AnimationStyleNormalizer) { + this._transitionEngine = new TransitionAnimationEngine(_driver, normalizer); + this._timelineEngine = new TimelineAnimationEngine(_driver, normalizer); this._transitionEngine.onRemovalComplete = (element: any, context: any) => this.onRemovalComplete(element, context); @@ -40,7 +40,8 @@ export class AnimationEngine { let trigger = this._triggerCache[cacheKey]; if (!trigger) { const errors: any[] = []; - const ast = buildAnimationAst(metadata as AnimationMetadata, errors) as TriggerAst; + const ast = + buildAnimationAst(this._driver, metadata as AnimationMetadata, errors) as TriggerAst; if (errors.length) { throw new Error( `The animation trigger "${name}" has failed to build due to the following errors:\n - ${errors.join("\n - ")}`); diff --git a/packages/animations/browser/src/render/shared.ts b/packages/animations/browser/src/render/shared.ts index e10cf6563a..9a2f810410 100644 --- a/packages/animations/browser/src/render/shared.ts +++ b/packages/animations/browser/src/render/shared.ts @@ -166,6 +166,21 @@ if (typeof Element != 'undefined') { }; } +let _CACHED_BODY: {style: any}|null = null; +export function validateStyleProperty(prop: string): boolean { + if (!_CACHED_BODY) { + _CACHED_BODY = getBodyNode() || {}; + } + return _CACHED_BODY !.style ? prop in _CACHED_BODY !.style : true; +} + +export function getBodyNode(): any|null { + if (typeof document != 'undefined') { + return document.body; + } + return null; +} + export const matchesElement = _matches; export const containsElement = _contains; export const invokeQuery = _query; diff --git a/packages/animations/browser/src/render/timeline_animation_engine.ts b/packages/animations/browser/src/render/timeline_animation_engine.ts index db07331d27..b217b9f1cf 100644 --- a/packages/animations/browser/src/render/timeline_animation_engine.ts +++ b/packages/animations/browser/src/render/timeline_animation_engine.ts @@ -28,7 +28,7 @@ export class TimelineAnimationEngine { register(id: string, metadata: AnimationMetadata|AnimationMetadata[]) { const errors: any[] = []; - const ast = buildAnimationAst(metadata, errors); + const ast = buildAnimationAst(this._driver, metadata, errors); if (errors.length) { throw new Error( `Unable to build the animation due to the following errors: ${errors.join("\n")}`); diff --git a/packages/animations/browser/src/render/transition_animation_engine.ts b/packages/animations/browser/src/render/transition_animation_engine.ts index 5e2d6763d1..50b6e581a5 100644 --- a/packages/animations/browser/src/render/transition_animation_engine.ts +++ b/packages/animations/browser/src/render/transition_animation_engine.ts @@ -16,7 +16,7 @@ import {AnimationStyleNormalizer} from '../dsl/style_normalization/animation_sty 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 {getOrSetAsInMap, listenOnPlayer, makeAnimationEvent, normalizeKeyframes, optimizeGroupPlayer} from './shared'; +import {getBodyNode, getOrSetAsInMap, listenOnPlayer, makeAnimationEvent, normalizeKeyframes, optimizeGroupPlayer} from './shared'; const QUEUED_CLASSNAME = 'ng-animate-queued'; const QUEUED_SELECTOR = '.ng-animate-queued'; @@ -1525,13 +1525,6 @@ function removeClass(element: any, className: string) { } } -function getBodyNode(): any|null { - if (typeof document != 'undefined') { - return document.body; - } - return null; -} - function removeNodesAfterAnimationDone( engine: TransitionAnimationEngine, element: any, players: AnimationPlayer[]) { optimizeGroupPlayer(players).onDone(() => engine.processLeaveNode(element)); diff --git a/packages/animations/browser/src/render/web_animations/web_animations_driver.ts b/packages/animations/browser/src/render/web_animations/web_animations_driver.ts index b26cbe3eca..56510a1cca 100644 --- a/packages/animations/browser/src/render/web_animations/web_animations_driver.ts +++ b/packages/animations/browser/src/render/web_animations/web_animations_driver.ts @@ -8,11 +8,13 @@ import {AnimationPlayer, ɵStyleData} from '@angular/animations'; import {AnimationDriver} from '../animation_driver'; -import {containsElement, invokeQuery, matchesElement} from '../shared'; +import {containsElement, invokeQuery, matchesElement, validateStyleProperty} from '../shared'; import {WebAnimationsPlayer} from './web_animations_player'; export class WebAnimationsDriver implements AnimationDriver { + validateStyleProperty(prop: string): boolean { return validateStyleProperty(prop); } + matchesElement(element: any, selector: string): boolean { return matchesElement(element, selector); } diff --git a/packages/animations/browser/test/dsl/animation_spec.ts b/packages/animations/browser/test/dsl/animation_spec.ts index bc6da97a92..77a4f16569 100644 --- a/packages/animations/browser/test/dsl/animation_spec.ts +++ b/packages/animations/browser/test/dsl/animation_spec.ts @@ -177,10 +177,12 @@ export function main() { it('should throw if dynamic style substitutions are used without defaults within state() definitions', () => { - const steps = [state('final', style({ - 'width': '{{ one }}px', - 'borderRadius': '{{ two }}px {{ three }}px', - }))]; + const steps = [ + state('final', style({ + 'width': '{{ one }}px', + 'borderRadius': '{{ two }}px {{ three }}px', + })), + ]; expect(() => { validateAndThrowAnimationSequence(steps); }) .toThrowError( @@ -198,6 +200,14 @@ export function main() { .toThrowError( /state\("panfinal", ...\) must define default values for all the following style substitutions: greyColor/); }); + + it('should throw an error if an invalid CSS property is used in the animation', () => { + const steps = [animate(1000, style({abc: '500px'}))]; + + expect(() => { validateAndThrowAnimationSequence(steps); }) + .toThrowError( + /The provided animation property "abc" is not a supported CSS property for animations/); + }); }); describe('keyframe building', () => { @@ -388,14 +398,13 @@ export function main() { it('should allow multiple substitutions to occur within the same style value', () => { const steps = [ - style({transform: ''}), - animate(1000, style({transform: 'translateX({{ x }}) translateY({{ y }})'})) + style({borderRadius: '100px 100px'}), + animate(1000, style({borderRadius: '{{ one }}px {{ two }}'})), ]; const players = - invokeAnimationSequence(rootElement, steps, buildParams({x: '200px', y: '400px'})); + invokeAnimationSequence(rootElement, steps, buildParams({one: '200', two: '400px'})); expect(players[0].keyframes).toEqual([ - {offset: 0, transform: ''}, - {offset: 1, transform: 'translateX(200px) translateY(400px)'} + {offset: 0, borderRadius: '100px 100px'}, {offset: 1, borderRadius: '200px 400px'} ]); }); @@ -571,18 +580,12 @@ export function main() { () => { 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}), - ])), + animate(2000, keyframes([ + style({left: '0', top: '0', offset: 0}), + style({left: '40%', top: '50%', offset: .33}), + style({left: '60%', top: '80%', offset: .66}), + style({left: 'calc(100% - 100px)', top: '100%', offset: 1}), + ])), group([animate('2s', style({width: '200px'}))]), animate('2s', style({height: '300px'})), group([animate('2s', style({height: '500px', width: '500px'}))]) @@ -987,8 +990,9 @@ function invokeAnimationSequence( } function validateAndThrowAnimationSequence(steps: AnimationMetadata | AnimationMetadata[]) { + const driver = new MockAnimationDriver(); const errors: any[] = []; - const ast = buildAnimationAst(steps, errors); + const ast = buildAnimationAst(driver, steps, errors); if (errors.length) { throw new Error(errors.join('\n')); } 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 64bc1748db..0fb955b85d 100644 --- a/packages/animations/browser/test/render/transition_animation_engine_spec.ts +++ b/packages/animations/browser/test/render/transition_animation_engine_spec.ts @@ -657,8 +657,9 @@ function registerTrigger( element: any, engine: TransitionAnimationEngine, metadata: AnimationTriggerMetadata, id: string = DEFAULT_NAMESPACE_ID) { const errors: any[] = []; + const driver = new MockAnimationDriver(); const name = metadata.name; - const ast = buildAnimationAst(metadata as AnimationMetadata, errors) as TriggerAst; + const ast = buildAnimationAst(driver, metadata as AnimationMetadata, errors) as TriggerAst; if (errors.length) { } const trigger = buildTrigger(name, ast); diff --git a/packages/animations/browser/test/shared.ts b/packages/animations/browser/test/shared.ts index bff558cf74..12904ef157 100644 --- a/packages/animations/browser/test/shared.ts +++ b/packages/animations/browser/test/shared.ts @@ -11,12 +11,14 @@ import {trigger} from '@angular/animations'; import {TriggerAst} from '../src/dsl/animation_ast'; import {buildAnimationAst} from '../src/dsl/animation_ast_builder'; import {AnimationTrigger, buildTrigger} from '../src/dsl/animation_trigger'; +import {MockAnimationDriver} from '../testing/src/mock_animation_driver'; export function makeTrigger( name: string, steps: any, skipErrors: boolean = false): AnimationTrigger { + const driver = new MockAnimationDriver(); const errors: any[] = []; const triggerData = trigger(name, steps); - const triggerAst = buildAnimationAst(triggerData, errors) as TriggerAst; + const triggerAst = buildAnimationAst(driver, triggerData, errors) as TriggerAst; if (!skipErrors && errors.length) { const LINE_START = '\n - '; throw new Error( diff --git a/packages/animations/browser/testing/src/mock_animation_driver.ts b/packages/animations/browser/testing/src/mock_animation_driver.ts index a04e069808..fc48070ba0 100644 --- a/packages/animations/browser/testing/src/mock_animation_driver.ts +++ b/packages/animations/browser/testing/src/mock_animation_driver.ts @@ -8,15 +8,18 @@ import {AUTO_STYLE, AnimationPlayer, NoopAnimationPlayer, ɵStyleData} from '@angular/animations'; import {AnimationDriver} from '../../src/render/animation_driver'; -import {containsElement, invokeQuery, matchesElement} from '../../src/render/shared'; +import {containsElement, invokeQuery, matchesElement, validateStyleProperty} from '../../src/render/shared'; import {allowPreviousPlayerStylesMerge} from '../../src/util'; + /** * @experimental Animation support is experimental. */ export class MockAnimationDriver implements AnimationDriver { static log: AnimationPlayer[] = []; + validateStyleProperty(prop: string): boolean { return validateStyleProperty(prop); } + matchesElement(element: any, selector: string): boolean { return matchesElement(element, selector); } diff --git a/packages/core/test/animation/animation_integration_spec.ts b/packages/core/test/animation/animation_integration_spec.ts index 638bff692f..d387886fec 100644 --- a/packages/core/test/animation/animation_integration_spec.ts +++ b/packages/core/test/animation/animation_integration_spec.ts @@ -2330,7 +2330,7 @@ export function main() { trigger('child', [ transition(':enter', [ style({ opacity: 0 }), - animate(1500, style({ opactiy: 1 })) + animate(1500, style({ opacity: 1 })) ]) ]) ] diff --git a/tools/public_api_guard/animations/browser.d.ts b/tools/public_api_guard/animations/browser.d.ts index bc5b326649..50680563c2 100644 --- a/tools/public_api_guard/animations/browser.d.ts +++ b/tools/public_api_guard/animations/browser.d.ts @@ -7,5 +7,6 @@ export declare abstract class AnimationDriver { abstract containsElement(elm1: any, elm2: any): boolean; abstract matchesElement(element: any, selector: string): boolean; abstract query(element: any, selector: string, multi: boolean): any[]; + abstract validateStyleProperty(prop: string): boolean; static NOOP: AnimationDriver; } diff --git a/tools/public_api_guard/animations/browser/testing.d.ts b/tools/public_api_guard/animations/browser/testing.d.ts index d551ebef61..f964f50d99 100644 --- a/tools/public_api_guard/animations/browser/testing.d.ts +++ b/tools/public_api_guard/animations/browser/testing.d.ts @@ -7,6 +7,7 @@ export declare class MockAnimationDriver implements AnimationDriver { containsElement(elm1: any, elm2: any): boolean; matchesElement(element: any, selector: string): boolean; query(element: any, selector: string, multi: boolean): any[]; + validateStyleProperty(prop: string): boolean; static log: AnimationPlayer[]; }