From c7bd0ab85caadd17f9dcbf9dcf25da30065316f3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=A9ry=20Debongnie?= Date: Fri, 3 Jun 2022 13:37:11 +0200 Subject: [PATCH] [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 | 45 +++++++----- .../__snapshots__/t_on.test.ts.snap | 68 +++++++++++++++++++ tests/components/t_on.test.ts | 64 +++++++++++++++++ 3 files changed, 159 insertions(+), 18 deletions(-) diff --git a/src/runtime/blockdom/event_catcher.ts b/src/runtime/blockdom/event_catcher.ts index 1f54ae12..c3cc0b2b 100644 --- a/src/runtime/blockdom/event_catcher.ts +++ b/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 { diff --git a/tests/components/__snapshots__/t_on.test.ts.snap b/tests/components/__snapshots__/t_on.test.ts.snap index a0f8f501..d89228d3 100644 --- a/tests/components/__snapshots__/t_on.test.ts.snap +++ b/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(\`
\`); + + 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(\`
\`); + + 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 ) { diff --git a/tests/components/t_on.test.ts b/tests/components/t_on.test.ts index 43b7ff33..6acf4c6a 100644 --- a/tests/components/t_on.test.ts +++ b/tests/components/t_on.test.ts @@ -319,4 +319,68 @@ describe("t-on", () => { expect(fixture.innerHTML).toBe(""); expect(["hey"]).toBeLogged(); }); + + test("t-on on components and t-foreach", async () => { + class Child extends Component { + static template = xml`
`; + } + + class Parent extends Component { + static template = xml` + + + `; + static components = { Child }; + log(name: string) { + logStep(name); + } + } + await mount(Parent, fixture); + expect(fixture.innerHTML).toBe("
John
Raoul
Gérald
"); + 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`
`; + } + + class Parent extends Component { + static template = xml` + + `; + static components = { Child }; + state = useState({ name: "aaron" }); + log(name: string) { + logStep(name); + } + } + const parent = await mount(Parent, fixture); + expect(fixture.innerHTML).toBe("
aaron
"); + expect([]).toBeLogged(); + + fixture.querySelectorAll("div")[0].click(); + await nextTick(); + expect(["aaron"]).toBeLogged(); + + parent.state.name = "lucas"; + await nextTick(); + expect(fixture.innerHTML).toBe("
lucas
"); + + fixture.querySelectorAll("div")[0].click(); + await nextTick(); + expect(["lucas"]).toBeLogged(); + }); });