From 5c4d95541e498b0ffe78e3fd5182d2242abfc542 Mon Sep 17 00:00:00 2001 From: Kara Erickson Date: Wed, 30 Jan 2019 21:54:44 -0800 Subject: [PATCH] fix(ivy): mark views dirty by default when events fire (#28474) In Ivy, we support a new manual mode that allows for stricter control over change detection in OnPush components. Specifically, in this mode, events do not automatically mark OnPush views as dirty. Only changed inputs and manual calls to `markDirty()` actually mark a view dirty. However, this mode cannot be the default for OnPush components if we want to be backwards compatible with View Engine. This commit re-adds the legacy logic for OnPush components where events always mark views dirty and makes it the default behavior. Note: It is still TODO to add a public API for manual change detection. PR Close #28474 --- packages/core/src/render3/instructions.ts | 50 ++- packages/core/src/render3/interfaces/view.ts | 33 +- .../bundling/todo/bundle.golden_symbols.json | 2 +- packages/core/test/linker/integration_spec.ts | 65 ++-- .../test/render3/change_detection_spec.ts | 317 +++++++++++++----- 5 files changed, 332 insertions(+), 135 deletions(-) diff --git a/packages/core/src/render3/instructions.ts b/packages/core/src/render3/instructions.ts index da37de5ac0..84aae207a1 100644 --- a/packages/core/src/render3/instructions.ts +++ b/packages/core/src/render3/instructions.ts @@ -956,13 +956,14 @@ function listenerInternal( // The first argument of `listen` function in Procedural Renderer is: // - either a target name (as a string) in case of global target (window, document, body) // - or element reference (in all other cases) + listenerFn = wrapListener(tNode, lView, listenerFn, false /** preventDefault */); const cleanupFn = renderer.listen(resolved.name || target, eventName, listenerFn); lCleanup.push(listenerFn, cleanupFn); useCaptureOrSubIdx = lCleanupIndex + 1; } else { - const wrappedListener = wrapListenerWithPreventDefault(listenerFn); - target.addEventListener(eventName, wrappedListener, useCapture); - lCleanup.push(wrappedListener); + listenerFn = wrapListener(tNode, lView, listenerFn, true /** preventDefault */); + target.addEventListener(eventName, listenerFn, useCapture); + lCleanup.push(listenerFn); } const idxOrTargetGetter = eventTargetResolver ? @@ -2578,17 +2579,41 @@ function markDirtyIfOnPush(lView: LView, viewIndex: number): void { } } -/** Wraps an event listener with preventDefault behavior. */ -function wrapListenerWithPreventDefault(listenerFn: (e?: any) => any): EventListener { - return function wrapListenerIn_preventDefault(e: Event) { - if (listenerFn(e) === false) { +/** + * Wraps an event listener with a function that marks ancestors dirty and prevents default behavior, + * if applicable. + * + * @param tNode The TNode associated with this listener + * @param lView The LView that contains this listener + * @param listenerFn The listener function to call + * @param wrapWithPreventDefault Whether or not to prevent default behavior + * (the procedural renderer does this already, so in those cases, we should skip) + */ +function wrapListener( + tNode: TNode, lView: LView, listenerFn: (e?: any) => any, + wrapWithPreventDefault: boolean): EventListener { + // Note: we are performing most of the work in the listener function itself + // to optimize listener registration. + return function wrapListenerIn_markDirtyAndPreventDefault(e: Event) { + // In order to be backwards compatible with View Engine, events on component host nodes + // must also mark the component view itself dirty (i.e. the view that it owns). + const startView = + tNode.flags & TNodeFlags.isComponent ? getComponentViewByIndex(tNode.index, lView) : lView; + + // See interfaces/view.ts for more on LViewFlags.ManualOnPush + if ((lView[FLAGS] & LViewFlags.ManualOnPush) === 0) { + markViewDirty(startView); + } + + const result = listenerFn(e); + if (wrapWithPreventDefault && result === false) { e.preventDefault(); // Necessary for legacy browsers that don't support preventDefault (e.g. IE) e.returnValue = false; } + return result; }; } - /** * Marks current view and all ancestors dirty. * @@ -2600,12 +2625,15 @@ function wrapListenerWithPreventDefault(listenerFn: (e?: any) => any): EventList * @param lView The starting LView to mark dirty * @returns the root LView */ -export function markViewDirty(lView: LView): LView { +export function markViewDirty(lView: LView): LView|null { while (lView && !(lView[FLAGS] & LViewFlags.IsRoot)) { lView[FLAGS] |= LViewFlags.Dirty; lView = lView[PARENT] !; } - lView[FLAGS] |= LViewFlags.Dirty; + // Detached views do not have a PARENT and also aren't root views + if (lView) { + lView[FLAGS] |= LViewFlags.Dirty; + } return lView; } @@ -2791,7 +2819,7 @@ function executeViewQueryFn(lView: LView, tView: TView, component: T): void { */ export function markDirty(component: T) { ngDevMode && assertDefined(component, 'component'); - const rootView = markViewDirty(getComponentViewByInstance(component)); + const rootView = markViewDirty(getComponentViewByInstance(component)) !; ngDevMode && assertDefined(rootView[CONTEXT], 'rootContext should be defined'); scheduleTick(rootView[CONTEXT] as RootContext, RootContextFlags.DetectChanges); diff --git a/packages/core/src/render3/interfaces/view.ts b/packages/core/src/render3/interfaces/view.ts index 32e93e38f0..20f41e30f4 100644 --- a/packages/core/src/render3/interfaces/view.ts +++ b/packages/core/src/render3/interfaces/view.ts @@ -241,24 +241,41 @@ export const enum LViewFlags { /** Whether this view has default change detection strategy (checks always) or onPush */ CheckAlways = 0b00000010000, + /** + * Whether or not manual change detection is turned on for onPush components. + * + * This is a special mode that only marks components dirty in two cases: + * 1) There has been a change to an @Input property + * 2) `markDirty()` has been called manually by the user + * + * Note that in this mode, the firing of events does NOT mark components + * dirty automatically. + * + * Manual mode is turned off by default for backwards compatibility, as events + * automatically mark OnPush components dirty in View Engine. + * + * TODO: Add a public API to ChangeDetectionStrategy to turn this mode on + */ + ManualOnPush = 0b00000100000, + /** Whether or not this view is currently dirty (needing check) */ - Dirty = 0b00000100000, + Dirty = 0b000001000000, /** Whether or not this view is currently attached to change detection tree. */ - Attached = 0b00001000000, + Attached = 0b000010000000, /** Whether or not this view is destroyed. */ - Destroyed = 0b00010000000, + Destroyed = 0b000100000000, /** Whether or not this view is the root view */ - IsRoot = 0b00100000000, + IsRoot = 0b001000000000, /** - * Index of the current init phase on last 23 bits + * Index of the current init phase on last 22 bits */ - IndexWithinInitPhaseIncrementer = 0b01000000000, - IndexWithinInitPhaseShift = 9, - IndexWithinInitPhaseReset = 0b00111111111, + IndexWithinInitPhaseIncrementer = 0b010000000000, + IndexWithinInitPhaseShift = 10, + IndexWithinInitPhaseReset = 0b001111111111, } /** diff --git a/packages/core/test/bundling/todo/bundle.golden_symbols.json b/packages/core/test/bundling/todo/bundle.golden_symbols.json index 04f07b1937..5a68a1ff42 100644 --- a/packages/core/test/bundling/todo/bundle.golden_symbols.json +++ b/packages/core/test/bundling/todo/bundle.golden_symbols.json @@ -1239,6 +1239,6 @@ "name": "walkUpViews" }, { - "name": "wrapListenerWithPreventDefault" + "name": "wrapListener" } ] \ No newline at end of file diff --git a/packages/core/test/linker/integration_spec.ts b/packages/core/test/linker/integration_spec.ts index 612402757e..af5087c199 100644 --- a/packages/core/test/linker/integration_spec.ts +++ b/packages/core/test/linker/integration_spec.ts @@ -637,44 +637,43 @@ function declareTests(config?: {useJit: boolean}) { })); } - fixmeIvy('FW-758: OnPush events not marking view dirty when using renderer2') - .it('should be checked when an event is fired', () => { - TestBed.configureTestingModule( - {declarations: [MyComp, PushCmp, EventCmp], imports: [CommonModule]}); - const template = ''; - TestBed.overrideComponent(MyComp, {set: {template}}); - const fixture = TestBed.createComponent(MyComp); + it('should be checked when an event is fired', () => { + TestBed.configureTestingModule( + {declarations: [MyComp, PushCmp, EventCmp], imports: [CommonModule]}); + const template = ''; + TestBed.overrideComponent(MyComp, {set: {template}}); + const fixture = TestBed.createComponent(MyComp); - const cmpEl = fixture.debugElement.children[0]; - const cmp = cmpEl.componentInstance; - fixture.detectChanges(); - fixture.detectChanges(); - expect(cmp.numberOfChecks).toEqual(1); + const cmpEl = fixture.debugElement.children[0]; + const cmp = cmpEl.componentInstance; + fixture.detectChanges(); + fixture.detectChanges(); + expect(cmp.numberOfChecks).toEqual(1); - // regular element - cmpEl.children[0].triggerEventHandler('click', {}); - fixture.detectChanges(); - fixture.detectChanges(); - expect(cmp.numberOfChecks).toEqual(2); + // regular element + cmpEl.children[0].triggerEventHandler('click', {}); + fixture.detectChanges(); + fixture.detectChanges(); + expect(cmp.numberOfChecks).toEqual(2); - // element inside of an *ngIf - cmpEl.children[1].triggerEventHandler('click', {}); - fixture.detectChanges(); - fixture.detectChanges(); - expect(cmp.numberOfChecks).toEqual(3); + // element inside of an *ngIf + cmpEl.children[1].triggerEventHandler('click', {}); + fixture.detectChanges(); + fixture.detectChanges(); + expect(cmp.numberOfChecks).toEqual(3); - // element inside a nested component - cmpEl.children[2].children[0].triggerEventHandler('click', {}); - fixture.detectChanges(); - fixture.detectChanges(); - expect(cmp.numberOfChecks).toEqual(4); + // element inside a nested component + cmpEl.children[2].children[0].triggerEventHandler('click', {}); + fixture.detectChanges(); + fixture.detectChanges(); + expect(cmp.numberOfChecks).toEqual(4); - // host element - cmpEl.triggerEventHandler('click', {}); - fixture.detectChanges(); - fixture.detectChanges(); - expect(cmp.numberOfChecks).toEqual(5); - }); + // host element + cmpEl.triggerEventHandler('click', {}); + fixture.detectChanges(); + fixture.detectChanges(); + expect(cmp.numberOfChecks).toEqual(5); + }); it('should not affect updating properties on the component', () => { TestBed.configureTestingModule({declarations: [MyComp, [[PushCmpWithRef]]]}); diff --git a/packages/core/test/render3/change_detection_spec.ts b/packages/core/test/render3/change_detection_spec.ts index 9cef99c1ab..ee923f4baa 100644 --- a/packages/core/test/render3/change_detection_spec.ts +++ b/packages/core/test/render3/change_detection_spec.ts @@ -11,11 +11,12 @@ import {withBody} from '@angular/private/testing'; import {ChangeDetectionStrategy, ChangeDetectorRef, DoCheck, RendererType2} from '../../src/core'; import {whenRendered} from '../../src/render3/component'; -import {LifecycleHooksFeature, NgOnChangesFeature, defineComponent, defineDirective, getRenderedText, templateRefExtractor} from '../../src/render3/index'; +import {LifecycleHooksFeature, NgOnChangesFeature, defineComponent, defineDirective, getCurrentView, getRenderedText, templateRefExtractor} from '../../src/render3/index'; import {bind, container, containerRefreshEnd, containerRefreshStart, detectChanges, directiveInject, element, elementEnd, elementProperty, elementStart, embeddedViewEnd, embeddedViewStart, interpolation1, interpolation2, listener, markDirty, reference, text, template, textBinding, tick} from '../../src/render3/instructions'; import {RenderFlags} from '../../src/render3/interfaces/definition'; import {RElement, Renderer3, RendererFactory3} from '../../src/render3/interfaces/renderer'; +import {FLAGS, LViewFlags} from '../../src/render3/interfaces/view'; import {ComponentFixture, containerEl, createComponent, renderComponent, requestAnimationFrame} from './render_util'; @@ -183,41 +184,39 @@ describe('change detection', () => { myApp.name = 'Bess'; tick(myApp); + expect(comp.doCheckCount).toEqual(2); + // View should update, as changed input marks view dirty expect(getRenderedText(myApp)).toEqual('2 - Bess'); myApp.name = 'George'; tick(myApp); + // View should update, as changed input marks view dirty + expect(comp.doCheckCount).toEqual(3); expect(getRenderedText(myApp)).toEqual('3 - George'); tick(myApp); + expect(comp.doCheckCount).toEqual(4); + // View should not be updated to "4", as inputs have not changed. expect(getRenderedText(myApp)).toEqual('3 - George'); }); - it('should not check OnPush components in update mode when component events occur, unless marked dirty', - () => { - const myApp = renderComponent(MyApp); - expect(comp.doCheckCount).toEqual(1); - expect(getRenderedText(myApp)).toEqual('1 - Nancy'); + it('should check OnPush components in update mode when component events occur', () => { + const myApp = renderComponent(MyApp); + expect(comp.doCheckCount).toEqual(1); + expect(getRenderedText(myApp)).toEqual('1 - Nancy'); - const button = containerEl.querySelector('button') !; - button.click(); - requestAnimationFrame.flush(); - // No ticks should have been scheduled. - expect(comp.doCheckCount).toEqual(1); - expect(getRenderedText(myApp)).toEqual('1 - Nancy'); + const button = containerEl.querySelector('button') !; + button.click(); + requestAnimationFrame.flush(); + // No ticks should have been scheduled. + expect(comp.doCheckCount).toEqual(1); + expect(getRenderedText(myApp)).toEqual('1 - Nancy'); - tick(myApp); - // The comp should still be clean. So doCheck will run, but the view should display 1. - expect(comp.doCheckCount).toEqual(2); - expect(getRenderedText(myApp)).toEqual('1 - Nancy'); - - markDirty(comp); - requestAnimationFrame.flush(); - // Now that markDirty has been manually called, the view should be dirty and a tick - // should be scheduled to check the view. - expect(comp.doCheckCount).toEqual(3); - expect(getRenderedText(myApp)).toEqual('3 - Nancy'); - }); + tick(myApp); + // Because the onPush comp should be dirty, it should update once CD runs + expect(comp.doCheckCount).toEqual(2); + expect(getRenderedText(myApp)).toEqual('2 - Nancy'); + }); it('should not check OnPush components in update mode when parent events occur', () => { function noop() {} @@ -238,76 +237,230 @@ describe('change detection', () => { (button as HTMLButtonElement).click(); tick(buttonParent); // The comp should still be clean. So doCheck will run, but the view should display 1. + expect(comp.doCheckCount).toEqual(2); expect(getRenderedText(buttonParent)).toEqual('1 - Nancy'); }); - it('should not check parent OnPush components in update mode when child events occur, unless marked dirty', - () => { - let parent: ButtonParent; + it('should check parent OnPush components in update mode when child events occur', () => { + let parent: ButtonParent; - class ButtonParent implements DoCheck { - doCheckCount = 0; - ngDoCheck(): void { this.doCheckCount++; } + class ButtonParent implements DoCheck { + doCheckCount = 0; + ngDoCheck(): void { this.doCheckCount++; } - static ngComponentDef = defineComponent({ - type: ButtonParent, - selectors: [['button-parent']], - factory: () => parent = new ButtonParent(), - consts: 2, - vars: 1, - /** {{ doCheckCount }} - */ - template: (rf: RenderFlags, ctx: ButtonParent) => { - if (rf & RenderFlags.Create) { - text(0); - element(1, 'my-comp'); - } - if (rf & RenderFlags.Update) { - textBinding(0, interpolation1('', ctx.doCheckCount, ' - ')); - } - }, - directives: () => [MyComponent], - changeDetection: ChangeDetectionStrategy.OnPush - }); - } + static ngComponentDef = defineComponent({ + type: ButtonParent, + selectors: [['button-parent']], + factory: () => parent = new ButtonParent(), + consts: 2, + vars: 1, + /** {{ doCheckCount }} - */ + template: (rf: RenderFlags, ctx: ButtonParent) => { + if (rf & RenderFlags.Create) { + text(0); + element(1, 'my-comp'); + } + if (rf & RenderFlags.Update) { + textBinding(0, interpolation1('', ctx.doCheckCount, ' - ')); + } + }, + directives: () => [MyComponent], + changeDetection: ChangeDetectionStrategy.OnPush + }); + } - const MyButtonApp = createComponent('my-button-app', function(rf: RenderFlags, ctx: any) { - if (rf & RenderFlags.Create) { - element(0, 'button-parent'); + const MyButtonApp = createComponent('my-button-app', function(rf: RenderFlags, ctx: any) { + if (rf & RenderFlags.Create) { + element(0, 'button-parent'); + } + }, 1, 0, [ButtonParent]); + + const myButtonApp = renderComponent(MyButtonApp); + expect(parent !.doCheckCount).toEqual(1); + expect(comp !.doCheckCount).toEqual(1); + expect(getRenderedText(myButtonApp)).toEqual('1 - 1 - Nancy'); + + tick(myButtonApp); + expect(parent !.doCheckCount).toEqual(2); + // parent isn't checked, so child doCheck won't run + expect(comp !.doCheckCount).toEqual(1); + expect(getRenderedText(myButtonApp)).toEqual('1 - 1 - Nancy'); + + const button = containerEl.querySelector('button'); + button !.click(); + requestAnimationFrame.flush(); + // No ticks should have been scheduled. + expect(parent !.doCheckCount).toEqual(2); + expect(comp !.doCheckCount).toEqual(1); + + tick(myButtonApp); + expect(parent !.doCheckCount).toEqual(3); + expect(comp !.doCheckCount).toEqual(2); + expect(getRenderedText(myButtonApp)).toEqual('3 - 2 - Nancy'); + }); + + describe('Manual mode', () => { + class ManualComponent implements DoCheck { + /* @Input() */ + name = 'Nancy'; + doCheckCount = 0; + + ngDoCheck(): void { this.doCheckCount++; } + + onClick() {} + + static ngComponentDef = defineComponent({ + type: ManualComponent, + selectors: [['manual-comp']], + factory: () => comp = new ManualComponent(), + consts: 2, + vars: 2, + /** + * {{ doCheckCount }} - {{ name }} + * + */ + template: (rf: RenderFlags, ctx: ManualComponent) => { + if (rf & RenderFlags.Create) { + // This is temporarily the only way to turn on manual change detection + // because public API has not yet been added. + const view = getCurrentView() as any; + view[FLAGS] |= LViewFlags.ManualOnPush; + + text(0); + elementStart(1, 'button'); + { + listener('click', () => { ctx.onClick(); }); + } + elementEnd(); + } + if (rf & RenderFlags.Update) { + textBinding(0, interpolation2('', ctx.doCheckCount, ' - ', ctx.name, '')); + } + }, + changeDetection: ChangeDetectionStrategy.OnPush, + inputs: {name: 'name'} + }); + } + + class ManualApp { + name: string = 'Nancy'; + + static ngComponentDef = defineComponent({ + type: ManualApp, + selectors: [['manual-app']], + factory: () => new ManualApp(), + consts: 1, + vars: 1, + /** */ + template: (rf: RenderFlags, ctx: ManualApp) => { + if (rf & RenderFlags.Create) { + element(0, 'manual-comp'); + } + if (rf & RenderFlags.Update) { + elementProperty(0, 'name', bind(ctx.name)); + } + + }, + directives: () => [ManualComponent] + }); + } + + + it('should not check OnPush components in update mode when component events occur, unless marked dirty', + () => { + const myApp = renderComponent(ManualApp); + expect(comp.doCheckCount).toEqual(1); + expect(getRenderedText(myApp)).toEqual('1 - Nancy'); + + const button = containerEl.querySelector('button') !; + button.click(); + requestAnimationFrame.flush(); + // No ticks should have been scheduled. + expect(comp.doCheckCount).toEqual(1); + expect(getRenderedText(myApp)).toEqual('1 - Nancy'); + + tick(myApp); + // The comp should still be clean. So doCheck will run, but the view should display 1. + expect(comp.doCheckCount).toEqual(2); + expect(getRenderedText(myApp)).toEqual('1 - Nancy'); + + markDirty(comp); + requestAnimationFrame.flush(); + // Now that markDirty has been manually called, the view should be dirty and a tick + // should be scheduled to check the view. + expect(comp.doCheckCount).toEqual(3); + expect(getRenderedText(myApp)).toEqual('3 - Nancy'); + }); + + it('should not check parent OnPush components in update mode when child events occur, unless marked dirty', + () => { + let parent: ButtonParent; + + class ButtonParent implements DoCheck { + doCheckCount = 0; + ngDoCheck(): void { this.doCheckCount++; } + + static ngComponentDef = defineComponent({ + type: ButtonParent, + selectors: [['button-parent']], + factory: () => parent = new ButtonParent(), + consts: 2, + vars: 1, + /** {{ doCheckCount }} - */ + template: (rf: RenderFlags, ctx: ButtonParent) => { + if (rf & RenderFlags.Create) { + text(0); + element(1, 'manual-comp'); + } + if (rf & RenderFlags.Update) { + textBinding(0, interpolation1('', ctx.doCheckCount, ' - ')); + } + }, + directives: () => [ManualComponent], + changeDetection: ChangeDetectionStrategy.OnPush + }); } - }, 1, 0, [ButtonParent]); - const myButtonApp = renderComponent(MyButtonApp); - expect(parent !.doCheckCount).toEqual(1); - expect(comp !.doCheckCount).toEqual(1); - expect(getRenderedText(myButtonApp)).toEqual('1 - 1 - Nancy'); + const MyButtonApp = + createComponent('my-button-app', function(rf: RenderFlags, ctx: any) { + if (rf & RenderFlags.Create) { + element(0, 'button-parent'); + } + }, 1, 0, [ButtonParent]); - tick(myButtonApp); - expect(parent !.doCheckCount).toEqual(2); - // parent isn't checked, so child doCheck won't run - expect(comp !.doCheckCount).toEqual(1); - expect(getRenderedText(myButtonApp)).toEqual('1 - 1 - Nancy'); + const myButtonApp = renderComponent(MyButtonApp); + expect(parent !.doCheckCount).toEqual(1); + expect(comp !.doCheckCount).toEqual(1); + expect(getRenderedText(myButtonApp)).toEqual('1 - 1 - Nancy'); - const button = containerEl.querySelector('button'); - button !.click(); - requestAnimationFrame.flush(); - // No ticks should have been scheduled. - expect(parent !.doCheckCount).toEqual(2); - expect(comp !.doCheckCount).toEqual(1); + tick(myButtonApp); + expect(parent !.doCheckCount).toEqual(2); + // parent isn't checked, so child doCheck won't run + expect(comp !.doCheckCount).toEqual(1); + expect(getRenderedText(myButtonApp)).toEqual('1 - 1 - Nancy'); - tick(myButtonApp); - expect(parent !.doCheckCount).toEqual(3); - // parent isn't checked, so child doCheck won't run - expect(comp !.doCheckCount).toEqual(1); - expect(getRenderedText(myButtonApp)).toEqual('1 - 1 - Nancy'); + const button = containerEl.querySelector('button'); + button !.click(); + requestAnimationFrame.flush(); + // No ticks should have been scheduled. + expect(parent !.doCheckCount).toEqual(2); + expect(comp !.doCheckCount).toEqual(1); - markDirty(comp); - requestAnimationFrame.flush(); - // Now that markDirty has been manually called, both views should be dirty and a tick - // should be scheduled to check the view. - expect(parent !.doCheckCount).toEqual(4); - expect(comp !.doCheckCount).toEqual(2); - expect(getRenderedText(myButtonApp)).toEqual('4 - 2 - Nancy'); - }); + tick(myButtonApp); + expect(parent !.doCheckCount).toEqual(3); + // parent isn't checked, so child doCheck won't run + expect(comp !.doCheckCount).toEqual(1); + expect(getRenderedText(myButtonApp)).toEqual('1 - 1 - Nancy'); + + markDirty(comp); + requestAnimationFrame.flush(); + // Now that markDirty has been manually called, both views should be dirty and a tick + // should be scheduled to check the view. + expect(parent !.doCheckCount).toEqual(4); + expect(comp !.doCheckCount).toEqual(2); + expect(getRenderedText(myButtonApp)).toEqual('4 - 2 - Nancy'); + }); + }); }); describe('ChangeDetectorRef', () => {