From 6d057cc05dc5226f8f9f7ef60b57c765e530bb3e Mon Sep 17 00:00:00 2001 From: Pawel Kozlowski Date: Wed, 13 Feb 2019 12:10:20 +0100 Subject: [PATCH] fix(ivy): should mark OnPush ancestor of dynamically created views as dirty (#28687) While marking a given views tree as dirty we should go all the way to the root of the views tree and cross boundaries of dynamically inserted views. In other words the markForCheck functionality should consider parents of dynamically inserted views. PR Close #28687 --- packages/core/src/render3/instructions.ts | 17 +++--- packages/core/src/render3/util.ts | 2 +- .../test/acceptance/change_detection_spec.ts | 61 ++++++++++++++++++- 3 files changed, 70 insertions(+), 10 deletions(-) diff --git a/packages/core/src/render3/instructions.ts b/packages/core/src/render3/instructions.ts index 4e84daea32..fdb2291800 100644 --- a/packages/core/src/render3/instructions.ts +++ b/packages/core/src/render3/instructions.ts @@ -30,7 +30,7 @@ import {AttributeMarker, InitialInputData, InitialInputs, LocalRefExtractor, Pro import {PlayerFactory} from './interfaces/player'; import {CssSelectorList, NG_PROJECT_AS_ATTR_NAME} from './interfaces/projection'; import {LQueries} from './interfaces/query'; -import {GlobalTargetResolver, ProceduralRenderer3, RComment, RElement, RNode, RText, Renderer3, RendererFactory3, isProceduralRenderer} from './interfaces/renderer'; +import {GlobalTargetResolver, ProceduralRenderer3, RComment, RElement, RText, Renderer3, RendererFactory3, isProceduralRenderer} from './interfaces/renderer'; import {SanitizerFn} from './interfaces/sanitization'; import {BINDING_INDEX, CLEANUP, CONTAINER_INDEX, CONTEXT, DECLARATION_VIEW, FLAGS, HEADER_OFFSET, HOST, INJECTOR, InitPhaseState, LView, LViewFlags, NEXT, OpaqueViewState, PARENT, QUERIES, RENDERER, RENDERER_FACTORY, RootContext, RootContextFlags, SANITIZER, TAIL, TData, TVIEW, TView, T_HOST} from './interfaces/view'; import {assertNodeOfPossibleTypes, assertNodeType} from './node_assert'; @@ -41,7 +41,7 @@ import {getInitialClassNameValue, getInitialStyleStringValue, initializeStaticCo import {BoundPlayerFactory} from './styling/player_factory'; import {ANIMATION_PROP_PREFIX, allocateDirectiveIntoContext, createEmptyStylingContext, forceClassesAsString, forceStylesAsString, getStylingContext, hasClassInput, hasStyleInput, hasStyling, isAnimationProp} from './styling/util'; import {NO_CHANGE} from './tokens'; -import {INTERPOLATION_DELIMITER, findComponentView, getComponentViewByIndex, getNativeByIndex, getNativeByTNode, getRootContext, getRootView, getTNode, isComponent, isComponentDef, isContentQueryHost, loadInternal, readElementValue, readPatchedLView, renderStringify} from './util'; +import {INTERPOLATION_DELIMITER, findComponentView, getComponentViewByIndex, getNativeByIndex, getNativeByTNode, getRootContext, getRootView, getTNode, isComponent, isComponentDef, isContentQueryHost, isRootView, loadInternal, readElementValue, readPatchedLView, renderStringify} from './util'; @@ -2729,15 +2729,16 @@ function wrapListener( * @returns the root LView */ export function markViewDirty(lView: LView): LView|null { - while (lView && !(lView[FLAGS] & LViewFlags.IsRoot)) { + while (lView) { lView[FLAGS] |= LViewFlags.Dirty; + // Stop traversing up as soon as you find a root view that wasn't attached to any container + if (isRootView(lView) && lView[CONTAINER_INDEX] === -1) { + return lView; + } + // continue otherwise lView = lView[PARENT] !; } - // Detached views do not have a PARENT and also aren't root views - if (lView) { - lView[FLAGS] |= LViewFlags.Dirty; - } - return lView; + return null; } /** diff --git a/packages/core/src/render3/util.ts b/packages/core/src/render3/util.ts index e07469bf69..4f7b2991be 100644 --- a/packages/core/src/render3/util.ts +++ b/packages/core/src/render3/util.ts @@ -16,7 +16,7 @@ import {NO_PARENT_INJECTOR, RelativeInjectorLocation, RelativeInjectorLocationFl import {TContainerNode, TElementNode, TNode, TNodeFlags, TNodeType} from './interfaces/node'; import {RComment, RElement, RText} from './interfaces/renderer'; import {StylingContext} from './interfaces/styling'; -import {CONTEXT, DECLARATION_VIEW, FLAGS, HEADER_OFFSET, HOST, LView, LViewFlags, PARENT, RootContext, TData, TVIEW, TView, T_HOST} from './interfaces/view'; +import {CONTEXT, DECLARATION_VIEW, FLAGS, HEADER_OFFSET, HOST, LView, LViewFlags, PARENT, RootContext, TData, TVIEW, T_HOST} from './interfaces/view'; diff --git a/packages/core/test/acceptance/change_detection_spec.ts b/packages/core/test/acceptance/change_detection_spec.ts index 8978ca4b17..340a224321 100644 --- a/packages/core/test/acceptance/change_detection_spec.ts +++ b/packages/core/test/acceptance/change_detection_spec.ts @@ -7,7 +7,7 @@ */ -import {ApplicationRef, Component, Directive, EmbeddedViewRef, TemplateRef, ViewContainerRef} from '@angular/core'; +import {ApplicationRef, ChangeDetectionStrategy, Component, ComponentFactoryResolver, ComponentRef, Directive, EmbeddedViewRef, NgModule, TemplateRef, Type, ViewChild, ViewContainerRef} from '@angular/core'; import {TestBed} from '@angular/core/testing'; import {expect} from '@angular/platform-browser/testing/src/matchers'; @@ -68,4 +68,63 @@ describe('change detection', () => { }); + describe('markForCheck', () => { + + it('should mark OnPush ancestor of dynamically created component views as dirty', () => { + + @Component({ + selector: `test-cmpt`, + template: `{{counter}}|`, + changeDetection: ChangeDetectionStrategy.OnPush + }) + class TestCmpt { + counter = 0; + @ViewChild('vc', {read: ViewContainerRef}) vcRef !: ViewContainerRef; + + constructor(private _cfr: ComponentFactoryResolver) {} + + createComponentView(cmptType: Type): ComponentRef { + const cf = this._cfr.resolveComponentFactory(cmptType); + return this.vcRef.createComponent(cf); + } + } + + @Component({ + selector: 'dynamic-cmpt', + template: `dynamic`, + changeDetection: ChangeDetectionStrategy.OnPush + }) + class DynamicCmpt { + } + + @NgModule({declarations: [DynamicCmpt], entryComponents: [DynamicCmpt]}) + class DynamicModule { + } + + TestBed.configureTestingModule({imports: [DynamicModule], declarations: [TestCmpt]}); + + const fixture = TestBed.createComponent(TestCmpt); + + // initial CD to have query results + // NOTE: we call change detection without checkNoChanges to have clearer picture + fixture.detectChanges(false); + expect(fixture.nativeElement).toHaveText('0|'); + + // insert a dynamic component + const dynamicCmptRef = fixture.componentInstance.createComponentView(DynamicCmpt); + fixture.detectChanges(false); + expect(fixture.nativeElement).toHaveText('0|dynamic'); + + // update model in the OnPush component - should not update UI + fixture.componentInstance.counter = 1; + fixture.detectChanges(false); + expect(fixture.nativeElement).toHaveText('0|dynamic'); + + // now mark the dynamically inserted component as dirty + dynamicCmptRef.changeDetectorRef.markForCheck(); + fixture.detectChanges(false); + expect(fixture.nativeElement).toHaveText('1|dynamic'); + }); + }); + }); \ No newline at end of file