From 0ddf0c489576f25f30bd6872c13209c6d5b14d0b Mon Sep 17 00:00:00 2001 From: Pete Bacon Darwin Date: Fri, 2 Aug 2019 12:42:04 +0100 Subject: [PATCH] fix(compiler): do not remove whitespace wrapping i18n expansions (#31962) Similar to interpolation, we do not want to completely remove whitespace nodes that are siblings of an expansion. For example, the following template ```html
items left {count, plural, =1 {item} other {items}}
``` was being collapsed to ```html
items left{count, plural, =1 {item} other {items}}
``` which results in the text looking like ``` items left4 ``` instead it should be collapsed to ```html
items left {count, plural, =1 {item} other {items}}
``` which results in the text looking like ``` items left 4 ``` --- **Analysis of the code and manual testing has shown that this does not cause the generated ids to change, so there is no breaking change here.** PR Close #31962 --- .../compliance/r3_view_compiler_i18n_spec.ts | 47 +++++++++++-------- .../src/ml_parser/html_whitespaces.ts | 28 +++++++++-- .../compiler/test/i18n/i18n_parser_spec.ts | 9 +++- .../test/ml_parser/html_whitespaces_spec.ts | 16 ++++++- .../compiler/test/ml_parser/lexer_spec.ts | 24 ++++++++++ packages/core/test/acceptance/i18n_spec.ts | 21 +++++---- 6 files changed, 110 insertions(+), 35 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 37b9876369..f0390bf501 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 @@ -2600,14 +2600,16 @@ describe('i18n support in the view compiler', () => { function MyComponent_div_2_Template(rf, ctx) { if (rf & 1) { $r3$.ɵɵelementStart(0, "div", $_c2$); - $r3$.ɵɵi18n(1, $I18N_3$); + i0.ɵɵtext(1, " "); + $r3$.ɵɵi18n(2, $I18N_3$); + i0.ɵɵtext(3, " "); $r3$.ɵɵelementEnd(); } if (rf & 2) { const $ctx_r0$ = $r3$.ɵɵnextContext(); - $r3$.ɵɵselect(1); + $r3$.ɵɵselect(2); $r3$.ɵɵi18nExp($ctx_r0$.age); - $r3$.ɵɵi18nApply(1); + $r3$.ɵɵi18nApply(2); } } const $_c3$ = ["title", "icu and text"]; @@ -2646,7 +2648,7 @@ describe('i18n support in the view compiler', () => { $r3$.ɵɵelementStart(0, "div"); $r3$.ɵɵi18n(1, $I18N_0$); $r3$.ɵɵelementEnd(); - $r3$.ɵɵtemplate(2, MyComponent_div_2_Template, 2, 1, "div", $_c0$); + $r3$.ɵɵtemplate(2, MyComponent_div_2_Template, 4, 1, "div", $_c0$); $r3$.ɵɵtemplate(3, MyComponent_div_3_Template, 4, 2, "div", $_c1$); } if (rf & 2) { @@ -2730,7 +2732,7 @@ describe('i18n support in the view compiler', () => { const $_c2$ = [1, "other"]; var $I18N_0$; if (ngI18nClosureMode) { - const $MSG_EXTERNAL_5791551881115084301$$APP_SPEC_TS_0$ = goog.getMsg("{$icu}{$startBoldText}Other content{$closeBoldText}{$startTagDiv}{$startItalicText}Another content{$closeItalicText}{$closeTagDiv}", { + const $MSG_EXTERNAL_5791551881115084301$$APP_SPEC_TS_0$ = goog.getMsg(" {$icu} {$startBoldText}Other content{$closeBoldText}{$startTagDiv}{$startItalicText}Another content{$closeItalicText}{$closeTagDiv}", { "startBoldText": "\uFFFD#2\uFFFD", "closeBoldText": "\uFFFD/#2\uFFFD", "startTagDiv": "\uFFFD#3\uFFFD", @@ -2742,7 +2744,7 @@ describe('i18n support in the view compiler', () => { $I18N_0$ = $MSG_EXTERNAL_5791551881115084301$$APP_SPEC_TS_0$; } else { - $I18N_0$ = $r3$.ɵɵi18nLocalize("{$icu}{$startBoldText}Other content{$closeBoldText}{$startTagDiv}{$startItalicText}Another content{$closeItalicText}{$closeTagDiv}", { + $I18N_0$ = $r3$.ɵɵi18nLocalize(" {$icu} {$startBoldText}Other content{$closeBoldText}{$startTagDiv}{$startItalicText}Another content{$closeItalicText}{$closeTagDiv}", { "startBoldText": "\uFFFD#2\uFFFD", "closeBoldText": "\uFFFD/#2\uFFFD", "startTagDiv": "\uFFFD#3\uFFFD", @@ -2848,14 +2850,14 @@ describe('i18n support in the view compiler', () => { }); var $I18N_0$; if (ngI18nClosureMode) { - const $MSG_EXTERNAL_2967249209167308918$$APP_SPEC_TS_0$ = goog.getMsg("{$icu}{$icu_1}", { + const $MSG_EXTERNAL_2967249209167308918$$APP_SPEC_TS_0$ = goog.getMsg(" {$icu} {$icu_1} ", { "icu": $I18N_1$, "icu_1": $I18N_2$ }); $I18N_0$ = $MSG_EXTERNAL_2967249209167308918$$APP_SPEC_TS_0$; } else { - $I18N_0$ = $r3$.ɵɵi18nLocalize("{$icu}{$icu_1}", { + $I18N_0$ = $r3$.ɵɵi18nLocalize(" {$icu} {$icu_1} ", { "icu": $I18N_1$, "icu_1": $I18N_2$ }); @@ -2930,7 +2932,7 @@ describe('i18n support in the view compiler', () => { }); var $I18N_0$; if (ngI18nClosureMode) { - const $MSG_APP_SPEC_TS_0$ = goog.getMsg("{$icu}{$startTagDiv}{$icu}{$closeTagDiv}{$startTagDiv_1}{$icu}{$closeTagDiv}", { + const $MSG_APP_SPEC_TS_0$ = goog.getMsg(" {$icu} {$startTagDiv} {$icu} {$closeTagDiv}{$startTagDiv_1} {$icu} {$closeTagDiv}", { "startTagDiv": "\uFFFD#2\uFFFD", "closeTagDiv": "[\uFFFD/#2\uFFFD|\uFFFD/#1:1\uFFFD\uFFFD/*3:1\uFFFD]", "startTagDiv_1": "\uFFFD*3:1\uFFFD\uFFFD#1:1\uFFFD", @@ -2939,7 +2941,7 @@ describe('i18n support in the view compiler', () => { $I18N_0$ = $MSG_APP_SPEC_TS_0$; } else { - $I18N_0$ = $r3$.ɵɵi18nLocalize("{$icu}{$startTagDiv}{$icu}{$closeTagDiv}{$startTagDiv_1}{$icu}{$closeTagDiv}", { + $I18N_0$ = $r3$.ɵɵi18nLocalize(" {$icu} {$startTagDiv} {$icu} {$closeTagDiv}{$startTagDiv_1} {$icu} {$closeTagDiv}", { "startTagDiv": "\uFFFD#2\uFFFD", "closeTagDiv": "[\uFFFD/#2\uFFFD|\uFFFD/#1:1\uFFFD\uFFFD/*3:1\uFFFD]", "startTagDiv_1": "\uFFFD*3:1\uFFFD\uFFFD#1:1\uFFFD", @@ -3000,19 +3002,26 @@ describe('i18n support in the view compiler', () => { `; const output = String.raw ` - var $I18N_0$; + var $I18N_1$; if (ngI18nClosureMode) { const $MSG_EXTERNAL_343563413083115114$$APP_SPEC_TS_0$ = goog.getMsg("{VAR_SELECT_1, select, male {male of age: {VAR_SELECT, select, 10 {ten} 20 {twenty} 30 {thirty} other {other}}} female {female} other {other}}"); - $I18N_0$ = $MSG_EXTERNAL_343563413083115114$$APP_SPEC_TS_0$; + $I18N_1$ = $MSG_EXTERNAL_343563413083115114$$APP_SPEC_TS_0$; } else { - $I18N_0$ = $r3$.ɵɵi18nLocalize("{VAR_SELECT_1, select, male {male of age: {VAR_SELECT, select, 10 {ten} 20 {twenty} 30 {thirty} other {other}}} female {female} other {other}}"); + $I18N_1$ = $r3$.ɵɵi18nLocalize("{VAR_SELECT_1, select, male {male of age: {VAR_SELECT, select, 10 {ten} 20 {twenty} 30 {thirty} other {other}}} female {female} other {other}}"); } - $I18N_0$ = $r3$.ɵɵi18nPostprocess($I18N_0$, { + $I18N_1$ = $r3$.ɵɵi18nPostprocess($I18N_1$, { "VAR_SELECT": "\uFFFD0\uFFFD", "VAR_SELECT_1": "\uFFFD1\uFFFD" }); - … + var $I18N_0$; + if (ngI18nClosureMode) { + const $MSG_EXTERNAL_3052001905251380936$$APP_SPEC_TS_3$ = goog.getMsg(" {$icu} ", { "icu": $I18N_1$ }); + $I18N_0$ = $MSG_EXTERNAL_3052001905251380936$$APP_SPEC_TS_3$; + } + else { + $I18N_0$ = i0.ɵɵi18nLocalize(" {$icu} ", { "icu": $I18N_1$ }); + } … consts: 2, vars: 2, template: function MyComponent_Template(rf, ctx) { @@ -3117,7 +3126,7 @@ describe('i18n support in the view compiler', () => { }); var $I18N_0$; if (ngI18nClosureMode) { - const $MSG_EXTERNAL_1194472282609532229$$APP_SPEC_TS_0$ = goog.getMsg("{$icu}{$startTagSpan}{$icu_1}{$closeTagSpan}", { + const $MSG_EXTERNAL_1194472282609532229$$APP_SPEC_TS_0$ = goog.getMsg(" {$icu} {$startTagSpan} {$icu_1} {$closeTagSpan}", { "startTagSpan": "\uFFFD*2:1\uFFFD\uFFFD#1:1\uFFFD", "closeTagSpan": "\uFFFD/#1:1\uFFFD\uFFFD/*2:1\uFFFD", "icu": $I18N_1$, @@ -3126,7 +3135,7 @@ describe('i18n support in the view compiler', () => { $I18N_0$ = $MSG_EXTERNAL_1194472282609532229$$APP_SPEC_TS_0$; } else { - $I18N_0$ = $r3$.ɵɵi18nLocalize("{$icu}{$startTagSpan}{$icu_1}{$closeTagSpan}", { + $I18N_0$ = $r3$.ɵɵi18nLocalize(" {$icu} {$startTagSpan} {$icu_1} {$closeTagSpan}", { "startTagSpan": "\uFFFD*2:1\uFFFD\uFFFD#1:1\uFFFD", "closeTagSpan": "\uFFFD/#1:1\uFFFD\uFFFD/*2:1\uFFFD", "icu": $I18N_1$, @@ -3207,7 +3216,7 @@ describe('i18n support in the view compiler', () => { }); var $I18N_0$; if (ngI18nClosureMode) { - const $MSG_EXTERNAL_7186042105600518133$$APP_SPEC_TS_0$ = goog.getMsg("{$icu}{$startTagSpan}{$icu_1}{$closeTagSpan}", { + const $MSG_EXTERNAL_7186042105600518133$$APP_SPEC_TS_0$ = goog.getMsg(" {$icu} {$startTagSpan} {$icu_1} {$closeTagSpan}", { "startTagSpan": "\uFFFD*2:1\uFFFD\uFFFD#1:1\uFFFD", "closeTagSpan": "\uFFFD/#1:1\uFFFD\uFFFD/*2:1\uFFFD", "icu": $I18N_1$, @@ -3216,7 +3225,7 @@ describe('i18n support in the view compiler', () => { $I18N_0$ = $MSG_EXTERNAL_7186042105600518133$$APP_SPEC_TS_0$; } else { - $I18N_0$ = $r3$.ɵɵi18nLocalize("{$icu}{$startTagSpan}{$icu_1}{$closeTagSpan}", { + $I18N_0$ = $r3$.ɵɵi18nLocalize(" {$icu} {$startTagSpan} {$icu_1} {$closeTagSpan}", { "startTagSpan": "\uFFFD*2:1\uFFFD\uFFFD#1:1\uFFFD", "closeTagSpan": "\uFFFD/#1:1\uFFFD\uFFFD/*2:1\uFFFD", "icu": $I18N_1$, diff --git a/packages/compiler/src/ml_parser/html_whitespaces.ts b/packages/compiler/src/ml_parser/html_whitespaces.ts index 2eefa7a472..b2431c15c2 100644 --- a/packages/compiler/src/ml_parser/html_whitespaces.ts +++ b/packages/compiler/src/ml_parser/html_whitespaces.ts @@ -60,18 +60,20 @@ export class WhitespaceVisitor implements html.Visitor { } return new html.Element( - element.name, element.attrs, html.visitAll(this, element.children), element.sourceSpan, - element.startSourceSpan, element.endSourceSpan, element.i18n); + element.name, element.attrs, visitAllWithSiblings(this, element.children), + element.sourceSpan, element.startSourceSpan, element.endSourceSpan, element.i18n); } visitAttribute(attribute: html.Attribute, context: any): any { return attribute.name !== PRESERVE_WS_ATTR_NAME ? attribute : null; } - visitText(text: html.Text, context: any): any { + visitText(text: html.Text, context: SiblingVisitorContext|null): any { const isNotBlank = text.value.match(NO_WS_REGEXP); + const hasExpansionSibling = context && + (context.prev instanceof html.Expansion || context.next instanceof html.Expansion); - if (isNotBlank) { + if (isNotBlank || hasExpansionSibling) { return new html.Text( replaceNgsp(text.value).replace(WS_REPLACE_REGEXP, ' '), text.sourceSpan, text.i18n); } @@ -91,3 +93,21 @@ export function removeWhitespaces(htmlAstWithErrors: ParseTreeResult): ParseTree html.visitAll(new WhitespaceVisitor(), htmlAstWithErrors.rootNodes), htmlAstWithErrors.errors); } + +interface SiblingVisitorContext { + prev: html.Node|undefined; + next: html.Node|undefined; +} + +function visitAllWithSiblings(visitor: WhitespaceVisitor, nodes: html.Node[]): any[] { + const result: any[] = []; + + nodes.forEach((ast, i) => { + const context: SiblingVisitorContext = {prev: nodes[i - 1], next: nodes[i + 1]}; + const astResult = ast.visit(visitor, context); + if (astResult) { + result.push(astResult); + } + }); + return result; +} diff --git a/packages/compiler/test/i18n/i18n_parser_spec.ts b/packages/compiler/test/i18n/i18n_parser_spec.ts index b71dc0a22f..729cf37bc0 100644 --- a/packages/compiler/test/i18n/i18n_parser_spec.ts +++ b/packages/compiler/test/i18n/i18n_parser_spec.ts @@ -40,7 +40,7 @@ import {DEFAULT_INTERPOLATION_CONFIG} from '@angular/compiler/src/ml_parser/inte it('should not create a message for plain elements', () => { expect(_humanizeMessages('
')).toEqual([]); }); - it('should suppoprt void elements', () => { + it('should support void elements', () => { expect(_humanizeMessages('


')).toEqual([ [ [ @@ -173,6 +173,13 @@ import {DEFAULT_INTERPOLATION_CONFIG} from '@angular/compiler/src/ml_parser/inte ]); }); + it('should extract as ICU + ph when wrapped in whitespace in an element', () => { + expect(_humanizeMessages('
{count, plural, =0 {zero}}
')).toEqual([ + [[' ', '{count, plural, =0 {[zero]}}', ' '], 'm', 'd'], + [['{count, plural, =0 {[zero]}}'], '', ''], + ]); + }); + it('should extract as ICU when single child of a block', () => { expect(_humanizeMessages('{count, plural, =0 {zero}}')) .toEqual([ diff --git a/packages/compiler/test/ml_parser/html_whitespaces_spec.ts b/packages/compiler/test/ml_parser/html_whitespaces_spec.ts index 8da5898353..3b5c1ddc4e 100644 --- a/packages/compiler/test/ml_parser/html_whitespaces_spec.ts +++ b/packages/compiler/test/ml_parser/html_whitespaces_spec.ts @@ -9,14 +9,15 @@ import * as html from '../../src/ml_parser/ast'; import {HtmlParser} from '../../src/ml_parser/html_parser'; import {PRESERVE_WS_ATTR_NAME, removeWhitespaces} from '../../src/ml_parser/html_whitespaces'; +import {TokenizeOptions} from '../../src/ml_parser/lexer'; import {humanizeDom} from './ast_spec_utils'; { describe('removeWhitespaces', () => { - function parseAndRemoveWS(template: string): any[] { - return humanizeDom(removeWhitespaces(new HtmlParser().parse(template, 'TestComp'))); + function parseAndRemoveWS(template: string, options?: TokenizeOptions): any[] { + return humanizeDom(removeWhitespaces(new HtmlParser().parse(template, 'TestComp', options))); } it('should remove blank text nodes', () => { @@ -97,6 +98,17 @@ import {humanizeDom} from './ast_spec_utils'; ]); }); + it('should preserve whitespaces around ICU expansions', () => { + expect(parseAndRemoveWS(` {a, b, =4 {c}} `, {tokenizeExpansionForms: true})) + .toEqual([ + [html.Element, 'span', 0], + [html.Text, ' ', 1], + [html.Expansion, 'a', 'b', 1], + [html.ExpansionCase, '=4', 2], + [html.Text, ' ', 1], + ]); + }); + it('should preserve whitespaces inside 
 elements', () => {
       expect(parseAndRemoveWS(`
foo\nbar
`)).toEqual([
         [html.Element, 'pre', 0],
diff --git a/packages/compiler/test/ml_parser/lexer_spec.ts b/packages/compiler/test/ml_parser/lexer_spec.ts
index 62327c0787..b89734e4f8 100644
--- a/packages/compiler/test/ml_parser/lexer_spec.ts
+++ b/packages/compiler/test/ml_parser/lexer_spec.ts
@@ -798,6 +798,30 @@ import {ParseLocation, ParseSourceFile, ParseSourceSpan} from '../../src/parse_u
             ]);
       });
 
+      it('should parse an expansion form with whitespace surrounding it', () => {
+        expect(tokenizeAndHumanizeParts(
+                   '
 {a, b, =4 {c}} 
', {tokenizeExpansionForms: true}))
+            .toEqual([
+              [lex.TokenType.TAG_OPEN_START, '', 'div'],
+              [lex.TokenType.TAG_OPEN_END],
+              [lex.TokenType.TAG_OPEN_START, '', 'span'],
+              [lex.TokenType.TAG_OPEN_END],
+              [lex.TokenType.TEXT, ' '],
+              [lex.TokenType.EXPANSION_FORM_START],
+              [lex.TokenType.RAW_TEXT, 'a'],
+              [lex.TokenType.RAW_TEXT, 'b'],
+              [lex.TokenType.EXPANSION_CASE_VALUE, '=4'],
+              [lex.TokenType.EXPANSION_CASE_EXP_START],
+              [lex.TokenType.TEXT, 'c'],
+              [lex.TokenType.EXPANSION_CASE_EXP_END],
+              [lex.TokenType.EXPANSION_FORM_END],
+              [lex.TokenType.TEXT, ' '],
+              [lex.TokenType.TAG_CLOSE, '', 'span'],
+              [lex.TokenType.TAG_CLOSE, '', 'div'],
+              [lex.TokenType.EOF],
+            ]);
+      });
+
       it('should parse an expansion forms with elements in it', () => {
         expect(tokenizeAndHumanizeParts(
                    '{one.two, three, =4 {four a}}', {tokenizeExpansionForms: true}))
diff --git a/packages/core/test/acceptance/i18n_spec.ts b/packages/core/test/acceptance/i18n_spec.ts
index 1736bf56d9..c77b6a8dcb 100644
--- a/packages/core/test/acceptance/i18n_spec.ts
+++ b/packages/core/test/acceptance/i18n_spec.ts
@@ -606,10 +606,11 @@ onlyInIvy('Ivy i18n logic').describe('runtime i18n', () => {
               '{VAR_SELECT, select, 10 {dix} 20 {vingt} other {autre}}'
         }
       });
-      const fixture = initWithTemplate(AppComp, `
-        
-          {count, select, 10 {ten} 20 {twenty} other {other}}
-        
+      const fixture = initWithTemplate(
+          AppComp, `
+        ` +
+              `{count, select, 10 {ten} 20 {twenty} other {other}}` +
+              `
       `);
 
       const element = fixture.nativeElement;
@@ -931,8 +932,8 @@ onlyInIvy('Ivy i18n logic').describe('runtime i18n', () => {
             select,
             A {A }
             B {B }
-            other {other - {{ typeC // i18n(ph="PH WITH SPACES") }}} 
-          }  
+            other {other - {{ typeC // i18n(ph="PH WITH SPACES") }}}
+          }
           
 
           

@@ -1112,14 +1113,15 @@ onlyInIvy('Ivy i18n logic').describe('runtime i18n', () => {
             'début {$interpolation_1} milieu {$interpolation} fin',
         '{VAR_PLURAL, plural, =0 {no {START_BOLD_TEXT}emails{CLOSE_BOLD_TEXT}!} =1 {one {START_ITALIC_TEXT}email{CLOSE_ITALIC_TEXT}} other {{INTERPOLATION} emails}}':
             '{VAR_PLURAL, plural, =0 {aucun {START_BOLD_TEXT}email{CLOSE_BOLD_TEXT}!} =1 {un {START_ITALIC_TEXT}email{CLOSE_ITALIC_TEXT}} other {{INTERPOLATION} emails}}',
-        ' trad: {$icu}': ' traduction: {$icu}'
+        ' trad: {$icu} ': ' traduction: {$icu} '
       }
     });
     const fixture = TestBed.createComponent(MyApp);
     fixture.detectChanges();
     expect(fixture.nativeElement.innerHTML)
         .toEqual(
-            `
 traduction: un email
`);
+            `
 traduction: un email ` +
+            `
`);
 
     directiveInstances.forEach(instance => instance.klass = 'bar');
     fixture.componentRef.instance.exp1 = 2;
@@ -1127,7 +1129,8 @@ onlyInIvy('Ivy i18n logic').describe('runtime i18n', () => {
     fixture.detectChanges();
     expect(fixture.nativeElement.innerHTML)
         .toEqual(
-            `
 traduction: 2 emails
`);
+            `
 traduction: 2 emails ` +
+            `
`);
   });
 
   it('should handle i18n attribute with directive inputs', () => {