Skip to content

Commit

Permalink
fix(animations): normalize final styles in buildStyles (#42763)
Browse files Browse the repository at this point in the history
the final styles created in buildStyles lack normalization, meaning that pixel values remain as numbers (without "px") and so such properties fail to be correctly set/applied

Example: "width: 300" is applies as "width": "300" (and thus ignored) instead of the correct "width": "300px"

PR Close #42763
  • Loading branch information
dario-piotrowicz authored and alxhub committed Jul 20, 2021
1 parent 0dfb4e8 commit 3cddc3d
Show file tree
Hide file tree
Showing 7 changed files with 66 additions and 16 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import {buildAnimationTimelines} from './animation_timeline_builder';
import {TransitionMatcherFn} from './animation_transition_expr';
import {AnimationTransitionInstruction, createTransitionInstruction} from './animation_transition_instruction';
import {ElementInstructionMap} from './element_instruction_map';
import {AnimationStyleNormalizer} from './style_normalization/animation_style_normalizer';

const EMPTY_OBJECT = {};

Expand Down Expand Up @@ -99,7 +100,9 @@ function oneOrMoreTransitionsMatch(
}

export class AnimationStateStyles {
constructor(private styles: StyleAst, private defaultParams: {[key: string]: any}) {}
constructor(
private styles: StyleAst, private defaultParams: {[key: string]: any},
private normalizer: AnimationStyleNormalizer) {}

buildStyles(params: {[key: string]: any}, errors: string[]): ɵStyleData {
const finalStyles: ɵStyleData = {};
Expand All @@ -118,7 +121,9 @@ export class AnimationStateStyles {
if (val.length > 1) {
val = interpolateParams(val, combinedParams, errors);
}
finalStyles[prop] = val;
const normalizedProp = this.normalizer.normalizePropertyName(prop, errors);
val = this.normalizer.normalizeStyleValue(prop, normalizedProp, val, errors);
finalStyles[normalizedProp] = val;
});
}
});
Expand Down
17 changes: 10 additions & 7 deletions packages/animations/browser/src/dsl/animation_trigger.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,16 @@ import {copyStyles, interpolateParams} from '../util';

import {SequenceAst, StyleAst, TransitionAst, TriggerAst} from './animation_ast';
import {AnimationStateStyles, AnimationTransitionFactory} from './animation_transition_factory';
import {AnimationStyleNormalizer} from './style_normalization/animation_style_normalizer';



/**
* @publicApi
*/
export function buildTrigger(name: string, ast: TriggerAst): AnimationTrigger {
return new AnimationTrigger(name, ast);
export function buildTrigger(
name: string, ast: TriggerAst, normalizer: AnimationStyleNormalizer): AnimationTrigger {
return new AnimationTrigger(name, ast, normalizer);
}

/**
Expand All @@ -29,10 +31,11 @@ export class AnimationTrigger {
public fallbackTransition: AnimationTransitionFactory;
public states: {[stateName: string]: AnimationStateStyles} = {};

constructor(public name: string, public ast: TriggerAst) {
constructor(
public name: string, public ast: TriggerAst, private _normalizer: AnimationStyleNormalizer) {
ast.states.forEach(ast => {
const defaultParams = (ast.options && ast.options.params) || {};
this.states[ast.name] = new AnimationStateStyles(ast.style, defaultParams);
this.states[ast.name] = new AnimationStateStyles(ast.style, defaultParams, _normalizer);
});

balanceProperties(this.states, 'true', '1');
Expand All @@ -42,7 +45,7 @@ export class AnimationTrigger {
this.transitionFactories.push(new AnimationTransitionFactory(name, ast, this.states));
});

this.fallbackTransition = createFallbackTransition(name, this.states);
this.fallbackTransition = createFallbackTransition(name, this.states, this._normalizer);
}

get containsQueries() {
Expand All @@ -62,8 +65,8 @@ export class AnimationTrigger {
}

function createFallbackTransition(
triggerName: string,
states: {[stateName: string]: AnimationStateStyles}): AnimationTransitionFactory {
triggerName: string, states: {[stateName: string]: AnimationStateStyles},
normalizer: AnimationStyleNormalizer): AnimationTransitionFactory {
const matchers = [(fromState: any, toState: any) => true];
const animation: SequenceAst = {type: AnimationMetadataType.Sequence, steps: [], options: null};
const transition: TransitionAst = {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,9 @@ export class AnimationEngine {

constructor(
private bodyNode: any, private _driver: AnimationDriver,
normalizer: AnimationStyleNormalizer) {
this._transitionEngine = new TransitionAnimationEngine(bodyNode, _driver, normalizer);
this._timelineEngine = new TimelineAnimationEngine(bodyNode, _driver, normalizer);
private _normalizer: AnimationStyleNormalizer) {
this._transitionEngine = new TransitionAnimationEngine(bodyNode, _driver, _normalizer);
this._timelineEngine = new TimelineAnimationEngine(bodyNode, _driver, _normalizer);

this._transitionEngine.onRemovalComplete = (element: any, context: any) =>
this.onRemovalComplete(element, context);
Expand All @@ -48,7 +48,7 @@ export class AnimationEngine {
throw new Error(`The animation trigger "${
name}" has failed to build due to the following errors:\n - ${errors.join('\n - ')}`);
}
trigger = buildTrigger(name, ast);
trigger = buildTrigger(name, ast, this._normalizer);
this._triggerCache[cacheKey] = trigger;
}
this._transitionEngine.registerTrigger(namespaceId, name, trigger);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,6 @@ import {makeTrigger} from '../shared';
transition('off => on', animate(1000))
]);


expect(result.states['on'].buildStyles({}, [])).toEqual({width: 50});
expect(result.states['off'].buildStyles({}, [])).toEqual({width: 50});
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -736,7 +736,7 @@ function registerTrigger(
const ast = buildAnimationAst(driver, metadata as AnimationMetadata, errors) as TriggerAst;
if (errors.length) {
}
const trigger = buildTrigger(name, ast);
const trigger = buildTrigger(name, ast, new NoopAnimationStyleNormalizer());
engine.register(id, element);
engine.registerTrigger(id, name, trigger);
}
Expand Down
3 changes: 2 additions & 1 deletion packages/animations/browser/test/shared.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ 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 {NoopAnimationStyleNormalizer} from '../src/dsl/style_normalization/animation_style_normalizer';
import {MockAnimationDriver} from '../testing/src/mock_animation_driver';

export function makeTrigger(
Expand All @@ -24,5 +25,5 @@ export function makeTrigger(
throw new Error(`Animation parsing for the ${name} trigger have failed:${LINE_START}${
errors.join(LINE_START)}`);
}
return buildTrigger(name, triggerAst);
return buildTrigger(name, triggerAst, new NoopAnimationStyleNormalizer());
}
Original file line number Diff line number Diff line change
Expand Up @@ -512,6 +512,48 @@ describe('animation integration tests using web animations', function() {
expect(elm.style.getPropertyValue('display')).toEqual('inline-block');
expect(elm.style.getPropertyValue('position')).toEqual('fixed');
});

it('should set normalized style property values on animation end', () => {
@Component({
selector: 'ani-cmp',
template: `
<div #elm [@myAnimation]="myAnimationExp" style="width: 100%; font-size: 2rem"></div>
`,
animations: [
trigger(
'myAnimation',
[
state('go', style({width: 300, 'font-size': 14})),
transition('* => go', [animate('1s')])
]),
]
})
class Cmp {
@ViewChild('elm', {static: true}) public element: any;

public myAnimationExp = '';
}

TestBed.configureTestingModule({declarations: [Cmp]});

const engine = TestBed.inject(ɵAnimationEngine);
const fixture = TestBed.createComponent(Cmp);
const cmp = fixture.componentInstance;

const elm = cmp.element.nativeElement;
expect(elm.style.getPropertyValue('width')).toEqual('100%');
expect(elm.style.getPropertyValue('font-size')).toEqual('2rem');

cmp.myAnimationExp = 'go';
fixture.detectChanges();

const player = engine.players.pop()!;
player.finish();
player.destroy();

expect(elm.style.getPropertyValue('width')).toEqual('300px');
expect(elm.style.getPropertyValue('font-size')).toEqual('14px');
});
});
})();

Expand Down

0 comments on commit 3cddc3d

Please sign in to comment.