From 345a3cd9aa2caf631fb1a08166ea9d27f418e976 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matias=20Niemel=C3=A4?= Date: Tue, 7 May 2019 13:18:27 -0700 Subject: [PATCH] fix(ivy): ensure `select(n)` instructions are always generated around style/class bindings (#30311) Prior to this patch, the `select(n)` instruction would only be generated when property bindings are encountered which meant that styling-related bindings were skipped. This patch ensures that all styling-related bindings (i.e. class and style bindings) are always prepended with a `select()` instruction prior to being generated in AOT. PR Close #30311 --- .../compliance/r3_compiler_compliance_spec.ts | 1 + .../r3_view_compiler_styling_spec.ts | 70 ++++++++++++++++++- .../compiler/src/render3/view/template.ts | 16 +++-- 3 files changed, 78 insertions(+), 9 deletions(-) diff --git a/packages/compiler-cli/test/compliance/r3_compiler_compliance_spec.ts b/packages/compiler-cli/test/compliance/r3_compiler_compliance_spec.ts index e6eaa960ed..c69f86a36e 100644 --- a/packages/compiler-cli/test/compliance/r3_compiler_compliance_spec.ts +++ b/packages/compiler-cli/test/compliance/r3_compiler_compliance_spec.ts @@ -506,6 +506,7 @@ describe('compiler compliance', () => { $r3$.ɵɵelementEnd(); } if (rf & 2) { + $r3$.ɵɵselect(0); $r3$.ɵɵelementStyleProp(0, 0, ctx.color); $r3$.ɵɵelementClassProp(0, 0, ctx.error); $r3$.ɵɵelementStylingApply(0); 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 6a36aa5f78..11bc7f1c4b 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 @@ -389,6 +389,7 @@ describe('compiler compliance: styling', () => { $r3$.ɵɵelementEnd(); } if (rf & 2) { + $r3$.ɵɵselect(0); $r3$.ɵɵelementStyleMap(0, $ctx$.myStyleExp); $r3$.ɵɵelementStylingApply(0); } @@ -454,6 +455,7 @@ describe('compiler compliance: styling', () => { $r3$.ɵɵelementEnd(); } if (rf & 2) { + $r3$.ɵɵselect(0); $r3$.ɵɵelementClassMap(0, $r3$.ɵɵinterpolation1("foo foo-", $ctx$.fooId, "")); $r3$.ɵɵelementStylingApply(0); } @@ -468,6 +470,7 @@ describe('compiler compliance: styling', () => { $r3$.ɵɵelementEnd(); } if (rf & 2) { + $r3$.ɵɵselect(0); $r3$.ɵɵelementClassMap(0, $r3$.ɵɵinterpolation2("foo foo-", $ctx$.fooId, "-", $ctx$.fooUsername, "")); $r3$.ɵɵelementStylingApply(0); } @@ -482,6 +485,7 @@ describe('compiler compliance: styling', () => { $r3$.ɵɵelementEnd(); } if (rf & 2) { + $r3$.ɵɵselect(0); $r3$.ɵɵelementClassMap(0, $ctx$.exp); $r3$.ɵɵelementStylingApply(0); } @@ -538,11 +542,11 @@ describe('compiler compliance: styling', () => { $r3$.ɵɵelementEnd(); } if (rf & 2) { + $r3$.ɵɵselect(0); $r3$.ɵɵelementStyleMap(0, $ctx$.myStyleExp); $r3$.ɵɵelementStyleProp(0, 0, $ctx$.myWidth); $r3$.ɵɵelementStyleProp(0, 1, $ctx$.myHeight); $r3$.ɵɵelementStylingApply(0); - $r3$.ɵɵselect(0); $r3$.ɵɵelementAttribute(0, "style", $r3$.ɵɵbind("border-width: 10px"), $r3$.ɵɵsanitizeStyle); } }, @@ -598,6 +602,7 @@ describe('compiler compliance: styling', () => { $r3$.ɵɵelementEnd(); } if (rf & 2) { + $r3$.ɵɵselect(0); $r3$.ɵɵelementStyleProp(0, 0, ctx.myImage); $r3$.ɵɵelementStylingApply(0); } @@ -639,6 +644,7 @@ describe('compiler compliance: styling', () => { $r3$.ɵɵelementEnd(); } if (rf & 2) { + $r3$.ɵɵselect(0); $r3$.ɵɵelementStyleProp(0, 0, 12, "px"); $r3$.ɵɵelementStylingApply(0); } @@ -704,6 +710,7 @@ describe('compiler compliance: styling', () => { $r3$.ɵɵelementEnd(); } if (rf & 2) { + $r3$.ɵɵselect(0); $r3$.ɵɵelementClassMap(0,$ctx$.myClassExp); $r3$.ɵɵelementStylingApply(0); } @@ -760,11 +767,11 @@ describe('compiler compliance: styling', () => { $r3$.ɵɵelementEnd(); } if (rf & 2) { + $r3$.ɵɵselect(0); $r3$.ɵɵelementClassMap(0, $ctx$.myClassExp); $r3$.ɵɵelementClassProp(0, 0, $ctx$.yesToApple); $r3$.ɵɵelementClassProp(0, 1, $ctx$.yesToOrange); $r3$.ɵɵelementStylingApply(0); - $r3$.ɵɵselect(0); $r3$.ɵɵelementAttribute(0, "class", $r3$.ɵɵbind("banana")); } }, @@ -853,7 +860,7 @@ describe('compiler compliance: styling', () => { }); describe('[style] mixed with [class]', () => { - it('should combine [style] and [class] bindings into a single instruction', () => { + it('should split [style] and [class] bindings into a separate instructions', () => { const files = { app: { 'spec.ts': ` @@ -882,6 +889,7 @@ describe('compiler compliance: styling', () => { $r3$.ɵɵelementEnd(); } if (rf & 2) { + $r3$.ɵɵselect(0); $r3$.ɵɵelementStyleMap(0, $ctx$.myStyleExp); $r3$.ɵɵelementClassMap(0, $ctx$.myClassExp); $r3$.ɵɵelementStylingApply(0); @@ -925,6 +933,7 @@ describe('compiler compliance: styling', () => { $r3$.ɵɵelementEnd(); } if (rf & 2) { + $r3$.ɵɵselect(0); $r3$.ɵɵelementStyleMap(0, $r3$.ɵɵpipeBind1(1, 0, $ctx$.myStyleExp)); $r3$.ɵɵelementClassMap(0, $r3$.ɵɵpipeBind1(2, 2, $ctx$.myClassExp)); $r3$.ɵɵelementStylingApply(0); @@ -982,6 +991,7 @@ describe('compiler compliance: styling', () => { $r3$.ɵɵelementEnd(); } if (rf & 2) { + $r3$.ɵɵselect(0); $r3$.ɵɵelementStyleMap(0, $r3$.ɵɵpipeBind2(1, 1, $ctx$.myStyleExp, 1000)); $r3$.ɵɵelementClassMap(0, $e2_styling$); $r3$.ɵɵelementStyleProp(0, 0, $r3$.ɵɵpipeBind2(2, 4, $ctx$.barExp, 3000)); @@ -997,6 +1007,59 @@ describe('compiler compliance: styling', () => { const result = compile(files, angularFiles); expectEmit(result.source, template, 'Incorrect template'); }); + + it('should always generate select() statements before any styling instructions', () => { + const files = { + app: { + 'spec.ts': ` + import {Component, NgModule} from '@angular/core'; + + @Component({ + selector: 'my-component', + template: \` +
+
+
+
+ \` + }) + export class MyComponent { + w1 = '100px'; + h1 = '100px'; + a1 = true; + r1 = true; + } + + @NgModule({declarations: [MyComponent]}) + export class MyModule {} + ` + } + }; + + const template = ` + … + template: function MyComponent_Template(rf, $ctx$) { + … + if (rf & 2) { + $r3$.ɵɵselect(0); + $r3$.ɵɵelementStyleProp(0, 0, $ctx$.w1); + $r3$.ɵɵelementStylingApply(0); + $r3$.ɵɵselect(1); + $r3$.ɵɵelementStyleProp(1, 0, $ctx$.h1); + $r3$.ɵɵelementStylingApply(1); + $r3$.ɵɵselect(2); + $r3$.ɵɵelementClassProp(2, 0, $ctx$.a1); + $r3$.ɵɵelementStylingApply(2); + $r3$.ɵɵselect(3); + $r3$.ɵɵelementClassProp(3, 0, $ctx$.r1); + $r3$.ɵɵelementStylingApply(3); + } + } + `; + + const result = compile(files, angularFiles); + expectEmit(result.source, template, 'Incorrect template'); + }); }); describe('@Component host styles/classes', () => { @@ -1171,6 +1234,7 @@ describe('compiler compliance: styling', () => { $r3$.ɵɵelementEnd(); } if (rf & 2) { + $r3$.ɵɵselect(0); $r3$.ɵɵelementStyleMap(0, ctx.myStyleExp); $r3$.ɵɵelementClassMap(0, ctx.myClassExp); $r3$.ɵɵelementStyleProp(0, 0, ctx.myHeightExp, null, true); diff --git a/packages/compiler/src/render3/view/template.ts b/packages/compiler/src/render3/view/template.ts index 16fe7d13d7..af7c30a612 100644 --- a/packages/compiler/src/render3/view/template.ts +++ b/packages/compiler/src/render3/view/template.ts @@ -666,7 +666,7 @@ export class TemplateDefinitionBuilder implements t.Visitor, LocalResolver // (things like `elementStyleProp`, `elementClassProp`, etc..) are applied later on in this // file this.processStylingInstruction( - implicit, + elementIndex, implicit, stylingBuilder.buildElementStylingInstruction(element.sourceSpan, this.constantPool), true); @@ -688,10 +688,13 @@ export class TemplateDefinitionBuilder implements t.Visitor, LocalResolver // update block of the template function AOT code. Instructions like `elementStyleProp`, // `elementStyleMap`, `elementClassMap`, `elementClassProp` and `elementStylingApply` // are all generated and assigned in the code below. - stylingBuilder.buildUpdateLevelInstructions(this._valueConverter).forEach(instruction => { + const stylingInstructions = stylingBuilder.buildUpdateLevelInstructions(this._valueConverter); + const limit = stylingInstructions.length - 1; + for (let i = 0; i <= limit; i++) { + const instruction = stylingInstructions[i]; this._bindingSlots += instruction.allocateBindingSlots; - this.processStylingInstruction(implicit, instruction, false); - }); + this.processStylingInstruction(elementIndex, 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 @@ -1007,14 +1010,15 @@ export class TemplateDefinitionBuilder implements t.Visitor, LocalResolver } private processStylingInstruction( - implicit: any, instruction: Instruction|null, createMode: boolean) { + elementIndex: number, implicit: any, instruction: Instruction|null, createMode: boolean) { if (instruction) { const paramsFn = () => instruction.buildParams(value => this.convertPropertyBinding(implicit, value, true)); if (createMode) { this.creationInstruction(instruction.sourceSpan, instruction.reference, paramsFn); } else { - this.updateInstruction(-1, instruction.sourceSpan, instruction.reference, paramsFn); + this.updateInstruction( + elementIndex, instruction.sourceSpan, instruction.reference, paramsFn); } } }