From 68a9a01a648f9332b34f272146546faaca934476 Mon Sep 17 00:00:00 2001 From: Pete Bacon Darwin Date: Sun, 16 Aug 2020 16:11:30 +0100 Subject: [PATCH] fix(localize): parse all parts of a translation with nested HTML (#38452) Previously nested container placeholders (i.e. HTML elements) were not being fully parsed from translation files. This resulted in bad translation of messages that contain these placeholders. Note that this causes the canonical message ID to change for such messages. Currently all messages generated from templates use "legacy" message ids that are not affected by this change, so this fix should not be seen as a breaking change. Fixes #38422 PR Close #38452 --- .../message_serializer.ts | 26 ++-------- .../xliff1_translation_parser_spec.ts | 40 ++++++++++++++ .../xliff2_translation_parser_spec.ts | 52 +++++++++++++++++-- .../xtb_translation_parser_spec.ts | 32 ++++++++++++ 4 files changed, 124 insertions(+), 26 deletions(-) diff --git a/packages/localize/src/tools/src/translate/translation_files/message_serialization/message_serializer.ts b/packages/localize/src/tools/src/translate/translation_files/message_serialization/message_serializer.ts index ea9cdefacc..995e6fbebd 100644 --- a/packages/localize/src/tools/src/translate/translation_files/message_serialization/message_serializer.ts +++ b/packages/localize/src/tools/src/translate/translation_files/message_serialization/message_serializer.ts @@ -75,29 +75,9 @@ export class MessageSerializer extends BaseVisitor { } visitContainedNodes(nodes: Node[]): void { - const length = nodes.length; - let index = 0; - while (index < length) { - if (!this.isPlaceholderContainer(nodes[index])) { - const startOfContainedNodes = index; - while (index < length - 1) { - index++; - if (this.isPlaceholderContainer(nodes[index])) { - break; - } - } - if (index - startOfContainedNodes > 1) { - // Only create a container if there are two or more contained Nodes in a row - this.renderer.startContainer(); - visitAll(this, nodes.slice(startOfContainedNodes, index - 1)); - this.renderer.closeContainer(); - } - } - if (index < length) { - nodes[index].visit(this, undefined); - } - index++; - } + this.renderer.startContainer(); + visitAll(this, nodes); + this.renderer.closeContainer(); } visitPlaceholder(name: string, body: string|undefined): void { diff --git a/packages/localize/src/tools/test/translate/translation_files/translation_parsers/xliff1_translation_parser_spec.ts b/packages/localize/src/tools/test/translate/translation_files/translation_parsers/xliff1_translation_parser_spec.ts index de347c5de8..c69b839433 100644 --- a/packages/localize/src/tools/test/translate/translation_files/translation_parsers/xliff1_translation_parser_spec.ts +++ b/packages/localize/src/tools/test/translate/translation_files/translation_parsers/xliff1_translation_parser_spec.ts @@ -212,6 +212,46 @@ describe('Xliff1TranslationParser', () => { ['INTERPOLATION', 'START_BOLD_TEXT', 'CLOSE_BOLD_TEXT'])); }); + it('should extract nested placeholder containers (i.e. nested HTML elements)', () => { + /** + * Source HTML: + * + * ``` + *
+ * translatable element with placeholders {{ interpolation}} + *
+ * ``` + */ + const XLIFF = [ + ``, + ` `, + ` `, + ` `, + ` translatable element with placeholders `, + ` tnemele elbatalsnart sredlohecalp htiw`, + ` `, + ` file.ts`, + ` 3`, + ` `, + ` `, + ` `, + ` `, + ``, + ].join('\n'); + const result = doParse('/some/file.xlf', XLIFF); + expect(result.translations[ɵcomputeMsgId( + 'translatable {$START_TAG_SPAN}element {$START_BOLD_TEXT}with placeholders' + + '{$CLOSE_BOLD_TEXT}{$CLOSE_TAG_SPAN} {$INTERPOLATION}')]) + .toEqual(ɵmakeParsedTranslation( + ['', '', ' tnemele', ' elbatalsnart ', 'sredlohecalp htiw', ''], [ + 'START_TAG_SPAN', + 'INTERPOLATION', + 'CLOSE_TAG_SPAN', + 'START_BOLD_TEXT', + 'CLOSE_BOLD_TEXT', + ])); + }); + it('should extract translations with placeholders containing hyphens', () => { /** * Source HTML: diff --git a/packages/localize/src/tools/test/translate/translation_files/translation_parsers/xliff2_translation_parser_spec.ts b/packages/localize/src/tools/test/translate/translation_files/translation_parsers/xliff2_translation_parser_spec.ts index 82ef7484ad..9bcae2900b 100644 --- a/packages/localize/src/tools/test/translate/translation_files/translation_parsers/xliff2_translation_parser_spec.ts +++ b/packages/localize/src/tools/test/translate/translation_files/translation_parsers/xliff2_translation_parser_spec.ts @@ -172,13 +172,13 @@ describe( * Source HTML: * * ``` - *
translatable element >with placeholders {{ interpolation}}
+ *
translatable element with placeholders {{ interpolation}}
* ``` */ const XLIFF = [ ``, ` `, - ` `, + ` `, ` `, ` file.ts:3`, ` `, @@ -193,12 +193,58 @@ describe( const result = doParse('/some/file.xlf', XLIFF); expect( result.translations[ɵcomputeMsgId( - 'translatable element {$START_BOLD_TEXT}with placeholders{$LOSE_BOLD_TEXT} {$INTERPOLATION}')]) + 'translatable element {$START_BOLD_TEXT}with placeholders{$CLOSE_BOLD_TEXT} {$INTERPOLATION}')]) .toEqual(ɵmakeParsedTranslation( ['', ' tnemele elbatalsnart ', 'sredlohecalp htiw', ''], ['INTERPOLATION', 'START_BOLD_TEXT', 'CLOSE_BOLD_TEXT'])); }); + it('should extract nested placeholder containers (i.e. nested HTML elements)', () => { + /** + * Source HTML: + * + * ``` + *
+ * translatable element with placeholders {{ interpolation}} + *
+ * ``` + */ + const XLIFF = [ + ``, + ` `, + ` `, + ` `, + ` file.ts:3`, + ` `, + ` `, + ` translatable element with placeholders` + + ` `, + ` tnemele` + + ` elbatalsnart sredlohecalp htiw`, + ` `, + ` `, + ` `, + ``, + ].join('\n'); + const result = doParse('/some/file.xlf', XLIFF); + expect( + result.translations[ɵcomputeMsgId( + 'translatable {$START_TAG_SPAN}element {$START_BOLD_TEXT}with placeholders' + + '{$CLOSE_BOLD_TEXT}{$CLOSE_TAG_SPAN} {$INTERPOLATION}')]) + .toEqual(ɵmakeParsedTranslation( + ['', '', ' tnemele', ' elbatalsnart ', 'sredlohecalp htiw', ''], [ + 'START_TAG_SPAN', + 'INTERPOLATION', + 'CLOSE_TAG_SPAN', + 'START_BOLD_TEXT', + 'CLOSE_BOLD_TEXT', + ])); + }); + it('should extract translations with simple ICU expressions', () => { /** * Source HTML: diff --git a/packages/localize/src/tools/test/translate/translation_files/translation_parsers/xtb_translation_parser_spec.ts b/packages/localize/src/tools/test/translate/translation_files/translation_parsers/xtb_translation_parser_spec.ts index 77a568044d..20ce004695 100644 --- a/packages/localize/src/tools/test/translate/translation_files/translation_parsers/xtb_translation_parser_spec.ts +++ b/packages/localize/src/tools/test/translate/translation_files/translation_parsers/xtb_translation_parser_spec.ts @@ -140,6 +140,38 @@ describe('XtbTranslationParser', () => { ɵmakeParsedTranslation(['', 'rab', ''], ['START_PARAGRAPH', 'CLOSE_PARAGRAPH'])); }); + it('should extract nested placeholder containers (i.e. nested HTML elements)', () => { + /** + * Source HTML: + * + * ``` + *
+ * translatable element with placeholders {{ interpolation}} + *
+ * ``` + */ + const XLIFF = [ + ``, + ``, + ` ` + + ` tnemele elbatalsnart sredlohecalp htiw` + + ``, + ``, + ].join('\n'); + const result = doParse('/some/file.xtb', XLIFF); + expect(result.translations[ɵcomputeMsgId( + 'translatable {$START_TAG_SPAN}element {$START_BOLD_TEXT}with placeholders' + + '{$CLOSE_BOLD_TEXT}{$CLOSE_TAG_SPAN} {$INTERPOLATION}')]) + .toEqual(ɵmakeParsedTranslation( + ['', '', ' tnemele', ' elbatalsnart ', 'sredlohecalp htiw', ''], [ + 'START_TAG_SPAN', + 'INTERPOLATION', + 'CLOSE_TAG_SPAN', + 'START_BOLD_TEXT', + 'CLOSE_BOLD_TEXT', + ])); + }); + it('should extract translations with simple ICU expressions', () => { const XTB = [ ``,