honeymoose
/
angular-cn
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

fix(ivy): ensure animation @trigger ordering is correctly delivered to the renderer (#28165) 

In Ivy when elements are created a series of static attribute names are provided
over to the construction instruction of that element. Static attribute names
include non-binding attribues (like `<div selected>`) as well as animation bindings
that do not have a RHS value (like `<div @foo>`). Because of this distinction,
value-less animation triggers are rendered first before value-full animation
bindings are and this improper ordering has caused various existing tests to fail.
This patch ensures that animation bindings are evaluated in the order that they
exist within the HTML template code (or host binding code).

PR Close #28165
This commit is contained in:
Matias Niemelä 2019-01-15 13:46:15 -08:00 committed by Alex Rickabaugh
parent 0d6913f037
commit e172e97e13
4 changed files with 141 additions and 154 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

22
packages/compiler-cli/test/compliance/r3_view_compiler_spec.ts
View File

@ -122,7 +122,7 @@ describe('r3_view_compiler', () => {
});
describe('animations', () => {
it('should keep @attr but suppress [@attr]', () => {
it('should not register any @attr attributes as static attributes', () => {
const files: MockDirectory = {
app: {
'example.ts': `
@ -130,7 +130,7 @@ describe('r3_view_compiler', () => {
@Component({
selector: 'my-app',
template: '<div @attrOnly [@myAnimation]="exp"></div>'
template: '<div @attr [@binding]="exp"></div>'
})
export class MyApp {
}
@ -141,14 +141,14 @@ describe('r3_view_compiler', () => {
};
const template = `
const _c0 = ["@attrOnly", ""];
// ...
template: function MyApp_Template(rf, ctx) {
if (rf & 1) {
$i0$.ɵelement(0, "div", _c0);
// ...
$i0$.ɵelement(0, "div");
}
if (rf & 2) {
$i0$.ɵelementProperty(0, "@attr", …);
$i0$.ɵelementProperty(0, "@binding", …);
}
// ...
}`;
const result = compile(files, angularFiles);
expectEmit(result.source, template, 'Incorrect initialization attributes');
@ -173,14 +173,12 @@ describe('r3_view_compiler', () => {
};
const template = `
const _c0 = [3, "@mySelector"];
// ...
template: function MyApp_Template(rf, ctx) {
if (rf & 1) {
$i0$.ɵelementStart(0, "div", _c0);
// ...
$i0$.ɵelementStart(0, "div");
…
$i0$.ɵelementProperty(0, "@mySelector", …);
}
// ...
}`;
const result = compile(files, angularFiles);
expectEmit(result.source, template, 'Incorrect initialization attributes');

12
packages/compiler-cli/test/compliance/r3_view_compiler_styling_spec.ts
View File

@ -214,21 +214,21 @@ describe('compiler compliance: styling', () => {
};
const template = `
const $e1_attrs$ = ["@bar", ""];
const $e2_attrs$ = ["@baz", ""];
…
MyComponent.ngComponentDef = $r3$.ɵdefineComponent({
…
consts: 3,
vars: 1,
vars: 3,
template: function MyComponent_Template(rf, $ctx$) {
if (rf & 1) {
$r3$.ɵelement(0, "div");
$r3$.ɵelement(1, "div", $e1_attrs$);
$r3$.ɵelement(2, "div", $e2_attrs$);
$r3$.ɵelement(1, "div");
$r3$.ɵelement(2, "div");
}
if (rf & 2) {
$r3$.ɵelementProperty(0, "@foo", $r3$.ɵbind(ctx.exp));
$r3$.ɵelementProperty(1, "@bar", $r3$.ɵbind(undefined));
$r3$.ɵelementProperty(2, "@baz", $r3$.ɵbind(undefined));
}
},
encapsulation: 2
@ -280,7 +280,7 @@ describe('compiler compliance: styling', () => {
vars: 1,
template: function MyComponent_Template(rf, ctx) {
if (rf & 1) {
$r3$.ɵelementStart(0, "div", _c0);
$r3$.ɵelementStart(0, "div");
$r3$.ɵlistener("@myAnimation.start", function MyComponent_Template_div_animation_myAnimation_start_0_listener($event) { return ctx.onStart($event); });
$r3$.ɵlistener("@myAnimation.done", function MyComponent_Template_div_animation_myAnimation_done_0_listener($event) { return ctx.onDone($event); });
$r3$.ɵelementEnd();

113
packages/compiler/src/render3/view/template.ts
View File

@ -29,7 +29,7 @@ import {error} from '../../util';
import * as t from '../r3_ast';
import {Identifiers as R3} from '../r3_identifiers';
import {htmlAstToRender3Ast} from '../r3_template_transform';
import {getSyntheticPropertyName, prepareSyntheticListenerFunctionName, prepareSyntheticListenerName, prepareSyntheticPropertyName} from '../util';
import {prepareSyntheticListenerFunctionName, prepareSyntheticListenerName, prepareSyntheticPropertyName} from '../util';
import {R3QueryMetadata} from './api';
import {I18nContext} from './i18n/context';
@ -571,8 +571,8 @@ export class TemplateDefinitionBuilder implements t.Visitor<void>, LocalResolver
// this will build the instructions so that they fall into the following syntax
// add attributes for directive matching purposes
attributes.push(...this.prepareSyntheticAndSelectOnlyAttrs(
allOtherInputs, element.outputs, stylingBuilder));
attributes.push(
...this.prepareSelectOnlyAttrs(allOtherInputs, element.outputs, stylingBuilder));
parameters.push(this.toAttrsParam(attributes));
// local refs (ex.: <div #foo #bar="baz">)
@ -686,8 +686,14 @@ export class TemplateDefinitionBuilder implements t.Visitor<void>, LocalResolver
this.processStylingInstruction(implicit, instruction, false);
});
// the reason why `undefined` is used is because the renderer understands this as a
// special value to symbolize that there is no RHS to this binding
// TODO (matsko): revisit this once FW-959 is approached
const emptyValueBindInstruction = o.importExpr(R3.bind).callFn([o.literal(undefined)]);
// Generate element input bindings
allOtherInputs.forEach((input: t.BoundAttribute) => {
const instruction = mapBindingToInstruction(input.type);
if (input.type === BindingType.Animation) {
const value = input.value.visit(this._valueConverter);
@ -696,21 +702,19 @@ export class TemplateDefinitionBuilder implements t.Visitor<void>, LocalResolver
// 2. [@binding]="{value:fooExp, params:{...}}"
// 3. [@binding]
// 4. @binding
// only formats 1. and 2. include the actual binding of a value to
// an expression and therefore only those should be the only two that
// are allowed. The check below ensures that a binding with no expression
// does not get an empty `elementProperty` instruction created for it.
const hasValue = value && (value instanceof LiteralPrimitive) ? !!value.value : true;
if (hasValue) {
this.allocateBindingSlots(value);
const bindingName = prepareSyntheticPropertyName(input.name);
this.updateInstruction(input.sourceSpan, R3.elementProperty, () => {
return [
o.literal(elementIndex), o.literal(bindingName),
this.convertPropertyBinding(implicit, value)
];
});
}
// All formats will be valid for when a synthetic binding is created.
// The reasoning for this is because the renderer should get each
// synthetic binding value in the order of the array that they are
// defined in...
const hasValue = value instanceof LiteralPrimitive ? !!value.value : true;
this.allocateBindingSlots(value);
const bindingName = prepareSyntheticPropertyName(input.name);
this.updateInstruction(input.sourceSpan, R3.elementProperty, () => {
return [
o.literal(elementIndex), o.literal(bindingName),
(hasValue ? this.convertPropertyBinding(implicit, value) : emptyValueBindInstruction)
];
});
} else if (instruction) {
const value = input.value.visit(this._valueConverter);
if (value !== undefined) {
@ -775,7 +779,7 @@ export class TemplateDefinitionBuilder implements t.Visitor<void>, LocalResolver
const attrsExprs: o.Expression[] = [];
template.attributes.forEach(
(a: t.TextAttribute) => { attrsExprs.push(asLiteral(a.name), asLiteral(a.value)); });
attrsExprs.push(...this.prepareSyntheticAndSelectOnlyAttrs(template.inputs, template.outputs));
attrsExprs.push(...this.prepareSelectOnlyAttrs(template.inputs, template.outputs));
parameters.push(this.toAttrsParam(attrsExprs));
// local refs (ex.: <ng-template #foo>)
@ -969,7 +973,7 @@ export class TemplateDefinitionBuilder implements t.Visitor<void>, LocalResolver
return originalSlots;
}
private allocateBindingSlots(value: AST) {
private allocateBindingSlots(value: AST|null) {
this._bindingSlots += value instanceof Interpolation ? value.expressions.length : 1;
}
@ -1018,21 +1022,15 @@ export class TemplateDefinitionBuilder implements t.Visitor<void>, LocalResolver
* STYLES, style1, value1, style2, value2,
* SELECT_ONLY, name1, name2, name2, ...]
* ```
*
* Note that this function will fully ignore all synthetic (@foo) attribute values
* because those values are intended to always be generated as property instructions.
*/
private prepareSyntheticAndSelectOnlyAttrs(
private prepareSelectOnlyAttrs(
inputs: t.BoundAttribute[], outputs: t.BoundEvent[],
styles?: StylingBuilder): o.Expression[] {
const attrExprs: o.Expression[] = [];
const nonSyntheticInputs: t.BoundAttribute[] = [];
const alreadySeen = new Set<string>();
function isASTWithSource(ast: AST): ast is ASTWithSource {
return ast instanceof ASTWithSource;
}
function isLiteralPrimitive(ast: AST): ast is LiteralPrimitive {
return ast instanceof LiteralPrimitive;
}
const attrExprs: o.Expression[] = [];
function addAttrExpr(key: string | number, value?: o.Expression): void {
if (typeof key === 'string') {
@ -1048,27 +1046,6 @@ export class TemplateDefinitionBuilder implements t.Visitor<void>, LocalResolver
}
}
if (inputs.length) {
const EMPTY_STRING_EXPR = asLiteral('');
inputs.forEach(input => {
if (input.type === BindingType.Animation) {
// @attributes are for Renderer2 animation @triggers, but this feature
// may be supported differently in future versions of angular. However,
// @triggers should always just be treated as regular attributes (it's up
// to the renderer to detect and use them in a special way).
const valueExp = input.value;
if (isASTWithSource(valueExp)) {
const literal = valueExp.ast;
if (isLiteralPrimitive(literal) && literal.value === undefined) {
addAttrExpr(prepareSyntheticPropertyName(input.name), EMPTY_STRING_EXPR);
}
}
} else {
nonSyntheticInputs.push(input);
}
});
}
// it's important that this occurs before SelectOnly because once `elementStart`
// comes across the SelectOnly marker then it will continue reading each value as
// as single property value cell by cell.
@ -1076,14 +1053,30 @@ export class TemplateDefinitionBuilder implements t.Visitor<void>, LocalResolver
styles.populateInitialStylingAttrs(attrExprs);
}
if (nonSyntheticInputs.length || outputs.length) {
addAttrExpr(core.AttributeMarker.SelectOnly);
nonSyntheticInputs.forEach((i: t.BoundAttribute) => addAttrExpr(i.name));
outputs.forEach((o: t.BoundEvent) => {
const name =
o.type === ParsedEventType.Animation ? getSyntheticPropertyName(o.name) : o.name;
addAttrExpr(name);
});
if (inputs.length || outputs.length) {
const attrsStartIndex = attrExprs.length;
for (let i = 0; i < inputs.length; i++) {
const input = inputs[i];
if (input.type !== BindingType.Animation) {
addAttrExpr(input.name);
}
}
for (let i = 0; i < outputs.length; i++) {
const output = outputs[i];
if (output.type !== ParsedEventType.Animation) {
addAttrExpr(output.name);
}
}
// this is a cheap way of adding the marker only after all the input/output
// values have been filtered (by not including the animation ones) and added
// to the expressions. The marker is important because it tells the runtime
// code that this is where attributes without values start...
if (attrExprs.length) {
attrExprs.splice(attrsStartIndex, 0, o.literal(core.AttributeMarker.SelectOnly));
}
}
return attrExprs;

148
packages/core/test/animation/animation_integration_spec.ts
View File

@ -1569,64 +1569,61 @@ const DEFAULT_COMPONENT_ID = '1';
}
});
fixmeIvy(
'FW-932: Animation @triggers are not reported to the renderer in Ivy as they are in VE')
.it('should animate removals of nodes to the `void` state for each animation trigger, but treat all auto styles as pre styles',
() => {
@Component({
selector: 'ani-cmp',
template: `
it('should animate removals of nodes to the `void` state for each animation trigger, but treat all auto styles as pre styles',
() => {
@Component({
selector: 'ani-cmp',
template: `
<div *ngIf="exp" class="ng-if" [@trig1]="exp2" @trig2></div>
`,
animations: [
trigger('trig1', [transition(
'state => void', [animate(1000, style({opacity: 0}))])]),
trigger(
'trig2', [transition(':leave', [animate(1000, style({width: '0px'}))])])
]
})
class Cmp {
public exp = true;
public exp2 = 'state';
}
animations: [
trigger(
'trig1', [transition('state => void', [animate(1000, style({opacity: 0}))])]),
trigger('trig2', [transition(':leave', [animate(1000, style({width: '0px'}))])])
]
})
class Cmp {
public exp = true;
public exp2 = 'state';
}
TestBed.configureTestingModule({declarations: [Cmp]});
TestBed.configureTestingModule({declarations: [Cmp]});
const engine = TestBed.get(ɵAnimationEngine);
const fixture = TestBed.createComponent(Cmp);
const cmp = fixture.componentInstance;
cmp.exp = true;
fixture.detectChanges();
engine.flush();
resetLog();
const engine = TestBed.get(ɵAnimationEngine);
const fixture = TestBed.createComponent(Cmp);
const cmp = fixture.componentInstance;
cmp.exp = true;
fixture.detectChanges();
engine.flush();
resetLog();
const element = getDOM().querySelector(fixture.nativeElement, '.ng-if');
assertHasParent(element, true);
const element = getDOM().querySelector(fixture.nativeElement, '.ng-if');
assertHasParent(element, true);
cmp.exp = false;
fixture.detectChanges();
engine.flush();
cmp.exp = false;
fixture.detectChanges();
engine.flush();
assertHasParent(element, true);
assertHasParent(element, true);
expect(getLog().length).toEqual(2);
expect(getLog().length).toEqual(2);
const player2 = getLog().pop() !;
const player1 = getLog().pop() !;
const player2 = getLog().pop() !;
const player1 = getLog().pop() !;
expect(player2.keyframes).toEqual([
{width: PRE_STYLE, offset: 0},
{width: '0px', offset: 1},
]);
expect(player2.keyframes).toEqual([
{width: PRE_STYLE, offset: 0},
{width: '0px', offset: 1},
]);
expect(player1.keyframes).toEqual([
{opacity: PRE_STYLE, offset: 0}, {opacity: '0', offset: 1}
]);
expect(player1.keyframes).toEqual([
{opacity: PRE_STYLE, offset: 0}, {opacity: '0', offset: 1}
]);
player2.finish();
player1.finish();
assertHasParent(element, false);
});
player2.finish();
player1.finish();
assertHasParent(element, false);
});
it('should properly cancel all existing animations when a removal occurs', () => {
@Component({
@ -3372,43 +3369,42 @@ const DEFAULT_COMPONENT_ID = '1';
expect(getLog().length).toEqual(1);
});
fixmeIvy('FW-951 - Attribute-only synthetic properties are treated differently in Ivy')
.it('should treat the property as true when the expression is missing', () => {
@Component({
selector: 'parent-cmp',
animations: [
trigger(
'myAnimation',
[
transition(
'* => go',
[
style({opacity: 0}),
animate(500, style({opacity: 1})),
]),
]),
],
template: `
it('should treat the property as true when the expression is missing', () => {
@Component({
selector: 'parent-cmp',
animations: [
trigger(
'myAnimation',
[
transition(
'* => go',
[
style({opacity: 0}),
animate(500, style({opacity: 1})),
]),
]),
],
template: `
<div @.disabled>
<div [@myAnimation]="exp"></div>
</div>
`
})
class Cmp {
exp = '';
}
})
class Cmp {
exp = '';
}
TestBed.configureTestingModule({declarations: [Cmp]});
TestBed.configureTestingModule({declarations: [Cmp]});
const fixture = TestBed.createComponent(Cmp);
const cmp = fixture.componentInstance;
fixture.detectChanges();
resetLog();
const fixture = TestBed.createComponent(Cmp);
const cmp = fixture.componentInstance;
fixture.detectChanges();
resetLog();
cmp.exp = 'go';
fixture.detectChanges();
expect(getLog().length).toEqual(0);
});
cmp.exp = 'go';
fixture.detectChanges();
expect(getLog().length).toEqual(0);
});
it('should respect parent/sub animations when the respective area in the DOM is disabled',
fakeAsync(() => {