From 0d6913f0376eb93e33b79c5b44890ed4c95762a1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matias=20Niemel=C3=A4?= Date: Wed, 16 Jan 2019 11:11:33 -0800 Subject: [PATCH] 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 --- .../compliance/r3_view_compiler_i18n_spec.ts | 6 +- .../r3_view_compiler_styling_spec.ts | 96 ++++++++++++++- .../compiler/src/render3/view/compiler.ts | 2 +- .../src/render3/view/styling_builder.ts | 32 +++-- .../compiler/src/render3/view/template.ts | 7 +- .../animation/animation_integration_spec.ts | 103 ++++++++-------- .../animation_query_integration_spec.ts | 110 +++++++++--------- .../core/test/render3/integration_spec.ts | 24 ++++ 8 files changed, 252 insertions(+), 128 deletions(-) diff --git a/packages/compiler-cli/test/compliance/r3_view_compiler_i18n_spec.ts b/packages/compiler-cli/test/compliance/r3_view_compiler_i18n_spec.ts index ccf6bb2eef..cae168aedb 100644 --- a/packages/compiler-cli/test/compliance/r3_view_compiler_i18n_spec.ts +++ b/packages/compiler-cli/test/compliance/r3_view_compiler_i18n_spec.ts @@ -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(); } } diff --git a/packages/compiler-cli/test/compliance/r3_view_compiler_styling_spec.ts b/packages/compiler-cli/test/compliance/r3_view_compiler_styling_spec.ts index f8d06cf28f..e93c462696 100644 --- a/packages/compiler-cli/test/compliance/r3_view_compiler_styling_spec.ts +++ b/packages/compiler-cli/test/compliance/r3_view_compiler_styling_spec.ts @@ -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: \` +
+ \` + }) + export class MyComponentWithInterpolation { + fooId = '123'; + } + + @Component({ + selector: 'my-component-with-muchos-interpolation', + template: \` +
+ \` + }) + export class MyComponentWithMuchosInterpolation { + fooId = '123'; + fooUsername = 'superfoo'; + } + + @Component({ + selector: 'my-component-without-interpolation', + template: \` +
+ \` + }) + 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")); diff --git a/packages/compiler/src/render3/view/compiler.ts b/packages/compiler/src/render3/view/compiler.ts index f8045eaaef..ceec9614be 100644 --- a/packages/compiler/src/render3/view/compiler.ts +++ b/packages/compiler/src/render3/view/compiler.ts @@ -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 diff --git a/packages/compiler/src/render3/view/styling_builder.ts b/packages/compiler/src/render3/view/styling_builder.ts index 9eab564f8e..0b2c322f91 100644 --- a/packages/compiler/src/render3/view/styling_builder.ts +++ b/packages/compiler/src/render3/view/styling_builder.ts @@ -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, 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); diff --git a/packages/compiler/src/render3/view/template.ts b/packages/compiler/src/render3/view/template.ts index 4f1f1762bf..7b31af58b5 100644 --- a/packages/compiler/src/render3/view/template.ts +++ b/packages/compiler/src/render3/view/template.ts @@ -602,11 +602,11 @@ export class TemplateDefinitionBuilder implements t.Visitor, 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, 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); }); diff --git a/packages/core/test/animation/animation_integration_spec.ts b/packages/core/test/animation/animation_integration_spec.ts index 79deb6699a..fc1e046894 100644 --- a/packages/core/test/animation/animation_integration_spec.ts +++ b/packages/core/test/animation/animation_integration_spec.ts @@ -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: `
@@ -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', () => { diff --git a/packages/core/test/animation/animation_query_integration_spec.ts b/packages/core/test/animation/animation_query_integration_spec.ts index b144c2893d..4969036d82 100644 --- a/packages/core/test/animation/animation_query_integration_spec.ts +++ b/packages/core/test/animation/animation_query_integration_spec.ts @@ -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: `
`, - 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`', () => { diff --git a/packages/core/test/render3/integration_spec.ts b/packages/core/test/render3/integration_spec.ts index df30a7b3ee..4550a91a47 100644 --- a/packages/core/test/render3/integration_spec.ts +++ b/packages/core/test/render3/integration_spec.ts @@ -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(); + }); }); });