Skip to content

Commit

Permalink
refactor(animations): refactor non-animatable check to be timeline ba…
Browse files Browse the repository at this point in the history 
…sed (#46666)

move the check for non-animatable properties from the animation building
phase to the application of the animation's transition instead, in such
a way we can check it against the keyframes of the transition's timeline
in order to only provide warnings for properties which are being
animated, thus not providing any warning for non-animatable properties
being applied to elements via the style function

this change has the benfit just mentioned above but it comes with two
drawbacks:
 - the warning handling is not done in the building time so it is a bit
inconsistent with other type of validations (such as the unsupported css
properties one for example)
 - before the warning was being applied only when the animation's data
was being parsed, so it happed only once but now since it is applied
when the animation is actually being prepared to be played, it happens
each time the animation runs

resolves #46602

PR Close #46666
  • Loading branch information
@dario-piotrowicz @jessicajaniuk
dario-piotrowicz authored and jessicajaniuk committed Jul 12, 2022
1 parent 93d17ee commit a4ec109
Show file tree
Hide file tree
Showing 5 changed files with 146 additions and 77 deletions.
19 changes: 1 addition & 18 deletions packages/animations/browser/src/dsl/animation_ast_builder.ts
View file
Expand Up @@ -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';
Expand Down Expand Up @@ -81,13 +81,6 @@ export class AnimationAstBuilderVisitor implements AnimationDslVisitor {
[...context.unsupportedCSSPropertiesFound.keys()],
);
}

if (context.nonAnimatableCSSPropertiesFound.size) {
pushNonAnimatablePropertiesWarning(
warnings,
[...context.nonAnimatableCSSPropertiesFound.keys()],
);
}
}

return ast;
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -535,7 +519,6 @@ export class AnimationAstBuilderContext {
public collectedStyles = new Map<string, Map<string, StyleTimeTuple>>();
public options: AnimationOptions|null = null;
public unsupportedCSSPropertiesFound: Set<string> = new Set<string>();
public readonly nonAnimatableCSSPropertiesFound: Set<string> = new Set<string>();
constructor(public errors: Error[]) {}
}

Expand Down
53 changes: 53 additions & 0 deletions packages/animations/browser/src/dsl/animation_transition_factory.ts
View file
Expand Up @@ -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';
Expand Down Expand Up @@ -91,13 +92,65 @@ 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,
nextStateStyles, timelines, queriedElementsList, preStyleMap, postStyleMap, totalTime);
}
}

/**
* 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<string>();

timelines.forEach(({keyframes}) => {
const nonAnimatablePropsInitialValues = new Map<string, string|number>();
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 {
Expand Down
8 changes: 0 additions & 8 deletions packages/animations/browser/src/warning_helpers.ts
View file
Expand Up @@ -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)`);
}
}
50 changes: 0 additions & 50 deletions packages/animations/browser/test/dsl/animation_spec.ts
View file
Expand Up @@ -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 = [
Expand Down
93 changes: 92 additions & 1 deletion packages/core/test/animation/animation_integration_spec.ts
View file
Expand Up @@ -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';
Expand Down Expand Up @@ -4049,6 +4049,97 @@ describe('animation tests', function() {
});
});
});

describe('non-animatable css props', () => {
function buildAndAnimateSimpleTestComponent(triggerAnimationData: AnimationMetadata[]) {
@Component({
selector: 'cmp',
template: `
<div *ngIf="exp" [@myAnimation]="exp">
<p *ngIf="exp"></p>
</div>
`,
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();
});
});
});
})();

Expand Down

0 comments on commit a4ec109

Please sign in to comment.