fix(ivy): compiler should generate restoreView() for local refs in listeners (#27034)

PR Close #27034

packages/compiler-cli/test/compliance/r3_view_compiler_listener_spec.ts

@@ -206,8 +206,10 @@ describe('compiler compliance: listen()', () => {
           vars: 0,
           template:  function MyComponent_Template(rf, ctx) {
             if (rf & 1) {
+              const $s$ = $r3$.ɵgetCurrentView();
               $r3$.ɵelementStart(0, "button", $e0_attrs$);
                 $r3$.ɵlistener("click", function MyComponent_Template_button_click_listener($event) {
+                   $r3$.ɵrestoreView($s$);
                    const $user$ = $r3$.ɵreference(3);
                    return ctx.onClick($user$.value);
                 });

packages/compiler/src/render3/view/template.ts

@@ -209,7 +209,8 @@ export class TemplateDefinitionBuilder implements t.Visitor<void>, LocalResolver
     const updateStatements = this._updateCodeFns.map((fn: () => o.Statement) => fn());
 
     //  Variable declaration must occur after binding resolution so we can generate context
-    //  instructions that build on each other. e.g. const b = x().$implicit(); const b = x();
+    //  instructions that build on each other.
+    // e.g. const b = nextContext().$implicit(); const b = nextContext();
     const creationVariables = this._bindingScope.viewSnapshotStatements();
     const updateVariables = this._bindingScope.variableDeclarations().concat(this._tempVariables);
 
@@ -1017,16 +1018,16 @@ export class TemplateDefinitionBuilder implements t.Visitor<void>, LocalResolver
       const retrievalLevel = this.level;
       const lhs = o.variable(variableName);
       this._bindingScope.set(
-          retrievalLevel, reference.name, lhs, DeclarationPriority.DEFAULT,
+          retrievalLevel, reference.name, lhs,
-          (scope: BindingScope, relativeLevel: number) => {
+          DeclarationPriority.DEFAULT, (scope: BindingScope, relativeLevel: number) => {
-            // e.g. x(2);
+            // e.g. nextContext(2);
             const nextContextStmt =
                 relativeLevel > 0 ? [generateNextContextExpr(relativeLevel).toStmt()] : [];
 
-            // e.g. const $foo$ = r(1);
+            // e.g. const $foo$ = reference(1);
             const refExpr = lhs.set(o.importExpr(R3.reference).callFn([o.literal(slot)]));
             return nextContextStmt.concat(refExpr.toConstDecl());
-          });
+          }, true);
       return [reference.name, reference.value];
     }));
 
@@ -1201,13 +1202,14 @@ export type DeclareLocalVarCallback = (scope: BindingScope, relativeLevel: numbe
 const SHARED_CONTEXT_KEY = '$$shared_ctx$$';
 
 /**
- * This is used when one refers to variable such as: 'let abc = x(2).$implicit`.
+ * This is used when one refers to variable such as: 'let abc = nextContext(2).$implicit`.
  * - key to the map is the string literal `"abc"`.
  * - value `retrievalLevel` is the level from which this value can be retrieved, which is 2 levels
  * up in example.
  * - value `lhs` is the left hand side which is an AST representing `abc`.
  * - value `declareLocalCallback` is a callback that is invoked when declaring the local.
  * - value `declare` is true if this value needs to be declared.
+ * - value `localRef` is true if we are storing a local reference
  * - value `priority` dictates the sorting priority of this var declaration compared
  * to other var declarations on the same retrieval level. For example, if there is a
  * context variable and a local ref accessing the same parent view, the context var
@@ -1217,6 +1219,7 @@ type BindingData = {
   retrievalLevel: number; lhs: o.ReadVarExpr; declareLocalCallback?: DeclareLocalVarCallback;
   declare: boolean;
   priority: number;
+  localRef: boolean;
 };
 
 /**
@@ -1253,14 +1256,15 @@ export class BindingScope implements LocalResolver {
             lhs: value.lhs,
             declareLocalCallback: value.declareLocalCallback,
             declare: false,
-            priority: value.priority
+            priority: value.priority,
+            localRef: value.localRef
           };
 
           // Cache the value locally.
           this.map.set(name, value);
           // Possibly generate a shared context var
           this.maybeGenerateSharedContextVar(value);
-          this.maybeRestoreView(value.retrievalLevel);
+          this.maybeRestoreView(value.retrievalLevel, value.localRef);
         }
 
         if (value.declareLocalCallback && !value.declare) {
@@ -1286,10 +1290,11 @@ export class BindingScope implements LocalResolver {
    * @param lhs AST representing the left hand side of the `let lhs = rhs;`.
    * @param priority The sorting priority of this var
    * @param declareLocalCallback The callback to invoke when declaring this local var
+   * @param localRef Whether or not this is a local ref
    */
   set(retrievalLevel: number, name: string, lhs: o.ReadVarExpr,
       priority: number = DeclarationPriority.DEFAULT,
-      declareLocalCallback?: DeclareLocalVarCallback): BindingScope {
+      declareLocalCallback?: DeclareLocalVarCallback, localRef?: true): BindingScope {
     !this.map.has(name) ||
         error(`The name ${name} is already defined in scope to be ${this.map.get(name)}`);
     this.map.set(name, {
@@ -1297,7 +1302,8 @@ export class BindingScope implements LocalResolver {
       lhs: lhs,
       declare: false,
       declareLocalCallback: declareLocalCallback,
-      priority: priority
+      priority: priority,
+      localRef: localRef || false
     });
     return this;
   }
@@ -1332,25 +1338,31 @@ export class BindingScope implements LocalResolver {
       retrievalLevel: retrievalLevel,
       lhs: lhs,
       declareLocalCallback: (scope: BindingScope, relativeLevel: number) => {
-        // const ctx_r0 = x(2);
+        // const ctx_r0 = nextContext(2);
         return [lhs.set(generateNextContextExpr(relativeLevel)).toConstDecl()];
       },
       declare: false,
-      priority: DeclarationPriority.SHARED_CONTEXT
+      priority: DeclarationPriority.SHARED_CONTEXT,
+      localRef: false
     });
   }
 
   getComponentProperty(name: string): o.Expression {
     const componentValue = this.map.get(SHARED_CONTEXT_KEY + 0) !;
     componentValue.declare = true;
-    this.maybeRestoreView(0);
+    this.maybeRestoreView(0, false);
     return componentValue.lhs.prop(name);
   }
 
-  maybeRestoreView(retrievalLevel: number) {
+  maybeRestoreView(retrievalLevel: number, localRefLookup: boolean) {
-    if (this.isListenerScope() && retrievalLevel < this.bindingLevel) {
+    // We want to restore the current view in listener fns if:
+    // 1 - we are accessing a value in a parent view, which requires walking the view tree rather
+    // than using the ctx arg. In this case, the retrieval and binding level will be different.
+    // 2 - we are looking up a local ref, which requires restoring the view where the local
+    // ref is stored
+    if (this.isListenerScope() && (retrievalLevel < this.bindingLevel || localRefLookup)) {
       if (!this.parent !.restoreViewVariable) {
-        // parent saves variable to generate a shared `const $s$ = gV();` instruction
+        // parent saves variable to generate a shared `const $s$ = getCurrentView();` instruction
         this.parent !.restoreViewVariable = o.variable(this.parent !.freshReferenceName());
       }
       this.restoreViewVariable = this.parent !.restoreViewVariable;
@@ -1358,14 +1370,14 @@ export class BindingScope implements LocalResolver {
   }
 
   restoreViewStatement(): o.Statement[] {
-    // rV($state$);
+    // restoreView($state$);
     return this.restoreViewVariable ?
         [instruction(null, R3.restoreView, [this.restoreViewVariable]).toStmt()] :
         [];
   }
 
   viewSnapshotStatements(): o.Statement[] {
-    // const $state$ = gV();
+    // const $state$ = getCurrentView();
     const getCurrentViewInstruction = instruction(null, R3.getCurrentView, []);
     return this.restoreViewVariable ?
         [this.restoreViewVariable.set(getCurrentViewInstruction).toConstDecl()] :

packages/core/test/render3/listeners_spec.ts

@@ -6,12 +6,13 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
-import {bind, defineComponent, defineDirective, markDirty, textBinding} from '../../src/render3/index';
+import {bind, defineComponent, defineDirective, markDirty, reference, 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 {getCurrentView, restoreView} from '../../src/render3/state';
 
 import {getRendererFactory2} from './imported_renderer2';
-import {ComponentFixture, containerEl, renderToHtml, requestAnimationFrame} from './render_util';
+import {ComponentFixture, containerEl, createComponent, getDirectiveOnNode, renderToHtml, requestAnimationFrame} from './render_util';
 
 
 describe('event listeners', () => {
@@ -723,4 +724,54 @@ describe('event listeners', () => {
 
   });
 
+  it('should support local refs in listeners', () => {
+    let compInstance: any;
+
+    const Comp = createComponent('comp', (rf: RenderFlags, ctx: any) => {});
+
+    /**
+     * <comp #comp></comp>
+     * <button (click)="onClick(comp)"></button>
+     */
+    class App {
+      comp: any = null;
+
+      onClick(comp: any) { this.comp = comp; }
+
+      static ngComponentDef = defineComponent({
+        type: App,
+        selectors: [['app']],
+        factory: () => new App(),
+        consts: 3,
+        vars: 0,
+        template: (rf: RenderFlags, ctx: App) => {
+          if (rf & RenderFlags.Create) {
+            const state = getCurrentView();
+            element(0, 'comp', null, ['comp', '']);
+            elementStart(2, 'button');
+            {
+              listener('click', function() {
+                restoreView(state);
+                const comp = reference(1);
+                return ctx.onClick(comp);
+              });
+            }
+            elementEnd();
+          }
+
+          // testing only
+          compInstance = getDirectiveOnNode(0);
+        },
+        directives: [Comp]
+      });
+    }
+
+    const fixture = new ComponentFixture(App);
+    expect(fixture.component.comp).toEqual(null);
+
+    const button = fixture.hostElement.querySelector('button') as HTMLButtonElement;
+    button.click();
+    expect(fixture.component.comp).toEqual(compInstance);
+  });
+
 });

packages/forms/test/template_integration_spec.ts

@@ -1489,36 +1489,35 @@ import {NgModelCustomComp, NgModelCustomWrapper} from './value_accessor_integrat
            expect(required.nativeElement.getAttribute('pattern')).toEqual(null);
          }));
 
-      fixmeIvy('ngModelChange callback never called') &&
+      it('should update control status', fakeAsync(() => {
-          it('should update control status', fakeAsync(() => {
+           const fixture = initTest(NgModelChangeState);
-               const fixture = initTest(NgModelChangeState);
+           const inputEl = fixture.debugElement.query(By.css('input'));
-               const inputEl = fixture.debugElement.query(By.css('input'));
+           const inputNativeEl = inputEl.nativeElement;
-               const inputNativeEl = inputEl.nativeElement;
+           const onNgModelChange = jasmine.createSpy('onNgModelChange');
-               const onNgModelChange = jasmine.createSpy('onNgModelChange');
+           fixture.componentInstance.onNgModelChange = onNgModelChange;
-               fixture.componentInstance.onNgModelChange = onNgModelChange;
+           fixture.detectChanges();
-               fixture.detectChanges();
+           tick();
-               tick();
 
-               expect(onNgModelChange).not.toHaveBeenCalled();
+           expect(onNgModelChange).not.toHaveBeenCalled();
 
-               inputNativeEl.value = 'updated';
+           inputNativeEl.value = 'updated';
-               onNgModelChange.and.callFake((ngModel: NgModel) => {
+           onNgModelChange.and.callFake((ngModel: NgModel) => {
-                 expect(ngModel.invalid).toBe(true);
+             expect(ngModel.invalid).toBe(true);
-                 expect(ngModel.value).toBe('updated');
+             expect(ngModel.value).toBe('updated');
-               });
+           });
-               dispatchEvent(inputNativeEl, 'input');
+           dispatchEvent(inputNativeEl, 'input');
-               expect(onNgModelChange).toHaveBeenCalled();
+           expect(onNgModelChange).toHaveBeenCalled();
-               tick();
+           tick();
 
-               inputNativeEl.value = '333';
+           inputNativeEl.value = '333';
-               onNgModelChange.and.callFake((ngModel: NgModel) => {
+           onNgModelChange.and.callFake((ngModel: NgModel) => {
-                 expect(ngModel.invalid).toBe(false);
+             expect(ngModel.invalid).toBe(false);
-                 expect(ngModel.value).toBe('333');
+             expect(ngModel.value).toBe('333');
-               });
+           });
-               dispatchEvent(inputNativeEl, 'input');
+           dispatchEvent(inputNativeEl, 'input');
-               expect(onNgModelChange).toHaveBeenCalledTimes(2);
+           expect(onNgModelChange).toHaveBeenCalledTimes(2);
-               tick();
+           tick();
-             }));
+         }));
 
     });