fix(ivy): events should not mark views dirty by default (#25969)

PR Close #25969

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 */

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, listenerFn);
-      const cleanupFn = renderer.listen(node.native, eventName, wrappedListener);
       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 with preventDefault behavior. */
- * Wraps an event listener so its host view and its ancestor views will be marked dirty
+export function wrapListenerWithPreventDefault(listenerFn: (e?: any) => any): EventListener {
- * whenever the event fires. Necessary to support OnPush components.
+  return function wrapListenerIn_preventDefault(e: Event) {
- */
-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);
     if (listenerFn(e) === false) {
       e.preventDefault();
       // Necessary for legacy browsers that don't support preventDefault (e.g. IE)
@@ -2548,8 +2530,8 @@ function updateViewQuery<T>(viewQuery: ComponentQuery<{}>| null, component: T):
  */
 export function markDirty<T>(component: T) {
   ngDevMode && assertDefined(component, 'component');
-  const lViewData = readPatchedLViewData(component) !;
+  const elementNode = getLElementFromComponent(component) !;
-  markViewDirty(lViewData);
+  markViewDirty(elementNode.data as LViewData);
 }
 
 ///////////////////////////////

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": "wrapListenerWithPreventDefault"
-  },
-  {
-    "name": "wrapListenerWithDirtyLogic"
   }
 ]

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 {
       <h1>todos</h1>
       <input class="new-todo" placeholder="What needs to be done?" autofocus=""
              [value]="newTodoText"
-             (keyup)="$event.code == 'Enter' ? addTodo() : newTodoText = $event.target.value">
+             (keyup)="$event.code == 'Enter' ? addTodo() : updateNewTodoValue($event.target.value)">
     </header>
     <section *ngIf="todoStore.todos.length > 0" class="main">
       <input *ngIf="todoStore.todos.length"
              #toggleall class="toggle-all" type="checkbox"
              [checked]="todoStore.allCompleted()"
-             (click)="todoStore.setAllTo(toggleall.checked)">
+             (click)="toggleAllTodos(toggleall.checked)">
       <ul class="todo-list">
         <li *ngFor="let todo of todoStore.todos"
             [class.completed]="todo.completed"
@@ -87,8 +87,8 @@ class TodoStore {
           <input *ngIf="todo.editing"
                  class="edit" #editedtodo
                  [value]="todo.title"
-                 (blur)="stopEditing(todo, editedtodo.value)"
+                 (blur)="updateEditingTodo(todo, editedtodo.value)"
-                 (keyup)="todo.title = $event.target.value"
+                 (keyup)="updateEditedTodoValue($event.target.value)"
                  (keyup)="$event.code == 'Enter' && updateEditingTodo(todo, editedtodo.value)"
                  (keyup)="$event.code == 'Escape' && cancelEditingTodo(todo)">
         </li>
@@ -115,37 +115,63 @@ class ToDoAppComponent {
 
   constructor(public todoStore: TodoStore) {}
 
-  stopEditing(todo: Todo, editedTitle: string) {
+  cancelEditingTodo(todo: Todo) {
-    todo.title = editedTitle;
     todo.editing = false;
+    markDirty(this);
   }
 
-  cancelEditingTodo(todo: Todo) { todo.editing = false; }
+  finishUpdatingTodo(todo: Todo, editedTitle: string) {
-
-  updateEditingTodo(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);
   }
 }
 

packages/core/test/bundling/todo_r2/bundle.golden_symbols.json

@@ -2466,10 +2466,7 @@
     "name": "weekGetter"
   },
   {
-    "name": "wrapListenerWithDirtyAndDefault"
+    "name": "wrapListenerWithPreventDefault"
-  },
-  {
-    "name": "wrapListenerWithDirtyLogic"
   },
   {
     "name": "wtfCreateScope"

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', () => {
+    it('should not check OnPush components in update mode when component events occur, unless marked dirty',
-      const myApp = renderComponent(MyApp);
+       () => {
-      expect(getRenderedText(myApp)).toEqual('1 - Nancy');
+         const myApp = renderComponent(MyApp);
+         expect(comp.doCheckCount).toEqual(1);
+         expect(getRenderedText(myApp)).toEqual('1 - Nancy');
 
-      const button = containerEl.querySelector('button') !;
+         const button = containerEl.querySelector('button') !;
-      button.click();
+         button.click();
-      requestAnimationFrame.flush();
+         requestAnimationFrame.flush();
-      expect(getRenderedText(myApp)).toEqual('2 - Nancy');
+         // No ticks should have been scheduled.
+         expect(comp.doCheckCount).toEqual(1);
+         expect(getRenderedText(myApp)).toEqual('1 - Nancy');
 
-      tick(myApp);
+         tick(myApp);
-      expect(getRenderedText(myApp)).toEqual('2 - Nancy');
+         // 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', () => {
+    it('should not check parent OnPush components in update mode when child events occur, unless marked dirty',
-      let parent: ButtonParent;
+       () => {
+         let parent: ButtonParent;
 
-      class ButtonParent implements DoCheck {
+         class ButtonParent implements DoCheck {
-        doCheckCount = 0;
+           doCheckCount = 0;
-        ngDoCheck(): void { this.doCheckCount++; }
+           ngDoCheck(): void { this.doCheckCount++; }
 
-        static ngComponentDef = defineComponent({
+           static ngComponentDef = defineComponent({
-          type: ButtonParent,
+             type: ButtonParent,
-          selectors: [['button-parent']],
+             selectors: [['button-parent']],
-          factory: () => parent = new ButtonParent(),
+             factory: () => parent = new ButtonParent(),
-          consts: 2,
+             consts: 2,
-          vars: 1,
+             vars: 1,
-          /** {{ doCheckCount }} - <my-comp></my-comp> */
+             /** {{ doCheckCount }} - <my-comp></my-comp> */
-          template: (rf: RenderFlags, ctx: ButtonParent) => {
+             template: (rf: RenderFlags, ctx: ButtonParent) => {
-            if (rf & RenderFlags.Create) {
+               if (rf & RenderFlags.Create) {
-              text(0);
+                 text(0);
-              element(1, 'my-comp');
+                 element(1, 'my-comp');
-            }
+               }
-            if (rf & RenderFlags.Update) {
+               if (rf & RenderFlags.Update) {
-              textBinding(0, interpolation1('', ctx.doCheckCount, ' - '));
+                 textBinding(0, interpolation1('', ctx.doCheckCount, ' - '));
-            }
+               }
-          },
+             },
-          directives: () => [MyComponent],
+             directives: () => [MyComponent],
-          changeDetection: ChangeDetectionStrategy.OnPush
+             changeDetection: ChangeDetectionStrategy.OnPush
-        });
+           });
-      }
+         }
 
-      const MyButtonApp = createComponent('my-button-app', function(rf: RenderFlags, ctx: any) {
+         const MyButtonApp = createComponent('my-button-app', function(rf: RenderFlags, ctx: any) {
-        if (rf & RenderFlags.Create) {
+           if (rf & RenderFlags.Create) {
-          element(0, 'button-parent');
+             element(0, 'button-parent');
-        }
+           }
-      }, 1, 0, [ButtonParent]);
+         }, 1, 0, [ButtonParent]);
 
-      const myButtonApp = renderComponent(MyButtonApp);
+         const myButtonApp = renderComponent(MyButtonApp);
-      expect(parent !.doCheckCount).toEqual(1);
+         expect(parent !.doCheckCount).toEqual(1);
-      expect(comp !.doCheckCount).toEqual(1);
+         expect(comp !.doCheckCount).toEqual(1);
-      expect(getRenderedText(myButtonApp)).toEqual('1 - 1 - Nancy');
+         expect(getRenderedText(myButtonApp)).toEqual('1 - 1 - Nancy');
 
-      tick(myButtonApp);
+         tick(myButtonApp);
-      expect(parent !.doCheckCount).toEqual(2);
+         expect(parent !.doCheckCount).toEqual(2);
-      // parent isn't checked, so child doCheck won't run
+         // parent isn't checked, so child doCheck won't run
-      expect(comp !.doCheckCount).toEqual(1);
+         expect(comp !.doCheckCount).toEqual(1);
-      expect(getRenderedText(myButtonApp)).toEqual('1 - 1 - Nancy');
+         expect(getRenderedText(myButtonApp)).toEqual('1 - 1 - Nancy');
 
-      const button = containerEl.querySelector('button');
+         const button = containerEl.querySelector('button');
-      button !.click();
+         button !.click();
-      requestAnimationFrame.flush();
+         requestAnimationFrame.flush();
-      expect(parent !.doCheckCount).toEqual(3);
+         // No ticks should have been scheduled.
-      expect(comp !.doCheckCount).toEqual(2);
+         expect(parent !.doCheckCount).toEqual(2);
-      expect(getRenderedText(myButtonApp)).toEqual('3 - 2 - Nancy');
+         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', () => {

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++) {
        *  <button (click)="onClick(i)"> Click me </button>
+       *    {{ 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;