fix(compiler): skipping leading whitespace should not break placeholder source-spans (#39589)

petebacondarwin · mhevery · commit 2b684b7e5bd2 · 2020-11-06T09:09:48.000-08:00
Tokenized text node may have leading whitespace skipped from their source-span. But the source-span is used to compute where there are interpolated blocks, resulting in placeholder nodes whose source-spans are offset by the amount of skipped characters. This fix uses the `fullStart` location of text source-spans for computing the source-span of placeholders, so that they are accurate. Fixes #39195 PR Close #39589
diff --git a/packages/compiler/src/i18n/i18n_parser.ts b/packages/compiler/src/i18n/i18n_parser.ts
@@ -191,7 +191,7 @@ class _I18nVisitor implements html.Visitor {
 
 function getOffsetSourceSpan(
     sourceSpan: ParseSourceSpan, {start, end}: {start: number, end: number}): ParseSourceSpan {
-  return new ParseSourceSpan(sourceSpan.start.moveBy(start), sourceSpan.start.moveBy(end));
+  return new ParseSourceSpan(sourceSpan.fullStart.moveBy(start), sourceSpan.fullStart.moveBy(end));
 }
 
 const _CUSTOM_PH_EXP =
diff --git a/packages/compiler/src/render3/view/template.ts b/packages/compiler/src/render3/view/template.ts
@@ -52,7 +52,7 @@ const NG_PROJECT_AS_ATTR_NAME = 'ngProjectAs';
 const GLOBAL_TARGET_RESOLVERS = new Map<string, o.ExternalReference>(
     [['window', R3.resolveWindow], ['document', R3.resolveDocument], ['body', R3.resolveBody]]);
 
-const LEADING_TRIVIA_CHARS = [' ', '\n', '\r', '\t'];
+export const LEADING_TRIVIA_CHARS = [' ', '\n', '\r', '\t'];
 
 //  if (rf & flags) { .. }
 export function renderFlagCheckIfStmt(
diff --git a/packages/compiler/test/render3/view/i18n_spec.ts b/packages/compiler/test/render3/view/i18n_spec.ts
@@ -18,6 +18,7 @@ import {serializeIcuNode} from '../../../src/render3/view/i18n/icu_serializer';
 import {serializeI18nMessageForLocalize} from '../../../src/render3/view/i18n/localize_utils';
 import {I18nMeta, parseI18nMeta} from '../../../src/render3/view/i18n/meta';
 import {formatI18nPlaceholderName} from '../../../src/render3/view/i18n/util';
+import {LEADING_TRIVIA_CHARS} from '../../../src/render3/view/template';
 
 import {parseR3 as parse} from './util';
 
@@ -355,7 +356,7 @@ describe('serializeI18nMessageForGetMsg', () => {
 
 describe('serializeI18nMessageForLocalize', () => {
   const serialize = (input: string) => {
-    const tree = parse(`<div i18n>${input}</div>`);
+    const tree = parse(`<div i18n>${input}</div>`, {leadingTriviaChars: LEADING_TRIVIA_CHARS});
     const root = tree.nodes[0] as t.Element;
     return serializeI18nMessageForLocalize(root.i18n as i18n.Message);
   };
@@ -446,7 +447,7 @@ describe('serializeI18nMessageForLocalize', () => {
        expect(messageParts[3].text).toEqual('');
        expect(messageParts[3].sourceSpan.toString()).toEqual('');
        expect(messageParts[4].text).toEqual(' D');
-       expect(messageParts[4].sourceSpan.toString()).toEqual(' D');
+       expect(messageParts[4].sourceSpan.toString()).toEqual('D');
 
        expect(placeHolders[0].text).toEqual('START_TAG_SPAN');
        expect(placeHolders[0].sourceSpan.toString()).toEqual('<span>');
@@ -478,6 +479,26 @@ describe('serializeI18nMessageForLocalize', () => {
        expect(humanizeSourceSpan(placeHolders[2].sourceSpan)).toEqual('"</b>" (22-26)');
      });
 
+  it('should create the correct placeholder source-spans when there is skipped leading whitespace',
+     () => {
+       const {messageParts, placeHolders} = serialize('<b>   {{value}}</b>');
+       expect(messageParts[0].text).toEqual('');
+       expect(humanizeSourceSpan(messageParts[0].sourceSpan)).toEqual('"" (10-10)');
+       expect(messageParts[1].text).toEqual('   ');
+       expect(humanizeSourceSpan(messageParts[1].sourceSpan)).toEqual('"   " (13-16)');
+       expect(messageParts[2].text).toEqual('');
+       expect(humanizeSourceSpan(messageParts[2].sourceSpan)).toEqual('"" (25-25)');
+       expect(messageParts[3].text).toEqual('');
+       expect(humanizeSourceSpan(messageParts[3].sourceSpan)).toEqual('"" (29-29)');
+
+       expect(placeHolders[0].text).toEqual('START_BOLD_TEXT');
+       expect(humanizeSourceSpan(placeHolders[0].sourceSpan)).toEqual('"<b>   " (10-16)');
+       expect(placeHolders[1].text).toEqual('INTERPOLATION');
+       expect(humanizeSourceSpan(placeHolders[1].sourceSpan)).toEqual('"{{value}}" (16-25)');
+       expect(placeHolders[2].text).toEqual('CLOSE_BOLD_TEXT');
+       expect(humanizeSourceSpan(placeHolders[2].sourceSpan)).toEqual('"</b>" (25-29)');
+     });
+
   it('should serialize simple ICU for `$localize()`', () => {
     expect(serialize('{age, plural, 10 {ten} other {other}}')).toEqual({
       messageParts: [literal('{VAR_PLURAL, plural, 10 {ten} other {other}}')],
diff --git a/packages/compiler/test/render3/view/util.ts b/packages/compiler/test/render3/view/util.ts
@@ -78,11 +78,13 @@ export function toStringExpression(expr: e.AST): string {
 
 // Parse an html string to IVY specific info
 export function parseR3(
-    input: string, options: {preserveWhitespaces?: boolean} = {}): Render3ParseResult {
+    input: string, options: {preserveWhitespaces?: boolean, leadingTriviaChars?: string[]} = {}):
+    Render3ParseResult {
   const htmlParser = new HtmlParser();
 
-  const parseResult =
-      htmlParser.parse(input, 'path:://to/template', {tokenizeExpansionForms: true});
+  const parseResult = htmlParser.parse(
+      input, 'path:://to/template',
+      {tokenizeExpansionForms: true, leadingTriviaChars: options.leadingTriviaChars});
 
   if (parseResult.errors.length > 0) {
     const msg = parseResult.errors.map(e => e.toString()).join('\n');

Original file line number	Diff line number	Diff line change
`@@ -191,7 +191,7 @@ class _I18nVisitor implements html.Visitor {`
`191`	`191`
`192`	`192`	`function getOffsetSourceSpan(`
`193`	`193`	`sourceSpan: ParseSourceSpan, {start, end}: {start: number, end: number}): ParseSourceSpan {`
`194`		`- return new ParseSourceSpan(sourceSpan.start.moveBy(start), sourceSpan.start.moveBy(end));`
	`194`	`+ return new ParseSourceSpan(sourceSpan.fullStart.moveBy(start), sourceSpan.fullStart.moveBy(end));`
`195`	`195`	`}`
`196`	`196`
`197`	`197`	`const _CUSTOM_PH_EXP =`