From ae4c9d1eca5864f0cb9126d3539580fc4ae2cee9 Mon Sep 17 00:00:00 2001
From: Aaron Bohy <aab@odoo.com>
Date: Mon, 20 Apr 2020 12:16:16 +0000
Subject: [PATCH] [FIX] web: toggle optional column in list with owl component

Have a list view with an Owl field component (e.g. widget="badge"),
visible. Open the optional field dropdown and make another field
visible.

Before this commit, the '_onToggleOptionalColumn' handler was
executed twice, because two click events where triggered: one on
the label, and one on its associated input (because of the for
attribute, see [1]).

As a consequence, the new field was added, and directly removed
from the view. This could even cause subsequent crashes.

The difference between an Owl component and a legacy widget is
that the Owl component is async (more than a nextTick) as we wait
for the nextAnimationFrame to render. When the rendering is fast
enough (e.g. without Owl components), the click event isn't remap
to the input yet (because we empty the list HTML node), so the
handler is executed only once.

[1] https://developer.mozilla.org/en-US/docs/Web/HTML/Element/label

Issue reported on task 2204306

closes odoo/odoo#49920

X-original-commit: b02d347e8770835ad28a6487071930a904bce101
Signed-off-by: Lucas Perais (lpe) <lpe@odoo.com>
Signed-off-by: Aaron Bohy (aab) <aab@odoo.com>
---
 .../static/src/js/views/list/list_renderer.js |  5 ++
 addons/web/static/tests/views/list_tests.js   | 57 +++++++++++++++++++
 2 files changed, 62 insertions(+)

diff --git a/addons/web/static/src/js/views/list/list_renderer.js b/addons/web/static/src/js/views/list/list_renderer.js
index 5cf4b022c5d..68a449a9246 100644
--- a/addons/web/static/src/js/views/list/list_renderer.js
+++ b/addons/web/static/src/js/views/list/list_renderer.js
@@ -1109,6 +1109,11 @@ var ListRenderer = BasicRenderer.extend({
     _onToggleOptionalColumn: function (ev) {
         var self = this;
         ev.stopPropagation();
+        // when the input's label is clicked, the click event is also raised on the
+        // input itself (https://developer.mozilla.org/en-US/docs/Web/HTML/Element/label),
+        // so this handler is executed twice (except if the rendering is quick enough,
+        // as when we render, we empty the HTML)
+        ev.preventDefault();
         var input = ev.currentTarget.querySelector('input');
         var fieldIndex = this.optionalColumnsEnabled.indexOf(input.name);
         if (fieldIndex >= 0) {
diff --git a/addons/web/static/tests/views/list_tests.js b/addons/web/static/tests/views/list_tests.js
index 50e5e6cb9c9..c22e164bdf2 100644
--- a/addons/web/static/tests/views/list_tests.js
+++ b/addons/web/static/tests/views/list_tests.js
@@ -5,6 +5,7 @@ var AbstractStorageService = require('web.AbstractStorageService');
 var BasicModel = require('web.BasicModel');
 var core = require('web.core');
 var basicFields = require('web.basic_fields');
+var fieldRegistry = require('web.field_registry');
 var FormView = require('web.FormView');
 var ListRenderer = require('web.ListRenderer');
 var ListView = require('web.ListView');
@@ -8961,6 +8962,62 @@ QUnit.module('Views', {
         list.destroy();
     });
 
+    QUnit.test('list view with optional fields and async rendering', async function (assert) {
+        assert.expect(14);
+
+        const prom = testUtils.makeTestPromise();
+        const FieldChar = fieldRegistry.get('char');
+        fieldRegistry.add('asyncwidget', FieldChar.extend({
+            async _render() {
+                assert.ok(true, 'the rendering must be async');
+                this._super(...arguments);
+                await prom;
+            },
+        }));
+
+        const RamStorageService = AbstractStorageService.extend({
+            storage: new RamStorage(),
+        });
+
+        const list = await createView({
+            View: ListView,
+            model: 'foo',
+            data: this.data,
+            arch: `
+                <tree>
+                    <field name="m2o"/>
+                    <field name="foo" widget="asyncwidget" optional="hide"/>
+                </tree>`,
+            services: {
+                local_storage: RamStorageService,
+            },
+        });
+
+        assert.containsN(list, 'th', 2);
+        assert.isNotVisible(list.$('.o_optional_columns_dropdown'));
+
+        // add an optional field (we click on the label on purpose, as it will trigger
+        // a second event on the input)
+        await testUtils.dom.click(list.$('table .o_optional_columns_dropdown_toggle'));
+        assert.isVisible(list.$('.o_optional_columns_dropdown'));
+        assert.containsNone(list.$('.o_optional_columns_dropdown'), 'input:checked');
+        await testUtils.dom.click(list.$('div.o_optional_columns div.dropdown-item:first label'));
+
+        assert.containsN(list, 'th', 2);
+        assert.isVisible(list.$('.o_optional_columns_dropdown'));
+        assert.containsNone(list.$('.o_optional_columns_dropdown'), 'input:checked');
+
+        prom.resolve();
+        await testUtils.nextTick();
+
+        assert.containsN(list, 'th', 3);
+        assert.isVisible(list.$('.o_optional_columns_dropdown'));
+        assert.containsOnce(list.$('.o_optional_columns_dropdown'), 'input:checked');
+
+        list.destroy();
+        delete fieldRegistry.map.asyncwidget;
+    });
+
     QUnit.test('change the viewType of the current action', async function (assert) {
         assert.expect(25);