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', () => {