From 5653874683404436ef70ce66b64873c508be5b47 Mon Sep 17 00:00:00 2001 From: Kara Erickson Date: Fri, 14 Sep 2018 15:58:57 -0700 Subject: [PATCH] fix(ivy): events should not mark views dirty by default (#25969) PR Close #25969 --- .../core/src/render3/context_discovery.ts | 2 +- packages/core/src/render3/instructions.ts | 32 +--- .../bundling/todo/bundle.golden_symbols.json | 8 +- packages/core/test/bundling/todo/index.ts | 58 +++++-- .../todo_r2/bundle.golden_symbols.json | 5 +- .../test/render3/change_detection_spec.ts | 143 +++++++++++------- packages/core/test/render3/listeners_spec.ts | 29 +++- 7 files changed, 167 insertions(+), 110 deletions(-) diff --git a/packages/core/src/render3/context_discovery.ts b/packages/core/src/render3/context_discovery.ts index 88d218ba46..3c16dd5cde 100644 --- a/packages/core/src/render3/context_discovery.ts +++ b/packages/core/src/render3/context_discovery.ts @@ -29,7 +29,7 @@ export const MONKEY_PATCH_KEY_NAME = '__ngContext__'; * of the context. */ export interface LContext { - /** The component\'s view data */ + /** The component's parent view data */ lViewData: LViewData; /** The index instance of the LNode */ diff --git a/packages/core/src/render3/instructions.ts b/packages/core/src/render3/instructions.ts index 3c6873a8fc..505dafe4a7 100644 --- a/packages/core/src/render3/instructions.ts +++ b/packages/core/src/render3/instructions.ts @@ -1234,11 +1234,10 @@ export function listener( // In order to match current behavior, native DOM event listeners must be added for all // events (including outputs). if (isProceduralRenderer(renderer)) { - const wrappedListener = wrapListenerWithDirtyLogic(viewData, listenerFn); - const cleanupFn = renderer.listen(node.native, eventName, wrappedListener); + const cleanupFn = renderer.listen(node.native, eventName, listenerFn); storeCleanupFn(viewData, cleanupFn); } else { - const wrappedListener = wrapListenerWithDirtyAndDefault(viewData, listenerFn); + const wrappedListener = wrapListenerWithPreventDefault(listenerFn); node.native.addEventListener(eventName, wrappedListener, useCapture); const cleanupInstances = getCleanup(viewData); cleanupInstances.push(wrappedListener); @@ -2331,26 +2330,9 @@ export function markDirtyIfOnPush(node: LElementNode): void { } } -/** - * Wraps an event listener so its host view and its ancestor views will be marked dirty - * whenever the event fires. Necessary to support OnPush components. - */ -export function wrapListenerWithDirtyLogic( - view: LViewData, listenerFn: (e?: any) => any): (e: Event) => any { - return function(e: any) { - markViewDirty(view); - return listenerFn(e); - }; -} - -/** - * Wraps an event listener so its host view and its ancestor views will be marked dirty - * whenever the event fires. Also wraps with preventDefault behavior. - */ -export function wrapListenerWithDirtyAndDefault( - view: LViewData, listenerFn: (e?: any) => any): EventListener { - return function wrapListenerIn_markViewDirty(e: Event) { - markViewDirty(view); +/** Wraps an event listener with preventDefault behavior. */ +export function wrapListenerWithPreventDefault(listenerFn: (e?: any) => any): EventListener { + return function wrapListenerIn_preventDefault(e: Event) { if (listenerFn(e) === false) { e.preventDefault(); // Necessary for legacy browsers that don't support preventDefault (e.g. IE) @@ -2548,8 +2530,8 @@ function updateViewQuery(viewQuery: ComponentQuery<{}>| null, component: T): */ export function markDirty(component: T) { ngDevMode && assertDefined(component, 'component'); - const lViewData = readPatchedLViewData(component) !; - markViewDirty(lViewData); + const elementNode = getLElementFromComponent(component) !; + markViewDirty(elementNode.data as LViewData); } /////////////////////////////// diff --git a/packages/core/test/bundling/todo/bundle.golden_symbols.json b/packages/core/test/bundling/todo/bundle.golden_symbols.json index 4ade6663ef..4633982ba1 100644 --- a/packages/core/test/bundling/todo/bundle.golden_symbols.json +++ b/packages/core/test/bundling/todo/bundle.golden_symbols.json @@ -812,6 +812,9 @@ { "name": "makeParamDecorator" }, + { + "name": "markDirty" + }, { "name": "markDirtyIfOnPush" }, @@ -1023,9 +1026,6 @@ "name": "walkUpViews" }, { - "name": "wrapListenerWithDirtyAndDefault" - }, - { - "name": "wrapListenerWithDirtyLogic" + "name": "wrapListenerWithPreventDefault" } ] \ No newline at end of file diff --git a/packages/core/test/bundling/todo/index.ts b/packages/core/test/bundling/todo/index.ts index bb445b9b5b..eef6e234ae 100644 --- a/packages/core/test/bundling/todo/index.ts +++ b/packages/core/test/bundling/todo/index.ts @@ -9,7 +9,7 @@ import '@angular/core/test/bundling/util/src/reflect_metadata'; import {CommonModule} from '@angular/common'; -import {Component, Injectable, NgModule, ViewEncapsulation, ɵrenderComponent as renderComponent} from '@angular/core'; +import {Component, Injectable, NgModule, ViewEncapsulation, ɵmarkDirty as markDirty, ɵrenderComponent as renderComponent} from '@angular/core'; class Todo { editing: boolean; @@ -66,13 +66,13 @@ class TodoStore {

todos

+ (keyup)="$event.code == 'Enter' ? addTodo() : updateNewTodoValue($event.target.value)">
+ (click)="toggleAllTodos(toggleall.checked)">
    • @@ -115,37 +115,63 @@ class ToDoAppComponent { constructor(public todoStore: TodoStore) {} - stopEditing(todo: Todo, editedTitle: string) { - todo.title = editedTitle; + cancelEditingTodo(todo: Todo) { todo.editing = false; + markDirty(this); } - cancelEditingTodo(todo: Todo) { todo.editing = false; } - - updateEditingTodo(todo: Todo, editedTitle: string) { + finishUpdatingTodo(todo: Todo, editedTitle: string) { editedTitle = editedTitle.trim(); - todo.editing = false; if (editedTitle.length === 0) { - return this.todoStore.remove(todo); + this.remove(todo); } todo.title = editedTitle; + this.cancelEditingTodo(todo); } - editTodo(todo: Todo) { todo.editing = true; } + editTodo(todo: Todo) { + todo.editing = true; + markDirty(this); + } - removeCompleted() { this.todoStore.removeCompleted(); } + removeCompleted() { + this.todoStore.removeCompleted(); + markDirty(this); + } - toggleCompletion(todo: Todo) { this.todoStore.toggleCompletion(todo); } + toggleCompletion(todo: Todo) { + this.todoStore.toggleCompletion(todo); + markDirty(this); + } - remove(todo: Todo) { this.todoStore.remove(todo); } + remove(todo: Todo) { + this.todoStore.remove(todo); + markDirty(this); + } addTodo() { if (this.newTodoText.trim().length) { this.todoStore.add(this.newTodoText); this.newTodoText = ''; } + markDirty(this); + } + + toggleAllTodos(checked: boolean) { + this.todoStore.setAllTo(checked); + markDirty(this); + } + + updateEditedTodoValue(todo: Todo, value: string) { + todo.title = value; + markDirty(this); + } + + updateNewTodoValue(value: string) { + this.newTodoText = value; + markDirty(this); } } diff --git a/packages/core/test/bundling/todo_r2/bundle.golden_symbols.json b/packages/core/test/bundling/todo_r2/bundle.golden_symbols.json index ccaa3c9c33..b6f7911576 100644 --- a/packages/core/test/bundling/todo_r2/bundle.golden_symbols.json +++ b/packages/core/test/bundling/todo_r2/bundle.golden_symbols.json @@ -2466,10 +2466,7 @@ "name": "weekGetter" }, { - "name": "wrapListenerWithDirtyAndDefault" - }, - { - "name": "wrapListenerWithDirtyLogic" + "name": "wrapListenerWithPreventDefault" }, { "name": "wtfCreateScope" diff --git a/packages/core/test/render3/change_detection_spec.ts b/packages/core/test/render3/change_detection_spec.ts index 6c0bb46a68..44ab58f0ee 100644 --- a/packages/core/test/render3/change_detection_spec.ts +++ b/packages/core/test/render3/change_detection_spec.ts @@ -192,18 +192,31 @@ describe('change detection', () => { expect(getRenderedText(myApp)).toEqual('3 - George'); }); - it('should check OnPush components in update mode when component events occur', () => { - const myApp = renderComponent(MyApp); - expect(getRenderedText(myApp)).toEqual('1 - Nancy'); + 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'); - const button = containerEl.querySelector('button') !; - button.click(); - requestAnimationFrame.flush(); - expect(getRenderedText(myApp)).toEqual('2 - 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); - expect(getRenderedText(myApp)).toEqual('2 - 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 OnPush components in update mode when parent events occur', () => { function noop() {} @@ -222,62 +235,78 @@ describe('change detection', () => { const button = containerEl.querySelector('button#parent') !; (button as HTMLButtonElement).click(); - requestAnimationFrame.flush(); + tick(buttonParent); + // The comp should still be clean. So doCheck will run, but the view should display 1. expect(getRenderedText(buttonParent)).toEqual('1 - Nancy'); }); - it('should check parent OnPush components in update mode when child events occur', () => { - let parent: ButtonParent; + 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++; } + 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'); - } - }, 1, 0, [ButtonParent]); + 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'); + 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'); + 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(); - expect(parent !.doCheckCount).toEqual(3); - expect(comp !.doCheckCount).toEqual(2); - expect(getRenderedText(myButtonApp)).toEqual('3 - 2 - 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); + // 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', () => { diff --git a/packages/core/test/render3/listeners_spec.ts b/packages/core/test/render3/listeners_spec.ts index 2d8d104ea5..ae0e89052b 100644 --- a/packages/core/test/render3/listeners_spec.ts +++ b/packages/core/test/render3/listeners_spec.ts @@ -6,12 +6,12 @@ * found in the LICENSE file at https://angular.io/license */ -import {defineComponent, defineDirective} from '../../src/render3/index'; +import {bind, defineComponent, defineDirective, markDirty, textBinding} from '../../src/render3/index'; import {container, containerRefreshEnd, containerRefreshStart, element, elementEnd, elementStart, embeddedViewEnd, embeddedViewStart, listener, text} from '../../src/render3/instructions'; import {RenderFlags} from '../../src/render3/interfaces/definition'; import {getRendererFactory2} from './imported_renderer2'; -import {ComponentFixture, containerEl, renderComponent, renderToHtml} from './render_util'; +import {ComponentFixture, containerEl, renderComponent, renderToHtml, requestAnimationFrame} from './render_util'; describe('event listeners', () => { @@ -363,6 +363,7 @@ describe('event listeners', () => { /** * % for (let i = 0; i < ctx.buttons; i++) { * + * {{ counters[i] }} * % } */ class AppComp { @@ -385,13 +386,20 @@ describe('event listeners', () => { containerRefreshStart(0); { for (let i = 0; i < ctx.buttons; i++) { - if (embeddedViewStart(1, 2, 0)) { + const rf1 = embeddedViewStart(1, 4, 1); + if (rf1 & RenderFlags.Create) { elementStart(0, 'button'); { listener('click', function() { return ctx.onClick(i); }); text(1, 'Click me'); } elementEnd(); + elementStart(2, 'div'); + { text(3); } + elementEnd(); + } + if (rf1 & RenderFlags.Update) { + textBinding(3, bind(ctx.counters[i])); } embeddedViewEnd(); } @@ -405,12 +413,27 @@ describe('event listeners', () => { const fixture = new ComponentFixture(AppComp, {rendererFactory: getRendererFactory2(document)}); const comp = fixture.component; const buttons = fixture.hostElement.querySelectorAll('button') !; + const divs = fixture.hostElement.querySelectorAll('div'); buttons[0].click(); expect(comp.counters).toEqual([1, 0]); + expect(divs[0].textContent).toEqual('0'); + expect(divs[1].textContent).toEqual('0'); + + markDirty(comp); + requestAnimationFrame.flush(); + expect(divs[0].textContent).toEqual('1'); + expect(divs[1].textContent).toEqual('0'); buttons[1].click(); expect(comp.counters).toEqual([1, 1]); + expect(divs[0].textContent).toEqual('1'); + expect(divs[1].textContent).toEqual('0'); + + markDirty(comp); + requestAnimationFrame.flush(); + expect(divs[0].textContent).toEqual('1'); + expect(divs[1].textContent).toEqual('1'); // the listener should be removed when the view is removed comp.buttons = 0;