From c013dd4ca6a288e8dd742e42b98ebcc6f967e819 Mon Sep 17 00:00:00 2001 From: Andrew Kushnir Date: Tue, 11 Feb 2020 14:44:13 -0800 Subject: [PATCH] fix(core): better handing of ICUs outside of i18n blocks (#35347) Currently the logic that handles ICUs located outside of i18n blocks may throw exceptions at runtime. The problem is caused by the fact that we store incorrect TNode index for previous TNode (index that includes HEADER_OFFSET) and do not store a flag whether this TNode is a parent or a sibling node. As a result, the logic that assembles the final output uses incorrect TNodes and in some cases throws exceptions (when incompatible structure is extracted from tView.data due to the incorrect index). This commit adjusts the index and captures whether TNode is a parent to make sure underlying logic manipulates correct TNode. PR Close #35347 --- packages/core/src/render3/i18n.ts | 31 +++++-- packages/core/src/render3/state.ts | 4 +- packages/core/test/acceptance/i18n_spec.ts | 97 ++++++++++++++++++++++ packages/core/test/render3/i18n_spec.ts | 5 +- 4 files changed, 125 insertions(+), 12 deletions(-) diff --git a/packages/core/src/render3/i18n.ts b/packages/core/src/render3/i18n.ts index a679190614..205433cf13 100644 --- a/packages/core/src/render3/i18n.ts +++ b/packages/core/src/render3/i18n.ts @@ -390,11 +390,19 @@ function i18nStartFirstPass( parentIndexStack[parentIndexPointer] = parentIndex; const createOpCodes: I18nMutateOpCodes = []; // If the previous node wasn't the direct parent then we have a translation without top level - // element and we need to keep a reference of the previous element if there is one + // element and we need to keep a reference of the previous element if there is one. We should also + // keep track whether an element was a parent node or not, so that the logic that consumes + // the generated `I18nMutateOpCode`s can leverage this information to properly set TNode state + // (whether it's a parent or sibling). if (index > 0 && previousOrParentTNode !== parentTNode) { + let previousTNodeIndex = previousOrParentTNode.index - HEADER_OFFSET; + // If current TNode is a sibling node, encode it using a negative index. This information is + // required when the `Select` action is processed (see the `readCreateOpCodes` function). + if (!getIsParent()) { + previousTNodeIndex = ~previousTNodeIndex; + } // Create an OpCode to select the previous TNode - createOpCodes.push( - previousOrParentTNode.index << I18nMutateOpCode.SHIFT_REF | I18nMutateOpCode.Select); + createOpCodes.push(previousTNodeIndex << I18nMutateOpCode.SHIFT_REF | I18nMutateOpCode.Select); } const updateOpCodes: I18nUpdateOpCodes = []; const icuExpressions: TIcu[] = []; @@ -414,12 +422,16 @@ function i18nStartFirstPass( } } else { const phIndex = parseInt(value.substr(1), 10); - // The value represents a placeholder that we move to the designated index + const isElement = value.charAt(0) === TagType.ELEMENT; + // The value represents a placeholder that we move to the designated index. + // Note: positive indicies indicate that a TNode with a given index should also be marked as + // parent while executing `Select` instruction. createOpCodes.push( - phIndex << I18nMutateOpCode.SHIFT_REF | I18nMutateOpCode.Select, + (isElement ? phIndex : ~phIndex) << I18nMutateOpCode.SHIFT_REF | + I18nMutateOpCode.Select, parentIndex << I18nMutateOpCode.SHIFT_PARENT | I18nMutateOpCode.AppendChild); - if (value.charAt(0) === TagType.ELEMENT) { + if (isElement) { parentIndexStack[++parentIndexPointer] = parentIndex = phIndex; } } @@ -757,12 +769,15 @@ function readCreateOpCodes( appendI18nNode(tView, currentTNode !, destinationTNode, previousTNode, lView); break; case I18nMutateOpCode.Select: - const nodeIndex = opCode >>> I18nMutateOpCode.SHIFT_REF; + // Negative indicies indicate that a given TNode is a sibling node, not a parent node + // (see `i18nStartFirstPass` for additional information). + const isParent = opCode >= 0; + const nodeIndex = (isParent ? opCode : ~opCode) >>> I18nMutateOpCode.SHIFT_REF; visitedNodes.push(nodeIndex); previousTNode = currentTNode; currentTNode = getTNode(tView, nodeIndex); if (currentTNode) { - setPreviousOrParentTNode(currentTNode, currentTNode.type === TNodeType.Element); + setPreviousOrParentTNode(currentTNode, isParent); } break; case I18nMutateOpCode.ElementEnd: diff --git a/packages/core/src/render3/state.ts b/packages/core/src/render3/state.ts index 8c1bfa479d..432f355173 100644 --- a/packages/core/src/render3/state.ts +++ b/packages/core/src/render3/state.ts @@ -270,9 +270,9 @@ export function getPreviousOrParentTNode(): TNode { return instructionState.lFrame.previousOrParentTNode; } -export function setPreviousOrParentTNode(tNode: TNode, _isParent: boolean) { +export function setPreviousOrParentTNode(tNode: TNode, isParent: boolean) { instructionState.lFrame.previousOrParentTNode = tNode; - instructionState.lFrame.isParent = _isParent; + instructionState.lFrame.isParent = isParent; } export function getIsParent(): boolean { diff --git a/packages/core/test/acceptance/i18n_spec.ts b/packages/core/test/acceptance/i18n_spec.ts index 8b5c83e323..7e52d460be 100644 --- a/packages/core/test/acceptance/i18n_spec.ts +++ b/packages/core/test/acceptance/i18n_spec.ts @@ -530,6 +530,36 @@ onlyInIvy('Ivy i18n logic').describe('runtime i18n', () => { expect(element).toHaveText('autre'); }); + it('with no root node and text surrounding ICU', () => { + loadTranslations({ + [computeMsgId('{VAR_SELECT, select, 10 {Ten} 20 {Twenty} other {Other}}')]: + '{VAR_SELECT, select, 10 {Dix} 20 {Vingt} other {Autre}}' + }); + const fixture = initWithTemplate(AppComp, ` + ICU start --> + {count, select, 10 {Ten} 20 {Twenty} other {Other}} + <-- ICU end + `); + + const element = fixture.nativeElement; + expect(element.textContent).toContain('ICU start --> Autre <-- ICU end'); + }); + + it('with no root node and text and DOM nodes surrounding ICU', () => { + loadTranslations({ + [computeMsgId('{VAR_SELECT, select, 10 {Ten} 20 {Twenty} other {Other}}')]: + '{VAR_SELECT, select, 10 {Dix} 20 {Vingt} other {Autre}}' + }); + const fixture = initWithTemplate(AppComp, ` + ICU start --> + {count, select, 10 {Ten} 20 {Twenty} other {Other}} + <-- ICU end + `); + + const element = fixture.nativeElement; + expect(element.textContent).toContain('ICU start --> Autre <-- ICU end'); + }); + it('with no i18n tag', () => { loadTranslations({ [computeMsgId('{VAR_SELECT, select, 10 {ten} 20 {twenty} other {other}}')]: @@ -2100,6 +2130,73 @@ onlyInIvy('Ivy i18n logic').describe('runtime i18n', () => { "ng-reflect-ng-if": "false" }-->`); }); + + describe('ngTemplateOutlet', () => { + it('should work with i18n content that includes elements', () => { + loadTranslations({ + [computeMsgId('{$START_TAG_SPAN}A{$CLOSE_TAG_SPAN} B ')]: + '{$START_TAG_SPAN}a{$CLOSE_TAG_SPAN} b', + }); + + const fixture = initWithTemplate(AppComp, ` + + + A B + + `); + expect(fixture.nativeElement.textContent).toContain('a b'); + }); + + it('should work with i18n content that includes other templates (*ngIf)', () => { + loadTranslations({ + [computeMsgId('{$START_TAG_SPAN}A{$CLOSE_TAG_SPAN} B ')]: + '{$START_TAG_SPAN}a{$CLOSE_TAG_SPAN} b', + }); + + const fixture = initWithTemplate(AppComp, ` + + + A B + + `); + expect(fixture.nativeElement.textContent).toContain('a b'); + }); + + it('should work with i18n content that includes projection', () => { + loadTranslations({ + [computeMsgId('{$START_TAG_NG_CONTENT}{$CLOSE_TAG_NG_CONTENT} B ')]: + '{$START_TAG_NG_CONTENT}{$CLOSE_TAG_NG_CONTENT} b', + }); + + @Component({ + selector: 'projector', + template: ` + + + B + + ` + }) + class Projector { + } + + @Component({ + selector: 'app', + template: ` + a + ` + }) + class AppComponent { + } + + TestBed.configureTestingModule({declarations: [AppComponent, Projector]}); + + const fixture = TestBed.createComponent(AppComponent); + fixture.detectChanges(); + + expect(fixture.nativeElement.textContent).toContain('a b'); + }); + }); }); function initWithTemplate(compType: Type, template: string) { diff --git a/packages/core/test/render3/i18n_spec.ts b/packages/core/test/render3/i18n_spec.ts index 0da67146a4..0535ce454b 100644 --- a/packages/core/test/render3/i18n_spec.ts +++ b/packages/core/test/render3/i18n_spec.ts @@ -194,6 +194,7 @@ describe('Runtime i18n', () => { let nbConsts = 3; let index = 1; const firstTextNode = 3; + const rootTemplate = 2; let opCodes = getOpCodes(() => { ɵɵi18nStart(index, MSG_DIV); }, null, nbConsts, index); expect(opCodes).toEqual({ @@ -202,7 +203,7 @@ describe('Runtime i18n', () => { '', nbConsts, index << I18nMutateOpCode.SHIFT_PARENT | I18nMutateOpCode.AppendChild, - 2 << I18nMutateOpCode.SHIFT_REF | I18nMutateOpCode.Select, + ~rootTemplate << I18nMutateOpCode.SHIFT_REF | I18nMutateOpCode.Select, index << I18nMutateOpCode.SHIFT_PARENT | I18nMutateOpCode.AppendChild, '!', nbConsts + 1, @@ -234,7 +235,7 @@ describe('Runtime i18n', () => { 'before', nbConsts, spanElement << I18nMutateOpCode.SHIFT_PARENT | I18nMutateOpCode.AppendChild, - bElementSubTemplate << I18nMutateOpCode.SHIFT_REF | I18nMutateOpCode.Select, + ~bElementSubTemplate << I18nMutateOpCode.SHIFT_REF | I18nMutateOpCode.Select, spanElement << I18nMutateOpCode.SHIFT_PARENT | I18nMutateOpCode.AppendChild, 'after', nbConsts + 1,