Skip to content

Commit c61ea1d

Browse files
ocombeAndrewKushnir
ocombe
authored and
AndrewKushnir
committed
fix(ivy): support for multiple ICU expressions in the same i18n block (angular#28083)
There were two issues with multiple ICU expressions in the same i18n block: - the regexp that was used to parse the text wasn't able to handle multiple ICU expressions, I've replaced it with parsing the text and searching for brackets (which is what we ended up doing in the end anyway) - we allocate node indexes for nodes generated by the ICU expressions which increases the expando value, but we would create the nodes for those cases during the update phase. In the mean time we would create some nodes during the creation phase (comment nodes for ICU expressions, text nodes, ...) with an auto increment index. This means that any node created after an ICU expression would get the following index value, but the ICU case nodes expected to use the same index as well... There was a mismatch between the auto generated index, and the expected index which was causing problems when we needed to select those nodes for updates later on. To fix it, I've added the expected node index to the list of mutate codes that we generate, and we do not use an auto increment value anymore. FW-905 #resolve PR Close angular#28083
1 parent 47665c9 commit c61ea1d

File tree

3 files changed

+246
-73
lines changed

3 files changed

+246
-73
lines changed

‎packages/core/src/render3/i18n.ts‎

Lines changed: 22 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -395,21 +395,19 @@ function i18nStartFirstPass(
395395
}
396396
} else {
397397
// Even indexes are text (including bindings & ICU expressions)
398-
const parts = value.split(ICU_REGEXP);
398+
const parts = extractParts(value);
399399
for (let j = 0; j < parts.length; j++) {
400-
value = parts[j];
401-
402400
if (j & 1) {
403401
// Odd indexes are ICU expressions
404402
// Create the comment node that will anchor the ICU expression
405403
allocExpando(viewData);
406404
const icuNodeIndex = tView.blueprint.length - 1 - HEADER_OFFSET;
407405
createOpCodes.push(
408-
COMMENT_MARKER, ngDevMode ? `ICU ${icuNodeIndex}` : '',
406+
COMMENT_MARKER, ngDevMode ? `ICU ${icuNodeIndex}` : '', icuNodeIndex,
409407
parentIndex << I18nMutateOpCode.SHIFT_PARENT | I18nMutateOpCode.AppendChild);
410408

411409
// Update codes for the ICU expression
412-
const icuExpression = parseICUBlock(value.substr(1, value.length - 2));
410+
const icuExpression = parts[j] as IcuExpression;
413411
const mask = getBindingMask(icuExpression);
414412
icuStart(icuExpressions, icuExpression, icuNodeIndex, icuNodeIndex);
415413
// Since this is recursive, the last TIcu that was pushed is the one we want
@@ -422,19 +420,21 @@ function i18nStartFirstPass(
422420
mask, // mask of all the bindings of this ICU expression
423421
2, // skip 2 opCodes if not changed
424422
icuNodeIndex << I18nUpdateOpCode.SHIFT_REF | I18nUpdateOpCode.IcuUpdate, tIcuIndex);
425-
} else if (value !== '') {
423+
} else if (parts[j] !== '') {
424+
const text = parts[j] as string;
426425
// Even indexes are text (including bindings)
427-
const hasBinding = value.match(BINDING_REGEXP);
426+
const hasBinding = text.match(BINDING_REGEXP);
428427
// Create text nodes
429428
allocExpando(viewData);
429+
const textNodeIndex = tView.blueprint.length - 1 - HEADER_OFFSET;
430430
createOpCodes.push(
431431
// If there is a binding, the value will be set during update
432-
hasBinding ? '' : value,
432+
hasBinding ? '' : text, textNodeIndex,
433433
parentIndex << I18nMutateOpCode.SHIFT_PARENT | I18nMutateOpCode.AppendChild);
434434

435435
if (hasBinding) {
436436
addAllToArray(
437-
generateBindingUpdateOpCodes(value, tView.blueprint.length - 1 - HEADER_OFFSET),
437+
generateBindingUpdateOpCodes(text, tView.blueprint.length - 1 - HEADER_OFFSET),
438438
updateOpCodes);
439439
}
440440
}
@@ -580,8 +580,7 @@ function i18nEndFirstPass(tView: TView) {
580580

581581
// The last placeholder that was added before `i18nEnd`
582582
const previousOrParentTNode = getPreviousOrParentTNode();
583-
const visitedPlaceholders =
584-
readCreateOpCodes(rootIndex, tI18n.create, tI18n.expandoStartIndex, viewData);
583+
const visitedPlaceholders = readCreateOpCodes(rootIndex, tI18n.create, tI18n.icus, viewData);
585584

586585
// Remove deleted placeholders
587586
// The last placeholder that was added before `i18nEnd` is `previousOrParentTNode`
@@ -593,7 +592,7 @@ function i18nEndFirstPass(tView: TView) {
593592
}
594593

595594
function readCreateOpCodes(
596-
index: number, createOpCodes: I18nMutateOpCodes, expandoStartIndex: number,
595+
index: number, createOpCodes: I18nMutateOpCodes, icus: TIcu[] | null,
597596
viewData: LView): number[] {
598597
const renderer = getLView()[RENDERER];
599598
let currentTNode: TNode|null = null;
@@ -603,10 +602,10 @@ function readCreateOpCodes(
603602
const opCode = createOpCodes[i];
604603
if (typeof opCode == 'string') {
605604
const textRNode = createTextNode(opCode, renderer);
605+
const textNodeIndex = createOpCodes[++i] as number;
606606
ngDevMode && ngDevMode.rendererCreateTextNode++;
607607
previousTNode = currentTNode;
608-
currentTNode =
609-
createNodeAtIndex(expandoStartIndex++, TNodeType.Element, textRNode, null, null);
608+
currentTNode = createNodeAtIndex(textNodeIndex, TNodeType.Element, textRNode, null, null);
610609
setIsParent(false);
611610
} else if (typeof opCode == 'number') {
612611
switch (opCode & I18nMutateOpCode.MASK_OPCODE) {
@@ -658,29 +657,31 @@ function readCreateOpCodes(
658657
switch (opCode) {
659658
case COMMENT_MARKER:
660659
const commentValue = createOpCodes[++i] as string;
660+
const commentNodeIndex = createOpCodes[++i] as number;
661661
ngDevMode && assertEqual(
662662
typeof commentValue, 'string',
663663
`Expected "${commentValue}" to be a comment node value`);
664664
const commentRNode = renderer.createComment(commentValue);
665665
ngDevMode && ngDevMode.rendererCreateComment++;
666666
previousTNode = currentTNode;
667-
currentTNode = createNodeAtIndex(
668-
expandoStartIndex++, TNodeType.IcuContainer, commentRNode, null, null);
667+
currentTNode =
668+
createNodeAtIndex(commentNodeIndex, TNodeType.IcuContainer, commentRNode, null, null);
669669
attachPatchData(commentRNode, viewData);
670670
(currentTNode as TIcuContainerNode).activeCaseIndex = null;
671671
// We will add the case nodes later, during the update phase
672672
setIsParent(false);
673673
break;
674674
case ELEMENT_MARKER:
675675
const tagNameValue = createOpCodes[++i] as string;
676+
const elementNodeIndex = createOpCodes[++i] as number;
676677
ngDevMode && assertEqual(
677678
typeof tagNameValue, 'string',
678679
`Expected "${tagNameValue}" to be an element node tag name`);
679680
const elementRNode = renderer.createElement(tagNameValue);
680681
ngDevMode && ngDevMode.rendererCreateElement++;
681682
previousTNode = currentTNode;
682683
currentTNode = createNodeAtIndex(
683-
expandoStartIndex++, TNodeType.Element, elementRNode, tagNameValue, null);
684+
elementNodeIndex, TNodeType.Element, elementRNode, tagNameValue, null);
684685
break;
685686
default:
686687
throw new Error(`Unable to determine the type of mutate operation for "${opCode}"`);
@@ -759,7 +760,7 @@ function readUpdateOpCodes(
759760
icuTNode.activeCaseIndex = caseIndex !== -1 ? caseIndex : null;
760761

761762
// Add the nodes for the new case
762-
readCreateOpCodes(-1, tIcu.create[caseIndex], tIcu.expandoStartIndex, viewData);
763+
readCreateOpCodes(-1, tIcu.create[caseIndex], icus, viewData);
763764
caseCreated = true;
764765
break;
765766
case I18nUpdateOpCode.IcuUpdate:
@@ -1400,7 +1401,7 @@ function parseNodes(
14001401
icuCase.vars--;
14011402
} else {
14021403
icuCase.create.push(
1403-
ELEMENT_MARKER, tagName,
1404+
ELEMENT_MARKER, tagName, newIndex,
14041405
parentIndex << I18nMutateOpCode.SHIFT_PARENT | I18nMutateOpCode.AppendChild);
14051406
const elAttrs = element.attributes;
14061407
for (let i = 0; i < elAttrs.length; i++) {
@@ -1446,7 +1447,7 @@ function parseNodes(
14461447
const value = currentNode.textContent || '';
14471448
const hasBinding = value.match(BINDING_REGEXP);
14481449
icuCase.create.push(
1449-
hasBinding ? '' : value,
1450+
hasBinding ? '' : value, newIndex,
14501451
parentIndex << I18nMutateOpCode.SHIFT_PARENT | I18nMutateOpCode.AppendChild);
14511452
icuCase.remove.push(newIndex << I18nMutateOpCode.SHIFT_REF | I18nMutateOpCode.Remove);
14521453
if (hasBinding) {
@@ -1461,7 +1462,7 @@ function parseNodes(
14611462
const newLocal = ngDevMode ? `nested ICU ${nestedIcuIndex}` : '';
14621463
// Create the comment node that will anchor the ICU expression
14631464
icuCase.create.push(
1464-
COMMENT_MARKER, newLocal,
1465+
COMMENT_MARKER, newLocal, newIndex,
14651466
parentIndex << I18nMutateOpCode.SHIFT_PARENT | I18nMutateOpCode.AppendChild);
14661467
const nestedIcu = nestedIcus[nestedIcuIndex];
14671468
nestedIcusToCreate.push([nestedIcu, newIndex]);

‎packages/core/test/i18n_integration_spec.ts‎

Lines changed: 17 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,8 @@ const TRANSLATIONS: any = {
5050
'{$startTagNgTemplate}{$startTagSpan}Bonjour{$closeTagSpan}{$closeTagNgTemplate}{$startTagNgContainer}{$startTagSpan_1}Bonjour{$closeTagSpan}{$closeTagNgContainer}',
5151
'{VAR_SELECT, select, 10 {ten} 20 {twenty} other {other}}':
5252
'{VAR_SELECT, select, 10 {dix} 20 {vingt} other {autres}}',
53+
'{VAR_SELECT, select, 1 {one} 2 {two} other {more than two}}':
54+
'{VAR_SELECT, select, 1 {un} 2 {deux} other {plus que deux}}',
5355
'{VAR_SELECT, select, 10 {10 - {$startBoldText}ten{$closeBoldText}} 20 {20 - {$startItalicText}twenty{$closeItalicText}} other {{$startTagDiv}{$startUnderlinedText}other{$closeUnderlinedText}{$closeTagDiv}}}':
5456
'{VAR_SELECT, select, 10 {10 - {$startBoldText}dix{$closeBoldText}} 20 {20 - {$startItalicText}vingt{$closeItalicText}} other {{$startTagDiv}{$startUnderlinedText}autres{$closeUnderlinedText}{$closeTagDiv}}}',
5557
'{VAR_SELECT_2, select, 10 {ten - {VAR_SELECT, select, 1 {one} 2 {two} other {more than two}}} 20 {twenty - {VAR_SELECT_1, select, 1 {one} 2 {two} other {more than two}}} other {other}}':
@@ -384,23 +386,21 @@ onlyInIvy('Ivy i18n logic').describe('i18n', function() {
384386
expect(italicTags[0].innerHTML).toBe('vingt');
385387
});
386388

387-
fixmeIvy('FW-905: Multiple ICUs in one i18n block are not processed')
388-
.it('should handle multiple ICUs in one block', () => {
389-
const template = `
389+
it('should handle multiple ICUs in one block', () => {
390+
const template = `
390391
<div i18n>
391392
{age, select, 10 {ten} 20 {twenty} other {other}} -
392393
{count, select, 1 {one} 2 {two} other {more than two}}
393394
</div>
394395
`;
395-
const fixture = getFixtureWithOverrides({template});
396+
const fixture = getFixtureWithOverrides({template});
396397

397-
const element = fixture.nativeElement.firstChild;
398-
expect(element).toHaveText('vingt - deux');
399-
});
398+
const element = fixture.nativeElement.firstChild;
399+
expect(element).toHaveText('vingt - deux');
400+
});
400401

401-
fixmeIvy('FW-906: Multiple ICUs wrapped in HTML tags in one i18n block throw an error')
402-
.it('should handle multiple ICUs in one i18n block wrapped in HTML elements', () => {
403-
const template = `
402+
it('should handle multiple ICUs in one i18n block wrapped in HTML elements', () => {
403+
const template = `
404404
<div i18n>
405405
<span>
406406
{age, select, 10 {ten} 20 {twenty} other {other}}
@@ -410,14 +410,14 @@ onlyInIvy('Ivy i18n logic').describe('i18n', function() {
410410
</span>
411411
</div>
412412
`;
413-
const fixture = getFixtureWithOverrides({template});
413+
const fixture = getFixtureWithOverrides({template});
414414

415-
const element = fixture.nativeElement.firstChild;
416-
const spans = element.getElementsByTagName('span');
417-
expect(spans.length).toBe(2);
418-
expect(spans[0].innerHTML).toBe('vingt');
419-
expect(spans[1].innerHTML).toBe('deux');
420-
});
415+
const element = fixture.nativeElement.firstChild;
416+
const spans = element.getElementsByTagName('span');
417+
expect(spans.length).toBe(2);
418+
expect(spans[0]).toHaveText('vingt');
419+
expect(spans[1]).toHaveText('deux');
420+
});
421421

422422
it('should handle ICUs inside a template in i18n block', () => {
423423
const template = `

0 commit comments

Comments
 (0)