From baf103c98f717e23426a396f3a7f45eecc470c8b Mon Sep 17 00:00:00 2001 From: Olivier Combe Date: Tue, 29 Jan 2019 15:28:31 +0100 Subject: [PATCH] fix(ivy): don't increment `expandoStartIndex` after directives are matched (#28424) i18n instructions create text nodes dynamically and save them between bindings and the expando block in `LView`. e.g., they try to create the following order in `LView`. ``` | -- elements -- | -- bindings -- | -- dynamic i18n text -- | -- expando (dirs, injectors) -- | ``` Each time a new text node is created, it is pushed to the end of the array and the `expandoStartIndex` marker is incremented, so the section begins slightly later. This happens in `allocExpando`. This is fine if no directives have been created yet. The end of the array will be in the "dynamic text node" section. | -- elements -- | -- bindings -- | -- dynamic i18n text -- | However, this approach doesn't work if dynamic text nodes are created after directives are matched (for example when the directive uses host bindings). In that case, there are already directives and injectors saved in the "expando" section. So pushing to the end of `LView` actually pushes after the expando section. What we get is this: ``` | -- elements -- | -- bindings -- | -- dynamic i18n text -- | -- expando -- | -- dynamic i18n text-- | ``` In this case, the `expandoStartIndex` shouldn't be incremented because we are not inserting anything before the expando section (it's now after the expando section). But because it is incremented in the code right now, it's now pointing to an index in the middle of the expando section. This PR fixes that so that we only increment the `expandoStartIndex` if nothing was pushed into the expando section. FW-978 #resolve PR Close #28424 --- packages/core/src/render3/i18n.ts | 32 ++--- packages/core/src/render3/instructions.ts | 21 +++- packages/core/src/render3/interfaces/i18n.ts | 16 --- packages/core/test/render3/i18n_spec.ts | 118 ++++++++++++++++--- 4 files changed, 134 insertions(+), 53 deletions(-) diff --git a/packages/core/src/render3/i18n.ts b/packages/core/src/render3/i18n.ts index c858ef845d..31d6ff3125 100644 --- a/packages/core/src/render3/i18n.ts +++ b/packages/core/src/render3/i18n.ts @@ -352,13 +352,19 @@ export function i18nStart(index: number, message: string, subTemplateIndex?: num } } +// Count for the number of vars that will be allocated for each i18n block. +// It is global because this is used in multiple functions that include loops and recursive calls. +// This is reset to 0 when `i18nStartFirstPass` is called. +let i18nVarsCount: number; + /** * See `i18nStart` above. */ function i18nStartFirstPass( tView: TView, index: number, message: string, subTemplateIndex?: number) { const viewData = getLView(); - const expandoStartIndex = tView.blueprint.length - HEADER_OFFSET; + const startIndex = tView.blueprint.length - HEADER_OFFSET; + i18nVarsCount = 0; const previousOrParentTNode = getPreviousOrParentTNode(); const parentTNode = getIsParent() ? getPreviousOrParentTNode() : previousOrParentTNode && previousOrParentTNode.parent; @@ -409,8 +415,7 @@ function i18nStartFirstPass( if (j & 1) { // Odd indexes are ICU expressions // Create the comment node that will anchor the ICU expression - allocExpando(viewData); - const icuNodeIndex = tView.blueprint.length - 1 - HEADER_OFFSET; + const icuNodeIndex = startIndex + i18nVarsCount++; createOpCodes.push( COMMENT_MARKER, ngDevMode ? `ICU ${icuNodeIndex}` : '', icuNodeIndex, parentIndex << I18nMutateOpCode.SHIFT_PARENT | I18nMutateOpCode.AppendChild); @@ -434,27 +439,25 @@ function i18nStartFirstPass( // Even indexes are text (including bindings) const hasBinding = text.match(BINDING_REGEXP); // Create text nodes - allocExpando(viewData); - const textNodeIndex = tView.blueprint.length - 1 - HEADER_OFFSET; + const textNodeIndex = startIndex + i18nVarsCount++; createOpCodes.push( // If there is a binding, the value will be set during update hasBinding ? '' : text, textNodeIndex, parentIndex << I18nMutateOpCode.SHIFT_PARENT | I18nMutateOpCode.AppendChild); if (hasBinding) { - addAllToArray( - generateBindingUpdateOpCodes(text, tView.blueprint.length - 1 - HEADER_OFFSET), - updateOpCodes); + addAllToArray(generateBindingUpdateOpCodes(text, textNodeIndex), updateOpCodes); } } } } } + allocExpando(viewData, i18nVarsCount); + // NOTE: local var needed to properly assert the type of `TI18n`. const tI18n: TI18n = { - vars: tView.blueprint.length - HEADER_OFFSET - expandoStartIndex, - expandoStartIndex, + vars: i18nVarsCount, create: createOpCodes, update: updateOpCodes, icus: icuExpressions.length ? icuExpressions : null, @@ -1381,18 +1384,15 @@ function icuStart( const tIcu: TIcu = { type: icuExpression.type, vars, - expandoStartIndex: expandoStartIndex + 1, childIcus, + childIcus, cases: icuExpression.cases, create: createCodes, remove: removeCodes, update: updateCodes }; tIcus.push(tIcu); - const lView = getLView(); - const worstCaseSize = Math.max(...vars); - for (let i = 0; i < worstCaseSize; i++) { - allocExpando(lView); - } + // Adding the maximum possible of vars needed (based on the cases with the most vars) + i18nVarsCount += Math.max(...vars); } /** diff --git a/packages/core/src/render3/instructions.ts b/packages/core/src/render3/instructions.ts index 0ae70f4391..28935daa5c 100644 --- a/packages/core/src/render3/instructions.ts +++ b/packages/core/src/render3/instructions.ts @@ -267,13 +267,24 @@ export function assignTViewNodeToLView( * i18nApply() or ComponentFactory.create), we need to adjust the blueprint for future * template passes. */ -export function allocExpando(view: LView) { +export function allocExpando(view: LView, numSlotsToAlloc: number) { const tView = view[TVIEW]; if (tView.firstTemplatePass) { - tView.expandoStartIndex++; - tView.blueprint.push(null); - tView.data.push(null); - view.push(null); + for (let i = 0; i < numSlotsToAlloc; i++) { + tView.blueprint.push(null); + tView.data.push(null); + view.push(null); + } + + // We should only increment the expando start index if there aren't already directives + // and injectors saved in the "expando" section + if (!tView.expandoInstructions) { + tView.expandoStartIndex += numSlotsToAlloc; + } else { + // Since we're adding the dynamic nodes into the expando section, we need to let the host + // bindings know that they should skip x slots + tView.expandoInstructions.push(numSlotsToAlloc); + } } } diff --git a/packages/core/src/render3/interfaces/i18n.ts b/packages/core/src/render3/interfaces/i18n.ts index 02da87ebc9..5fc0a7cf91 100644 --- a/packages/core/src/render3/interfaces/i18n.ts +++ b/packages/core/src/render3/interfaces/i18n.ts @@ -245,14 +245,6 @@ export interface TI18n { */ vars: number; - /** - * Index in EXPANDO where the i18n stores its DOM nodes. - * - * When the bindings are processed by the `i18nEnd` instruction it is necessary to know where the - * newly created DOM nodes will be inserted. - */ - expandoStartIndex: number; - /** * A set of OpCodes which will create the Text Nodes and ICU anchors for the translation blocks. * @@ -332,14 +324,6 @@ export interface TIcu { */ childIcus: number[][]; - /** - * Index in EXPANDO where the i18n stores its DOM nodes. - * - * When the bindings are processed by the `i18nEnd` instruction it is necessary to know where the - * newly created DOM nodes will be inserted. - */ - expandoStartIndex: number; - /** * A list of case values which the current ICU will try to match. * diff --git a/packages/core/test/render3/i18n_spec.ts b/packages/core/test/render3/i18n_spec.ts index 04be71d7b9..91da947040 100644 --- a/packages/core/test/render3/i18n_spec.ts +++ b/packages/core/test/render3/i18n_spec.ts @@ -8,14 +8,13 @@ import {noop} from '../../../compiler/src/render3/view/util'; import {Component as _Component} from '../../src/core'; -import {defineComponent} from '../../src/render3/definition'; +import {defineComponent, defineDirective} from '../../src/render3/definition'; import {getTranslationForTemplate, i18n, i18nApply, i18nAttributes, i18nEnd, i18nExp, i18nPostprocess, i18nStart} from '../../src/render3/i18n'; import {RenderFlags} from '../../src/render3/interfaces/definition'; +import {AttributeMarker} from '../../src/render3/interfaces/node'; import {getNativeByIndex} from '../../src/render3/util'; - import {NgIf} from './common_with_def'; - -import {element, elementEnd, elementStart, template, text, nextContext, bind, elementProperty, projectionDef, projection, elementContainerStart, elementContainerEnd} from '../../src/render3/instructions'; +import {allocHostVars, element, elementEnd, elementStart, template, text, nextContext, bind, elementProperty, projectionDef, projection, elementContainerStart, elementContainerEnd} from '../../src/render3/instructions'; import {COMMENT_MARKER, ELEMENT_MARKER, I18nMutateOpCode, I18nUpdateOpCode, I18nUpdateOpCodes, TI18n} from '../../src/render3/interfaces/i18n'; import {HEADER_OFFSET, LView, TVIEW} from '../../src/render3/interfaces/view'; import {ComponentFixture, TemplateFixture} from './render_util'; @@ -83,7 +82,6 @@ describe('Runtime i18n', () => { expect(opCodes).toEqual({ vars: 1, - expandoStartIndex: nbConsts, create: [ 'simple text', nbConsts, index << I18nMutateOpCode.SHIFT_PARENT | I18nMutateOpCode.AppendChild @@ -105,7 +103,6 @@ describe('Runtime i18n', () => { expect(opCodes).toEqual({ vars: 5, - expandoStartIndex: nbConsts, create: [ 'Hello ', nbConsts, @@ -142,7 +139,6 @@ describe('Runtime i18n', () => { expect(opCodes).toEqual({ vars: 1, - expandoStartIndex: nbConsts, create: ['', nbConsts, index << I18nMutateOpCode.SHIFT_PARENT | I18nMutateOpCode.AppendChild], update: [ @@ -164,7 +160,6 @@ describe('Runtime i18n', () => { expect(opCodes).toEqual({ vars: 1, - expandoStartIndex: nbConsts, create: ['', nbConsts, index << I18nMutateOpCode.SHIFT_PARENT | I18nMutateOpCode.AppendChild], update: [ @@ -198,7 +193,6 @@ describe('Runtime i18n', () => { expect(opCodes).toEqual({ vars: 2, - expandoStartIndex: nbConsts, create: [ '', nbConsts, @@ -229,7 +223,6 @@ describe('Runtime i18n', () => { expect(opCodes).toEqual({ vars: 2, - expandoStartIndex: nbConsts, create: [ spanElement << I18nMutateOpCode.SHIFT_REF | I18nMutateOpCode.Select, index << I18nMutateOpCode.SHIFT_PARENT | I18nMutateOpCode.AppendChild, @@ -257,7 +250,6 @@ describe('Runtime i18n', () => { expect(opCodes).toEqual({ vars: 1, - expandoStartIndex: nbConsts, create: [ bElement << I18nMutateOpCode.SHIFT_REF | I18nMutateOpCode.Select, index << I18nMutateOpCode.SHIFT_PARENT | I18nMutateOpCode.AppendChild, @@ -291,7 +283,6 @@ describe('Runtime i18n', () => { expect(opCodes).toEqual({ vars: 5, - expandoStartIndex: nbConsts, create: [ COMMENT_MARKER, 'ICU 1', icuCommentNodeIndex, index << I18nMutateOpCode.SHIFT_PARENT | I18nMutateOpCode.AppendChild @@ -308,7 +299,6 @@ describe('Runtime i18n', () => { icus: [{ type: 1, vars: [4, 3, 3], - expandoStartIndex: icuCommentNodeIndex + 1, childIcus: [[], [], []], cases: ['0', '1', 'other'], create: [ @@ -407,7 +397,6 @@ describe('Runtime i18n', () => { expect(opCodes).toEqual({ vars: 6, - expandoStartIndex: nbConsts, create: [ COMMENT_MARKER, 'ICU 1', icuCommentNodeIndex, index << I18nMutateOpCode.SHIFT_PARENT | I18nMutateOpCode.AppendChild @@ -425,7 +414,6 @@ describe('Runtime i18n', () => { { type: 0, vars: [1, 1, 1], - expandoStartIndex: lastTextNodeIndex + 1, childIcus: [[], [], []], cases: ['cat', 'dog', 'other'], create: [ @@ -455,7 +443,6 @@ describe('Runtime i18n', () => { { type: 1, vars: [1, 4], - expandoStartIndex: icuCommentNodeIndex + 1, childIcus: [[], [0]], cases: ['0', 'other'], create: [ @@ -1350,6 +1337,105 @@ describe('Runtime i18n', () => { expect(fixture.html).toEqual(`
trad 1
`); }); + it('should work with directives and host bindings', () => { + let directiveInstances: Directive[] = []; + + class Directive { + // @HostBinding('className') + klass = 'foo'; + + static ngDirectiveDef = defineDirective({ + type: Directive, + selectors: [['', 'dir', '']], + factory: () => { + const instance = new Directive(); + directiveInstances.push(instance); + return instance; + }, + hostBindings: (rf: RenderFlags, ctx: any, elementIndex: number) => { + if (rf & RenderFlags.Create) { + allocHostVars(1); + } + if (rf & RenderFlags.Update) { + elementProperty(elementIndex, 'className', bind(ctx.klass), null, true); + } + } + }); + } + + // Translated template: + //
+ // trad {�0�, plural, + // =0 {no emails!} + // =1 {one email} + // other {�0� emails} + // } + //
+ + const MSG_DIV_1 = `trad {�0�, plural, + =0 {no emails!} + =1 {one email} + other {�0� emails} + }`; + const MSG_DIV_1_ATTR_1 = ['title', `start �1� middle �0� end`]; + + class MyApp { + exp1 = 1; + exp2 = 2; + + static ngComponentDef = defineComponent({ + type: MyApp, + selectors: [['my-app']], + factory: () => new MyApp(), + consts: 6, + vars: 5, + directives: [Directive], + template: (rf: RenderFlags, ctx: MyApp) => { + if (rf & RenderFlags.Create) { + elementStart(0, 'div', [AttributeMarker.SelectOnly, 'dir']); + { + i18nAttributes(1, MSG_DIV_1_ATTR_1); + i18nStart(2, MSG_DIV_1); + { + elementStart(3, 'b', [AttributeMarker.SelectOnly, 'dir']); // Will be removed + { i18nAttributes(4, MSG_DIV_1_ATTR_1); } + elementEnd(); + } + i18nEnd(); + } + elementEnd(); + element(5, 'div', [AttributeMarker.SelectOnly, 'dir']); + } + if (rf & RenderFlags.Update) { + i18nExp(bind(ctx.exp1)); + i18nExp(bind(ctx.exp2)); + i18nApply(1); + i18nExp(bind(ctx.exp1)); + i18nApply(2); + i18nExp(bind(ctx.exp1)); + i18nExp(bind(ctx.exp2)); + i18nApply(4); + } + } + }); + } + + const fixture = new ComponentFixture(MyApp); + // the "test" attribute should not be reflected in the DOM as it is here only for directive + // matching purposes + expect(fixture.html) + .toEqual( + `
trad one email
`); + + directiveInstances.forEach(instance => instance.klass = 'bar'); + fixture.component.exp1 = 2; + fixture.component.exp2 = 3; + fixture.update(); + expect(fixture.html) + .toEqual( + `
trad 2 emails
`); + }); + describe('projection', () => { it('should project the translations', () => { @Component({selector: 'child', template: '

'})