From 3ebe985c3c08b39d60d2564bb6421536a36310d5 Mon Sep 17 00:00:00 2001
From: Aaron Bohy <aab@odoo.com>
Date: Tue, 11 Jun 2019 14:48:37 +0200
Subject: [PATCH] [FIX] qweb: no more duplicated nodes during transitions

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
---
 src/qweb_extensions.ts                      |  34 +++--
 tests/__snapshots__/animations.test.ts.snap |  16 +--
 tests/animations.test.ts                    | 133 +++++++++++++++++++-
 tools/playground/samples.js                 |   4 +-
 4 files changed, 163 insertions(+), 24 deletions(-)

diff --git a/src/qweb_extensions.ts b/src/qweb_extensions.ts
index 1da7d45a1..072138f20 100644
--- a/src/qweb_extensions.ts
+++ b/src/qweb_extensions.ts
@@ -1,4 +1,5 @@
 import { QWeb, UTILS } from "./qweb_core";
+import { VNode } from "./vdom";
 
 /**
  * Owl QWeb Extensions
@@ -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 = () => {
@@ -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 = () => {
@@ -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]);
@@ -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
@@ -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
@@ -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"
@@ -434,7 +444,7 @@ QWeb.addDirective({
       finalizeWidgetCode = `let finalize = () => {
           ${finalizeWidgetCode}
         };
-        utils.transitionRemove(vn.elm, '${transition}', finalize);`;
+        utils.transitionRemove(vn, '${transition}', finalize);`;
     }
 
     let createHook = "";
diff --git a/tests/__snapshots__/animations.test.ts.snap b/tests/__snapshots__/animations.test.ts.snap
index 6700d253d..b581c5157 100644
--- a/tests/__snapshots__/animations.test.ts.snap
+++ b/tests/__snapshots__/animations.test.ts.snap
@@ -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;});
@@ -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;});
@@ -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\`});
@@ -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\`});
diff --git a/tests/animations.test.ts b/tests/animations.test.ts
index c72d7eef4..8d8069325 100644
--- a/tests/animations.test.ts
+++ b/tests/animations.test.ts
@@ -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();
     });
@@ -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>'
+    );
+  });
 });
diff --git a/tools/playground/samples.js b/tools/playground/samples.js
index e1ad026a6..08d3f6ba2 100644
--- a/tools/playground/samples.js
+++ b/tools/playground/samples.js
@@ -187,7 +187,7 @@ const ANIMATION_CSS = `button {
 
 .flash {
     background-position: center;
-    transition: background 0.5s;
+    transition: background .6s;
 }
 
 .flash:active {
@@ -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;