From 54ef63b0f4180426384ec98685339ebb0112e88c Mon Sep 17 00:00:00 2001
From: Kara Erickson <karakara@google.com>
Date: Mon, 22 Jul 2019 20:55:07 -0700
Subject: [PATCH] fix(ivy): support ICU expressions inserted in
 ngTemplateOutlets inside ngFors (#31789)

This commit fixes a bug where ICU expressions inserted into ngTemplateOutlets
that are inside ngFor blocks would throw an error. We were assuming in view
insertion code that text nodes would always exist by the time a view\`s
creation block had executed. This is not true for text nodes created dynamically
by ICUs because this happens in the update block (in `i18nApply`).

This change ensures such dynamically created nodes are skipped when encountered
too early (as they will be attached later by i18n code anyway).

PR Close #31789
---
 .../core/src/render3/node_manipulation.ts     | 56 ++++++++++---------
 packages/core/test/acceptance/i18n_spec.ts    | 32 +++++++++++
 2 files changed, 63 insertions(+), 25 deletions(-)

diff --git a/packages/core/src/render3/node_manipulation.ts b/packages/core/src/render3/node_manipulation.ts
index d1f710784c..05fe6294ef 100644
--- a/packages/core/src/render3/node_manipulation.ts
+++ b/packages/core/src/render3/node_manipulation.ts
@@ -70,32 +70,38 @@ const enum WalkTNodeTreeAction {
 function executeActionOnElementOrContainer(
     action: WalkTNodeTreeAction, renderer: Renderer3, parent: RElement | null,
     lNodeToHandle: RNode | LContainer | LView, beforeNode?: RNode | null) {
-  ngDevMode && assertDefined(lNodeToHandle, '\'lNodeToHandle\' is undefined');
-  let lContainer: LContainer|undefined;
-  let isComponent = false;
-  // We are expecting an RNode, but in the case of a component or LContainer the `RNode` is wrapped
-  // in an array which needs to be unwrapped. We need to know if it is a component and if
-  // it has LContainer so that we can process all of those cases appropriately.
-  if (isLContainer(lNodeToHandle)) {
-    lContainer = lNodeToHandle;
-  } else if (isLView(lNodeToHandle)) {
-    isComponent = true;
-    ngDevMode && assertDefined(lNodeToHandle[HOST], 'HOST must be defined for a component LView');
-    lNodeToHandle = lNodeToHandle[HOST] !;
-  }
-  const rNode: RNode = unwrapRNode(lNodeToHandle);
-  ngDevMode && assertDomNode(rNode);
+  // If this slot was allocated for a text node dynamically created by i18n, the text node itself
+  // won't be created until i18nApply() in the update block, so this node should be skipped.
+  // For more info, see "ICU expressions should work inside an ngTemplateOutlet inside an ngFor"
+  // in `i18n_spec.ts`.
+  if (lNodeToHandle != null) {
+    let lContainer: LContainer|undefined;
+    let isComponent = false;
+    // We are expecting an RNode, but in the case of a component or LContainer the `RNode` is
+    // wrapped
+    // in an array which needs to be unwrapped. We need to know if it is a component and if
+    // it has LContainer so that we can process all of those cases appropriately.
+    if (isLContainer(lNodeToHandle)) {
+      lContainer = lNodeToHandle;
+    } else if (isLView(lNodeToHandle)) {
+      isComponent = true;
+      ngDevMode && assertDefined(lNodeToHandle[HOST], 'HOST must be defined for a component LView');
+      lNodeToHandle = lNodeToHandle[HOST] !;
+    }
+    const rNode: RNode = unwrapRNode(lNodeToHandle);
+    ngDevMode && assertDomNode(rNode);
 
-  if (action === WalkTNodeTreeAction.Insert) {
-    nativeInsertBefore(renderer, parent !, rNode, beforeNode || null);
-  } else if (action === WalkTNodeTreeAction.Detach) {
-    nativeRemoveNode(renderer, rNode, isComponent);
-  } else if (action === WalkTNodeTreeAction.Destroy) {
-    ngDevMode && ngDevMode.rendererDestroyNode++;
-    (renderer as ProceduralRenderer3).destroyNode !(rNode);
-  }
-  if (lContainer != null) {
-    executeActionOnContainer(renderer, action, lContainer, parent, beforeNode);
+    if (action === WalkTNodeTreeAction.Insert) {
+      nativeInsertBefore(renderer, parent !, rNode, beforeNode || null);
+    } else if (action === WalkTNodeTreeAction.Detach) {
+      nativeRemoveNode(renderer, rNode, isComponent);
+    } else if (action === WalkTNodeTreeAction.Destroy) {
+      ngDevMode && ngDevMode.rendererDestroyNode++;
+      (renderer as ProceduralRenderer3).destroyNode !(rNode);
+    }
+    if (lContainer != null) {
+      executeActionOnContainer(renderer, action, lContainer, parent, beforeNode);
+    }
   }
 }
 
diff --git a/packages/core/test/acceptance/i18n_spec.ts b/packages/core/test/acceptance/i18n_spec.ts
index 1988f9d778..1736bf56d9 100644
--- a/packages/core/test/acceptance/i18n_spec.ts
+++ b/packages/core/test/acceptance/i18n_spec.ts
@@ -921,6 +921,38 @@ onlyInIvy('Ivy i18n logic').describe('runtime i18n', () => {
       expect(fixture.debugElement.nativeElement.innerHTML).not.toContain('A - Type A');
       expect(fixture.debugElement.nativeElement.innerHTML).toContain('other - Type C');
     });
+
+    it('should work inside an ngTemplateOutlet inside an ngFor', () => {
+      @Component({
+        selector: 'app',
+        template: `
+          <ng-template #myTemp i18n let-type>{
+            type,
+            select,
+            A {A }
+            B {B }
+            other {other - {{ typeC // i18n(ph="PH WITH SPACES") }}} 
+          }  
+          </ng-template>
+
+          <div *ngFor="let type of types">
+            <ng-container *ngTemplateOutlet="myTemp; context: {$implicit: type}">
+            </ng-container>
+          </div>
+        `
+      })
+      class AppComponent {
+        types = ['A', 'B', 'C'];
+      }
+
+      TestBed.configureTestingModule({declarations: [AppComponent]});
+
+      const fixture = TestBed.createComponent(AppComponent);
+      fixture.detectChanges();
+
+      expect(fixture.debugElement.nativeElement.innerHTML).toContain('A');
+      expect(fixture.debugElement.nativeElement.innerHTML).toContain('B');
+    });
   });
 
   describe('should support attributes', () => {