fix(core): projected views should be dirty checked when the declaring component is dirty checked. (#16592)

Previously a projected view was only dirty checked when the
component in which it was inserted was dirty checked.

This fix changes the behavior so that a view is also dirty checked if
the declaring component is dirty checked.

Note: This does not change the order of change detection,
only the fact whether a projected view is dirty checked or not.

Fixes #14321

packages/core/src/view/types.ts

@@ -141,37 +141,38 @@ export const enum NodeFlags {
   None = 0,
   TypeElement = 1 << 0,
   TypeText = 1 << 1,
+  ProjectedTemplate = 1 << 2,
   CatRenderNode = TypeElement | TypeText,
-  TypeNgContent = 1 << 2,
+  TypeNgContent = 1 << 3,
-  TypePipe = 1 << 3,
+  TypePipe = 1 << 4,
-  TypePureArray = 1 << 4,
+  TypePureArray = 1 << 5,
-  TypePureObject = 1 << 5,
+  TypePureObject = 1 << 6,
-  TypePurePipe = 1 << 6,
+  TypePurePipe = 1 << 7,
   CatPureExpression = TypePureArray | TypePureObject | TypePurePipe,
-  TypeValueProvider = 1 << 7,
+  TypeValueProvider = 1 << 8,
-  TypeClassProvider = 1 << 8,
+  TypeClassProvider = 1 << 9,
-  TypeFactoryProvider = 1 << 9,
+  TypeFactoryProvider = 1 << 10,
-  TypeUseExistingProvider = 1 << 10,
+  TypeUseExistingProvider = 1 << 11,
-  LazyProvider = 1 << 11,
+  LazyProvider = 1 << 12,
-  PrivateProvider = 1 << 12,
+  PrivateProvider = 1 << 13,
-  TypeDirective = 1 << 13,
+  TypeDirective = 1 << 14,
-  Component = 1 << 14,
+  Component = 1 << 15,
   CatProvider = TypeValueProvider | TypeClassProvider | TypeFactoryProvider |
       TypeUseExistingProvider | TypeDirective,
-  OnInit = 1 << 15,
+  OnInit = 1 << 16,
-  OnDestroy = 1 << 16,
+  OnDestroy = 1 << 17,
-  DoCheck = 1 << 17,
+  DoCheck = 1 << 18,
-  OnChanges = 1 << 18,
+  OnChanges = 1 << 19,
-  AfterContentInit = 1 << 19,
+  AfterContentInit = 1 << 20,
-  AfterContentChecked = 1 << 20,
+  AfterContentChecked = 1 << 21,
-  AfterViewInit = 1 << 21,
+  AfterViewInit = 1 << 22,
-  AfterViewChecked = 1 << 22,
+  AfterViewChecked = 1 << 23,
-  EmbeddedViews = 1 << 23,
+  EmbeddedViews = 1 << 24,
-  ComponentView = 1 << 24,
+  ComponentView = 1 << 25,
-  TypeContentQuery = 1 << 25,
+  TypeContentQuery = 1 << 26,
-  TypeViewQuery = 1 << 26,
+  TypeViewQuery = 1 << 27,
-  StaticQuery = 1 << 27,
+  StaticQuery = 1 << 28,
-  DynamicQuery = 1 << 28,
+  DynamicQuery = 1 << 29,
   CatQuery = TypeContentQuery | TypeViewQuery,
 
   // mutually exclusive values...
@@ -328,7 +329,9 @@ export const enum ViewState {
   FirstCheck = 1 << 1,
   Attached = 1 << 2,
   ChecksEnabled = 1 << 3,
-  Destroyed = 1 << 4,
+  CheckProjectedView = 1 << 4,
+  CheckProjectedViews = 1 << 5,
+  Destroyed = 1 << 6,
 
   CatDetectChanges = Attached | ChecksEnabled,
   CatInit = BeforeFirstCheck | CatDetectChanges

packages/core/src/view/util.ts

@@ -117,6 +117,14 @@ export function markParentViewsForCheck(view: ViewData) {
   }
 }
 
+export function markParentViewsForCheckProjectedViews(view: ViewData, endView: ViewData) {
+  let currView: ViewData|null = view;
+  while (currView && currView !== endView) {
+    currView.state |= ViewState.CheckProjectedViews;
+    currView = currView.viewContainerParent || currView.parent;
+  }
+}
+
 export function dispatchEvent(
     view: ViewData, nodeIndex: number, eventName: string, event: any): boolean {
   const nodeDef = view.def.nodes[nodeIndex];

packages/core/src/view/view.ts

@@ -17,7 +17,8 @@ import {checkAndUpdateQuery, createQuery} from './query';
 import {createTemplateData, createViewContainerData} from './refs';
 import {checkAndUpdateTextDynamic, checkAndUpdateTextInline, createText} from './text';
 import {ArgumentType, CheckType, ElementData, NodeData, NodeDef, NodeFlags, ProviderData, RootData, Services, ViewData, ViewDefinition, ViewFlags, ViewHandleEventFn, ViewState, ViewUpdateFn, asElementData, asQueryList, asTextData} from './types';
-import {NOOP, checkBindingNoChanges, isComponentView, resolveViewDefinition} from './util';
+import {NOOP, checkBindingNoChanges, isComponentView, markParentViewsForCheckProjectedViews, resolveViewDefinition} from './util';
+import {detachProjectedView} from './view_attach';
 
 export function viewDef(
     flags: ViewFlags, nodes: NodeDef[], updateDirectives?: ViewUpdateFn,
@@ -314,12 +315,14 @@ function createViewNodes(view: ViewData) {
 }
 
 export function checkNoChangesView(view: ViewData) {
+  markProjectedViewsForCheck(view);
   Services.updateDirectives(view, CheckType.CheckNoChanges);
   execEmbeddedViewsAction(view, ViewAction.CheckNoChanges);
   Services.updateRenderer(view, CheckType.CheckNoChanges);
   execComponentViewsAction(view, ViewAction.CheckNoChanges);
   // Note: We don't check queries for changes as we didn't do this in v2.x.
   // TODO(tbosch): investigate if we can enable the check again in v5.x with a nicer error message.
+  view.state &= ~(ViewState.CheckProjectedViews | ViewState.CheckProjectedView);
 }
 
 export function checkAndUpdateView(view: ViewData) {
@@ -329,6 +332,7 @@ export function checkAndUpdateView(view: ViewData) {
   } else {
     view.state &= ~ViewState.FirstCheck;
   }
+  markProjectedViewsForCheck(view);
   Services.updateDirectives(view, CheckType.CheckAndUpdate);
   execEmbeddedViewsAction(view, ViewAction.CheckAndUpdate);
   execQueriesAction(
@@ -343,7 +347,6 @@ export function checkAndUpdateView(view: ViewData) {
   execComponentViewsAction(view, ViewAction.CheckAndUpdate);
   execQueriesAction(
       view, NodeFlags.TypeViewQuery, NodeFlags.DynamicQuery, CheckType.CheckAndUpdate);
-
   callLifecycleHooksChildrenFirst(
       view, NodeFlags.AfterViewChecked |
           (view.state & ViewState.FirstCheck ? NodeFlags.AfterViewInit : 0));
@@ -351,6 +354,7 @@ export function checkAndUpdateView(view: ViewData) {
   if (view.def.flags & ViewFlags.OnPush) {
     view.state &= ~ViewState.ChecksEnabled;
   }
+  view.state &= ~(ViewState.CheckProjectedViews | ViewState.CheckProjectedView);
 }
 
 export function checkAndUpdateNode(
@@ -363,6 +367,31 @@ export function checkAndUpdateNode(
   }
 }
 
+function markProjectedViewsForCheck(view: ViewData) {
+  const def = view.def;
+  if (!(def.nodeFlags & NodeFlags.ProjectedTemplate)) {
+    return;
+  }
+  for (let i = 0; i < def.nodes.length; i++) {
+    const nodeDef = def.nodes[i];
+    if (nodeDef.flags & NodeFlags.ProjectedTemplate) {
+      const projectedViews = asElementData(view, i).template._projectedViews;
+      if (projectedViews) {
+        for (let i = 0; i < projectedViews.length; i++) {
+          const projectedView = projectedViews[i];
+          projectedView.state |= ViewState.CheckProjectedView;
+          markParentViewsForCheckProjectedViews(projectedView, view);
+        }
+      }
+    } else if ((nodeDef.childFlags & NodeFlags.ProjectedTemplate) === 0) {
+      // a parent with leafs
+      // no child is a component,
+      // then skip the children
+      i += nodeDef.childCount;
+    }
+  }
+}
+
 function checkAndUpdateNodeInline(
     view: ViewData, nodeDef: NodeDef, v0?: any, v1?: any, v2?: any, v3?: any, v4?: any, v5?: any,
     v6?: any, v7?: any, v8?: any, v9?: any): boolean {
@@ -474,6 +503,7 @@ export function destroyView(view: ViewData) {
       view.disposables[i]();
     }
   }
+  detachProjectedView(view);
   if (view.renderer.destroyNode) {
     destroyViewNodes(view);
   }
@@ -498,7 +528,9 @@ function destroyViewNodes(view: ViewData) {
 enum ViewAction {
   CreateViewNodes,
   CheckNoChanges,
+  CheckNoChangesProjectedViews,
   CheckAndUpdate,
+  CheckAndUpdateProjectedViews,
   Destroy
 }
 
@@ -547,18 +579,44 @@ function callViewAction(view: ViewData, action: ViewAction) {
   const viewState = view.state;
   switch (action) {
     case ViewAction.CheckNoChanges:
-      if ((viewState & ViewState.CatDetectChanges) === ViewState.CatDetectChanges &&
+      if ((viewState & ViewState.Destroyed) === 0) {
-          (viewState & ViewState.Destroyed) === 0) {
+        if ((viewState & ViewState.CatDetectChanges) === ViewState.CatDetectChanges) {
           checkNoChangesView(view);
+        } else if (viewState & ViewState.CheckProjectedViews) {
+          execProjectedViewsAction(view, ViewAction.CheckNoChangesProjectedViews);
+        }
+      }
+      break;
+    case ViewAction.CheckNoChangesProjectedViews:
+      if ((viewState & ViewState.Destroyed) === 0) {
+        if (viewState & ViewState.CheckProjectedView) {
+          checkNoChangesView(view);
+        } else if (viewState & ViewState.CheckProjectedViews) {
+          execProjectedViewsAction(view, action);
+        }
       }
       break;
     case ViewAction.CheckAndUpdate:
-      if ((viewState & ViewState.CatDetectChanges) === ViewState.CatDetectChanges &&
+      if ((viewState & ViewState.Destroyed) === 0) {
-          (viewState & ViewState.Destroyed) === 0) {
+        if ((viewState & ViewState.CatDetectChanges) === ViewState.CatDetectChanges) {
           checkAndUpdateView(view);
+        } else if (viewState & ViewState.CheckProjectedViews) {
+          execProjectedViewsAction(view, ViewAction.CheckAndUpdateProjectedViews);
+        }
+      }
+      break;
+    case ViewAction.CheckAndUpdateProjectedViews:
+      if ((viewState & ViewState.Destroyed) === 0) {
+        if (viewState & ViewState.CheckProjectedView) {
+          checkAndUpdateView(view);
+        } else if (viewState & ViewState.CheckProjectedViews) {
+          execProjectedViewsAction(view, action);
+        }
       }
       break;
     case ViewAction.Destroy:
+      // Note: destroyView recurses over all views,
+      // so we don't need to special case projected views here.
       destroyView(view);
       break;
     case ViewAction.CreateViewNodes:
@@ -567,6 +625,11 @@ function callViewAction(view: ViewData, action: ViewAction) {
   }
 }
 
+function execProjectedViewsAction(view: ViewData, action: ViewAction) {
+  execEmbeddedViewsAction(view, action);
+  execComponentViewsAction(view, action);
+}
+
 function execQueriesAction(
     view: ViewData, queryFlags: NodeFlags, staticDynamicQueryFlag: NodeFlags,
     checkType: CheckType) {

packages/core/src/view/view_attach.ts

@@ -6,8 +6,8 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
-import {ElementData, Services, ViewData} from './types';
+import {ElementData, NodeDef, NodeFlags, Services, ViewData, ViewDefinition, ViewState} from './types';
-import {RenderNodeAction, declaredViewContainer, renderNode, visitRootRenderNodes} from './util';
+import {RenderNodeAction, declaredViewContainer, isComponentView, renderNode, visitRootRenderNodes} from './util';
 
 export function attachEmbeddedView(
     parentView: ViewData, elementData: ElementData, viewIndex: number | undefined | null,
@@ -25,6 +25,11 @@ export function attachEmbeddedView(
       projectedViews = dvcElementData.template._projectedViews = [];
     }
     projectedViews.push(view);
+    if (view.parent) {
+      // Note: we are changing the NodeDef here as we cannot calculate
+      // the fact whether a template is used for projection during compilation.
+      markNodeAsProjectedTemplate(view.parent.def, view.parentNodeDef !);
+    }
   }
 
   Services.dirtyParentQueries(view);
@@ -33,6 +38,19 @@ export function attachEmbeddedView(
   renderAttachEmbeddedView(elementData, prevView, view);
 }
 
+function markNodeAsProjectedTemplate(viewDef: ViewDefinition, nodeDef: NodeDef) {
+  if (nodeDef.flags & NodeFlags.ProjectedTemplate) {
+    return;
+  }
+  viewDef.nodeFlags |= NodeFlags.ProjectedTemplate;
+  nodeDef.flags |= NodeFlags.ProjectedTemplate;
+  let parentNodeDef = nodeDef.parent;
+  while (parentNodeDef) {
+    parentNodeDef.childFlags |= NodeFlags.ProjectedTemplate;
+    parentNodeDef = parentNodeDef.parent;
+  }
+}
+
 export function detachEmbeddedView(elementData: ElementData, viewIndex?: number): ViewData|null {
   const embeddedViews = elementData.viewContainer !._embeddedViews;
   if (viewIndex == null || viewIndex >= embeddedViews.length) {

packages/core/test/linker/change_detection_integration_spec.ts

@@ -8,11 +8,12 @@
 
 import {ElementSchemaRegistry} from '@angular/compiler/src/schema/element_schema_registry';
 import {TEST_COMPILER_PROVIDERS} from '@angular/compiler/testing/src/test_bindings';
-import {AfterContentChecked, AfterContentInit, AfterViewChecked, AfterViewInit, ChangeDetectionStrategy, ChangeDetectorRef, Component, DebugElement, Directive, DoCheck, EventEmitter, HostBinding, Inject, Injectable, Input, OnChanges, OnDestroy, OnInit, Output, Pipe, PipeTransform, RenderComponentType, Renderer, RendererFactory2, RootRenderer, SimpleChange, SimpleChanges, TemplateRef, Type, ViewChild, ViewContainerRef, WrappedValue} from '@angular/core';
+import {AfterContentChecked, AfterContentInit, AfterViewChecked, AfterViewInit, ChangeDetectionStrategy, ChangeDetectorRef, Component, ContentChild, DebugElement, Directive, DoCheck, EventEmitter, HostBinding, Inject, Injectable, Input, OnChanges, OnDestroy, OnInit, Output, Pipe, PipeTransform, RenderComponentType, Renderer, RendererFactory2, RootRenderer, SimpleChange, SimpleChanges, TemplateRef, Type, ViewChild, ViewContainerRef, WrappedValue} from '@angular/core';
 import {ComponentFixture, TestBed, fakeAsync} from '@angular/core/testing';
 import {By} from '@angular/platform-browser/src/dom/debug/by';
 import {getDOM} from '@angular/platform-browser/src/dom/dom_adapter';
 import {expect} from '@angular/platform-browser/testing/src/matchers';
+
 import {DomElementSchemaRegistry} from '../../../compiler/index';
 import {MockSchemaRegistry} from '../../../compiler/testing/index';
 
@@ -1309,6 +1310,120 @@ export function main() {
            ctx.detectChanges();
            expect(renderLog.loggedValues).toEqual(['Tom']);
          });
+
+      describe('projected views', () => {
+        let log: string[];
+
+        @Directive({selector: '[i]'})
+        class DummyDirective {
+          @Input()
+          i: any;
+        }
+
+        @Component({
+          selector: 'main-cmp',
+          template:
+              `<span [i]="log('start')"></span><outer-cmp><ng-template><span [i]="log('tpl')"></span></ng-template></outer-cmp>`
+        })
+        class MainComp {
+          constructor(public cdRef: ChangeDetectorRef) {}
+          log(id: string) { log.push(`main-${id}`); }
+        }
+
+        @Component({
+          selector: 'outer-cmp',
+          template:
+              `<span [i]="log('start')"></span><inner-cmp [outerTpl]="tpl"><ng-template><span [i]="log('tpl')"></span></ng-template></inner-cmp>`
+        })
+        class OuterComp {
+          @ContentChild(TemplateRef)
+          tpl: TemplateRef<any>;
+
+          constructor(public cdRef: ChangeDetectorRef) {}
+          log(id: string) { log.push(`outer-${id}`); }
+        }
+
+        @Component({
+          selector: 'inner-cmp',
+          template:
+              `<span [i]="log('start')"></span>><ng-container [ngTemplateOutlet]="outerTpl"></ng-container><ng-container [ngTemplateOutlet]="tpl"></ng-container>`
+        })
+        class InnerComp {
+          @ContentChild(TemplateRef)
+          tpl: TemplateRef<any>;
+
+          @Input()
+          outerTpl: TemplateRef<any>
+
+          constructor(public cdRef: ChangeDetectorRef) {}
+          log(id: string) { log.push(`inner-${id}`); }
+        }
+
+        let ctx: ComponentFixture<MainComp>;
+        let mainComp: MainComp;
+        let outerComp: OuterComp;
+        let innerComp: InnerComp;
+
+        beforeEach(() => {
+          log = [];
+          ctx = TestBed
+                    .configureTestingModule(
+                        {declarations: [MainComp, OuterComp, InnerComp, DummyDirective]})
+                    .createComponent(MainComp);
+          mainComp = ctx.componentInstance;
+          outerComp = ctx.debugElement.query(By.directive(OuterComp)).injector.get(OuterComp);
+          innerComp = ctx.debugElement.query(By.directive(InnerComp)).injector.get(InnerComp);
+        });
+
+        it('should dirty check projected views in regular order', () => {
+          ctx.detectChanges(false);
+          expect(log).toEqual(
+              ['main-start', 'outer-start', 'inner-start', 'main-tpl', 'outer-tpl']);
+
+          log = [];
+          ctx.detectChanges(false);
+          expect(log).toEqual(
+              ['main-start', 'outer-start', 'inner-start', 'main-tpl', 'outer-tpl']);
+        });
+
+        it('should not dirty check projected views if neither the declaration nor the insertion place is dirty checked',
+           () => {
+             ctx.detectChanges(false);
+             log = [];
+             mainComp.cdRef.detach();
+             ctx.detectChanges(false);
+
+             expect(log).toEqual([]);
+           });
+
+        it('should dirty check projected views if the insertion place is dirty checked', () => {
+          ctx.detectChanges(false);
+          log = [];
+
+          innerComp.cdRef.detectChanges();
+          expect(log).toEqual(['inner-start', 'main-tpl', 'outer-tpl']);
+        });
+
+        it('should dirty check projected views if the declaration place is dirty checked', () => {
+          ctx.detectChanges(false);
+          log = [];
+          innerComp.cdRef.detach();
+          mainComp.cdRef.detectChanges();
+
+          expect(log).toEqual(['main-start', 'outer-start', 'main-tpl', 'outer-tpl']);
+
+          log = [];
+          outerComp.cdRef.detectChanges();
+
+          expect(log).toEqual(['outer-start', 'outer-tpl']);
+
+          log = [];
+          outerComp.cdRef.detach();
+          mainComp.cdRef.detectChanges();
+
+          expect(log).toEqual(['main-start', 'main-tpl']);
+        });
+      });
     });
 
     describe('class binding', () => {