From 0717fe241dcaa40490bde4471391583071f3bb70 Mon Sep 17 00:00:00 2001
From: Lucas Perais <lpe@odoo.com>
Date: Mon, 13 Feb 2023 11:51:17 +0100
Subject: [PATCH] [FIX] compiler: t-key and t-ref together

Have a t-ref on a DOM node with a t-key.
Change the t-key.

Before this commit, the old block removed its element from the component's refs *after*
the new block had been mounted, meaning that in effect, the resulting
ref at the end of the whole patch was null.

After this commit, we only remove an element from the component's ref if that very same
element was indeed the ref. (otherwise it means someone else has changed the ref.)
---
 src/compiler/code_generator.ts                |  6 ++-
 src/runtime/template_helpers.ts               | 11 +++++
 .../compiler/__snapshots__/misc.test.ts.snap  |  6 +--
 .../compiler/__snapshots__/t_ref.test.ts.snap | 24 ++++++-----
 .../__snapshots__/hooks.test.ts.snap          | 16 +++++---
 .../__snapshots__/refs.test.ts.snap           | 36 ++++++++++++++---
 .../__snapshots__/slots.test.ts.snap          |  4 +-
 .../__snapshots__/t_call.test.ts.snap         |  6 +--
 tests/components/refs.test.ts                 | 40 ++++++++++++++++++-
 9 files changed, 117 insertions(+), 32 deletions(-)

diff --git a/src/compiler/code_generator.ts b/src/compiler/code_generator.ts
index 97ee84095..3b9a0e723 100644
--- a/src/compiler/code_generator.ts
+++ b/src/compiler/code_generator.ts
@@ -672,8 +672,9 @@ export class CodeGenerator {
       this.target.hasRef = true;
       const isDynamic = INTERP_REGEXP.test(ast.ref);
       if (isDynamic) {
+        this.helpers.add("singleRefSetter");
         const str = replaceDynamicParts(ast.ref, (expr) => this.captureExpression(expr, true));
-        const idx = block!.insertData(`(el) => refs[${str}] = el`, "ref");
+        const idx = block!.insertData(`singleRefSetter(refs, ${str})`, "ref");
         attrs["block-ref"] = String(idx);
       } else {
         let name = ast.ref;
@@ -686,7 +687,8 @@ export class CodeGenerator {
           info[1] = `multiRefSetter(refs, \`${name}\`)`;
         } else {
           let id = generateId("ref");
-          this.target.refInfo[name] = [id, `(el) => refs[\`${name}\`] = el`];
+          this.helpers.add("singleRefSetter");
+          this.target.refInfo[name] = [id, `singleRefSetter(refs, \`${name}\`)`];
           const index = block!.data.push(id) - 1;
           attrs["block-ref"] = String(index);
         }
diff --git a/src/runtime/template_helpers.ts b/src/runtime/template_helpers.ts
index 82a68e1ae..0085d7deb 100644
--- a/src/runtime/template_helpers.ts
+++ b/src/runtime/template_helpers.ts
@@ -202,6 +202,16 @@ function multiRefSetter(refs: RefMap, name: string): RefSetter {
   };
 }
 
+function singleRefSetter(refs: RefMap, name: string): RefSetter {
+  let _el: HTMLElement | null = null;
+  return (el) => {
+    if (el || refs[name] === _el) {
+      refs[name] = el;
+      _el = el;
+    }
+  };
+}
+
 /**
  * Validate the component props (or next props) against the (static) props
  * description.  This is potentially an expensive operation: it may needs to
@@ -260,6 +270,7 @@ export const helpers = {
   prepareList,
   setContextValue,
   multiRefSetter,
+  singleRefSetter,
   shallowEqual,
   toNumber,
   validateProps,
diff --git a/tests/compiler/__snapshots__/misc.test.ts.snap b/tests/compiler/__snapshots__/misc.test.ts.snap
index 33f7acc24..b7a7ee296 100644
--- a/tests/compiler/__snapshots__/misc.test.ts.snap
+++ b/tests/compiler/__snapshots__/misc.test.ts.snap
@@ -182,7 +182,7 @@ exports[`misc other complex template 1`] = `
 "function anonymous(app, bdom, helpers
 ) {
   let { text, createBlock, list, multi, html, toggler, comment } = bdom;
-  let { prepareList, withKey } = helpers;
+  let { prepareList, withKey, singleRefSetter } = helpers;
   const callTemplate_1 = app.getTemplate(\`LOAD_INFOS_TEMPLATE\`);
   const comp1 = app.createComponent(\`BundlesList\`, true, false, false, false);
   const comp2 = app.createComponent(\`BundlesList\`, true, false, false, false);
@@ -218,8 +218,8 @@ exports[`misc other complex template 1`] = `
   
   return function template(ctx, node, key = \\"\\") {
     const refs = this.__owl__.refs;
-    const ref1 = (el) => refs[\`search_input\`] = el;
-    const ref2 = (el) => refs[\`settings_menu\`] = el;
+    const ref1 = singleRefSetter(refs, \`search_input\`);
+    const ref2 = singleRefSetter(refs, \`settings_menu\`);
     let b2,b4,b14,b17,b22,b23,b24,b25;
     let attr1 = \`/runbot/\${ctx['project'].slug}\`;
     let txt1 = ctx['project'].name;
diff --git a/tests/compiler/__snapshots__/t_ref.test.ts.snap b/tests/compiler/__snapshots__/t_ref.test.ts.snap
index 6048615ae..22ece76fe 100644
--- a/tests/compiler/__snapshots__/t_ref.test.ts.snap
+++ b/tests/compiler/__snapshots__/t_ref.test.ts.snap
@@ -4,13 +4,14 @@ exports[`t-ref can get a dynamic ref on a node 1`] = `
 "function anonymous(app, bdom, helpers
 ) {
   let { text, createBlock, list, multi, html, toggler, comment } = bdom;
+  let { singleRefSetter } = helpers;
   
   let block1 = createBlock(\`<div><span block-ref=\\"0\\"/></div>\`);
   
   return function template(ctx, node, key = \\"\\") {
     const refs = this.__owl__.refs;
     const v1 = ctx['id'];
-    let ref1 = (el) => refs[\`myspan\${v1}\`] = el;
+    let ref1 = singleRefSetter(refs, \`myspan\${v1}\`);
     return block1([ref1]);
   }
 }"
@@ -20,13 +21,14 @@ exports[`t-ref can get a dynamic ref on a node, alternate syntax 1`] = `
 "function anonymous(app, bdom, helpers
 ) {
   let { text, createBlock, list, multi, html, toggler, comment } = bdom;
+  let { singleRefSetter } = helpers;
   
   let block1 = createBlock(\`<div><span block-ref=\\"0\\"/></div>\`);
   
   return function template(ctx, node, key = \\"\\") {
     const refs = this.__owl__.refs;
     const v1 = ctx['id'];
-    let ref1 = (el) => refs[\`myspan\${v1}\`] = el;
+    let ref1 = singleRefSetter(refs, \`myspan\${v1}\`);
     return block1([ref1]);
   }
 }"
@@ -36,12 +38,13 @@ exports[`t-ref can get a ref on a node 1`] = `
 "function anonymous(app, bdom, helpers
 ) {
   let { text, createBlock, list, multi, html, toggler, comment } = bdom;
+  let { singleRefSetter } = helpers;
   
   let block1 = createBlock(\`<div><span block-ref=\\"0\\"/></div>\`);
   
   return function template(ctx, node, key = \\"\\") {
     const refs = this.__owl__.refs;
-    const ref1 = (el) => refs[\`myspan\`] = el;
+    const ref1 = singleRefSetter(refs, \`myspan\`);
     return block1([ref1]);
   }
 }"
@@ -66,12 +69,13 @@ exports[`t-ref ref in a t-call 2`] = `
 "function anonymous(app, bdom, helpers
 ) {
   let { text, createBlock, list, multi, html, toggler, comment } = bdom;
+  let { singleRefSetter } = helpers;
   
   let block1 = createBlock(\`<div>1<span block-ref=\\"0\\"/>2</div>\`);
   
   return function template(ctx, node, key = \\"\\") {
     const refs = this.__owl__.refs;
-    const ref1 = (el) => refs[\`name\`] = el;
+    const ref1 = singleRefSetter(refs, \`name\`);
     return block1([ref1]);
   }
 }"
@@ -81,13 +85,14 @@ exports[`t-ref ref in a t-if 1`] = `
 "function anonymous(app, bdom, helpers
 ) {
   let { text, createBlock, list, multi, html, toggler, comment } = bdom;
+  let { singleRefSetter } = helpers;
   
   let block1 = createBlock(\`<div><block-child-0/></div>\`);
   let block2 = createBlock(\`<span block-ref=\\"0\\"/>\`);
   
   return function template(ctx, node, key = \\"\\") {
     const refs = this.__owl__.refs;
-    const ref1 = (el) => refs[\`name\`] = el;
+    const ref1 = singleRefSetter(refs, \`name\`);
     let b2;
     if (ctx['condition']) {
       b2 = block2([ref1]);
@@ -101,7 +106,7 @@ exports[`t-ref refs in a loop 1`] = `
 "function anonymous(app, bdom, helpers
 ) {
   let { text, createBlock, list, multi, html, toggler, comment } = bdom;
-  let { prepareList, withKey } = helpers;
+  let { prepareList, singleRefSetter, withKey } = helpers;
   
   let block1 = createBlock(\`<div><block-child-0/></div>\`);
   let block3 = createBlock(\`<div block-ref=\\"0\\"><block-text-1/></div>\`);
@@ -115,7 +120,7 @@ exports[`t-ref refs in a loop 1`] = `
       const key1 = ctx['item'];
       const tKey_1 = ctx['item'];
       const v1 = ctx['item'];
-      let ref1 = (el) => refs[(v1)] = el;
+      let ref1 = singleRefSetter(refs, (v1));
       let txt1 = ctx['item'];
       c_block2[i1] = withKey(block3([ref1, txt1]), tKey_1 + key1);
     }
@@ -129,14 +134,15 @@ exports[`t-ref two refs, one in a t-if 1`] = `
 "function anonymous(app, bdom, helpers
 ) {
   let { text, createBlock, list, multi, html, toggler, comment } = bdom;
+  let { singleRefSetter } = helpers;
   
   let block1 = createBlock(\`<div><block-child-0/><p block-ref=\\"0\\"/></div>\`);
   let block2 = createBlock(\`<span block-ref=\\"0\\"/>\`);
   
   return function template(ctx, node, key = \\"\\") {
     const refs = this.__owl__.refs;
-    const ref1 = (el) => refs[\`name\`] = el;
-    const ref2 = (el) => refs[\`p\`] = el;
+    const ref1 = singleRefSetter(refs, \`name\`);
+    const ref2 = singleRefSetter(refs, \`p\`);
     let b2;
     if (ctx['condition']) {
       b2 = block2([ref1]);
diff --git a/tests/components/__snapshots__/hooks.test.ts.snap b/tests/components/__snapshots__/hooks.test.ts.snap
index 3fd4d391f..e7c427f7e 100644
--- a/tests/components/__snapshots__/hooks.test.ts.snap
+++ b/tests/components/__snapshots__/hooks.test.ts.snap
@@ -4,14 +4,15 @@ exports[`hooks autofocus hook input in a t-if 1`] = `
 "function anonymous(app, bdom, helpers
 ) {
   let { text, createBlock, list, multi, html, toggler, comment } = bdom;
+  let { singleRefSetter } = helpers;
   
   let block1 = createBlock(\`<div><input block-ref=\\"0\\"/><block-child-0/></div>\`);
   let block2 = createBlock(\`<input block-ref=\\"0\\"/>\`);
   
   return function template(ctx, node, key = \\"\\") {
     const refs = this.__owl__.refs;
-    const ref1 = (el) => refs[\`input1\`] = el;
-    const ref2 = (el) => refs[\`input2\`] = el;
+    const ref1 = singleRefSetter(refs, \`input1\`);
+    const ref2 = singleRefSetter(refs, \`input2\`);
     let b2;
     if (ctx['state'].flag) {
       b2 = block2([ref2]);
@@ -25,13 +26,14 @@ exports[`hooks autofocus hook simple input 1`] = `
 "function anonymous(app, bdom, helpers
 ) {
   let { text, createBlock, list, multi, html, toggler, comment } = bdom;
+  let { singleRefSetter } = helpers;
   
   let block1 = createBlock(\`<div><input block-ref=\\"0\\"/><input block-ref=\\"1\\"/></div>\`);
   
   return function template(ctx, node, key = \\"\\") {
     const refs = this.__owl__.refs;
-    const ref1 = (el) => refs[\`input1\`] = el;
-    const ref2 = (el) => refs[\`input2\`] = el;
+    const ref1 = singleRefSetter(refs, \`input1\`);
+    const ref2 = singleRefSetter(refs, \`input2\`);
     return block1([ref1, ref2]);
   }
 }"
@@ -264,12 +266,13 @@ exports[`hooks useEffect hook effect can depend on stuff in dom 1`] = `
 "function anonymous(app, bdom, helpers
 ) {
   let { text, createBlock, list, multi, html, toggler, comment } = bdom;
+  let { singleRefSetter } = helpers;
   
   let block2 = createBlock(\`<div block-ref=\\"0\\"/>\`);
   
   return function template(ctx, node, key = \\"\\") {
     const refs = this.__owl__.refs;
-    const ref1 = (el) => refs[\`div\`] = el;
+    const ref1 = singleRefSetter(refs, \`div\`);
     let b2;
     if (ctx['state'].value) {
       b2 = block2([ref1]);
@@ -353,12 +356,13 @@ exports[`hooks useRef hook: basic use 1`] = `
 "function anonymous(app, bdom, helpers
 ) {
   let { text, createBlock, list, multi, html, toggler, comment } = bdom;
+  let { singleRefSetter } = helpers;
   
   let block1 = createBlock(\`<div><button block-ref=\\"0\\"><block-text-1/></button></div>\`);
   
   return function template(ctx, node, key = \\"\\") {
     const refs = this.__owl__.refs;
-    const ref1 = (el) => refs[\`button\`] = el;
+    const ref1 = singleRefSetter(refs, \`button\`);
     let txt1 = ctx['value'];
     return block1([ref1, txt1]);
   }
diff --git a/tests/components/__snapshots__/refs.test.ts.snap b/tests/components/__snapshots__/refs.test.ts.snap
index e90390d48..88b0284b1 100644
--- a/tests/components/__snapshots__/refs.test.ts.snap
+++ b/tests/components/__snapshots__/refs.test.ts.snap
@@ -4,12 +4,13 @@ exports[`refs basic use 1`] = `
 "function anonymous(app, bdom, helpers
 ) {
   let { text, createBlock, list, multi, html, toggler, comment } = bdom;
+  let { singleRefSetter } = helpers;
   
   let block1 = createBlock(\`<div block-ref=\\"0\\"/>\`);
   
   return function template(ctx, node, key = \\"\\") {
     const refs = this.__owl__.refs;
-    const ref1 = (el) => refs[\`div\`] = el;
+    const ref1 = singleRefSetter(refs, \`div\`);
     return block1([ref1]);
   }
 }"
@@ -19,7 +20,7 @@ exports[`refs can use 2 refs with same name in a t-if/t-else situation 1`] = `
 "function anonymous(app, bdom, helpers
 ) {
   let { text, createBlock, list, multi, html, toggler, comment } = bdom;
-  let { multiRefSetter } = helpers;
+  let { singleRefSetter, multiRefSetter } = helpers;
   
   let block2 = createBlock(\`<div block-ref=\\"0\\"/>\`);
   let block3 = createBlock(\`<span block-ref=\\"0\\"/>\`);
@@ -42,13 +43,14 @@ exports[`refs refs and recursive templates 1`] = `
 "function anonymous(app, bdom, helpers
 ) {
   let { text, createBlock, list, multi, html, toggler, comment } = bdom;
+  let { singleRefSetter } = helpers;
   const comp1 = app.createComponent(\`Test\`, true, false, false, false);
   
   let block1 = createBlock(\`<p block-ref=\\"0\\"><block-text-1/><block-child-0/></p>\`);
   
   return function template(ctx, node, key = \\"\\") {
     const refs = this.__owl__.refs;
-    const ref1 = (el) => refs[\`root\`] = el;
+    const ref1 = singleRefSetter(refs, \`root\`);
     let b2;
     let txt1 = ctx['props'].tree.value;
     if (ctx['props'].tree.child) {
@@ -59,11 +61,33 @@ exports[`refs refs and recursive templates 1`] = `
 }"
 `;
 
+exports[`refs refs and t-key 1`] = `
+"function anonymous(app, bdom, helpers
+) {
+  let { text, createBlock, list, multi, html, toggler, comment } = bdom;
+  let { singleRefSetter } = helpers;
+  
+  let block2 = createBlock(\`<button block-handler-0=\\"click\\"/>\`);
+  let block3 = createBlock(\`<p block-ref=\\"0\\"/>\`);
+  
+  return function template(ctx, node, key = \\"\\") {
+    const refs = this.__owl__.refs;
+    const ref1 = singleRefSetter(refs, \`root\`);
+    const v1 = ctx['state'];
+    let hdlr1 = [()=>v1.renderId++, ctx];
+    const b2 = block2([hdlr1]);
+    const tKey_1 = ctx['state'].renderId;
+    const b3 = toggler(tKey_1, block3([ref1]));
+    return multi([b2, b3]);
+  }
+}"
+`;
+
 exports[`refs refs are properly bound in slots 1`] = `
 "function anonymous(app, bdom, helpers
 ) {
   let { text, createBlock, list, multi, html, toggler, comment } = bdom;
-  let { capture, markRaw } = helpers;
+  let { capture, singleRefSetter, markRaw } = helpers;
   const comp1 = app.createComponent(\`Dialog\`, true, true, false, true);
   
   let block1 = createBlock(\`<div><span class=\\"counter\\"><block-text-0/></span><block-child-0/></div>\`);
@@ -71,7 +95,7 @@ exports[`refs refs are properly bound in slots 1`] = `
   
   function slot1(ctx, node, key = \\"\\") {
     const refs = this.__owl__.refs;
-    const ref1 = (el) => refs[\`myButton\`] = el;
+    const ref1 = singleRefSetter(refs, \`myButton\`);
     let hdlr1 = [ctx['doSomething'], ctx];
     return block2([hdlr1, ref1]);
   }
@@ -104,7 +128,7 @@ exports[`refs throws if there are 2 same refs at the same time 1`] = `
 "function anonymous(app, bdom, helpers
 ) {
   let { text, createBlock, list, multi, html, toggler, comment } = bdom;
-  let { multiRefSetter } = helpers;
+  let { singleRefSetter, multiRefSetter } = helpers;
   
   let block2 = createBlock(\`<div block-ref=\\"0\\"/>\`);
   let block3 = createBlock(\`<span block-ref=\\"0\\"/>\`);
diff --git a/tests/components/__snapshots__/slots.test.ts.snap b/tests/components/__snapshots__/slots.test.ts.snap
index cf61d22aa..2a3521ea1 100644
--- a/tests/components/__snapshots__/slots.test.ts.snap
+++ b/tests/components/__snapshots__/slots.test.ts.snap
@@ -158,7 +158,7 @@ exports[`slots can render node with t-ref and Component in same slot 1`] = `
 "function anonymous(app, bdom, helpers
 ) {
   let { text, createBlock, list, multi, html, toggler, comment } = bdom;
-  let { markRaw } = helpers;
+  let { singleRefSetter, markRaw } = helpers;
   const comp1 = app.createComponent(\`Child\`, true, false, false, true);
   const comp2 = app.createComponent(\`Child\`, true, true, false, true);
   
@@ -166,7 +166,7 @@ exports[`slots can render node with t-ref and Component in same slot 1`] = `
   
   function slot1(ctx, node, key = \\"\\") {
     const refs = this.__owl__.refs;
-    const ref1 = (el) => refs[\`div\`] = el;
+    const ref1 = singleRefSetter(refs, \`div\`);
     const b2 = block2([ref1]);
     const b3 = comp1({}, key + \`__1\`, node, this, null);
     return multi([b2, b3]);
diff --git a/tests/components/__snapshots__/t_call.test.ts.snap b/tests/components/__snapshots__/t_call.test.ts.snap
index 7cffa0aea..ee11586e4 100644
--- a/tests/components/__snapshots__/t_call.test.ts.snap
+++ b/tests/components/__snapshots__/t_call.test.ts.snap
@@ -534,7 +534,7 @@ exports[`t-call t-call-context: ComponentNode is not looked up in the context 2`
 "function anonymous(app, bdom, helpers
 ) {
   let { text, createBlock, list, multi, html, toggler, comment } = bdom;
-  let { bind, capture, isBoundary, withDefault, setContextValue, markRaw } = helpers;
+  let { singleRefSetter, bind, capture, isBoundary, withDefault, setContextValue, markRaw } = helpers;
   const comp1 = app.createComponent(\`Child\`, true, true, false, false);
   
   let block2 = createBlock(\`<div block-ref=\\"0\\">outside slot</div>\`);
@@ -543,7 +543,7 @@ exports[`t-call t-call-context: ComponentNode is not looked up in the context 2`
   
   function slot1(ctx, node, key = \\"\\") {
     const refs = this.__owl__.refs;
-    const ref2 = (el) => refs[\`myRef2\`] = el;
+    const ref2 = singleRefSetter(refs, \`myRef2\`);
     ctx = Object.create(ctx);
     ctx[isBoundary] = 1
     const b4 = block4([ref2]);
@@ -555,7 +555,7 @@ exports[`t-call t-call-context: ComponentNode is not looked up in the context 2`
   
   return function template(ctx, node, key = \\"\\") {
     const refs = this.__owl__.refs;
-    const ref1 = (el) => refs[\`myRef\`] = el;
+    const ref1 = singleRefSetter(refs, \`myRef\`);
     const b2 = block2([ref1]);
     const ctx1 = capture(ctx);
     const b6 = comp1({prop: bind(this, ctx['method']),slots: markRaw({'default': {__render: slot1.bind(this), __ctx: ctx1}})}, key + \`__1\`, node, this, null);
diff --git a/tests/components/refs.test.ts b/tests/components/refs.test.ts
index b67b559e7..4e2cf07e0 100644
--- a/tests/components/refs.test.ts
+++ b/tests/components/refs.test.ts
@@ -1,4 +1,13 @@
-import { App, Component, mount, onMounted, useRef, useState, xml } from "../../src/index";
+import {
+  App,
+  Component,
+  mount,
+  onMounted,
+  onPatched,
+  useRef,
+  useState,
+  xml,
+} from "../../src/index";
 import { logStep, makeTestFixture, nextAppError, nextTick, snapshotEverything } from "../helpers";
 
 snapshotEverything();
@@ -127,4 +136,33 @@ describe("refs", () => {
     expect(fixture.innerHTML).toBe("<p>a<p>b</p></p>");
     expect(["<p>b</p>", "<p>a<p>b</p></p>"]).toBeLogged();
   });
+
+  test("refs and t-key", async () => {
+    let el;
+    class Test extends Component {
+      static components = {};
+      static template = xml`
+        <button t-on-click="() => state.renderId++" />
+        <p t-ref="root" t-key="state.renderId"/>`;
+      root = useRef("root");
+      state = useState({ renderId: 1 });
+
+      setup() {
+        onMounted(() => {
+          el = this.root.el;
+        });
+        onPatched(() => {
+          el = this.root.el;
+        });
+      }
+    }
+    await mount(Test, fixture);
+
+    expect(el).toBe(fixture.querySelector("p"));
+    const _el = el;
+    fixture.querySelector("button")!.click();
+    await nextTick();
+    expect(el).not.toBe(_el);
+    expect(el).toBe(fixture.querySelector("p"));
+  });
 });