Skip to content

Commit

Permalink
fix(compiler): do not remove whitespace wrapping i18n expansions (#31962
Browse files Browse the repository at this point in the history 
)

Similar to interpolation, we do not want to completely remove whitespace
nodes that are siblings of an expansion.

For example, the following template

```html
<div>
  <strong>items left<strong> {count, plural, =1 {item} other {items}}
</div>
```

was being collapsed to

```html
<div><strong>items left<strong>{count, plural, =1 {item} other {items}}</div>
```

which results in the text looking like

```
items left4
```

instead it should be collapsed to

```html
<div><strong>items left<strong> {count, plural, =1 {item} other {items}}</div>
```

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
  • Loading branch information
@petebacondarwin @kara
petebacondarwin authored and kara committed Aug 9, 2019
1 parent 90ddee7 commit 6ec91dd
Show file tree
Hide file tree
Showing 6 changed files with 110 additions and 35 deletions.
47 changes: 28 additions & 19 deletions packages/compiler-cli/test/compliance/r3_view_compiler_i18n_spec.ts
View file
Expand Up @@ -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"];
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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",
Expand All @@ -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",
Expand Down Expand Up @@ -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$
});
Expand Down Expand Up @@ -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",
Expand All @@ -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",
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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$,
Expand All @@ -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$,
Expand Down Expand Up @@ -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$,
Expand All @@ -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$,
Expand Down
28 changes: 24 additions & 4 deletions packages/compiler/src/ml_parser/html_whitespaces.ts
View file
Expand Up @@ -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);
}
Expand All @@ -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;
}
9 changes: 8 additions & 1 deletion packages/compiler/test/i18n/i18n_parser_spec.ts
View file
Expand Up @@ -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('<div></div>')).toEqual([]); });

it('should suppoprt void elements', () => {
it('should support void elements', () => {
expect(_humanizeMessages('<div i18n="m|d"><p><br></p></div>')).toEqual([
[
[
Expand Down Expand Up @@ -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('<div i18n="m|d"> {count, plural, =0 {zero}} </div>')).toEqual([
[[' ', '<ph icu name="ICU">{count, plural, =0 {[zero]}}</ph>', ' '], 'm', 'd'],
[['{count, plural, =0 {[zero]}}'], '', ''],
]);
});

it('should extract as ICU when single child of a block', () => {
expect(_humanizeMessages('<!-- i18n:m|d -->{count, plural, =0 {zero}}<!-- /i18n -->'))
.toEqual([
Expand Down
16 changes: 14 additions & 2 deletions packages/compiler/test/ml_parser/html_whitespaces_spec.ts
View file
Expand Up @@ -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', () => {
Expand Down Expand Up @@ -97,6 +98,17 @@ import {humanizeDom} from './ast_spec_utils';
]);
});

it('should preserve whitespaces around ICU expansions', () => {
expect(parseAndRemoveWS(`<span> {a, b, =4 {c}} </span>`, {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 <pre> elements', () => {
expect(parseAndRemoveWS(`<pre><strong>foo</strong>\n<strong>bar</strong></pre>`)).toEqual([
[html.Element, 'pre', 0],
Expand Down
24 changes: 24 additions & 0 deletions packages/compiler/test/ml_parser/lexer_spec.ts
View file
Expand Up @@ -798,6 +798,30 @@ import {ParseLocation, ParseSourceFile, ParseSourceSpan} from '../../src/parse_u
]);
});

it('should parse an expansion form with whitespace surrounding it', () => {
expect(tokenizeAndHumanizeParts(
'<div><span> {a, b, =4 {c}} </span></div>', {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 <b>a</b>}}', {tokenizeExpansionForms: true}))
Expand Down
21 changes: 12 additions & 9 deletions packages/core/test/acceptance/i18n_spec.ts
View file
Expand Up @@ -606,10 +606,11 @@ onlyInIvy('Ivy i18n logic').describe('runtime i18n', () => {
'{VAR_SELECT, select, 10 {dix} 20 {vingt} other {autre}}'
}
});
const fixture = initWithTemplate(AppComp, `
<ng-template i18n tplRef>
{count, select, 10 {ten} 20 {twenty} other {other}}
</ng-template>
const fixture = initWithTemplate(
AppComp, `
<ng-template i18n tplRef>` +
`{count, select, 10 {ten} 20 {twenty} other {other}}` +
`</ng-template>
`);

const element = fixture.nativeElement;
Expand Down Expand Up @@ -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") }}}
}
</ng-template>
<div *ngFor="let type of types">
Expand Down Expand Up @@ -1112,22 +1113,24 @@ 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(
`<div test="" title="début 2 milieu 1 fin" class="foo"> traduction: un <i>email</i><!--ICU 20--></div><div test="" class="foo"></div>`);
`<div test="" title="début 2 milieu 1 fin" class="foo"> traduction: un <i>email</i><!--ICU 20--> ` +
`</div><div test="" class="foo"></div>`);

directiveInstances.forEach(instance => instance.klass = 'bar');
fixture.componentRef.instance.exp1 = 2;
fixture.componentRef.instance.exp2 = 3;
fixture.detectChanges();
expect(fixture.nativeElement.innerHTML)
.toEqual(
`<div test="" title="début 3 milieu 2 fin" class="bar"> traduction: 2 emails<!--ICU 20--></div><div test="" class="bar"></div>`);
`<div test="" title="début 3 milieu 2 fin" class="bar"> traduction: 2 emails<!--ICU 20--> ` +
`</div><div test="" class="bar"></div>`);
});

it('should handle i18n attribute with directive inputs', () => {
Expand Down

0 comments on commit 6ec91dd

Please sign in to comment.