Skip to content

Commit

Permalink
[FIX] compiler: better handle update of properties with same value
Browse files Browse the repository at this point in the history
Before this commit, owl would incorrectly skip patching html properties
when the new value is the same as the value used in the previous render.
However, this is incorrect, since the user may have changed the value by
clicking on a checkbox, or changing some text in an input.

So, to be more correct, owl has to force the update whenever it
encounters a prop.  This is however hard to do without impacting the
main update loop, so we kind of work around the problem by using String
and Boolean instance, instead of primitive values.
  • Loading branch information
ged-odoo authored and sdegueldre committed Jul 8, 2022
1 parent 6c72e0a commit f8073cb
Show file tree
Hide file tree
Showing 5 changed files with 84 additions and 14 deletions.
16 changes: 11 additions & 5 deletions src/compiler/code_generator.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { isProp } from "../runtime/blockdom/attributes";
import {
compileExpr,
compileExprToArray,
Expand All @@ -22,12 +23,12 @@ import {
ASTTif,
ASTTKey,
ASTTOut,
ASTTSet,
ASTTPortal,
ASTTranslation,
ASTTSet,
ASTType,
ASTTPortal,
EventHandlers,
Attrs,
EventHandlers,
} from "./parser";

type BlockType = "block" | "text" | "multi" | "list" | "html" | "comment";
Expand Down Expand Up @@ -578,13 +579,18 @@ export class CodeGenerator {
attrName = key.slice(7);
attrs["block-attribute-" + idx] = attrName;
} else if (key.startsWith("t-att")) {
attrName = key === "t-att" ? null : key.slice(6);
expr = compileExpr(ast.attrs[key]);
if (attrName && isProp(ast.tag, attrName)) {
// we force a new string or new boolean to bypass the equality check in blockdom when patching same value
const C = attrName === "value" ? "String" : "Boolean";
expr = `new ${C}(${expr})`;
}
const idx = block!.insertData(expr, "attr");
if (key === "t-att") {
attrs[`block-attributes`] = String(idx);
} else {
attrName = key.slice(6);
attrs[`block-attribute-${idx}`] = attrName;
attrs[`block-attribute-${idx}`] = attrName!;
}
} else if (this.translatableAttributes.includes(key)) {
attrs[key] = this.translateFn(ast.attrs[key]);
Expand Down
2 changes: 1 addition & 1 deletion src/runtime/blockdom/attributes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ export function updateClass(this: HTMLElement, val: any, oldVal: any) {
export function makePropSetter(name: string): Setter<HTMLElement> {
return function setProp(this: HTMLElement, value: any) {
// support 0, fallback to empty string for other falsy values
(this as any)[name] = value === 0 ? 0 : value || "";
(this as any)[name] = value === 0 ? 0 : value ? value.valueOf() : "";
};
}

Expand Down
38 changes: 33 additions & 5 deletions tests/compiler/__snapshots__/attributes.test.ts.snap
Original file line number Diff line number Diff line change
Expand Up @@ -745,7 +745,7 @@ exports[`special cases for some specific html attributes/properties input of typ
let block1 = createBlock(\`<input type=\\"checkbox\\" block-attribute-0=\\"indeterminate\\"/>\`);
return function template(ctx, node, key = \\"\\") {
let attr1 = ctx['v'];
let attr1 = new Boolean(ctx['v']);
return block1([attr1]);
}
}"
Expand All @@ -759,7 +759,21 @@ exports[`special cases for some specific html attributes/properties input type=
let block1 = createBlock(\`<input type=\\"checkbox\\" block-attribute-0=\\"checked\\"/>\`);
return function template(ctx, node, key = \\"\\") {
let attr1 = ctx['flag'];
let attr1 = new Boolean(ctx['flag']);
return block1([attr1]);
}
}"
`;
exports[`special cases for some specific html attributes/properties input with t-att-value (patching with same value 1`] = `
"function anonymous(app, bdom, helpers
) {
let { text, createBlock, list, multi, html, toggler, comment } = bdom;
let block1 = createBlock(\`<input block-attribute-0=\\"value\\"/>\`);
return function template(ctx, node, key = \\"\\") {
let attr1 = new String(ctx['v']);
return block1([attr1]);
}
}"
Expand All @@ -773,7 +787,21 @@ exports[`special cases for some specific html attributes/properties input with t
let block1 = createBlock(\`<input block-attribute-0=\\"value\\"/>\`);
return function template(ctx, node, key = \\"\\") {
let attr1 = ctx['v'];
let attr1 = new String(ctx['v']);
return block1([attr1]);
}
}"
`;
exports[`special cases for some specific html attributes/properties input, type checkbox, with t-att-checked (patching with same value 1`] = `
"function anonymous(app, bdom, helpers
) {
let { text, createBlock, list, multi, html, toggler, comment } = bdom;
let block1 = createBlock(\`<input type=\\"checkbox\\" block-attribute-0=\\"checked\\"/>\`);
return function template(ctx, node, key = \\"\\") {
let attr1 = new Boolean(ctx['v']);
return block1([attr1]);
}
}"
Expand All @@ -787,7 +815,7 @@ exports[`special cases for some specific html attributes/properties select with
let block1 = createBlock(\`<select block-attribute-0=\\"value\\"><option value=\\"potato\\">Potato</option><option value=\\"tomato\\">Tomato</option><option value=\\"onion\\">Onion</option></select>\`);
return function template(ctx, node, key = \\"\\") {
let attr1 = ctx['value'];
let attr1 = new String(ctx['value']);
return block1([attr1]);
}
}"
Expand All @@ -801,7 +829,7 @@ exports[`special cases for some specific html attributes/properties textarea wit
let block1 = createBlock(\`<textarea block-attribute-0=\\"value\\"/>\`);
return function template(ctx, node, key = \\"\\") {
let attr1 = ctx['v'];
let attr1 = new String(ctx['v']);
return block1([attr1]);
}
}"
Expand Down
6 changes: 3 additions & 3 deletions tests/compiler/__snapshots__/misc.test.ts.snap
Original file line number Diff line number Diff line change
Expand Up @@ -269,15 +269,15 @@ exports[`misc other complex template 1`] = `
ctx[\`category\`] = v_block15[i1];
const key1 = ctx['category'].id;
let attr6 = ctx['category'].id;
let attr7 = ctx['category'].id==ctx['options'].active_category_id;
let attr7 = new Boolean(ctx['category'].id==ctx['options'].active_category_id);
let txt5 = ctx['category'].name;
c_block15[i1] = withKey(block16([attr6, attr7, txt5]), key1);
}
ctx = ctx.__proto__;
const b15 = list(c_block15);
b14 = block14([], [b15]);
}
let attr8 = ctx['search'].value;
let attr8 = new String(ctx['search'].value);
let hdlr4 = [ctx['updateFilter'], ctx];
let hdlr5 = [ctx['updateFilter'], ctx];
let hdlr6 = [ctx['clearSearch'], ctx];
Expand All @@ -291,7 +291,7 @@ exports[`misc other complex template 1`] = `
if (!ctx['trigger'].manual&&ctx['trigger'].project_id===ctx['project'].id&&ctx['trigger'].category_id===ctx['options'].active_category_id) {
let attr9 = \`trigger_\${ctx['trigger'].id}\`;
let attr10 = \`trigger_\${ctx['trigger'].id}\`;
let attr11 = ctx['options'].trigger_display[ctx['trigger'].id];
let attr11 = new Boolean(ctx['options'].trigger_display[ctx['trigger'].id]);
let attr12 = ctx['trigger'].id;
let hdlr7 = [ctx['updateTriggerDisplay'], ctx];
let attr13 = \`trigger_\${ctx['trigger'].id}\`;
Expand Down
36 changes: 36 additions & 0 deletions tests/compiler/attributes.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -429,6 +429,42 @@ describe("special cases for some specific html attributes/properties", () => {
expect(input.value).toBe("potato");
});

test("input with t-att-value (patching with same value", () => {
// render input with initial value
const template = `<input t-att-value="v"/>`;
const bnode1 = renderToBdom(template, { v: "zucchini" });
const fixture = makeTestFixture();
mount(bnode1, fixture);
const input = fixture.querySelector("input")!;
expect(input.value).toBe("zucchini");

// change value manually in input, to simulate user input
input.value = "tomato";
expect(input.value).toBe("tomato");

const bnode2 = renderToBdom(template, { v: "zucchini" });
patch(bnode1, bnode2);
expect(input.value).toBe("zucchini");
});

test("input, type checkbox, with t-att-checked (patching with same value", () => {
// render input with initial value
const template = `<input type="checkbox" t-att-checked="v"/>`;
const bnode1 = renderToBdom(template, { v: true });
const fixture = makeTestFixture();
mount(bnode1, fixture);
const input = fixture.querySelector("input")!;
expect(input.checked).toBe(true);

// change checked manually in input, to simulate user input
input.checked = false;
expect(input.checked).toBe(false);

const bnode2 = renderToBdom(template, { v: true });
patch(bnode1, bnode2);
expect(input.checked).toBe(true);
});

test("input of type checkbox with t-att-indeterminate", () => {
const template = `<input type="checkbox" t-att-indeterminate="v"/>`;
const bnode1 = renderToBdom(template, { v: true });
Expand Down

0 comments on commit f8073cb

Please sign in to comment.