fix(ivy): ensure interpolated style/classes do not cause tracking issues for bindings (#28190)

With the refactoring or how styles/classes are implmented in Ivy,
interpolation has caused the binding code to mess up since interpolation
itself takes up its own slot in Ivy's memory management code. This patch
makes sure that interpolation works as expected with class and style
bindings.

Jira issue: FW-944

PR Close #28190
This commit is contained in:
Matias Niemelä 2019-01-16 11:11:33 -08:00 committed by Alex Rickabaugh
parent 896cf35afb
commit 0d6913f037
8 changed files with 252 additions and 128 deletions

View File

@ -1270,12 +1270,10 @@ describe('i18n support in the view compiler', () => {
template: function MyComponent_Template(rf, ctx) {
if (rf & 1) {
$r3$.ɵelementStart(0, "span", $_c0$);
$r3$.ɵi18nStart(1, $MSG_EXTERNAL_5295701706185791735$$APP_SPEC_TS_0$);
$r3$.ɵi18nEnd();
$r3$.ɵi18n(1, $MSG_EXTERNAL_5295701706185791735$$APP_SPEC_TS_0$);
$r3$.ɵelementEnd();
$r3$.ɵelementStart(2, "span", $_c1$);
$r3$.ɵi18nStart(3, $MSG_EXTERNAL_4722270221386399294$$APP_SPEC_TS_2$);
$r3$.ɵi18nEnd();
$r3$.ɵi18n(3, $MSG_EXTERNAL_4722270221386399294$$APP_SPEC_TS_2$);
$r3$.ɵelementEnd();
}
}

View File

@ -395,6 +395,99 @@ describe('compiler compliance: styling', () => {
expectEmit(result.source, template, 'Incorrect template');
});
it('should correctly count the total slots required when style/class bindings include interpolation',
() => {
const files = {
app: {
'spec.ts': `
import {Component, NgModule} from '@angular/core';
@Component({
selector: 'my-component-with-interpolation',
template: \`
<div class="foo foo-{{ fooId }}"></div>
\`
})
export class MyComponentWithInterpolation {
fooId = '123';
}
@Component({
selector: 'my-component-with-muchos-interpolation',
template: \`
<div class="foo foo-{{ fooId }}-{{ fooUsername }}"></div>
\`
})
export class MyComponentWithMuchosInterpolation {
fooId = '123';
fooUsername = 'superfoo';
}
@Component({
selector: 'my-component-without-interpolation',
template: \`
<div [class]="exp"></div>
\`
})
export class MyComponentWithoutInterpolation {
exp = 'bar';
}
@NgModule({declarations: [MyComponentWithInterpolation, MyComponentWithMuchosInterpolation, MyComponentWithoutInterpolation]})
export class MyModule {}
`
}
};
const template = `
consts: 1,
vars: 1,
template: function MyComponentWithInterpolation_Template(rf, $ctx$) {
if (rf & 1) {
$r3$.ɵelementStart(0, "div");
$r3$.ɵelementStyling();
$r3$.ɵelementEnd();
}
if (rf & 2) {
$r3$.ɵelementStylingMap(0, $r3$.ɵinterpolation1("foo foo-", $ctx$.fooId, ""));
$r3$.ɵelementStylingApply(0);
}
}
consts: 1,
vars: 2,
template: function MyComponentWithMuchosInterpolation_Template(rf, $ctx$) {
if (rf & 1) {
$r3$.ɵelementStart(0, "div");
$r3$.ɵelementStyling();
$r3$.ɵelementEnd();
}
if (rf & 2) {
$r3$.ɵelementStylingMap(0, $r3$.ɵinterpolation2("foo foo-", $ctx$.fooId, "-", $ctx$.fooUsername, ""));
$r3$.ɵelementStylingApply(0);
}
}
consts: 1,
vars: 0,
template: function MyComponentWithoutInterpolation_Template(rf, $ctx$) {
if (rf & 1) {
$r3$.ɵelementStart(0, "div");
$r3$.ɵelementStyling();
$r3$.ɵelementEnd();
}
if (rf & 2) {
$r3$.ɵelementStylingMap(0, $ctx$.exp);
$r3$.ɵelementStylingApply(0);
}
}
`;
const result = compile(files, angularFiles);
expectEmit(result.source, template, 'Incorrect template');
});
it('should place initial, multi, singular and application followed by attribute style instructions in the template code in that order',
() => {
const files = {
@ -688,8 +781,7 @@ describe('compiler compliance: styling', () => {
vars: 2,
template: function MyComponent_Template(rf, $ctx$) {
if (rf & 1) {
$r3$.ɵelementStart(0, "div", $e0_attrs$);
$r3$.ɵelementEnd();
$r3$.ɵelement(0, "div", $e0_attrs$);
}
if (rf & 2) {
$r3$.ɵelementAttribute(0, "class", $r3$.ɵbind("round"));

View File

@ -747,7 +747,7 @@ function createHostBindingsFunction(
createStatements.push(createStylingStmt(hostInstruction, bindingContext, bindingFn));
}
if (styleBuilder.hasBindingsOrInitialValues()) {
if (styleBuilder.hasBindings) {
// singular style/class bindings (things like `[style.prop]` and `[class.name]`)
// MUST be registered on a given element within the component/directive
// templateFn/hostBindingsFn functions. The instruction below will figure out

View File

@ -7,7 +7,7 @@
*/
import {ConstantPool} from '../../constant_pool';
import {AttributeMarker} from '../../core';
import {AST, BindingType} from '../../expression_parser/ast';
import {AST, BindingType, Interpolation} from '../../expression_parser/ast';
import * as o from '../../output/output_ast';
import {ParseSourceSpan} from '../../parse_util';
import * as t from '../r3_ast';
@ -23,6 +23,7 @@ import {ValueConverter} from './template';
export interface Instruction {
sourceSpan: ParseSourceSpan|null;
reference: o.ExternalReference;
allocateBindingSlots: number;
buildParams(convertFn: (value: any) => o.Expression): o.Expression[];
}
@ -36,7 +37,6 @@ interface BoundStylingEntry {
value: AST;
}
/**
* Produces creation/update instructions for all styling bindings (class and style)
*
@ -73,7 +73,7 @@ export class StylingBuilder {
* Whether or not there are any styling bindings present
* (i.e. `[style]`, `[class]`, `[style.prop]` or `[class.name]`)
*/
private _hasBindings = false;
public hasBindings = false;
/** the input for [class] (if it exists) */
private _classMapInput: BoundStylingEntry|null = null;
@ -110,8 +110,6 @@ export class StylingBuilder {
constructor(private _elementIndexExpr: o.Expression, private _directiveExpr: o.Expression|null) {}
hasBindingsOrInitialValues() { return this._hasBindings || this._hasInitialValues; }
/**
* Registers a given input to the styling builder to be later used when producing AOT code.
*
@ -158,7 +156,7 @@ export class StylingBuilder {
this._styleMapInput = entry;
}
this._lastStylingInput = entry;
this._hasBindings = true;
this.hasBindings = true;
return entry;
}
@ -172,7 +170,7 @@ export class StylingBuilder {
this._classMapInput = entry;
}
this._lastStylingInput = entry;
this._hasBindings = true;
this.hasBindings = true;
return entry;
}
@ -235,6 +233,7 @@ export class StylingBuilder {
return {
sourceSpan,
reference: R3.elementHostAttrs,
allocateBindingSlots: 0,
buildParams: () => {
this.populateInitialStylingAttrs(attrs);
return [this._directiveExpr !, getConstantLiteralFromArray(constantPool, attrs)];
@ -252,9 +251,10 @@ export class StylingBuilder {
*/
buildElementStylingInstruction(sourceSpan: ParseSourceSpan|null, constantPool: ConstantPool):
Instruction|null {
if (this._hasBindings) {
if (this.hasBindings) {
return {
sourceSpan,
allocateBindingSlots: 0,
reference: R3.elementStyling,
buildParams: () => {
// a string array of every style-based binding
@ -315,18 +315,27 @@ export class StylingBuilder {
buildElementStylingMapInstruction(valueConverter: ValueConverter): Instruction|null {
if (this._classMapInput || this._styleMapInput) {
const stylingInput = this._classMapInput ! || this._styleMapInput !;
let totalBindingSlotsRequired = 0;
// these values must be outside of the update block so that they can
// be evaluted (the AST visit call) during creation time so that any
// pipes can be picked up in time before the template is built
const mapBasedClassValue =
this._classMapInput ? this._classMapInput.value.visit(valueConverter) : null;
if (mapBasedClassValue instanceof Interpolation) {
totalBindingSlotsRequired += mapBasedClassValue.expressions.length;
}
const mapBasedStyleValue =
this._styleMapInput ? this._styleMapInput.value.visit(valueConverter) : null;
if (mapBasedStyleValue instanceof Interpolation) {
totalBindingSlotsRequired += mapBasedStyleValue.expressions.length;
}
return {
sourceSpan: stylingInput.sourceSpan,
reference: R3.elementStylingMap,
allocateBindingSlots: totalBindingSlotsRequired,
buildParams: (convertFn: (value: any) => o.Expression) => {
const params: o.Expression[] = [this._elementIndexExpr];
@ -356,12 +365,14 @@ export class StylingBuilder {
private _buildSingleInputs(
reference: o.ExternalReference, inputs: BoundStylingEntry[], mapIndex: Map<string, number>,
allowUnits: boolean, valueConverter: ValueConverter): Instruction[] {
let totalBindingSlotsRequired = 0;
return inputs.map(input => {
const bindingIndex: number = mapIndex.get(input.name) !;
const value = input.value.visit(valueConverter);
totalBindingSlotsRequired += (value instanceof Interpolation) ? value.expressions.length : 0;
return {
sourceSpan: input.sourceSpan,
reference,
allocateBindingSlots: totalBindingSlotsRequired, reference,
buildParams: (convertFn: (value: any) => o.Expression) => {
const params = [this._elementIndexExpr, o.literal(bindingIndex), convertFn(value)];
if (allowUnits) {
@ -401,6 +412,7 @@ export class StylingBuilder {
return {
sourceSpan: this._lastStylingInput ? this._lastStylingInput.sourceSpan : null,
reference: R3.elementStylingApply,
allocateBindingSlots: 0,
buildParams: () => {
const params: o.Expression[] = [this._elementIndexExpr];
if (this._directiveExpr) {
@ -417,7 +429,7 @@ export class StylingBuilder {
*/
buildUpdateLevelInstructions(valueConverter: ValueConverter) {
const instructions: Instruction[] = [];
if (this._hasBindings) {
if (this.hasBindings) {
const mapInstruction = this.buildElementStylingMapInstruction(valueConverter);
if (mapInstruction) {
instructions.push(mapInstruction);

View File

@ -602,11 +602,11 @@ export class TemplateDefinitionBuilder implements t.Visitor<void>, LocalResolver
return element.children.length > 0;
};
const createSelfClosingInstruction = !stylingBuilder.hasBindingsOrInitialValues() &&
!isNgContainer && element.outputs.length === 0 && i18nAttrs.length === 0 && !hasChildren();
const createSelfClosingInstruction = !stylingBuilder.hasBindings && !isNgContainer &&
element.outputs.length === 0 && i18nAttrs.length === 0 && !hasChildren();
const createSelfClosingI18nInstruction = !createSelfClosingInstruction &&
!stylingBuilder.hasBindingsOrInitialValues() && hasTextChildrenOnly(element.children);
!stylingBuilder.hasBindings && hasTextChildrenOnly(element.children);
if (createSelfClosingInstruction) {
this.creationInstruction(element.sourceSpan, R3.element, trimTrailingNulls(parameters));
@ -682,6 +682,7 @@ export class TemplateDefinitionBuilder implements t.Visitor<void>, LocalResolver
// `elementStylingMap`, `elementClassProp` and `elementStylingApply` are all generated
// and assign in the code below.
stylingBuilder.buildUpdateLevelInstructions(this._valueConverter).forEach(instruction => {
this._bindingSlots += instruction.allocateBindingSlots;
this.processStylingInstruction(implicit, instruction, false);
});

View File

@ -2915,11 +2915,10 @@ const DEFAULT_COMPONENT_ID = '1';
expect(cmp.log).toEqual(['parent-done', 'child-done']);
}));
fixmeIvy(
'FW-944 - style/class bindings lose track of consts/vars when interpolation is present')
.it('should fire callbacks and collect the correct the totalTime and element details for any queried sub animations',
fakeAsync(() => {
@Component({
it('should fire callbacks and collect the correct the totalTime and element details for any queried sub animations',
fakeAsync(
() => {
@Component({
selector: 'my-cmp',
template: `
<div class="parent" [@parent]="exp" (@parent.done)="cb('all','done', $event)">
@ -2955,62 +2954,62 @@ const DEFAULT_COMPONENT_ID = '1';
]
})
class Cmp {
log: string[] = [];
events: {[name: string]: any} = {};
// TODO(issue/24571): remove '!'.
exp !: string;
items: any = [0, 1, 2, 3];
log: string[] = [];
events: {[name: string]: any} = {};
// TODO(issue/24571): remove '!'.
exp !: string;
items: any = [0, 1, 2, 3];
cb(name: string, phase: string, event: AnimationEvent) {
this.log.push(name + '-' + phase);
this.events[name] = event;
}
}
cb(name: string, phase: string, event: AnimationEvent) {
this.log.push(name + '-' + phase);
this.events[name] = event;
}
}
TestBed.configureTestingModule({declarations: [Cmp]});
TestBed.configureTestingModule({declarations: [Cmp]});
const engine = TestBed.get(ɵAnimationEngine);
const fixture = TestBed.createComponent(Cmp);
const cmp = fixture.componentInstance;
cmp.exp = 'go';
fixture.detectChanges();
engine.flush();
flushMicrotasks();
const engine = TestBed.get(ɵAnimationEngine);
const fixture = TestBed.createComponent(Cmp);
const cmp = fixture.componentInstance;
cmp.exp = 'go';
fixture.detectChanges();
engine.flush();
flushMicrotasks();
expect(cmp.log).toEqual(['c-0-start', 'c-1-start', 'c-2-start', 'c-3-start']);
cmp.log = [];
expect(cmp.log).toEqual(['c-0-start', 'c-1-start', 'c-2-start', 'c-3-start']);
cmp.log = [];
const players = getLog();
// 1 + 4 + 4 = 9 players
expect(players.length).toEqual(9);
const players = getLog();
// 1 + 4 + 4 = 9 players
expect(players.length).toEqual(9);
const [pA, pq1a, pq1b, pq1c, pq1d, pq2a, pq2b, pq2c, pq2d] = getLog();
pA.finish();
pq1a.finish();
pq1b.finish();
pq1c.finish();
pq1d.finish();
flushMicrotasks();
const [pA, pq1a, pq1b, pq1c, pq1d, pq2a, pq2b, pq2c, pq2d] = getLog();
pA.finish();
pq1a.finish();
pq1b.finish();
pq1c.finish();
pq1d.finish();
flushMicrotasks();
expect(cmp.log).toEqual([]);
pq2a.finish();
pq2b.finish();
pq2c.finish();
pq2d.finish();
flushMicrotasks();
expect(cmp.log).toEqual([]);
pq2a.finish();
pq2b.finish();
pq2c.finish();
pq2d.finish();
flushMicrotasks();
expect(cmp.log).toEqual(
['all-done', 'c-0-done', 'c-1-done', 'c-2-done', 'c-3-done']);
expect(cmp.log).toEqual(
['all-done', 'c-0-done', 'c-1-done', 'c-2-done', 'c-3-done']);
expect(cmp.events['c-0'].totalTime).toEqual(4100); // 1000 + 1000 + 1800 + 300
expect(cmp.events['c-0'].element.innerText.trim()).toEqual('0');
expect(cmp.events['c-1'].totalTime).toEqual(4100);
expect(cmp.events['c-1'].element.innerText.trim()).toEqual('1');
expect(cmp.events['c-2'].totalTime).toEqual(4100);
expect(cmp.events['c-2'].element.innerText.trim()).toEqual('2');
expect(cmp.events['c-3'].totalTime).toEqual(4100);
expect(cmp.events['c-3'].element.innerText.trim()).toEqual('3');
}));
expect(cmp.events['c-0'].totalTime).toEqual(4100); // 1000 + 1000 + 1800 + 300
expect(cmp.events['c-0'].element.innerText.trim()).toEqual('0');
expect(cmp.events['c-1'].totalTime).toEqual(4100);
expect(cmp.events['c-1'].element.innerText.trim()).toEqual('1');
expect(cmp.events['c-2'].totalTime).toEqual(4100);
expect(cmp.events['c-2'].element.innerText.trim()).toEqual('2');
expect(cmp.events['c-3'].totalTime).toEqual(4100);
expect(cmp.events['c-3'].element.innerText.trim()).toEqual('3');
}));
});
describe('animation control flags', () => {

View File

@ -295,73 +295,71 @@ import {HostListener} from '../../src/metadata/directives';
expect(p6.element.classList.contains('b3')).toBeTruthy();
});
fixmeIvy(
'FW-944 - style/class bindings lose track of consts/vars when interpolation is present')
.it('should be able to query all active animations using :animating in a query', () => {
@Component({
selector: 'ani-cmp',
template: `
it('should be able to query all active animations using :animating in a query', () => {
@Component({
selector: 'ani-cmp',
template: `
<div [@myAnimation]="exp" #parent>
<div *ngFor="let item of items" class="item e-{{ item }}">
</div>
</div>
`,
animations: [
trigger(
'myAnimation',
[
transition(
'* => a',
[
query(
'.item:nth-child(odd)',
[
style({opacity: 0}),
animate(1000, style({opacity: 1})),
]),
]),
transition(
'* => b',
[
query(
'.item:animating',
[
style({opacity: 1}),
animate(1000, style({opacity: 0})),
]),
]),
]),
]
})
class Cmp {
public exp: any;
public items: number[] = [0, 1, 2, 3, 4];
}
animations: [
trigger(
'myAnimation',
[
transition(
'* => a',
[
query(
'.item:nth-child(odd)',
[
style({opacity: 0}),
animate(1000, style({opacity: 1})),
]),
]),
transition(
'* => b',
[
query(
'.item:animating',
[
style({opacity: 1}),
animate(1000, style({opacity: 0})),
]),
]),
]),
]
})
class Cmp {
public exp: any;
public items: number[] = [0, 1, 2, 3, 4];
}
TestBed.configureTestingModule({declarations: [Cmp]});
TestBed.configureTestingModule({declarations: [Cmp]});
const engine = TestBed.get(ɵAnimationEngine);
const fixture = TestBed.createComponent(Cmp);
const cmp = fixture.componentInstance;
const engine = TestBed.get(ɵAnimationEngine);
const fixture = TestBed.createComponent(Cmp);
const cmp = fixture.componentInstance;
cmp.exp = 'a';
fixture.detectChanges();
engine.flush();
cmp.exp = 'a';
fixture.detectChanges();
engine.flush();
let players = getLog();
expect(players.length).toEqual(3);
resetLog();
let players = getLog();
expect(players.length).toEqual(3);
resetLog();
cmp.exp = 'b';
fixture.detectChanges();
engine.flush();
cmp.exp = 'b';
fixture.detectChanges();
engine.flush();
players = getLog();
expect(players.length).toEqual(3);
expect(players[0].element.classList.contains('e-0')).toBeTruthy();
expect(players[1].element.classList.contains('e-2')).toBeTruthy();
expect(players[2].element.classList.contains('e-4')).toBeTruthy();
});
players = getLog();
expect(players.length).toEqual(3);
expect(players[0].element.classList.contains('e-0')).toBeTruthy();
expect(players[1].element.classList.contains('e-2')).toBeTruthy();
expect(players[2].element.classList.contains('e-4')).toBeTruthy();
});
it('should be able to query all actively queued animation triggers via `@*:animating`',
() => {

View File

@ -1916,6 +1916,30 @@ describe('render3 integration test', () => {
fixture.update();
expect(target.style.getPropertyValue('width')).toEqual('777px');
});
it('should properly handle and render interpolation for class attribute bindings', () => {
const App = createComponent('app', function(rf: RenderFlags, ctx: any) {
if (rf & RenderFlags.Create) {
elementStart(0, 'div');
elementStyling();
elementEnd();
}
if (rf & RenderFlags.Update) {
elementStylingMap(0, interpolation2('-', ctx.name, '-', ctx.age, '-'));
elementStylingApply(0);
}
}, 1, 2);
const fixture = new ComponentFixture(App);
const target = fixture.hostElement.querySelector('div') !;
expect(target.classList.contains('-fred-36-')).toBeFalsy();
fixture.component.name = 'fred';
fixture.component.age = '36';
fixture.update();
expect(target.classList.contains('-fred-36-')).toBeTruthy();
});
});
});