diff --git a/CHANGELOG.md b/CHANGELOG.md index 314835ed..cfef4401 100644 --- a/CHANGELOG.md +++ b/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 - -``` - -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 - - - -``` - -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 diff --git a/doc/reference/event_handling.md b/doc/reference/event_handling.md index 6059602f..ca85ee2b 100644 --- a/doc/reference/event_handling.md +++ b/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: ``` + +## On Components + +The `t-on` directive also works on a child component: + +```xml +
+ in some template + +
+``` + +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). diff --git a/src/app/template_helpers.ts b/src/app/template_helpers.ts index a3495277..caa2fbd4 100644 --- a/src/app/template_helpers.ts +++ b/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, }; diff --git a/src/blockdom/event_catcher.ts b/src/blockdom/event_catcher.ts new file mode 100644 index 00000000..c888a971 --- /dev/null +++ b/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 { + return new VCatcher(child, handlers); + }; +} diff --git a/src/blockdom/events.ts b/src/blockdom/events.ts index 4f663bcb..f4bdfe60 100644 --- a/src/blockdom/events.ts +++ b/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) { diff --git a/src/blockdom/index.ts b/src/blockdom/index.ts index 0b6d05b0..0bc65806 100644 --- a/src/blockdom/index.ts +++ b/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 { mount(parent: HTMLElement, afterNode: Node | null): void; diff --git a/src/compiler/code_generator.ts b/src/compiler/code_generator.ts index 6fa9136c..b9df5324 100644 --- a/src/compiler/code_generator.ts +++ b/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 = 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); } diff --git a/src/compiler/parser.ts b/src/compiler/parser.ts index 385bb2d0..86159959 100644 --- a/src/compiler/parser.ts +++ b/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 }; } // ----------------------------------------------------------------------------- diff --git a/tests/blockdom/event_catcher.test.ts b/tests/blockdom/event_catcher.test.ts new file mode 100644 index 00000000..10a84b7e --- /dev/null +++ b/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("
"); + let n = 0; + let ctx = {}; + let handler = [() => n++, ctx]; + const tree = catcher(block(), [handler]); + + mount(tree, fixture); + expect(fixture.innerHTML).toBe("
"); + + 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("
"); + const parentBlock = createBlock(""); + let n = 0; + let ctx = {}; + let handler = [() => n++, ctx]; + const tree = parentBlock([], [catcher(childBlock(), [handler])]); + + mount(tree, fixture); + expect(fixture.innerHTML).toBe(""); + + expect(n).toBe(0); + fixture.querySelector("div")!.click(); + expect(n).toBe(1); + fixture.querySelector("button")!.click(); + expect(n).toBe(1); +}); diff --git a/tests/compiler/parser.test.ts b/tests/compiler/parser.test.ts index bbf4ccb1..13f8bbe0 100644 --- a/tests/compiler/parser.test.ts +++ b/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(``)).toThrow( - "t-on is no longer supported on components. Consider passing a callback in props." - ); + expect(parse(``)).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" } } }, }, }, diff --git a/tests/components/__snapshots__/t_on.test.ts.snap b/tests/components/__snapshots__/t_on.test.ts.snap index e3a33a24..c2dee374 100644 --- a/tests/components/__snapshots__/t_on.test.ts.snap +++ b/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(\`\`); + + 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(\`

\`); + + 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(\`\`); + + 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 ) { diff --git a/tests/components/t_on.test.ts b/tests/components/t_on.test.ts index 09d05981..613f3dd8 100644 --- a/tests/components/t_on.test.ts +++ b/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`"); + fixture.querySelector("button")!.click(); + await nextTick(); + expect(fixture.innerHTML).toBe(""); + }); + + test("t-on on components, variation", async () => { + class Child extends Component { + static template = xml`

"); + + fixture.querySelector("span")!.click(); + await nextTick(); + expect(fixture.innerHTML).toBe("

"); + + fixture.querySelector("button")!.click(); + await nextTick(); + expect(fixture.innerHTML).toBe("

"); + + fixture.querySelector("p")!.click(); + await nextTick(); + expect(fixture.innerHTML).toBe("

"); + }); });