[FIX] qweb/component: fix scoping issue with t-model and t-foreach

Without this fix, the handler set by t-model did not capture properly
the expression that needs to be updated.

closes #474

src/component/directive.ts

@@ -229,19 +229,7 @@ QWeb.addDirective({
     let defID = ctx.generateID();
     let componentID = ctx.generateID();
 
-    let locationExpr = `\`__${ctx.generateID()}__`;
+    const templateKey = ctx.generateTemplateKey();
-    for (let i = 0; i < ctx.loopNumber - 1; i++) {
-      locationExpr += `\${i${i + 1}}__`;
-    }
-    if (ctx.lastNodeKey || ctx.currentKey) {
-      const k = ctx.lastNodeKey || ctx.currentKey;
-      ctx.addLine(`let templateId${componentID} = ${locationExpr}\` + ${k};`);
-    } else {
-      locationExpr += ctx.loopNumber ? `\${i${ctx.loopNumber}}__\`` : "`";
-      ctx.addLine(`let templateId${componentID} = ${locationExpr};`);
-    }
-    const templateId = `templateId${componentID}`;
-
     let ref = node.getAttribute("t-ref");
     let refExpr = "";
     let refKey: string = "";
@@ -334,7 +322,7 @@ QWeb.addDirective({
     }
 
     ctx.addLine(
-      `let w${componentID} = ${templateId} in parent.__owl__.cmap ? parent.__owl__.children[parent.__owl__.cmap[${templateId}]] : false;`
+      `let w${componentID} = ${templateKey} in parent.__owl__.cmap ? parent.__owl__.children[parent.__owl__.cmap[${templateKey}]] : false;`
     );
     let shouldProxy = !ctx.parentNode;
     if (shouldProxy) {
@@ -421,7 +409,7 @@ QWeb.addDirective({
       `if (!W${componentID}) {throw new Error('Cannot find the definition of component "' + componentKey${componentID} + '"')}`
     );
     ctx.addLine(`w${componentID} = new W${componentID}(parent, props${componentID});`);
-    ctx.addLine(`parent.__owl__.cmap[${templateId}] = w${componentID}.__owl__.id;`);
+    ctx.addLine(`parent.__owl__.cmap[${templateKey}] = w${componentID}.__owl__.id;`);
 
     if (hasSlots) {
       const clone = <Element>node.cloneNode(true);
@@ -451,7 +439,7 @@ QWeb.addDirective({
     ctx.addLine(`let def${defID} = w${componentID}.__prepare(extra.fiber, ${scopeVars}, sibling);`);
     // hack: specify empty remove hook to prevent the node from being removed from the DOM
     ctx.addLine(
-      `let pvnode = h('dummy', {key: ${templateId}, hook: {insert(vn) { let nvn=w${componentID}.__mount(fiber, pvnode.elm);pvnode.elm=nvn.elm;${refExpr}${transitionsInsertCode}},remove() {},destroy(vn) {${finalizeComponentCode}}}});`
+      `let pvnode = h('dummy', {key: ${templateKey}, hook: {insert(vn) { let nvn=w${componentID}.__mount(fiber, pvnode.elm);pvnode.elm=nvn.elm;${refExpr}${transitionsInsertCode}},remove() {},destroy(vn) {${finalizeComponentCode}}}});`
     );
     ctx.addLine(`const fiber = w${componentID}.__owl__.currentFiber;`);
     ctx.addLine(

src/qweb/compilation_context.ts

@@ -47,6 +47,30 @@ export class CompilationContext {
     return id;
   }
 
+  /**
+   * This method generates a "template key", which is basically a unique key
+   * which depends on the currently set keys, and on the iteration numbers (if
+   * we are in a loop).
+   *
+   * Such a key is necessary when we need to associate an id to some element
+   * generated by a template (for example, a component)
+   */
+  generateTemplateKey(): string {
+    const id = this.generateID();
+    let locationExpr = `\`__${this.generateID()}__`;
+    for (let i = 0; i < this.loopNumber - 1; i++) {
+      locationExpr += `\${i${i + 1}}__`;
+    }
+    if (this.lastNodeKey || this.currentKey) {
+      const k = this.lastNodeKey || this.currentKey;
+      this.addLine(`let k${id} = ${locationExpr}\` + ${k};`);
+    } else {
+      locationExpr += this.loopNumber ? `\${i${this.loopNumber}}__\`` : "`";
+      this.addLine(`let k${id} = ${locationExpr};`);
+    }
+    return `k${id}`;
+  }
+
   generateCode(): string[] {
     const shouldTrackScope = this.shouldTrackScope && this.scopeVars.length;
     if (shouldTrackScope) {

src/qweb/extensions.ts

@@ -231,7 +231,17 @@ QWeb.addDirective({
     const type = node.getAttribute("type");
     let handler;
     let event = fullName.includes(".lazy") ? "change" : "input";
-    const expr = ctx.formatExpression(value);
+
+    // we keep here a reference to the "base expression" (if the expression
+    // is `t-model="some.expr.value", then the base expression is "some.expr").
+    // This is necessary so we can capture it in the handler closure.
+    let expr = ctx.formatExpression(value);
+    const index = expr.lastIndexOf(".");
+    const baseExpr = expr.slice(0, index);
+    ctx.addLine(`let expr${nodeID} = ${baseExpr};`);
+
+    expr = `expr${nodeID}.${expr.slice(index + 1)}`;
+    const key = ctx.generateTemplateKey();
     if (node.tagName === "select") {
       ctx.addLine(`p${nodeID}.props = {value: ${expr}};`);
       addNodeHook("create", `n.elm.value=${expr};`);
@@ -255,10 +265,8 @@ QWeb.addDirective({
       }
       handler = `(ev) => {${expr} = ${valueCode}}`;
     }
-    ctx.addLine(
+    ctx.addLine(`extra.handlers[${key}] = extra.handlers[${key}] || (${handler});`);
-      `extra.handlers['${event}' + ${nodeID}] = extra.handlers['${event}' + ${nodeID}] || (${handler});`
+    ctx.addLine(`p${nodeID}.on['${event}'] = extra.handlers[${key}];`);
-    );
-    ctx.addLine(`p${nodeID}.on['${event}'] = extra.handlers['${event}' + ${nodeID}];`);
   }
 });
 

tests/__snapshots__/animations.test.ts.snap

@@ -12,8 +12,8 @@ exports[`animations t-transition combined with component 1`] = `
     let c1 = [], p1 = {key:1};
     var vn1 = h('div', p1, c1);
     //COMPONENT
-    let templateId3 = \`__4__\`;
+    let k4 = \`__5__\`;
-    let w3 = templateId3 in parent.__owl__.cmap ? parent.__owl__.children[parent.__owl__.cmap[templateId3]] : false;
+    let w3 = k4 in parent.__owl__.cmap ? parent.__owl__.children[parent.__owl__.cmap[k4]] : false;
     let props3 = {};
     if (w3 && w3.__owl__.currentFiber && !w3.__owl__.vnode) {
         w3.destroy();
@@ -28,9 +28,9 @@ exports[`animations t-transition combined with component 1`] = `
         let W3 = context.constructor.components[componentKey3] || QWeb.components[componentKey3]|| context['Child'];
         if (!W3) {throw new Error('Cannot find the definition of component \\"' + componentKey3 + '\\"')}
         w3 = new W3(parent, props3);
-        parent.__owl__.cmap[templateId3] = w3.__owl__.id;
+        parent.__owl__.cmap[k4] = w3.__owl__.id;
         let def2 = w3.__prepare(extra.fiber, undefined, undefined, sibling);
-        let pvnode = h('dummy', {key: templateId3, hook: {insert(vn) { let nvn=w3.__mount(fiber, pvnode.elm);pvnode.elm=nvn.elm;utils.transitionInsert(vn, 'chimay');},remove() {},destroy(vn) {let finalize = () => {
+        let pvnode = h('dummy', {key: k4, hook: {insert(vn) { let nvn=w3.__mount(fiber, pvnode.elm);pvnode.elm=nvn.elm;utils.transitionInsert(vn, 'chimay');},remove() {},destroy(vn) {let finalize = () => {
           w3.destroy();
         };
         utils.transitionRemove(vn, 'chimay', finalize);}}});
@@ -57,8 +57,8 @@ exports[`animations t-transition combined with t-component and t-if 1`] = `
     var vn1 = h('div', p1, c1);
     if (context['state'].display) {
         //COMPONENT
-        let templateId3 = \`__4__\`;
+        let k4 = \`__5__\`;
-        let w3 = templateId3 in parent.__owl__.cmap ? parent.__owl__.children[parent.__owl__.cmap[templateId3]] : false;
+        let w3 = k4 in parent.__owl__.cmap ? parent.__owl__.children[parent.__owl__.cmap[k4]] : false;
         let props3 = {};
         if (w3 && w3.__owl__.currentFiber && !w3.__owl__.vnode) {
             w3.destroy();
@@ -73,9 +73,9 @@ exports[`animations t-transition combined with t-component and t-if 1`] = `
             let W3 = context.constructor.components[componentKey3] || QWeb.components[componentKey3]|| context['Child'];
             if (!W3) {throw new Error('Cannot find the definition of component \\"' + componentKey3 + '\\"')}
             w3 = new W3(parent, props3);
-            parent.__owl__.cmap[templateId3] = w3.__owl__.id;
+            parent.__owl__.cmap[k4] = w3.__owl__.id;
             let def2 = w3.__prepare(extra.fiber, undefined, undefined, sibling);
-            let pvnode = h('dummy', {key: templateId3, hook: {insert(vn) { let nvn=w3.__mount(fiber, pvnode.elm);pvnode.elm=nvn.elm;utils.transitionInsert(vn, 'chimay');},remove() {},destroy(vn) {let finalize = () => {
+            let pvnode = h('dummy', {key: k4, hook: {insert(vn) { let nvn=w3.__mount(fiber, pvnode.elm);pvnode.elm=nvn.elm;utils.transitionInsert(vn, 'chimay');},remove() {},destroy(vn) {let finalize = () => {
           w3.destroy();
         };
         utils.transitionRemove(vn, 'chimay', finalize);}}});

tests/animations.test.ts

@@ -252,11 +252,11 @@ describe("animations", () => {
     widget.state.display = false;
     patchNextFrame(cb => {
       expect(fixture.innerHTML).toBe(
-        '<div><span class="chimay-leave chimay-leave-active" data-owl-key="__4__">blue</span></div>'
+        '<div><span class="chimay-leave chimay-leave-active" data-owl-key="__5__">blue</span></div>'
       );
       cb();
       expect(fixture.innerHTML).toBe(
-        '<div><span class="chimay-leave-active chimay-leave-to" data-owl-key="__4__">blue</span></div>'
+        '<div><span class="chimay-leave-active chimay-leave-to" data-owl-key="__5__">blue</span></div>'
       );
       def.resolve();
     });

tests/component/__snapshots__/props_validation.test.ts.snap

@@ -12,8 +12,8 @@ exports[`props validation props are validated in dev mode (code snapshot) 1`] =
     let c1 = [], p1 = {key:1};
     var vn1 = h('div', p1, c1);
     //COMPONENT
-    let templateId3 = \`__4__\`;
+    let k4 = \`__5__\`;
-    let w3 = templateId3 in parent.__owl__.cmap ? parent.__owl__.children[parent.__owl__.cmap[templateId3]] : false;
+    let w3 = k4 in parent.__owl__.cmap ? parent.__owl__.children[parent.__owl__.cmap[k4]] : false;
     let props3 = {message:1};
     if (w3 && w3.__owl__.currentFiber && !w3.__owl__.vnode) {
         w3.destroy();
@@ -28,9 +28,9 @@ exports[`props validation props are validated in dev mode (code snapshot) 1`] =
         let W3 = context.constructor.components[componentKey3] || QWeb.components[componentKey3]|| context['Child'];
         if (!W3) {throw new Error('Cannot find the definition of component \\"' + componentKey3 + '\\"')}
         w3 = new W3(parent, props3);
-        parent.__owl__.cmap[templateId3] = w3.__owl__.id;
+        parent.__owl__.cmap[k4] = w3.__owl__.id;
         let def2 = w3.__prepare(extra.fiber, undefined, undefined, sibling);
-        let pvnode = h('dummy', {key: templateId3, hook: {insert(vn) { let nvn=w3.__mount(fiber, pvnode.elm);pvnode.elm=nvn.elm;},remove() {},destroy(vn) {w3.destroy();}}});
+        let pvnode = h('dummy', {key: k4, hook: {insert(vn) { let nvn=w3.__mount(fiber, pvnode.elm);pvnode.elm=nvn.elm;},remove() {},destroy(vn) {w3.destroy();}}});
         const fiber = w3.__owl__.currentFiber;
         def2.then(function () {if (fiber.isCompleted) {return;} const vnode = fiber.vnode; pvnode.sel = vnode.sel; });
         c1.push(pvnode);

tests/component/component.test.ts

@@ -4271,6 +4271,31 @@ describe("t-model directive", () => {
     expect(comp.state.number).toBe("invalid");
     expect(fixture.innerHTML).toBe("<div><input><span>invalid</span></div>");
   });
+
+  test("in a t-foreach", async () => {
+    class SomeComponent extends Component<any, any> {
+      static template = xml`
+        <div>
+          <t t-foreach="state" t-as="thing" t-key="thing.id">
+            <input type="checkbox" t-model="thing.f"/>
+          </t>
+        </div>
+      `;
+      state = useState([{ f: false, id: 1 }, { f: false, id: 2 }, { f: false, id: 3 }]);
+    }
+    const comp = new SomeComponent();
+    await comp.mount(fixture);
+    expect(fixture.innerHTML).toBe(
+      '<div><input type="checkbox"><input type="checkbox"><input type="checkbox"></div>'
+    );
+
+    const input = fixture.querySelectorAll("input")[1]!;
+    input.click();
+    expect(comp.state[1].f).toBe(true);
+    expect(comp.state[0].f).toBe(false);
+    expect(comp.state[2].f).toBe(false);
+    expect(env.qweb.templates[SomeComponent.template].fn.toString()).toMatchSnapshot();
+  });
 });
 
 describe("environment and plugins", () => {

tests/router/__snapshots__/route_component.test.ts.snap

@@ -13,10 +13,10 @@ exports[`RouteComponent can render simple cases 1`] = `
     if (context['routeComponent']) {
         const nodeKey1 = context['env'].router.currentRouteName;
         //COMPONENT
-        let templateId3 = \`__4__\` + nodeKey1;
+        let k4 = \`__5__\` + nodeKey1;
-        let w3 = templateId3 in parent.__owl__.cmap ? parent.__owl__.children[parent.__owl__.cmap[templateId3]] : false;
+        let w3 = k4 in parent.__owl__.cmap ? parent.__owl__.children[parent.__owl__.cmap[k4]] : false;
-        let vn5 = {};
+        let vn6 = {};
-        result = vn5;
+        result = vn6;
         let props3 = Object.assign({}, context['env'].router.currentParams);
         if (w3 && w3.__owl__.currentFiber && !w3.__owl__.vnode) {
             w3.destroy();
@@ -25,18 +25,18 @@ exports[`RouteComponent can render simple cases 1`] = `
         if (w3) {
             w3.__updateProps(props3, extra.fiber, undefined, undefined, sibling);
             let pvnode = w3.__owl__.pvnode;
-            utils.defineProxy(vn5, pvnode);
+            utils.defineProxy(vn6, pvnode);
         } else {
             let componentKey3 = \`routeComponent\`;
             let W3 = context.constructor.components[componentKey3] || QWeb.components[componentKey3]|| context['routeComponent'];
             if (!W3) {throw new Error('Cannot find the definition of component \\"' + componentKey3 + '\\"')}
             w3 = new W3(parent, props3);
-            parent.__owl__.cmap[templateId3] = w3.__owl__.id;
+            parent.__owl__.cmap[k4] = w3.__owl__.id;
             let def2 = w3.__prepare(extra.fiber, undefined, undefined, sibling);
-            let pvnode = h('dummy', {key: templateId3, hook: {insert(vn) { let nvn=w3.__mount(fiber, pvnode.elm);pvnode.elm=nvn.elm;},remove() {},destroy(vn) {w3.destroy();}}});
+            let pvnode = h('dummy', {key: k4, hook: {insert(vn) { let nvn=w3.__mount(fiber, pvnode.elm);pvnode.elm=nvn.elm;},remove() {},destroy(vn) {w3.destroy();}}});
             const fiber = w3.__owl__.currentFiber;
             def2.then(function () {if (fiber.isCompleted) {return;} const vnode = fiber.vnode; pvnode.sel = vnode.sel; });
-            utils.defineProxy(vn5, pvnode);
+            utils.defineProxy(vn6, pvnode);
             w3.__owl__.pvnode = pvnode;
         }
         sibling = w3.__owl__.currentFiber || sibling;