From e17dc1a793817c535863d2e3e40754468d140fb8 Mon Sep 17 00:00:00 2001
From: Aaron Bohy <aab@odoo.com>
Date: Thu, 2 Jul 2020 11:52:07 +0000
Subject: [PATCH] [IMP] web: form: labels of several occurrences of a field

Before this commit, when the same field occurred multiple times in
a form view, all occurrences were associated with the same id, and
all occurrences of the corresponding label (if any) had the same
'for' attribute. As a consequence, besides the fact that there were
warnings in the chrome console, clicking on any occurrence of the
duplicated label focuses the first occurrence of the field.

This commit generates a unique id for each pair label/field, when
possible. However, there is a case that can't be handled
automatically: when the label isn't automatically generated (field
located outside a group, or with nolabel attribute set to "1").
Typically, in this case, there is a <label> node with "for"
attribute referencing the associated field. In this situation, the
id must be defined in the arch, and used in the "for" attribute:

  <form>
    <label for="phone"/><field name="phone"/>
    <label for="phone_2"/><field name="phone" id="phone_2"/>
  </form>

Task 2261697
---
 .../static/src/js/views/form/form_renderer.js |  62 +++---
 .../web/static/src/js/views/form/form_view.js |  36 ++-
 addons/web/static/tests/views/form_tests.js   | 210 ++++++++++++++++++
 doc/reference/views.rst                       |  24 ++
 odoo/addons/base/models/ir_ui_view.py         |  20 +-
 odoo/addons/base/tests/test_views.py          |   2 +-
 6 files changed, 314 insertions(+), 40 deletions(-)

diff --git a/addons/web/static/src/js/views/form/form_renderer.js b/addons/web/static/src/js/views/form/form_renderer.js
index 483db4b2b09..d35012bfc39 100644
--- a/addons/web/static/src/js/views/form/form_renderer.js
+++ b/addons/web/static/src/js/views/form/form_renderer.js
@@ -11,6 +11,9 @@ var viewUtils = require('web.viewUtils');
 var _t = core._t;
 var qweb = core.qweb;
 
+// symbol used as key to set the <field> node id on its widget
+const symbol = Symbol('form');
+
 var FormRenderer = BasicRenderer.extend(WidgetAdapterMixin, {
     className: "o_form_view",
     events: _.extend({}, BasicRenderer.prototype.events, {
@@ -28,9 +31,12 @@ var FormRenderer = BasicRenderer.extend(WidgetAdapterMixin, {
 
     /**
      * @override
+     * @param {Object} params.fieldIdsToNames maps <field> node ids to field names
+     *   (useful when there are several occurrences of the same field in the arch)
      */
-    init: function () {
+    init: function (parent, state, params) {
         this._super.apply(this, arguments);
+        this.fieldIdsToNames = params.fieldIdsToNames;
         this.idsForLabels = {};
         this.lastActivatedFieldIndex = -1;
         this.alertFields = {};
@@ -113,20 +119,21 @@ var FormRenderer = BasicRenderer.extend(WidgetAdapterMixin, {
      * @returns {string[]}
      */
     canBeSaved: function () {
-        var self = this;
         var fieldNames = this._super.apply(this, arguments);
 
         var $labels = this.$('label');
         $labels.removeClass('o_field_invalid');
 
-        _.each(fieldNames, function (fieldName) {
-            var idForLabel = self.idsForLabels[fieldName];
+        const allWidgets = this.allFieldWidgets[this.state.id] || [];
+        const widgets = allWidgets.filter(w => fieldNames.includes(w.name));
+        for (const widget of widgets) {
+            const idForLabel = this.idsForLabels[widget[symbol]];
             if (idForLabel) {
                 $labels
                     .filter('[for=' + idForLabel + ']')
                     .addClass('o_field_invalid');
             }
-        });
+        }
         return fieldNames;
     },
     /*
@@ -171,7 +178,7 @@ var FormRenderer = BasicRenderer.extend(WidgetAdapterMixin, {
         var self = this;
         return this._super.apply(this, arguments).then(function (resetWidgets) {
             _.each(resetWidgets, function (widget) {
-                self._setIDForLabel(widget, self.idsForLabels[widget.name]);
+                self._setIDForLabel(widget, self.idsForLabels[widget[symbol]]);
             });
             if (self.$('.o_field_invalid').length) {
                 self.canBeSaved(self.state.id);
@@ -372,16 +379,14 @@ var FormRenderer = BasicRenderer.extend(WidgetAdapterMixin, {
     },
     /**
      * @private
-     * @param {string} name
+     * @param {string} uid a <field> node id
      * @returns {string}
      */
-    _getIDForLabel: function (name) {
-        var idForLabel = this.idsForLabels[name];
-        if (!idForLabel) {
-            idForLabel = _.uniqueId('o_field_input_');
-            this.idsForLabels[name] = idForLabel;
+    _getIDForLabel: function (uid) {
+        if (!this.idsForLabels[uid]) {
+            this.idsForLabels[uid] = _.uniqueId('o_field_input_');
         }
-        return idForLabel;
+        return this.idsForLabels[uid];
     },
     /**
      * @override
@@ -396,7 +401,9 @@ var FormRenderer = BasicRenderer.extend(WidgetAdapterMixin, {
      */
     _postProcessField: function (widget, node) {
         this._super.apply(this, arguments);
-        this._setIDForLabel(widget, this._getIDForLabel(node.attrs.name));
+        // set the node id on the widget, as it might be necessary later (tooltips, confirmChange...)
+        widget[symbol] = node.attrs.id;
+        this._setIDForLabel(widget, this._getIDForLabel(node.attrs.id));
         if (JSON.parse(node.attrs.default_focus || "0")) {
             this.defaultFocusField = widget;
         }
@@ -656,12 +663,12 @@ var FormRenderer = BasicRenderer.extend(WidgetAdapterMixin, {
     },
     /**
      * @private
-     * @param {string} label
+     * @param {Object} node
      * @returns {jQueryElement}
      */
-    _renderInnerGroupLabel: function (label) {
+    _renderInnerGroupLabel: function (node) {
         return $('<td/>', {class: 'o_td_label'})
-            .append(this._renderTagLabel(label));
+            .append(this._renderTagLabel(node));
     },
     /**
      * Render a node, from the arch of the view. It is a generic method, that
@@ -846,17 +853,22 @@ var FormRenderer = BasicRenderer.extend(WidgetAdapterMixin, {
     _renderTagLabel: function (node) {
         var self = this;
         var text;
-        var fieldName = node.tag === 'label' ? node.attrs.for : node.attrs.name;
+        let fieldName;
+        if (node.tag === 'label') {
+            fieldName = this.fieldIdsToNames[node.attrs.for]; // 'for' references a <field> node id
+        } else {
+            fieldName = node.attrs.name;
+        }
         if ('string' in node.attrs) { // allow empty string
             text = node.attrs.string;
         } else if (fieldName) {
             text = this.state.fields[fieldName].string;
-        } else  {
+        } else {
             return this._renderGenericTag(node);
         }
         var $result = $('<label>', {
             class: 'o_form_label',
-            for: this._getIDForLabel(fieldName),
+            for: this._getIDForLabel(node.tag === 'label' ? node.attrs.for : node.attrs.id),
             text: text,
         });
         if (node.tag === 'label') {
@@ -1039,16 +1051,8 @@ var FormRenderer = BasicRenderer.extend(WidgetAdapterMixin, {
 
         // Attach the tooltips on the fields' label
         _.each(this.allFieldWidgets[this.state.id], function (widget) {
-            var idForLabel = self.idsForLabels[widget.name];
-            // We usually don't support multiple widgets for the same field on the
-            // same view but it is the case with the new settings view on V11.0.
-            // Therefore, we need to retrieve the correct label since it could be
-            // displayed multiple times on the view, otherwise, for example the
-            // enterprise label will be displayed as many times as the field
-            // exists on settings.
-            var $widgets = self.$('.o_field_widget[name=' + widget.name + ']');
+            const idForLabel = self.idsForLabels[widget[symbol]];
             var $label = idForLabel ? self.$('.o_form_label[for=' + idForLabel + ']') : $();
-            $label = $label.eq($widgets.index(widget.$el));
             self._addFieldTooltip(widget, $label);
             if (widget.attrs.widget === 'upgrade_boolean') {
                 // this widget needs a reference to its $label to be correctly
diff --git a/addons/web/static/src/js/views/form/form_view.js b/addons/web/static/src/js/views/form/form_view.js
index 37cca1a0917..e3ec5177df3 100644
--- a/addons/web/static/src/js/views/form/form_view.js
+++ b/addons/web/static/src/js/views/form/form_view.js
@@ -2,11 +2,11 @@ odoo.define('web.FormView', function (require) {
 "use strict";
 
 var BasicView = require('web.BasicView');
-var config = require('web.config');
 var Context = require('web.Context');
 var core = require('web.core');
 var FormController = require('web.FormController');
 var FormRenderer = require('web.FormRenderer');
+const { generateID } = require('web.utils');
 
 var _lt = core._lt;
 
@@ -48,6 +48,7 @@ var FormView = BasicView.extend({
 
         this.rendererParams.mode = mode;
         this.rendererParams.isFromFormViewDialog = params.isFromFormViewDialog;
+        this.rendererParams.fieldIdsToNames = this.fieldsView.fieldIdsToNames;
     },
 
     //--------------------------------------------------------------------------
@@ -148,6 +149,39 @@ var FormView = BasicView.extend({
         }
         return Promise.all(defs);
     },
+    /**
+     * @override
+     */
+    _processArch(arch, fv) {
+        fv.fieldIdsToNames = {}; // maps field ids (identifying <field> nodes) to field names
+        return this._super(...arguments);
+    },
+    /**
+     * Override to populate the 'fieldIdsToNames' dict mapping <field> node ids
+     * to field names. Those ids are computed as follows:
+     *   - if set on the node, we use the 'id' attribute
+     *   - otherwise
+     *       - if this is the first occurrence of the field in the arch, we use
+     *         its name as id ('name' attribute)
+     *       - otherwise we generate an id by concatenating the field name with
+     *         a unique id
+     *       - in both cases, we set the id we generated in the attrs, as it
+     *         will be used by the renderer.
+     *
+     * @override
+     */
+    _processNode(node, fv) {
+        if (node.tag === 'field') {
+            const name = node.attrs.name;
+            let uid = node.attrs.id;
+            if (!uid) {
+                uid = name in fv.fieldIdsToNames ? `${name}__${generateID()}__` : name;
+                node.attrs.id = uid;
+            }
+            fv.fieldIdsToNames[uid] = name;
+        }
+        return this._super(...arguments);
+    },
     /**
      * We set here the limit for the number of records fetched (in one page).
      * This method is only called for subviews, not for main views.
diff --git a/addons/web/static/tests/views/form_tests.js b/addons/web/static/tests/views/form_tests.js
index 27461dda3f4..aad3eb70c1e 100644
--- a/addons/web/static/tests/views/form_tests.js
+++ b/addons/web/static/tests/views/form_tests.js
@@ -990,6 +990,216 @@ QUnit.module('Views', {
         form.destroy();
     });
 
+    QUnit.test('input ids for multiple occurrences of fields in form view', async function (assert) {
+        // A same field can occur several times in the view, but its id must be
+        // unique by occurrence, otherwise there is a warning in the console (in
+        // edit mode) as we get several inputs with the same "id" attribute, and
+        // several labels the same "for" attribute.
+        assert.expect(2);
+
+        var form = await createView({
+            View: FormView,
+            model: 'partner',
+            data: this.data,
+            arch: `
+                <form>
+                    <group>
+                        <field name="foo"/>
+                        <label for="qux"/>
+                        <div><field name="qux"/></div>
+                    </group>
+                    <group>
+                        <field name="foo"/>
+                        <label for="qux2"/>
+                        <div><field name="qux" id="qux2"/></div>
+                    </group>
+                </form>`,
+        });
+
+        const fieldIdAttrs = [...form.$('.o_field_widget')].map(n => n.getAttribute('id'));
+        const labelForAttrs = [...form.$('.o_form_label')].map(n => n.getAttribute('for'));
+
+        assert.strictEqual([...new Set(fieldIdAttrs)].length, 4,
+            "should have generated a unique id for each field occurrence");
+        assert.deepEqual(fieldIdAttrs, labelForAttrs,
+            "the for attribute of labels must coincide with field ids");
+
+        form.destroy();
+    });
+
+    QUnit.test('input ids for multiple occurrences of fields in sub form view (inline)', async function (assert) {
+        // A same field can occur several times in the view, but its id must be
+        // unique by occurrence, otherwise there is a warning in the console (in
+        // edit mode) as we get several inputs with the same "id" attribute, and
+        // several labels the same "for" attribute.
+        assert.expect(3);
+
+        var form = await createView({
+            View: FormView,
+            model: 'partner',
+            data: this.data,
+            arch: `
+                <form>
+                    <field name="p">
+                        <tree><field name="foo"/></tree>
+                        <form>
+                            <group>
+                                <field name="foo"/>
+                                <label for="qux"/>
+                                <div><field name="qux"/></div>
+                            </group>
+                            <group>
+                                <field name="foo"/>
+                                <label for="qux2"/>
+                                <div><field name="qux" id="qux2"/></div>
+                            </group>
+                        </form>
+                    </field>
+                </form>`,
+        });
+
+        await testUtils.dom.click(form.$('.o_field_x2many_list_row_add a'));
+
+        assert.containsOnce(document.body, '.modal .o_form_view');
+
+        const fieldIdAttrs = [...$('.modal .o_form_view .o_field_widget')].map(n => n.getAttribute('id'));
+        const labelForAttrs = [...$('.modal .o_form_view .o_form_label')].map(n => n.getAttribute('for'));
+
+        assert.strictEqual([...new Set(fieldIdAttrs)].length, 4,
+            "should have generated a unique id for each field occurrence");
+        assert.deepEqual(fieldIdAttrs, labelForAttrs,
+            "the for attribute of labels must coincide with field ids");
+
+        form.destroy();
+    });
+
+    QUnit.test('input ids for multiple occurrences of fields in sub form view (not inline)', async function (assert) {
+        // A same field can occur several times in the view, but its id must be
+        // unique by occurrence, otherwise there is a warning in the console (in
+        // edit mode) as we get several inputs with the same "id" attribute, and
+        // several labels the same "for" attribute.
+        assert.expect(3);
+
+        var form = await createView({
+            View: FormView,
+            model: 'partner',
+            data: this.data,
+            arch: '<form><field name="p"/></form>',
+            archs: {
+                'partner,false,list': '<tree><field name="foo"/></tree>',
+                'partner,false,form': `
+                    <form>
+                        <group>
+                            <field name="foo"/>
+                            <label for="qux"/>
+                            <div><field name="qux"/></div>
+                        </group>
+                        <group>
+                            <field name="foo"/>
+                            <label for="qux2"/>
+                            <div><field name="qux" id="qux2"/></div>
+                        </group>
+                    </form>`
+            },
+        });
+
+        await testUtils.dom.click(form.$('.o_field_x2many_list_row_add a'));
+
+        assert.containsOnce(document.body, '.modal .o_form_view');
+
+        const fieldIdAttrs = [...$('.modal .o_form_view .o_field_widget')].map(n => n.getAttribute('id'));
+        const labelForAttrs = [...$('.modal .o_form_view .o_form_label')].map(n => n.getAttribute('for'));
+
+        assert.strictEqual([...new Set(fieldIdAttrs)].length, 4,
+            "should have generated a unique id for each field occurrence");
+        assert.deepEqual(fieldIdAttrs, labelForAttrs,
+            "the for attribute of labels must coincide with field ids");
+
+        form.destroy();
+    });
+
+    QUnit.test('two occurrences of invalid field in form view', async function (assert) {
+        assert.expect(2);
+
+        this.data.partner.fields.trululu.required = true;
+
+        var form = await createView({
+            View: FormView,
+            model: 'partner',
+            data: this.data,
+            arch: `
+                <form>
+                    <group>
+                        <field name="trululu"/>
+                        <field name="trululu"/>
+                    </group>
+                </form>`,
+        });
+
+        await testUtils.form.clickSave(form);
+
+        assert.containsN(form, '.o_form_label.o_field_invalid', 2);
+        assert.containsN(form, '.o_field_many2one.o_field_invalid', 2);
+
+        form.destroy();
+    });
+
+    QUnit.test('tooltips on multiple occurrences of fields and labels', async function (assert) {
+        assert.expect(4);
+
+        const initialDebugMode = odoo.debug;
+        odoo.debug = false;
+
+        this.data.partner.fields.foo.help = 'foo tooltip';
+        this.data.partner.fields.bar.help = 'bar tooltip';
+
+        const form = await createView({
+            View: FormView,
+            model: 'partner',
+            data: this.data,
+            arch: `
+                <form>
+                    <group>
+                        <field name="foo"/>
+                        <label for="bar"/>
+                        <div><field name="bar"/></div>
+                    </group>
+                    <group>
+                        <field name="foo"/>
+                        <label for="bar2"/>
+                        <div><field name="bar" id="bar2"/></div>
+                    </group>
+                </form>`,
+        });
+
+        const $fooLabel1 = form.$('.o_form_label:nth(0)');
+        $fooLabel1.tooltip('show', false);
+        $fooLabel1.trigger($.Event('mouseenter'));
+        assert.strictEqual($('.tooltip .oe_tooltip_help').text().trim(), "foo tooltip");
+        $fooLabel1.trigger($.Event('mouseleave'));
+
+        const $fooLabel2 = form.$('.o_form_label:nth(2)');
+        $fooLabel2.tooltip('show', false);
+        $fooLabel2.trigger($.Event('mouseenter'));
+        assert.strictEqual($('.tooltip .oe_tooltip_help').text().trim(), "foo tooltip");
+        $fooLabel2.trigger($.Event('mouseleave'));
+
+        const $barLabel1 = form.$('.o_form_label:nth(1)');
+        $barLabel1.tooltip('show', false);
+        $barLabel1.trigger($.Event('mouseenter'));
+        assert.strictEqual($('.tooltip .oe_tooltip_help').text().trim(), "bar tooltip");
+        $barLabel1.trigger($.Event('mouseleave'));
+
+        const $barLabel2 = form.$('.o_form_label:nth(3)');
+        $barLabel2.tooltip('show', false);
+        $barLabel2.trigger($.Event('mouseenter'));
+        assert.strictEqual($('.tooltip .oe_tooltip_help').text().trim(), "bar tooltip");
+        $barLabel2.trigger($.Event('mouseleave'));
+
+        odoo.debug = initialDebugMode;
+        form.destroy();
+    });
+
     QUnit.test('readonly attrs on fields are re-evaluated on field change', async function (assert) {
         assert.expect(4);
 
diff --git a/doc/reference/views.rst b/doc/reference/views.rst
index 97d96fb7062..ee91fea5934 100644
--- a/doc/reference/views.rst
+++ b/doc/reference/views.rst
@@ -800,6 +800,9 @@ system. Available semantic components are:
 
   ``name`` (mandatory)
     the name of the field to render
+  ``id``
+    the node id. Useful when there are several occurrences of the same field in
+    the view (see ``label`` component below). Default is the field name.
   ``widget``
     fields have a default rendering based on their type
     (e.g. :class:`~odoo.fields.Char`,
@@ -871,6 +874,27 @@ system. Available semantic components are:
     for opening specific kanban view when selecting records from m2o/m2m in mobile
     environment
 
+``label``
+  when a ``field`` component isn't placed directly inside a ``group``, or when
+  its ``nolabel`` attribute is set, the field's label isn't automatically
+  displayed alongside its value. The ``label`` component is the manual alternative
+  of displaying the label of a field. Possible attributes are:
+  ``for`` (mandatory)
+    the reference to the field associated with the label. Can be either the name
+    of a field, or its id (``id`` attribute set on the ``field``). When there are
+    several occurrences of the same field in the view, and there are several
+    ``label`` components associated with these ``field`` nodes, those labels
+    must have unique ``for`` attributes (in this case referencing the ``id``
+    attribute of the corresponding ``field`` nodes).
+  ``string``
+    the label to display. Display the field's label (coming from the field
+    definition in the model) by default.
+  ``class``
+    same as for ``field`` component.
+  ``attrs``
+    same as for ``field`` component.
+
+
 Generic structure
 ~~~~~~~~~~~~~~~~~
 
diff --git a/odoo/addons/base/models/ir_ui_view.py b/odoo/addons/base/models/ir_ui_view.py
index 4931f3cc9d4..dd44aa799af 100644
--- a/odoo/addons/base/models/ir_ui_view.py
+++ b/odoo/addons/base/models/ir_ui_view.py
@@ -969,7 +969,7 @@ actual arch.
 
     def _postprocess_tag_field(self, node, name_manager, node_info):
         if node.get('name'):
-            attrs = {'select': node.get('select')}
+            attrs = {'id': node.get('id'), 'select': node.get('select')}
             field = name_manager.Model._fields.get(node.get('name'))
             if field:
                 # apply groups (no tested)
@@ -1224,7 +1224,7 @@ actual arch.
                     'without corresponding field or button, use \'class="o_form_label"\'.')
             self.handle_view_error(msg)
         else:
-            name_manager.must_have_name(for_, 'label for') # this could be done in check_attr
+            name_manager.must_have_name_or_id(for_, 'label for') # this could be done in check_attr
 
     def _validate_tag_page(self, node, name_manager, node_info):
         if node.getparent() is None or node.getparent().tag != 'notebook':
@@ -1908,13 +1908,15 @@ class NameManager:
         self.mandatory_fields = dict()
         self.mandatory_parent_fields = dict()
         self.available_actions = set()
-        self.mandatory_names = dict()
+        self.mandatory_names_or_ids = dict()
+        self.available_names_or_ids = set()
         self.validate = validate
         self.Model = Model
         self.fields_get = self.Model.fields_get()
 
     def has_field(self, name, info=()):
         self.available_fields.setdefault(name, {}).update(info)
+        self.available_names_or_ids.add(info.get('id') or name)
 
     def has_action(self, name):
         self.available_actions.add(name)
@@ -1929,8 +1931,8 @@ class NameManager:
         for name, use in name_uses.items():
             self.must_have_field(name, use)
 
-    def must_have_name(self, name, use):
-        self.mandatory_names[name] = use
+    def must_have_name_or_id(self, name, use):
+        self.mandatory_names_or_ids[name] = use
 
     def final_check(self):
         if self.mandatory_fields:
@@ -1943,11 +1945,11 @@ class NameManager:
         if not self.validate:
             return
 
-        for action, use in self.mandatory_names.items():
-            if action not in self.available_actions and action not in self.available_fields:
+        for action, use in self.mandatory_names_or_ids.items():
+            if action not in self.available_actions and action not in self.available_names_or_ids:
                 msg = _(
-                    "Name '%(name)s' used in '%(use)s' must be present in view but is missing.",
-                    name=action, use=use,
+                    "Name or id '%(name_or_id)s' used in '%(use)s' must be present in view but is missing.",
+                    name_or_id=action, use=use,
                 )
                 view.handle_view_error(msg)
 
diff --git a/odoo/addons/base/tests/test_views.py b/odoo/addons/base/tests/test_views.py
index 80e45066f97..df0ad72bde5 100644
--- a/odoo/addons/base/tests/test_views.py
+++ b/odoo/addons/base/tests/test_views.py
@@ -2130,7 +2130,7 @@ class TestViews(ViewCase):
         )
         self.assertInvalid(
             '<form><label for="model"/></form>',
-            "Name 'model' used in 'label for' must be present in view but is missing.",
+            "Name or id 'model' used in 'label for' must be present in view but is missing.",
         )
 
     def test_col_colspan_numerical(self):