[FIX] component: fix wrong behaviour when using t-on on t-component

Before this commit, using t-on on t-component was not correctly
implemented by owl: when more than one component shared the same
parentelement and have an handler with the same name, the second handler
would override the first (using an internal key). With this commit, we
simply recreate event handler for each instance of a catcher block. Note
that it means that using t-on on components is slightly slower now.

Also, this commit fixes an additional issue that was noticed: the
catcher block would not update its internal handler properly, so it
would not behave properly after being patched.

closes #1199

src/runtime/blockdom/event_catcher.ts

@@ -6,34 +6,38 @@ type EventsSpec = { [name: string]: number };
 type Catcher = (child: VNode, handlers: any[]) => VNode;
 
 export function createCatcher(eventsSpec: EventsSpec): Catcher {
-  let setupFns: any[] = [];
-  let removeFns: any[] = [];
-  for (let name in eventsSpec) {
-    let index = eventsSpec[name];
-    let { setup, remove } = createEventHandler(name);
-    setupFns[index] = setup;
-    removeFns[index] = remove;
-  }
-  let n = setupFns.length;
+  const n = Object.keys(eventsSpec).length;
 
   class VCatcher {
     child: VNode;
-    handlers: any[];
+    handlerData: any[];
+    handlerFns: any[] = [];
 
     parentEl?: HTMLElement | undefined;
-    afterNode: Node | null = null;
+    afterNode: Text | null = null;
 
     constructor(child: VNode, handlers: any[]) {
       this.child = child;
-      this.handlers = handlers;
+      this.handlerData = handlers;
     }
 
     mount(parent: HTMLElement, afterNode: Node | null) {
       this.parentEl = parent;
-      this.afterNode = afterNode;
       this.child.mount(parent, afterNode);
+      this.afterNode = document.createTextNode("");
+      parent.insertBefore(this.afterNode, afterNode);
+      this.wrapHandlerData();
+      for (let name in eventsSpec) {
+        const index = eventsSpec[name];
+        const handler = createEventHandler(name);
+        this.handlerFns[index] = handler;
+        handler.setup.call(parent, this.handlerData[index]);
+      }
+    }
+
+    wrapHandlerData() {
       for (let i = 0; i < n; i++) {
-        let handler = this.handlers[i];
+        let handler = this.handlerData[i];
         // handler = [...mods, fn, comp], so we need to replace second to last elem
         let idx = handler.length - 2;
         let origFn = handler[idx];
@@ -49,20 +53,24 @@ export function createCatcher(eventsSpec: EventsSpec): Catcher {
             currentNode = currentNode.nextSibling;
           }
         };
-        setupFns[i].call(parent, this.handlers[i]);
       }
     }
 
     moveBefore(other: VCatcher | null, afterNode: Node | null) {
-      this.afterNode = null;
       this.child.moveBefore(other ? other.child : null, afterNode);
+      this.parentEl!.insertBefore(this.afterNode!, afterNode);
     }
 
     patch(other: VCatcher, withBeforeRemove: boolean) {
       if (this === other) {
         return;
       }
-      this.handlers = other.handlers;
+      this.handlerData = other.handlerData;
+      this.wrapHandlerData();
+      for (let i = 0; i < n; i++) {
+        this.handlerFns[i].update.call(this.parentEl!, this.handlerData[i]);
+      }
+
       this.child.patch(other.child, withBeforeRemove);
     }
 
@@ -72,9 +80,10 @@ export function createCatcher(eventsSpec: EventsSpec): Catcher {
 
     remove() {
       for (let i = 0; i < n; i++) {
-        removeFns[i].call(this.parentEl!);
+        this.handlerFns[i].remove.call(this.parentEl!);
       }
       this.child.remove();
+      this.afterNode!.remove();
     }
 
     firstNode(): Node | undefined {

tests/components/__snapshots__/t_on.test.ts.snap

@@ -181,6 +181,42 @@ exports[`t-on t-on on components 2`] = `
 }"
 `;
 
+exports[`t-on t-on on components and t-foreach 1`] = `
+"function anonymous(app, bdom, helpers
+) {
+  let { text, createBlock, list, multi, html, toggler, component, comment } = bdom;
+  let { prepareList, createCatcher, withKey } = helpers;
+  const catcher1 = createCatcher({\\"click\\":0});
+  
+  return function template(ctx, node, key = \\"\\") {
+    ctx = Object.create(ctx);
+    const [k_block1, v_block1, l_block1, c_block1] = prepareList(['John','Raoul','Gérald']);;
+    for (let i1 = 0; i1 < l_block1; i1++) {
+      ctx[\`name\`] = v_block1[i1];
+      const key1 = ctx['name'];
+      const v1 = ctx['name'];
+      const hdlr1 = [()=>this.log(v1), ctx];
+      c_block1[i1] = withKey(catcher1(component(\`Child\`, {value: ctx['name']}, key + \`__1__\${key1}\`, node, ctx), [hdlr1]), key1);
+    }
+    return list(c_block1);
+  }
+}"
+`;
+
+exports[`t-on t-on on components and t-foreach 2`] = `
+"function anonymous(app, bdom, helpers
+) {
+  let { text, createBlock, list, multi, html, toggler, component, comment } = bdom;
+  
+  let block1 = createBlock(\`<div><block-text-0/></div>\`);
+  
+  return function template(ctx, node, key = \\"\\") {
+    let txt1 = ctx['props'].value;
+    return block1([txt1]);
+  }
+}"
+`;
+
 exports[`t-on t-on on components, variation 1`] = `
 "function anonymous(app, bdom, helpers
 ) {
@@ -240,6 +276,38 @@ exports[`t-on t-on on components, with 'prevent' modifier 2`] = `
 }"
 `;
 
+exports[`t-on t-on on components, with a handler update 1`] = `
+"function anonymous(app, bdom, helpers
+) {
+  let { text, createBlock, list, multi, html, toggler, component, comment } = bdom;
+  let { isBoundary, withDefault, setContextValue, createCatcher } = helpers;
+  const catcher1 = createCatcher({\\"click\\":0});
+  
+  return function template(ctx, node, key = \\"\\") {
+    ctx = Object.create(ctx);
+    ctx[isBoundary] = 1
+    setContextValue(ctx, \\"name\\", ctx['state'].name);
+    const v1 = ctx['name'];
+    const hdlr1 = [()=>this.log(v1), ctx];
+    return catcher1(component(\`Child\`, {value: ctx['name']}, key + \`__1\`, node, ctx), [hdlr1]);
+  }
+}"
+`;
+
+exports[`t-on t-on on components, with a handler update 2`] = `
+"function anonymous(app, bdom, helpers
+) {
+  let { text, createBlock, list, multi, html, toggler, component, comment } = bdom;
+  
+  let block1 = createBlock(\`<div><block-text-0/></div>\`);
+  
+  return function template(ctx, node, key = \\"\\") {
+    let txt1 = ctx['props'].value;
+    return block1([txt1]);
+  }
+}"
+`;
+
 exports[`t-on t-on on destroyed components 1`] = `
 "function anonymous(app, bdom, helpers
 ) {

tests/components/t_on.test.ts

@@ -319,4 +319,68 @@ describe("t-on", () => {
     expect(fixture.innerHTML).toBe("<button>button</button>");
     expect(["hey"]).toBeLogged();
   });
+
+  test("t-on on components and t-foreach", async () => {
+    class Child extends Component {
+      static template = xml`<div t-esc="props.value"/>`;
+    }
+
+    class Parent extends Component {
+      static template = xml`
+        <t t-foreach="['John', 'Raoul', 'Gérald']" t-as="name" t-key="name">
+          <Child t-on-click="() => this.log(name)" value="name"/>
+        </t>`;
+      static components = { Child };
+      log(name: string) {
+        logStep(name);
+      }
+    }
+    await mount(Parent, fixture);
+    expect(fixture.innerHTML).toBe("<div>John</div><div>Raoul</div><div>Gérald</div>");
+    expect([]).toBeLogged();
+    fixture.querySelectorAll("div")[0].click();
+    await nextTick();
+    expect(["John"]).toBeLogged();
+
+    fixture.querySelectorAll("div")[1].click();
+    await nextTick();
+    expect(["Raoul"]).toBeLogged();
+
+    fixture.querySelectorAll("div")[2].click();
+    await nextTick();
+    expect(["Gérald"]).toBeLogged();
+  });
+
+  test("t-on on components, with a handler update", async () => {
+    class Child extends Component {
+      static template = xml`<div t-esc="props.value"/>`;
+    }
+
+    class Parent extends Component {
+      static template = xml`
+          <t t-set="name" t-value="state.name"/>
+          <Child value="name" t-on-click="() => this.log(name)"
+        />`;
+      static components = { Child };
+      state = useState({ name: "aaron" });
+      log(name: string) {
+        logStep(name);
+      }
+    }
+    const parent = await mount(Parent, fixture);
+    expect(fixture.innerHTML).toBe("<div>aaron</div>");
+    expect([]).toBeLogged();
+
+    fixture.querySelectorAll("div")[0].click();
+    await nextTick();
+    expect(["aaron"]).toBeLogged();
+
+    parent.state.name = "lucas";
+    await nextTick();
+    expect(fixture.innerHTML).toBe("<div>lucas</div>");
+
+    fixture.querySelectorAll("div")[0].click();
+    await nextTick();
+    expect(["lucas"]).toBeLogged();
+  });
 });