diff --git a/packages/animations/browser/src/dsl/animation_ast_builder.ts b/packages/animations/browser/src/dsl/animation_ast_builder.ts index 324f0c7bd76c7..1ffb46bdf6f6d 100644 --- a/packages/animations/browser/src/dsl/animation_ast_builder.ts +++ b/packages/animations/browser/src/dsl/animation_ast_builder.ts @@ -11,7 +11,7 @@ import {invalidDefinition, invalidKeyframes, invalidOffset, invalidParallelAnima import {AnimationDriver} from '../render/animation_driver'; import {getOrSetDefaultValue} from '../render/shared'; import {convertToMap, copyObj, extractStyleParams, iteratorToArray, NG_ANIMATING_SELECTOR, NG_TRIGGER_SELECTOR, normalizeAnimationEntry, resolveTiming, SUBSTITUTION_EXPR_START, validateStyleParams, visitDslNode} from '../util'; -import {pushNonAnimatablePropertiesWarning, pushUnrecognizedPropertiesWarning} from '../warning_helpers'; +import {pushUnrecognizedPropertiesWarning} from '../warning_helpers'; import {AnimateAst, AnimateChildAst, AnimateRefAst, Ast, DynamicTimingAst, GroupAst, KeyframesAst, QueryAst, ReferenceAst, SequenceAst, StaggerAst, StateAst, StyleAst, TimingAst, TransitionAst, TriggerAst} from './animation_ast'; import {AnimationDslVisitor} from './animation_dsl_visitor'; @@ -81,13 +81,6 @@ export class AnimationAstBuilderVisitor implements AnimationDslVisitor { [...context.unsupportedCSSPropertiesFound.keys()], ); } - - if (context.nonAnimatableCSSPropertiesFound.size) { - pushNonAnimatablePropertiesWarning( - warnings, - [...context.nonAnimatableCSSPropertiesFound.keys()], - ); - } } return ast; @@ -321,15 +314,6 @@ export class AnimationAstBuilderVisitor implements AnimationDslVisitor { context.unsupportedCSSPropertiesFound.add(prop); return; } - - if (this._driver.validateAnimatableStyleProperty) { - if (!this._driver.validateAnimatableStyleProperty(prop)) { - context.nonAnimatableCSSPropertiesFound.add(prop); - // note: non animatable properties are not removed for the tuple just in case they are - // categorized as non animatable but can actually be animated - return; - } - } } // This is guaranteed to have a defined Map at this querySelector location making it @@ -535,7 +519,6 @@ export class AnimationAstBuilderContext { public collectedStyles = new Map>(); public options: AnimationOptions|null = null; public unsupportedCSSPropertiesFound: Set = new Set(); - public readonly nonAnimatableCSSPropertiesFound: Set = new Set(); constructor(public errors: Error[]) {} } diff --git a/packages/animations/browser/src/dsl/animation_transition_factory.ts b/packages/animations/browser/src/dsl/animation_transition_factory.ts index a541fa39f10ac..1bef38bf07d01 100644 --- a/packages/animations/browser/src/dsl/animation_transition_factory.ts +++ b/packages/animations/browser/src/dsl/animation_transition_factory.ts @@ -13,6 +13,7 @@ import {copyObj, interpolateParams, iteratorToArray} from '../util'; import {StyleAst, TransitionAst} from './animation_ast'; import {buildAnimationTimelines} from './animation_timeline_builder'; +import {AnimationTimelineInstruction} from './animation_timeline_instruction'; import {TransitionMatcherFn} from './animation_transition_expr'; import {AnimationTransitionInstruction, createTransitionInstruction} from './animation_transition_instruction'; import {ElementInstructionMap} from './element_instruction_map'; @@ -91,6 +92,10 @@ export class AnimationTransitionFactory { } }); + if (typeof ngDevMode === 'undefined' || ngDevMode) { + checkNonAnimatableInTimelines(timelines, this._triggerName, driver); + } + const queriedElementsList = iteratorToArray(queriedElements.values()); return createTransitionInstruction( element, this._triggerName, currentState, nextState, isRemoval, currentStateStyles, @@ -98,6 +103,54 @@ export class AnimationTransitionFactory { } } +/** + * Checks inside a set of timelines if they try to animate a css property which is not considered + * animatable, in that case it prints a warning on the console. + * Besides that the function doesn't have any other effect. + * + * Note: this check is done here after the timelines are built instead of doing on a lower level so + * that we can make sure that the warning appears only once per instruction (we can aggregate here + * all the issues instead of finding them separately). + * + * @param timelines The built timelines for the current instruction. + * @param triggerName The name of the trigger for the current instruction. + * @param driver Animation driver used to perform the check. + * + */ +function checkNonAnimatableInTimelines( + timelines: AnimationTimelineInstruction[], triggerName: string, driver: AnimationDriver): void { + if (!driver.validateAnimatableStyleProperty) { + return; + } + + const invalidNonAnimatableProps = new Set(); + + timelines.forEach(({keyframes}) => { + const nonAnimatablePropsInitialValues = new Map(); + keyframes.forEach(keyframe => { + for (const [prop, value] of keyframe.entries()) { + if (!driver.validateAnimatableStyleProperty!(prop)) { + if (nonAnimatablePropsInitialValues.has(prop) && !invalidNonAnimatableProps.has(prop)) { + const propInitialValue = nonAnimatablePropsInitialValues.get(prop); + if (propInitialValue !== value) { + invalidNonAnimatableProps.add(prop); + } + } else { + nonAnimatablePropsInitialValues.set(prop, value); + } + } + } + }); + }); + + if (invalidNonAnimatableProps.size > 0) { + console.warn( + `Warning: The animation trigger "${triggerName}" is attempting to animate the following` + + ' not animatable properties: ' + Array.from(invalidNonAnimatableProps).join(', ') + '\n' + + '(to check the list of all animatable properties visit https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_animated_properties)'); + } +} + function oneOrMoreTransitionsMatch( matchFns: TransitionMatcherFn[], currentState: any, nextState: any, element: any, params: {[key: string]: any}): boolean { diff --git a/packages/animations/browser/src/warning_helpers.ts b/packages/animations/browser/src/warning_helpers.ts index 6b5a444c70137..13bd9312b33c7 100644 --- a/packages/animations/browser/src/warning_helpers.ts +++ b/packages/animations/browser/src/warning_helpers.ts @@ -39,11 +39,3 @@ export function pushUnrecognizedPropertiesWarning(warnings: string[], props: str warnings.push(`The following provided properties are not recognized: ${props.join(', ')}`); } } - -export function pushNonAnimatablePropertiesWarning(warnings: string[], props: string[]): void { - if (props.length) { - warnings.push(`The following provided properties are not animatable: ${ - props.join( - ', ')}\n (see: https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_animated_properties)`); - } -} diff --git a/packages/animations/browser/test/dsl/animation_spec.ts b/packages/animations/browser/test/dsl/animation_spec.ts index 7c05d1a3b5682..8d79f9e56102b 100644 --- a/packages/animations/browser/test/dsl/animation_spec.ts +++ b/packages/animations/browser/test/dsl/animation_spec.ts @@ -244,56 +244,6 @@ function createDiv() { ]); }); - it('should provide a warning if a non-animatable CSS property is used in the animation', - () => { - const steps = [animate(1000, style({display: 'block'}))]; - - expect(getValidationWarningsForAnimationSequence(steps)).toEqual([ - 'The following provided properties are not animatable: display' + - '\n (see: https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_animated_properties)' - ]); - }); - - it('should not provide a non-animatable warning if an animatable CSS property is used in the animation', - () => { - const steps = [animate('1.5s', style({borderRadius: '1rem'}))]; - - expect(getValidationWarningsForAnimationSequence(steps)).toEqual([]); - }); - - it('should provide a warning if multiple non-animatable CSS properties are used in the animation', - () => { - const steps = [ - state('state', style({ - display: 'block', - })), - style({textAlign: 'right'}), animate(1000, style({'font-family': 'sans-serif'})) - ]; - - expect(getValidationWarningsForAnimationSequence(steps)).toEqual([ - 'The following provided properties are not animatable: display, textAlign, font-family' + - '\n (see: https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_animated_properties)' - ]); - }); - - it('should provide a warnings if both invalid and non-animatable CSS properties are used in the animation', - () => { - const steps = [ - state('state', style({ - display: 'block', - abc: 123, - })), - style({textAlign: 'right'}), - animate(1000, style({'font-family': 'sans-serif', xyz: 456})) - ]; - - expect(getValidationWarningsForAnimationSequence(steps)).toEqual([ - 'The following provided properties are not recognized: abc, xyz', - 'The following provided properties are not animatable: display, textAlign, font-family' + - '\n (see: https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_animated_properties)' - ]); - }); - it('should allow a vendor-prefixed property to be used in an animation sequence without throwing an error', () => { const steps = [ diff --git a/packages/core/test/animation/animation_integration_spec.ts b/packages/core/test/animation/animation_integration_spec.ts index 8891ab943c7b4..2e5fb154aa1db 100644 --- a/packages/core/test/animation/animation_integration_spec.ts +++ b/packages/core/test/animation/animation_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 {animate, animateChild, AnimationEvent, AnimationOptions, AUTO_STYLE, group, keyframes, query, state, style, transition, trigger, ɵPRE_STYLE as PRE_STYLE} from '@angular/animations'; +import {animate, animateChild, AnimationEvent, AnimationMetadata, AnimationOptions, AUTO_STYLE, 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, Inject, RendererFactory2, ViewChild} from '@angular/core'; @@ -4049,6 +4049,97 @@ describe('animation tests', function() { }); }); }); + + describe('non-animatable css props', () => { + function buildAndAnimateSimpleTestComponent(triggerAnimationData: AnimationMetadata[]) { + @Component({ + selector: 'cmp', + template: ` +
+

+
+ `, + animations: [trigger('myAnimation', [transition('void => *', triggerAnimationData)])] + }) + class Cmp { + exp: any = false; + } + + TestBed.configureTestingModule({declarations: [Cmp]}); + const engine = TestBed.inject(ɵAnimationEngine); + const fixture = TestBed.createComponent(Cmp); + const cmp = fixture.componentInstance; + cmp.exp = true; + fixture.detectChanges(); + engine.flush(); + } + + it('should show a warning if the animation tries to transition a non-animatable property', () => { + const spyWarn = spyOn(console, 'warn'); + buildAndAnimateSimpleTestComponent( + [style({display: 'block'}), animate(500, style({display: 'inline'}))]); + expect(spyWarn).toHaveBeenCalledOnceWith( + 'Warning: The animation trigger "myAnimation" is attempting to animate the following' + + ' not animatable properties: display' + + '\n' + + '(to check the list of all animatable properties visit https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_animated_properties)'); + }); + + it('should not show a warning if the animation tries to transition an animatable property', + () => { + const spyWarn = spyOn(console, 'warn'); + buildAndAnimateSimpleTestComponent( + [style({fontSize: 5}), animate(500, style({fontSize: 15}))]); + expect(spyWarn).not.toHaveBeenCalled(); + }); + + it('should show a single warning if the animation tries to transition multiple non-animatable properties', + () => { + const spyWarn = spyOn(console, 'warn'); + buildAndAnimateSimpleTestComponent([ + style({display: 'block', fontStyle: 'normal'}), + animate(500, style({display: 'inline', fontStyle: 'italic'})) + ]); + expect(spyWarn).toHaveBeenCalledOnceWith( + 'Warning: The animation trigger "myAnimation" is attempting to animate the following' + + ' not animatable properties: display, fontStyle' + + '\n' + + '(to check the list of all animatable properties visit https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_animated_properties)'); + }); + + it('should only warn on non-animatable properties if the animation tries to transition both animate and non-animatable properties', + () => { + const spyWarn = spyOn(console, 'warn'); + buildAndAnimateSimpleTestComponent([ + style({'flex-direction': 'column', fontSize: 5}), + animate(500, style({'flex-direction': 'row', fontSize: 10})) + ]); + expect(spyWarn).toHaveBeenCalledOnceWith( + 'Warning: The animation trigger "myAnimation" is attempting to animate the following' + + ' not animatable properties: flex-direction' + + '\n' + + '(to check the list of all animatable properties visit https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_animated_properties)'); + }); + + it('should not show a warning if the animation uses but does not animate a non-animatable property', + () => { + const spyWarn = spyOn(console, 'warn'); + buildAndAnimateSimpleTestComponent([transition('void => *', [style({display: 'block'})])]); + expect(spyWarn).not.toHaveBeenCalled(); + }); + + it('should not show a warning if the same non-animatable property is used (with different values) on different elements in the same transition', + () => { + const spyWarn = spyOn(console, 'warn'); + buildAndAnimateSimpleTestComponent([ + style({position: 'relative'}), + query(':enter', [style({ + position: 'absolute', + })]), + ]); + expect(spyWarn).not.toHaveBeenCalled(); + }); + }); }); })();