From fcc91d862ffe1aa4c9be847a43147fa670a77277 Mon Sep 17 00:00:00 2001 From: Tobias Bosch Date: Fri, 5 May 2017 13:49:59 -0700 Subject: [PATCH] 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 | 59 ++++----- packages/core/src/view/util.ts | 8 ++ packages/core/src/view/view.ts | 79 ++++++++++-- packages/core/src/view/view_attach.ts | 22 +++- .../change_detection_integration_spec.ts | 117 +++++++++++++++++- 5 files changed, 246 insertions(+), 39 deletions(-) diff --git a/packages/core/src/view/types.ts b/packages/core/src/view/types.ts index 67a985081e..46811ab273 100644 --- a/packages/core/src/view/types.ts +++ b/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, - TypePipe = 1 << 3, - TypePureArray = 1 << 4, - TypePureObject = 1 << 5, - TypePurePipe = 1 << 6, + TypeNgContent = 1 << 3, + TypePipe = 1 << 4, + TypePureArray = 1 << 5, + TypePureObject = 1 << 6, + TypePurePipe = 1 << 7, CatPureExpression = TypePureArray | TypePureObject | TypePurePipe, - TypeValueProvider = 1 << 7, - TypeClassProvider = 1 << 8, - TypeFactoryProvider = 1 << 9, - TypeUseExistingProvider = 1 << 10, - LazyProvider = 1 << 11, - PrivateProvider = 1 << 12, - TypeDirective = 1 << 13, - Component = 1 << 14, + TypeValueProvider = 1 << 8, + TypeClassProvider = 1 << 9, + TypeFactoryProvider = 1 << 10, + TypeUseExistingProvider = 1 << 11, + LazyProvider = 1 << 12, + PrivateProvider = 1 << 13, + TypeDirective = 1 << 14, + Component = 1 << 15, CatProvider = TypeValueProvider | TypeClassProvider | TypeFactoryProvider | TypeUseExistingProvider | TypeDirective, - OnInit = 1 << 15, - OnDestroy = 1 << 16, - DoCheck = 1 << 17, - OnChanges = 1 << 18, - AfterContentInit = 1 << 19, - AfterContentChecked = 1 << 20, - AfterViewInit = 1 << 21, - AfterViewChecked = 1 << 22, - EmbeddedViews = 1 << 23, - ComponentView = 1 << 24, - TypeContentQuery = 1 << 25, - TypeViewQuery = 1 << 26, - StaticQuery = 1 << 27, - DynamicQuery = 1 << 28, + OnInit = 1 << 16, + OnDestroy = 1 << 17, + DoCheck = 1 << 18, + OnChanges = 1 << 19, + AfterContentInit = 1 << 20, + AfterContentChecked = 1 << 21, + AfterViewInit = 1 << 22, + AfterViewChecked = 1 << 23, + EmbeddedViews = 1 << 24, + ComponentView = 1 << 25, + TypeContentQuery = 1 << 26, + TypeViewQuery = 1 << 27, + StaticQuery = 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 diff --git a/packages/core/src/view/util.ts b/packages/core/src/view/util.ts index c6fd9b333b..52210b9a70 100644 --- a/packages/core/src/view/util.ts +++ b/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]; diff --git a/packages/core/src/view/view.ts b/packages/core/src/view/view.ts index da82ed6a6c..f624c6c6c4 100644 --- a/packages/core/src/view/view.ts +++ b/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 && - (viewState & ViewState.Destroyed) === 0) { - checkNoChangesView(view); + if ((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 && - (viewState & ViewState.Destroyed) === 0) { - checkAndUpdateView(view); + if ((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) { diff --git a/packages/core/src/view/view_attach.ts b/packages/core/src/view/view_attach.ts index 200dacf4c7..a25af16337 100644 --- a/packages/core/src/view/view_attach.ts +++ b/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 {RenderNodeAction, declaredViewContainer, renderNode, visitRootRenderNodes} from './util'; +import {ElementData, NodeDef, NodeFlags, Services, ViewData, ViewDefinition, ViewState} from './types'; +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) { diff --git a/packages/core/test/linker/change_detection_integration_spec.ts b/packages/core/test/linker/change_detection_integration_spec.ts index 34a9afff00..8db439d1a4 100644 --- a/packages/core/test/linker/change_detection_integration_spec.ts +++ b/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: + `` + }) + class MainComp { + constructor(public cdRef: ChangeDetectorRef) {} + log(id: string) { log.push(`main-${id}`); } + } + + @Component({ + selector: 'outer-cmp', + template: + `` + }) + class OuterComp { + @ContentChild(TemplateRef) + tpl: TemplateRef; + + constructor(public cdRef: ChangeDetectorRef) {} + log(id: string) { log.push(`outer-${id}`); } + } + + @Component({ + selector: 'inner-cmp', + template: + `>` + }) + class InnerComp { + @ContentChild(TemplateRef) + tpl: TemplateRef; + + @Input() + outerTpl: TemplateRef + + constructor(public cdRef: ChangeDetectorRef) {} + log(id: string) { log.push(`inner-${id}`); } + } + + let ctx: ComponentFixture; + 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', () => {