From d43624b5c52bc0b5bd8b6bb36f2a828ff9cbd3eb Mon Sep 17 00:00:00 2001
From: Aaron Bohy <aab@odoo.com>
Date: Thu, 4 Apr 2019 11:50:01 +0000
Subject: [PATCH] [FIX] web: list: execute group button with keyboard
 navigation

Recent work on the list view allowed to add buttons in group
headers (see 015958dc17). However, we couldn't execute the action
of the button by pressing Enter, because the Enter keydown was
stopPropagated by another handler (folding the group).

This rev. makes this work, and also improves a bit the style of
the buttons in group headers.

Task 1915702
---
 .../static/src/js/views/list/list_renderer.js | 60 +++++++++++-----
 addons/web/static/src/scss/list_view.scss     |  3 +
 addons/web/static/tests/views/list_tests.js   | 72 +++++++++++++++++++
 3 files changed, 118 insertions(+), 17 deletions(-)

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 471598ca4e4..cbe99715fb5 100644
--- a/addons/web/static/src/js/views/list/list_renderer.js
+++ b/addons/web/static/src/js/views/list/list_renderer.js
@@ -444,7 +444,6 @@ var ListRenderer = BasicRenderer.extend({
      * @returns {jQuery} a <button> element
      */
     _renderGroupButton: function (list, node) {
-        var self = this;
         var $button = viewUtils.renderButtonFromNode(node, {
             extraClass: node.attrs.icon ? 'o_icon_button' : undefined,
             textAsTitle: !!node.attrs.icon,
@@ -452,20 +451,10 @@ var ListRenderer = BasicRenderer.extend({
         this._handleAttributes($button, node);
         this._registerModifiers(node, list.groupData, $button);
 
-        // TODO this should be moved to a handler
-        $button.on("click", function (e) {
-            e.stopPropagation();
-            if (node.attrs.type === 'edit') {
-                self.trigger_up('group_edit_button_clicked', {
-                    record: list.groupData,
-                });
-            } else {
-                self.trigger_up('button_clicked', {
-                    attrs: node.attrs,
-                    record: list.groupData,
-                });
-            }
-        });
+        // TODO this should be moved to event handlers
+        $button.on("click", this._onGroupButtonClicked.bind(this, list.groupData, node));
+        $button.on("keydown", this._onGroupButtonKeydown.bind(this));
+
         return $button;
     },
     /**
@@ -892,6 +881,40 @@ var ListRenderer = BasicRenderer.extend({
     // Handlers
     //--------------------------------------------------------------------------
 
+    /**
+     * @private
+     * @param {Object} record a record dataPoint on which the button applies
+     * @param {Object} node arch node of the button
+     * @param {Object} node.attrs the attrs of the button in the arch
+     * @param {jQueryEvent} ev
+     */
+    _onGroupButtonClicked: function (record, node, ev) {
+        ev.stopPropagation();
+        if (node.attrs.type === 'edit') {
+            this.trigger_up('group_edit_button_clicked', {
+                record: record,
+            });
+        } else {
+            this.trigger_up('button_clicked', {
+                attrs: node.attrs,
+                record: record,
+            });
+        }
+    },
+    /**
+     * If the user presses ENTER on a group header button, we want to execute
+     * the button action. This is done automatically as the click handler is
+     * called. However, we have to stop the propagation of the event to prevent
+     * another handler from closing the group (see _onKeyDown).
+     *
+     * @private
+     * @param {jQueryEvent} ev
+     */
+    _onGroupButtonKeydown: function (ev) {
+        if (ev.keyCode === $.ui.keyCode.ENTER) {
+            ev.stopPropagation();
+        }
+    },
     /**
      * Manages the keyboard events on the list. If the list is not editable, when the user navigates to
      * a cell using the keyboard, if he presses enter, enter the model represented by the line
@@ -947,8 +970,11 @@ var ListRenderer = BasicRenderer.extend({
                 break;
         }
         if ($futureCell) {
-            // If the cell contains activable elements, focus them instead
-            var $activables = $futureCell.find(':focusable');
+            // If the cell contains activable elements, focus them instead (except if it is in a
+            // group header, in which case we want to activate the whole header, so that we can
+            // open/close it with RIGHT/LEFT keystrokes)
+            var isInGroupHeader = $futureCell.closest('tr').hasClass('o_group_header');
+            var $activables = !isInGroupHeader && $futureCell.find(':focusable');
             if ($activables.length) {
                 $activables[0].focus();
             } else {
diff --git a/addons/web/static/src/scss/list_view.scss b/addons/web/static/src/scss/list_view.scss
index 86b989dbd10..45744a7a1f8 100644
--- a/addons/web/static/src/scss/list_view.scss
+++ b/addons/web/static/src/scss/list_view.scss
@@ -97,6 +97,7 @@
     // Grouped list views
     tbody > tr.o_group_header {
         > th, > td {
+            vertical-align: middle;
             padding-top: ($table-cell-padding-sm*2);
             padding-bottom: ($table-cell-padding-sm*2);
         }
@@ -105,8 +106,10 @@
         }
         .o_group_buttons {
             display: inline-block;
+            margin-left: 10px;
 
             > button {
+                @include o-hover-text-color($o-brand-primary, darken($o-brand-primary, 20%));
                 background-color: transparent;
                 border: none;
                 padding-top: 0;
diff --git a/addons/web/static/tests/views/list_tests.js b/addons/web/static/tests/views/list_tests.js
index 7aff294fb97..cb5ab7dd510 100644
--- a/addons/web/static/tests/views/list_tests.js
+++ b/addons/web/static/tests/views/list_tests.js
@@ -5732,6 +5732,78 @@ QUnit.module('Views', {
         list.destroy();
     });
 
+    QUnit.test('execute group header button with keyboard navigation', async function (assert) {
+        assert.expect(13);
+
+        var list = await createView({
+            View: ListView,
+            model: 'foo',
+            data: this.data,
+            arch: '<tree>' +
+                    '<field name="foo"/>' +
+                    '<groupby name="m2o">' +
+                        '<button type="object" name="some_method" string="Do this"/>' +
+                    '</groupby>' +
+                '</tree>',
+            groupBy: ['m2o'],
+            intercepts: {
+                execute_action: function (ev) {
+                    assert.strictEqual(ev.data.action_data.name, 'some_method');
+                },
+            },
+        });
+
+        assert.containsNone(list, '.o_data_row', "all groups should be closed");
+
+        // focus create button as a starting point
+        list.$('.o_list_button_add').focus();
+        assert.ok(document.activeElement.classList.contains('o_list_button_add'));
+        await testUtils.fields.triggerKeydown($(document.activeElement), 'down');
+        assert.strictEqual(document.activeElement.tagName, 'INPUT',
+            'focus should now be on the record selector (list header)');
+        await testUtils.fields.triggerKeydown($(document.activeElement), 'down');
+        assert.strictEqual(document.activeElement.textContent, 'Value 1 (3)',
+            'focus should be on first group header');
+
+        // unfold first group
+        await testUtils.fields.triggerKeydown($(document.activeElement), 'enter');
+        assert.containsN(list, '.o_data_row', 3, "first group should be open");
+
+        // move to first record of opened group
+        await testUtils.fields.triggerKeydown($(document.activeElement), 'down');
+        assert.strictEqual(document.activeElement.tagName, 'INPUT',
+            'focus should be in first row checkbox');
+
+        // move back to the group header
+        await testUtils.fields.triggerKeydown($(document.activeElement), 'up');
+        assert.ok(document.activeElement.classList.contains('o_group_name'),
+            'focus should be back on first group header');
+
+        // fold the group
+        await testUtils.fields.triggerKeydown($(document.activeElement), 'enter');
+        assert.ok(document.activeElement.classList.contains('o_group_name'),
+            'focus should still be on first group header');
+        assert.containsNone(list, '.o_data_row', "first group should now be folded");
+
+        // unfold the group
+        await testUtils.fields.triggerKeydown($(document.activeElement), 'enter');
+        assert.ok(document.activeElement.classList.contains('o_group_name'),
+            'focus should still be on first group header');
+        assert.containsN(list, '.o_data_row', 3, "first group should be open");
+
+        // simulate a move to the group header button with tab (we can't trigger a native event
+        // programmatically, see https://stackoverflow.com/a/32429197)
+        list.$('.o_group_header .o_group_buttons button:first').focus();
+        assert.strictEqual(document.activeElement.tagName, 'BUTTON',
+            'focus should be on the group header button');
+
+        // click on the button by pressing enter
+        await testUtils.fields.triggerKeydown($(document.activeElement), 'enter');
+        assert.containsN(list, '.o_data_row', 3, "first group should still be open");
+
+        list.destroy();
+    });
+
     QUnit.test('add a new row in grouped editable="top" list', async function (assert) {
         assert.expect(7);