[IMP] component: add support for t-on on compnents

CHANGELOG.md

@@ -44,7 +44,6 @@ All changes are documented here in no particular order.
 - breaking: style/class on components are now regular props ([details](#10-styleclass-on-components-are-now-regular-props))
 - new: components can use the `.bind` suffix to bind function props ([doc](doc/reference/props.md#binding-function-props))
 - breaking: `t-on` does not accept expressions, only functions ([details](#30-t-on-does-not-accept-expressions-only-functions))
-- breaking: `t-on` does not work on components any more ([details](#12-t-on-does-not-work-on-components-any-more))
 - new: an error is thrown if an handler defined in a `t-on-` directive is not a function (failed silently previously in some cases)
 - breaking: `t-component` no longer accepts strings ([details](#17-t-component-no-longer-accepts-strings))
 - new: the `this` variable in template expressions is now bound to the component
@@ -313,41 +312,6 @@ Documentation:
 - [Mounting a component](doc/reference/app.md#mount-helper)
 
 
-### 12. `t-on` does not work on components any more
-
-In owl 1, it was possible to bind an event listener on a component tag in a
-template:
-
-```xml
-<SomeComponent t-on-some-event="doSomething"/>
-```
-
-This does not work any more.
-
-Rationale: with the support of fragments, there is no longer a canonical html
-element that we can refer. So, this makes it difficult to implement correctly
-and efficiently. Also, we noticed in practice that the event system was an issue
-in some cases, when components need to communicate before they are mounted. In
-those cases, the better solution is to directly use a callback.  Also, another
-conceptual issue with this is that it kind of breaks the component encapsulation.
-The child component kind of leak its own implementation to the outside world.
-
-Migration: a quick fix that may work in some cases is to simply bind the event
-handler on a parent htmlelement. A better way to do it, if possible, is to change
-the component API to accept explicitely a callback as props. 
-
-```xml
-<SomeComponent onSomeEvent="doSomething"/>
-<!-- or alternatively: -->
-<SomeComponent onSomeEvent.bind="doSomething"/>
-```
-
-Note that one of the example above uses the `.bind` suffix, to bind the function
-prop to the component.  Most of the time, binding the function is necessary, and
-using the `.bind` suffix is very helpful in that case.
-
-Documentation: [Binding function props](doc/reference/props.md#binding-function-props)
-
 ### 13. Portal does no longer transfer DOM events
 
 In Owl 1, a Portal component would listen to events emitted on its portalled

doc/reference/event_handling.md

@@ -5,6 +5,7 @@
 - [Event Handling](#event-handling)
 - [Modifiers](#modifiers)
 - [Synthetic Events](#synthetic-events)
+- [On Components](#on-components)
 
 ## Event Handling
 
@@ -100,3 +101,19 @@ To enable it, one can just use the `.synthetic` suffix:
     </t>
 </div>
 ```
+
+## On Components
+
+The `t-on` directive also works on a child component:
+
+```xml
+<div>
+    in some template
+    <Child t-on-click="dosomething"/>
+</div>
+```
+
+This will catch all click events on any html element contained in the `Child`
+sub component. Note that if the child component is reduced to one (or more) text
+nodes, then clicking on it will not call the handler, since the event will be
+dispatched by the browser on the parent element (a `div` in this case).

src/app/template_helpers.ts

@@ -1,4 +1,4 @@
-import { BDom, multi, text, toggler } from "../blockdom";
+import { BDom, multi, text, toggler, createCatcher } from "../blockdom";
 import { validateProps } from "../component/props_validation";
 import { Markup } from "../utils";
 import { html } from "../blockdom/index";
@@ -199,4 +199,5 @@ export const helpers = {
   LazyValue,
   safeOutput,
   bind,
+  createCatcher,
 };

src/blockdom/event_catcher.ts

@@ -0,0 +1,89 @@
+import { createEventHandler } from "./events";
+import type { VNode } from "./index";
+
+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;
+
+  class VCatcher {
+    child: VNode;
+    handlers: any[];
+
+    parentEl?: HTMLElement | undefined;
+    afterNode: Node | null = null;
+
+    constructor(child: VNode, handlers: any[]) {
+      this.child = child;
+      this.handlers = handlers;
+    }
+
+    mount(parent: HTMLElement, afterNode: Node | null) {
+      this.parentEl = parent;
+      this.afterNode = afterNode;
+      this.child.mount(parent, afterNode);
+      for (let i = 0; i < n; i++) {
+        let origFn = this.handlers[i][0];
+        const self = this;
+        this.handlers[i][0] = function (ev: any) {
+          const target = ev.target;
+          let currentNode: any = self.child.firstNode();
+          const afterNode = self.afterNode;
+          while (currentNode !== afterNode) {
+            if (currentNode.contains(target)) {
+              return origFn.call(this, ev);
+            }
+            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);
+    }
+
+    patch(other: VCatcher, withBeforeRemove: boolean) {
+      if (this === other) {
+        return;
+      }
+      this.handlers = other.handlers;
+      this.child.patch(other.child, withBeforeRemove);
+    }
+
+    beforeRemove() {
+      this.child.beforeRemove();
+    }
+
+    remove() {
+      for (let i = 0; i < n; i++) {
+        removeFns[i].call(this.parentEl!);
+      }
+      this.child.remove();
+    }
+
+    firstNode(): Node | undefined {
+      return this.child.firstNode();
+    }
+
+    toString(): string {
+      return this.child.toString();
+    }
+  }
+
+  return function (child: VNode, handlers: any[]): VNode<VCatcher> {
+    return new VCatcher(child, handlers);
+  };
+}

src/blockdom/events.ts

@@ -5,6 +5,7 @@ type EventHandlerSetter = (this: HTMLElement, data: any) => void;
 interface EventHandlerCreator {
   setup: EventHandlerSetter;
   update: EventHandlerSetter;
+  remove: (this: HTMLElement) => void;
 }
 
 export function createEventHandler(rawEvent: string): EventHandlerCreator {
@@ -38,11 +39,15 @@ function createElementHandler(evName: string, capture: boolean = false): EventHa
     this.addEventListener(evName, listener, { capture });
   }
 
+  function remove(this: HTMLElement) {
+    delete (this as any)[eventKey];
+    this.removeEventListener(evName, listener, { capture });
+  }
   function update(this: HTMLElement, data: any) {
     (this as any)[eventKey] = data;
   }
 
-  return { setup, update };
+  return { setup, update, remove };
 }
 
 // Synthetic handler: a form of event delegation that allows placing only one
@@ -60,7 +65,12 @@ function createSyntheticHandler(evName: string, capture: boolean = false): Event
     _data[currentId] = data;
     (this as any)[eventKey] = _data;
   }
-  return { setup, update: setup };
+
+  function remove(this: HTMLElement) {
+    delete (this as any)[eventKey];
+  }
+
+  return { setup, update: setup, remove };
 }
 
 function nativeToSyntheticEvent(eventKey: string, event: Event) {

src/blockdom/index.ts

@@ -6,6 +6,7 @@ export { list } from "./list";
 export { multi } from "./multi";
 export { text, comment } from "./text";
 export { html } from "./html";
+export { createCatcher } from "./event_catcher";
 
 export interface VNode<T = any> {
   mount(parent: HTMLElement, afterNode: Node | null): void;

src/compiler/code_generator.ts

@@ -217,6 +217,9 @@ export class CodeGenerator {
   translatableAttributes: string[] = TRANSLATABLE_ATTRS;
   ast: AST;
   staticCalls: { id: string; template: string }[] = [];
+  // todo: merge with staticCalls
+  // todo: add a setCodeValue function => 2 args, call addLine, use it instead of addlin
+  eventCatchers: { id: string; expr: string }[] = [];
   helpers: Set<string> = new Set();
 
   constructor(ast: AST, options: CodeGenOptions) {
@@ -265,6 +268,9 @@ export class CodeGenerator {
     for (let { id, template } of this.staticCalls) {
       mainCode.push(`const ${id} = getTemplate(${template});`);
     }
+    for (let { id, expr } of this.eventCatchers) {
+      mainCode.push(`const ${id} = ${expr};`);
+    }
 
     // define all blocks
     if (this.blocks.length) {
@@ -1178,6 +1184,23 @@ export class CodeGenerator {
     if (ast.isDynamic) {
       blockExpr = `toggler(${expr}, ${blockExpr})`;
     }
+
+    // event handling
+    if (ast.on) {
+      this.helpers.add("createCatcher");
+      let name = this.generateId("catcher");
+      let spec: any = {};
+      let handlers: any[] = [];
+      for (let ev in ast.on) {
+        let handlerId = this.generateId("hdlr");
+        let idx = handlers.push(handlerId) - 1;
+        spec[ev] = idx;
+        const handler = this.generateHandlerCode(ev, ast.on[ev]);
+        this.addLine(`let ${handlerId} = ${handler};`);
+      }
+      blockExpr = `${name}(${blockExpr}, [${handlers.join(",")}])`;
+      this.eventCatchers.push({ id: name, expr: `createCatcher(${JSON.stringify(spec)})` });
+    }
     block = this.createBlock(block, "multi", ctx);
     this.insertBlock(blockExpr, block, ctx);
   }

src/compiler/parser.ts

@@ -119,6 +119,7 @@ export interface ASTComponent {
   name: string;
   isDynamic: boolean;
   dynamicProps: string | null;
+  on: null | { [key: string]: string };
   props: { [name: string]: string };
   slots: { [name: string]: { content: AST; attrs?: { [key: string]: string }; scope?: string } };
 }
@@ -650,7 +651,6 @@ function parseTSetNode(node: Element, ctx: ParsingContext): AST | null {
 
 // Error messages when trying to use an unsupported directive on a component
 const directiveErrorMap = new Map([
-  ["t-on", "t-on is no longer supported on components. Consider passing a callback in props."],
   [
     "t-ref",
     "t-ref is no longer supported on components. Consider exposing only the public part of the component's API through a callback prop.",
@@ -684,13 +684,19 @@ function parseComponent(node: Element, ctx: ParsingContext): AST | null {
 
   const defaultSlotScope = node.getAttribute("t-slot-scope");
   node.removeAttribute("t-slot-scope");
+  let on: ASTComponent["on"] = null;
 
   const props: ASTComponent["props"] = {};
   for (let name of node.getAttributeNames()) {
     const value = node.getAttribute(name)!;
     if (name.startsWith("t-")) {
-      const message = directiveErrorMap.get(name.split("-").slice(0, 2).join("-"));
-      throw new Error(message || `unsupported directive on Component: ${name}`);
+      if (name.startsWith("t-on-")) {
+        on = on || {};
+        on[name.slice(5)] = value;
+      } else {
+        const message = directiveErrorMap.get(name.split("-").slice(0, 2).join("-"));
+        throw new Error(message || `unsupported directive on Component: ${name}`);
+      }
     } else {
       props[name] = value;
     }
@@ -755,7 +761,7 @@ function parseComponent(node: Element, ctx: ParsingContext): AST | null {
       }
     }
   }
-  return { type: ASTType.TComponent, name, isDynamic, dynamicProps, props, slots };
+  return { type: ASTType.TComponent, name, isDynamic, dynamicProps, props, slots, on };
 }
 
 // -----------------------------------------------------------------------------

tests/blockdom/event_catcher.test.ts

@@ -0,0 +1,54 @@
+import { config, createBlock, createCatcher, mount } from "../../src/blockdom";
+import { makeTestFixture } from "./helpers";
+import { mainEventHandler } from "../../src/component/handler";
+
+//------------------------------------------------------------------------------
+// Setup and helpers
+//------------------------------------------------------------------------------
+
+let fixture: HTMLElement;
+config.mainEventHandler = mainEventHandler;
+
+beforeEach(() => {
+  fixture = makeTestFixture();
+});
+
+afterEach(() => {
+  fixture.remove();
+});
+
+test("simple event catcher", async () => {
+  const catcher = createCatcher({ click: 0 });
+  const block = createBlock("<div></div>");
+  let n = 0;
+  let ctx = {};
+  let handler = [() => n++, ctx];
+  const tree = catcher(block(), [handler]);
+
+  mount(tree, fixture);
+  expect(fixture.innerHTML).toBe("<div></div>");
+
+  expect(fixture.firstChild).toBeInstanceOf(HTMLDivElement);
+  expect(n).toBe(0);
+  (fixture.firstChild as HTMLDivElement).click();
+  expect(n).toBe(1);
+});
+
+test("do not catch events outside of itself", async () => {
+  const catcher = createCatcher({ click: 0 });
+  const childBlock = createBlock("<div></div>");
+  const parentBlock = createBlock("<button><block-child-0/></button>");
+  let n = 0;
+  let ctx = {};
+  let handler = [() => n++, ctx];
+  const tree = parentBlock([], [catcher(childBlock(), [handler])]);
+
+  mount(tree, fixture);
+  expect(fixture.innerHTML).toBe("<button><div></div></button>");
+
+  expect(n).toBe(0);
+  fixture.querySelector("div")!.click();
+  expect(n).toBe(1);
+  fixture.querySelector("button")!.click();
+  expect(n).toBe(1);
+});

tests/compiler/parser.test.ts

@@ -1011,6 +1011,7 @@ describe("qweb parser", () => {
         dynamicProps: null,
         props: {},
         slots: {},
+        on: null,
       },
       memo: "",
       hasNoFirst: true,
@@ -1217,6 +1218,7 @@ describe("qweb parser", () => {
       name: "MyComponent",
       dynamicProps: null,
       props: {},
+      on: null,
       slots: {},
       isDynamic: false,
     });
@@ -1229,6 +1231,7 @@ describe("qweb parser", () => {
       dynamicProps: null,
       props: { a: "1", b: "'b'" },
       isDynamic: false,
+      on: null,
       slots: {},
     });
   });
@@ -1240,14 +1243,21 @@ describe("qweb parser", () => {
       dynamicProps: "state",
       props: { a: "1" },
       isDynamic: false,
+      on: null,
       slots: {},
     });
   });
 
   test("component with event handler", async () => {
-    expect(() => parse(`<MyComponent t-on-click="someMethod"/>`)).toThrow(
-      "t-on is no longer supported on components. Consider passing a callback in props."
-    );
+    expect(parse(`<MyComponent t-on-click="someMethod"/>`)).toEqual({
+      type: ASTType.TComponent,
+      name: "MyComponent",
+      dynamicProps: null,
+      props: {},
+      isDynamic: false,
+      on: { click: "someMethod" },
+      slots: {},
+    });
   });
 
   test("component with t-ref", async () => {
@@ -1281,6 +1291,7 @@ describe("qweb parser", () => {
       dynamicProps: null,
       props: {},
       isDynamic: false,
+      on: null,
       slots: { default: { content: { type: ASTType.Text, value: "foo" } } },
     });
   });
@@ -1294,6 +1305,7 @@ describe("qweb parser", () => {
       dynamicProps: null,
       props: {},
       isDynamic: false,
+      on: null,
       slots: {
         default: { content: { type: ASTType.Text, value: "foo" }, attrs: { param: "param" } },
       },
@@ -1307,6 +1319,7 @@ describe("qweb parser", () => {
       isDynamic: false,
       dynamicProps: null,
       props: {},
+      on: null,
       slots: {
         default: {
           content: {
@@ -1348,6 +1361,7 @@ describe("qweb parser", () => {
       isDynamic: false,
       dynamicProps: null,
       props: {},
+      on: null,
       slots: { name: { content: { type: ASTType.Text, value: "foo" } } },
     });
   });
@@ -1359,6 +1373,7 @@ describe("qweb parser", () => {
       isDynamic: false,
       dynamicProps: null,
       props: {},
+      on: null,
       slots: { name: { content: { type: ASTType.Text, value: "foo" }, attrs: { param: "param" } } },
     });
   });
@@ -1376,6 +1391,7 @@ describe("qweb parser", () => {
       dynamicProps: null,
       props: {},
       isDynamic: false,
+      on: null,
       slots: {
         default: { content: { type: ASTType.Text, value: " " } },
         name: { content: { type: ASTType.Text, value: "foo" } },
@@ -1395,6 +1411,7 @@ describe("qweb parser", () => {
       dynamicProps: null,
       props: {},
       isDynamic: false,
+      on: null,
       slots: {
         a: { content: { type: ASTType.Text, value: "foo" } },
         b: { content: { type: ASTType.Text, value: "bar" } },
@@ -1409,6 +1426,7 @@ describe("qweb parser", () => {
       dynamicProps: null,
       props: {},
       isDynamic: true,
+      on: null,
       slots: {},
     });
   });
@@ -1420,6 +1438,7 @@ describe("qweb parser", () => {
       dynamicProps: null,
       props: { a: "1", b: "'b'" },
       isDynamic: true,
+      on: null,
       slots: {},
     });
   });
@@ -1431,6 +1450,7 @@ describe("qweb parser", () => {
       dynamicProps: "state",
       props: { a: "1" },
       isDynamic: true,
+      on: null,
       slots: {},
     });
   });
@@ -1454,6 +1474,7 @@ describe("qweb parser", () => {
       dynamicProps: null,
       props: {},
       isDynamic: false,
+      on: null,
       slots: { default: { content: { body: null, name: "subTemplate", type: ASTType.TCall } } },
     });
   });
@@ -1472,6 +1493,7 @@ describe("qweb parser", () => {
       dynamicProps: null,
       props: {},
       isDynamic: false,
+      on: null,
       slots: {
         default: {
           content: {
@@ -1480,6 +1502,7 @@ describe("qweb parser", () => {
             name: "Child",
             dynamicProps: null,
             props: {},
+            on: null,
             slots: { brol: { content: { type: ASTType.Text, value: "coucou" } } },
           },
         },
@@ -1501,6 +1524,7 @@ describe("qweb parser", () => {
       dynamicProps: null,
       props: {},
       isDynamic: false,
+      on: null,
       slots: {
         default: {
           content: {
@@ -1509,6 +1533,7 @@ describe("qweb parser", () => {
             name: "Child",
             dynamicProps: null,
             props: {},
+            on: null,
             slots: { brol: { content: { type: ASTType.Text, value: "coucou" } } },
           },
         },

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

@@ -121,6 +121,65 @@ exports[`t-on t-on method call in t-foreach 1`] = `
 }"
 `;
 
+exports[`t-on t-on on components 1`] = `
+"function anonymous(bdom, helpers
+) {
+  let { text, createBlock, list, multi, html, toggler, component, comment } = bdom;
+  let { createCatcher } = helpers;
+  const catcher1 = createCatcher({\\"click\\":0});
+  
+  return function template(ctx, node, key = \\"\\") {
+    let hdlr1 = [ctx['increment'], ctx];
+    return catcher1(component(\`Child\`, {value: ctx['state'].value}, key + \`__1\`, node, ctx), [hdlr1]);
+  }
+}"
+`;
+
+exports[`t-on t-on on components 2`] = `
+"function anonymous(bdom, helpers
+) {
+  let { text, createBlock, list, multi, html, toggler, component, comment } = bdom;
+  
+  let block1 = createBlock(\`<button><block-text-0/></button>\`);
+  
+  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(bdom, helpers
+) {
+  let { text, createBlock, list, multi, html, toggler, component, comment } = bdom;
+  let { createCatcher } = helpers;
+  const catcher1 = createCatcher({\\"click\\":0});
+  
+  let block1 = createBlock(\`<div><span/><block-child-0/><p/></div>\`);
+  
+  return function template(ctx, node, key = \\"\\") {
+    let hdlr1 = [ctx['increment'], ctx];
+    let b2 = catcher1(component(\`Child\`, {value: ctx['state'].value}, key + \`__1\`, node, ctx), [hdlr1]);
+    return block1([], [b2]);
+  }
+}"
+`;
+
+exports[`t-on t-on on components, variation 2`] = `
+"function anonymous(bdom, helpers
+) {
+  let { text, createBlock, list, multi, html, toggler, component, comment } = bdom;
+  
+  let block1 = createBlock(\`<button><block-text-0/></button>\`);
+  
+  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(bdom, helpers
 ) {

tests/components/t_on.test.ts

@@ -142,4 +142,58 @@ describe("t-on", () => {
     buttons[1].click();
     expect(comp.otherState.vals).toStrictEqual(["0_0", "1_1"]);
   });
+
+  test("t-on on components", async () => {
+    class Child extends Component {
+      static template = xml`<button t-esc="props.value"/>`;
+    }
+
+    class Parent extends Component {
+      static template = xml`<Child t-on-click="increment" value="state.value"/>`;
+      static components = { Child };
+      state = useState({ value: 1 });
+      increment() {
+        this.state.value++;
+      }
+    }
+    await mount(Parent, fixture);
+    expect(fixture.innerHTML).toBe("<button>1</button>");
+    fixture.querySelector("button")!.click();
+    await nextTick();
+    expect(fixture.innerHTML).toBe("<button>2</button>");
+  });
+
+  test("t-on on components, variation", async () => {
+    class Child extends Component {
+      static template = xml`<button t-esc="props.value"/>`;
+    }
+
+    class Parent extends Component {
+      static template = xml`
+        <div>
+          <span/>
+          <Child t-on-click="increment" value="state.value"/>
+          <p/>
+        </div>`;
+      static components = { Child };
+      state = useState({ value: 1 });
+      increment() {
+        this.state.value++;
+      }
+    }
+    await mount(Parent, fixture);
+    expect(fixture.innerHTML).toBe("<div><span></span><button>1</button><p></p></div>");
+
+    fixture.querySelector("span")!.click();
+    await nextTick();
+    expect(fixture.innerHTML).toBe("<div><span></span><button>1</button><p></p></div>");
+
+    fixture.querySelector("button")!.click();
+    await nextTick();
+    expect(fixture.innerHTML).toBe("<div><span></span><button>2</button><p></p></div>");
+
+    fixture.querySelector("p")!.click();
+    await nextTick();
+    expect(fixture.innerHTML).toBe("<div><span></span><button>2</button><p></p></div>");
+  });
 });