Skip to content

Commit

Permalink
[FIX] qweb: no more duplicated nodes during transitions
Browse files Browse the repository at this point in the history 
If a node was re-added while being removed (i.e. during the remove
transition), the node was duplicated in the DOM for the delay of
the remove transition. This rev. removes the old occurence directly
in that case.

Fixes #121
  • Loading branch information
@aab-odoo @ged-odoo
aab-odoo authored and ged-odoo committed Jun 13, 2019
1 parent 1c0d4b5 commit 3ebe985
Show file tree
Hide file tree
Showing 4 changed files with 163 additions and 24 deletions.
34 changes: 22 additions & 12 deletions src/qweb_extensions.ts
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { QWeb, UTILS } from "./qweb_core";
import { VNode } from "./vdom";

/**
* Owl QWeb Extensions
Expand Down Expand Up @@ -95,7 +96,17 @@ UTILS.nextFrame = function(cb: () => void) {
requestAnimationFrame(() => requestAnimationFrame(cb));
};

UTILS.transitionInsert = function(elm: HTMLElement, name: string) {
UTILS.transitionInsert = function(vn: VNode, name: string) {
const elm = <HTMLElement>vn.elm;
// remove potential duplicated vnode that is currently being removed, to
// prevent from having twice the same node in the DOM during an animation
const dup =
elm.parentElement &&
elm.parentElement!.querySelector(`*[data-owl-key='${vn.key}']`);
if (dup) {
dup.remove();
}

elm.classList.add(name + "-enter");
elm.classList.add(name + "-enter-active");
const finalize = () => {
Expand All @@ -109,11 +120,10 @@ UTILS.transitionInsert = function(elm: HTMLElement, name: string) {
});
};

UTILS.transitionRemove = function(
elm: HTMLElement,
name: string,
rm: () => void
) {
UTILS.transitionRemove = function(vn: VNode, name: string, rm: () => void) {
const elm = <HTMLElement>vn.elm;
elm.setAttribute("data-owl-key", vn.key!);

elm.classList.add(name + "-leave");
elm.classList.add(name + "-leave-active");
const finalize = () => {
Expand Down Expand Up @@ -170,8 +180,8 @@ QWeb.addDirective({
atNodeCreation({ value, addNodeHook }) {
let name = value;
const hooks = {
insert: `this.utils.transitionInsert(vn.elm, '${name}');`,
remove: `this.utils.transitionRemove(vn.elm, '${name}', rm);`
insert: `this.utils.transitionInsert(vn, '${name}');`,
remove: `this.utils.transitionRemove(vn, '${name}', rm);`
};
for (let hookName in hooks) {
addNodeHook(hookName, hooks[hookName]);
Expand Down Expand Up @@ -297,7 +307,7 @@ const T_WIDGET_MODS_CODE = Object.assign({}, MODS_CODE, {
* let nvn = w4._mount(vnode, vn.elm);
* pvnode.elm = nvn.elm;
* // what follows is only present if there are animations on the widget
* utils.transitionInsert(vn.elm, "fade");
* utils.transitionInsert(vn, "fade");
* },
* remove() {
* // override with empty function to prevent from removing the node
Expand All @@ -310,7 +320,7 @@ const T_WIDGET_MODS_CODE = Object.assign({}, MODS_CODE, {
* let finalize = () => {
* w4.destroy();
* };
* utils.transitionRemove(vn.elm, "fade", finalize);
* utils.transitionRemove(vn, "fade", finalize);
* }
* };
* // the pvnode is inserted at the correct position in the div's children
Expand Down Expand Up @@ -422,7 +432,7 @@ QWeb.addDirective({
}
let transitionsInsertCode = "";
if (transition) {
transitionsInsertCode = `utils.transitionInsert(vn.elm, '${transition}');`;
transitionsInsertCode = `utils.transitionInsert(vn, '${transition}');`;
}
let finalizeWidgetCode = `w${widgetID}.${
keepAlive ? "unmount" : "destroy"
Expand All @@ -434,7 +444,7 @@ QWeb.addDirective({
finalizeWidgetCode = `let finalize = () => {
${finalizeWidgetCode}
};
utils.transitionRemove(vn.elm, '${transition}', finalize);`;
utils.transitionRemove(vn, '${transition}', finalize);`;
}

let createHook = "";
Expand Down
16 changes: 8 additions & 8 deletions tests/__snapshots__/animations.test.ts.snap
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,10 @@ exports[`animations t-transition combined with t-widget 1`] = `
w4 = new W4(owner, props4);
context.__owl__.cmap[4] = w4.__owl__.id;
def3 = w4._prepare();
def3 = def3.then(vnode=>{let pvnode=h(vnode.sel, {key: 4, hook: {insert(vn) {let nvn=w4._mount(vnode, pvnode.elm);pvnode.elm=nvn.elm;utils.transitionInsert(vn.elm, 'chimay');},remove() {},destroy(vn) {let finalize = () => {
def3 = def3.then(vnode=>{let pvnode=h(vnode.sel, {key: 4, hook: {insert(vn) {let nvn=w4._mount(vnode, pvnode.elm);pvnode.elm=nvn.elm;utils.transitionInsert(vn, 'chimay');},remove() {},destroy(vn) {let finalize = () => {
w4.destroy();
};
utils.transitionRemove(vn.elm, 'chimay', finalize);}}});c1[_2_index]=pvnode;w4.__owl__.pvnode = pvnode;});
utils.transitionRemove(vn, 'chimay', finalize);}}});c1[_2_index]=pvnode;w4.__owl__.pvnode = pvnode;});
} else {
def3 = def3 || w4._updateProps(props4, extra.forceUpdate, extra.patchQueue);
def3 = def3.then(()=>{if (w4.__owl__.isDestroyed) {return};let pvnode=w4.__owl__.pvnode;c1[_2_index]=pvnode;});
Expand Down Expand Up @@ -74,10 +74,10 @@ exports[`animations t-transition combined with t-widget and t-if 1`] = `
w4 = new W4(owner, props4);
context.__owl__.cmap[4] = w4.__owl__.id;
def3 = w4._prepare();
def3 = def3.then(vnode=>{let pvnode=h(vnode.sel, {key: 4, hook: {insert(vn) {let nvn=w4._mount(vnode, pvnode.elm);pvnode.elm=nvn.elm;utils.transitionInsert(vn.elm, 'chimay');},remove() {},destroy(vn) {let finalize = () => {
def3 = def3.then(vnode=>{let pvnode=h(vnode.sel, {key: 4, hook: {insert(vn) {let nvn=w4._mount(vnode, pvnode.elm);pvnode.elm=nvn.elm;utils.transitionInsert(vn, 'chimay');},remove() {},destroy(vn) {let finalize = () => {
w4.destroy();
};
utils.transitionRemove(vn.elm, 'chimay', finalize);}}});c1[_2_index]=pvnode;w4.__owl__.pvnode = pvnode;});
utils.transitionRemove(vn, 'chimay', finalize);}}});c1[_2_index]=pvnode;w4.__owl__.pvnode = pvnode;});
} else {
def3 = def3 || w4._updateProps(props4, extra.forceUpdate, extra.patchQueue);
def3 = def3.then(()=>{if (w4.__owl__.isDestroyed) {return};let pvnode=w4.__owl__.pvnode;c1[_2_index]=pvnode;});
Expand All @@ -96,10 +96,10 @@ exports[`animations t-transition with no delay/duration 1`] = `
var vn1 = h('span', p1, c1);
p1.hook = {
insert: vn => {
this.utils.transitionInsert(vn.elm, 'jupiler');
this.utils.transitionInsert(vn, 'jupiler');
},
remove: (vn, rm) => {
this.utils.transitionRemove(vn.elm, 'jupiler', rm);
this.utils.transitionRemove(vn, 'jupiler', rm);
},
};
c1.push({text: \`blue\`});
Expand All @@ -115,10 +115,10 @@ exports[`animations t-transition, on a simple node (insert) 1`] = `
var vn1 = h('span', p1, c1);
p1.hook = {
insert: vn => {
this.utils.transitionInsert(vn.elm, 'chimay');
this.utils.transitionInsert(vn, 'chimay');
},
remove: (vn, rm) => {
this.utils.transitionRemove(vn.elm, 'chimay', rm);
this.utils.transitionRemove(vn, 'chimay', rm);
},
};
c1.push({text: \`blue\`});
Expand Down
133 changes: 131 additions & 2 deletions tests/animations.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -253,11 +253,11 @@ describe("animations", () => {
widget.state.display = false;
patchNextFrame(cb => {
expect(fixture.innerHTML).toBe(
'<div><span class="chimay-leave chimay-leave-active">blue</span></div>'
'<div><span class="chimay-leave chimay-leave-active" data-owl-key="4">blue</span></div>'
);
cb();
expect(fixture.innerHTML).toBe(
'<div><span class="chimay-leave-active chimay-leave-to">blue</span></div>'
'<div><span class="chimay-leave-active chimay-leave-to" data-owl-key="4">blue</span></div>'
);
def.resolve();
});
Expand Down Expand Up @@ -287,4 +287,133 @@ describe("animations", () => {
expect(widget.f).toHaveBeenCalledTimes(1);
unpatchNextFrame();
});

test("t-transition, remove and re-add before transitionend", async () => {
expect.assertions(11);

env.qweb.addTemplates(
`<templates>
<div t-name="Parent">
<button t-on-click="toggle">Toggle</button>
<span t-if="state.flag" t-transition="chimay">blue</span>
</div>
</templates>`
);
class Parent extends Widget {
constructor(parent) {
super(parent);
this.state = { flag: false };
}
toggle() {
this.state.flag = !this.state.flag;
}
}

const widget = new Parent(env);
await widget.mount(fixture);
let button = widget.el!.querySelector("button");

let def = makeDeferred();
let phase = "enter";
patchNextFrame(cb => {
let spans = fixture.querySelectorAll("span");
expect(spans.length).toBe(1);
expect(spans[0].className).toBe(`chimay-${phase} chimay-${phase}-active`);
cb();
expect(spans[0].className).toBe(
`chimay-${phase}-active chimay-${phase}-to`
);
def.resolve();
});

// click display the span
button!.click();
await def; // wait for the mocked repaint to be done
widget.el!.querySelector("span")!.dispatchEvent(new Event("transitionend")); // mock end of css transition
expect(fixture.innerHTML).toBe(
'<div><button>Toggle</button><span class="">blue</span></div>'
);

// click to remove the span, and click again to re-add it before transitionend
def = makeDeferred();
phase = "leave";
button!.click();

await def; // wait for the mocked repaint to be done
def = makeDeferred();
phase = "enter";
button!.click();

await def; // wait for the mocked repaint to be done
widget.el!.querySelector("span")!.dispatchEvent(new Event("transitionend")); // mock end of css transition
expect(fixture.innerHTML).toBe(
'<div><button>Toggle</button><span class="">blue</span></div>'
);
});

test("t-transition combined with t-widget, remove and re-add before transitionend", async () => {
expect.assertions(11);

env.qweb.addTemplates(
`<templates>
<div t-name="Parent">
<button t-on-click="toggle">Toggle</button>
<t t-if="state.flag" t-widget="Child" t-transition="chimay"/>
</div>
<span t-name="Child">blue</span>
</templates>`
);
class Child extends Widget {}
class Parent extends Widget {
widgets = { Child };
constructor(parent) {
super(parent);
this.state = { flag: false };
}
toggle() {
this.state.flag = !this.state.flag;
}
}

const widget = new Parent(env);
await widget.mount(fixture);
let button = widget.el!.querySelector("button");

let def = makeDeferred();
let phase = "enter";
patchNextFrame(cb => {
let spans = fixture.querySelectorAll("span");
expect(spans.length).toBe(1);
expect(spans[0].className).toBe(`chimay-${phase} chimay-${phase}-active`);
cb();
expect(spans[0].className).toBe(
`chimay-${phase}-active chimay-${phase}-to`
);
def.resolve();
});

// click display the span
button!.click();
await def; // wait for the mocked repaint to be done
widget.el!.querySelector("span")!.dispatchEvent(new Event("transitionend")); // mock end of css transition
expect(fixture.innerHTML).toBe(
'<div><button>Toggle</button><span class="">blue</span></div>'
);

// click to remove the span, and click again to re-add it before transitionend
def = makeDeferred();
phase = "leave";
button!.click();

await def; // wait for the mocked repaint to be done
def = makeDeferred();
phase = "enter";
button!.click();

await def; // wait for the mocked repaint to be done
widget.el!.querySelector("span")!.dispatchEvent(new Event("transitionend")); // mock end of css transition
expect(fixture.innerHTML).toBe(
'<div><button>Toggle</button><span class="">blue</span></div>'
);
});
});
4 changes: 2 additions & 2 deletions tools/playground/samples.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,7 @@ const ANIMATION_CSS = `button {
.flash {
background-position: center;
transition: background 0.5s;
transition: background .6s;
}
.flash:active {
Expand All @@ -208,7 +208,7 @@ const ANIMATION_CSS = `button {
}
.fade-enter-active, .fade-leave-active {
transition: opacity .5s;
transition: opacity .6s;
}
.fade-enter, .fade-leave-to {
opacity: 0;
Expand Down

0 comments on commit 3ebe985

Please sign in to comment.