From e4532cfe51238c7763702aee4d8e615aba212462 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=A9ry=20Debongnie?= Date: Sun, 30 Apr 2017 18:09:03 +0200 Subject: [PATCH] [FIX] web: various fixes in list view navigation With this commit, we try to improve the way navigation is done in the list view. In particular, we want to make sure that pressing TAB on the last editable cell move the cursor to the next line (and create a new line if necessary). Also, we add a few tests for keyboard navigation (it was almost not tested before this commit, and while working on this commit, it was broken at some point). --- .../static/src/js/fields/abstract_field.js | 2 +- .../web/static/src/js/fields/basic_fields.js | 17 +++ .../static/src/js/fields/relational_fields.js | 1 - .../src/js/views/basic/basic_renderer.js | 39 ++++--- .../js/views/list/list_editable_renderer.js | 20 ++-- addons/web/static/tests/views/form_tests.js | 29 +++++ addons/web/static/tests/views/list_tests.js | 106 ++++++++++++++++++ 7 files changed, 192 insertions(+), 22 deletions(-) diff --git a/addons/web/static/src/js/fields/abstract_field.js b/addons/web/static/src/js/fields/abstract_field.js index 9f977d41392..4948a3e3c31 100644 --- a/addons/web/static/src/js/fields/abstract_field.js +++ b/addons/web/static/src/js/fields/abstract_field.js @@ -344,7 +344,7 @@ var AbstractField = Widget.extend({ _setValue: function (value) { // we try to avoid doing useless work, if the value given has not // changed. Note that we compare the unparsed values. - if (this.lastSetValue === value) { + if (this.lastSetValue === value || (this.value === false && value === '')) { return; } this.lastSetValue = value; diff --git a/addons/web/static/src/js/fields/basic_fields.js b/addons/web/static/src/js/fields/basic_fields.js index a462efee6e0..e600954b967 100644 --- a/addons/web/static/src/js/fields/basic_fields.js +++ b/addons/web/static/src/js/fields/basic_fields.js @@ -55,6 +55,23 @@ var DebouncedField = AbstractField.extend({ } }, + //-------------------------------------------------------------------------- + // Public + //-------------------------------------------------------------------------- + + /** + * Debounced fields do not notify the rest of the environment immediately of + * their new state, so it is necessary to actually get their value when this + * method is called. + * + * @override + */ + commitChanges: function () { + if (this.mode === 'edit') { + this._setValue(this._getValue()); + } + }, + //-------------------------------------------------------------------------- // Private //-------------------------------------------------------------------------- diff --git a/addons/web/static/src/js/fields/relational_fields.js b/addons/web/static/src/js/fields/relational_fields.js index 2a3d5e01125..28d790b16c4 100644 --- a/addons/web/static/src/js/fields/relational_fields.js +++ b/addons/web/static/src/js/fields/relational_fields.js @@ -15,7 +15,6 @@ odoo.define('web.relational_fields', function (require) { var AbstractField = require('web.AbstractField'); var concurrency = require('web.concurrency'); -var config = require('web.config'); var ControlPanel = require('web.ControlPanel'); var dialogs = require('web.view_dialogs'); var core = require('web.core'); diff --git a/addons/web/static/src/js/views/basic/basic_renderer.js b/addons/web/static/src/js/views/basic/basic_renderer.js index ed9726478a0..e150b0b2dd0 100644 --- a/addons/web/static/src/js/views/basic/basic_renderer.js +++ b/addons/web/static/src/js/views/basic/basic_renderer.js @@ -147,32 +147,45 @@ var BasicRenderer = AbstractRenderer.extend({ }, /** * Activates the widget at the given index for the given record if possible - * or the "next" possible one. + * or the "next" possible one. Usually, a widget can be activated if it is + * in edit mode, and if it is visible. * * @private * @param {Object} record * @param {integer} currentIndex - * @param {integer} [inc=1] - the increment to use when searching for the - * "next" possible one + * @param {Object} [options] + * @param {integer} [options.inc=1] - the increment to use when searching for the + * "next" possible one + * @param {boolean} [options.wrap=true] if true, when we arrive at the end of the + * list of widget, we wrap around and try to activate widgets starting at + * the beginning. Otherwise, we just stop trying and return -1 * @returns {integer} the index of the widget that was activated or -1 if - * none was possible to activate + * none was possible to activate */ - _activateFieldWidget: function (record, currentIndex, inc) { - inc = inc === undefined ? 1 : inc; + _activateFieldWidget: function (record, currentIndex, options) { + options = options || {}; + _.defaults(options, {inc: 1, wrap: true}); - var activated; var recordWidgets = this.allFieldWidgets[record.id] || []; for (var i = 0 ; i < recordWidgets.length ; i++) { - activated = recordWidgets[currentIndex].activate(); + var activated = recordWidgets[currentIndex].activate(); if (activated) { return currentIndex; } - currentIndex += inc; + currentIndex += options.inc; if (currentIndex >= recordWidgets.length) { - currentIndex -= recordWidgets.length; + if (options.wrap) { + currentIndex -= recordWidgets.length; + } else { + return -1; + } } else if (currentIndex < 0) { - currentIndex += recordWidgets.length; + if (options.wrap) { + currentIndex += recordWidgets.length; + } else { + return -1; + } } } return -1; @@ -188,7 +201,7 @@ var BasicRenderer = AbstractRenderer.extend({ */ _activateNextFieldWidget: function (record, currentIndex) { currentIndex = (currentIndex + 1) % (this.allFieldWidgets[record.id] || []).length; - return this._activateFieldWidget(record, currentIndex, +1); + return this._activateFieldWidget(record, currentIndex, {inc: 1}); }, /** * This is a wrapper of the {@see _activateFieldWidget} function to select @@ -201,7 +214,7 @@ var BasicRenderer = AbstractRenderer.extend({ */ _activatePreviousFieldWidget: function (record, currentIndex) { currentIndex = currentIndex ? (currentIndex - 1) : ((this.allFieldWidgets[record.id] || []).length - 1); - return this._activateFieldWidget(record, currentIndex, -1); + return this._activateFieldWidget(record, currentIndex, {inc:-1}); }, /** * Does the necessary DOM updates to match the given modifiers data. The diff --git a/addons/web/static/src/js/views/list/list_editable_renderer.js b/addons/web/static/src/js/views/list/list_editable_renderer.js index 12ef168c131..3d34b80f846 100644 --- a/addons/web/static/src/js/views/list/list_editable_renderer.js +++ b/addons/web/static/src/js/views/list/list_editable_renderer.js @@ -335,22 +335,27 @@ ListRenderer.include({ * * @param {integer} rowIndex * @param {integer} colIndex + * @param {boolean} [wrap=true] if true and no widget could be selected from + * the colIndex to the last column, then we wrap around and try to select + * a widget starting from the beginning + * @return {Deferred} fails if no cell could be selected */ - _selectCell: function (rowIndex, colIndex) { + _selectCell: function (rowIndex, colIndex, wrap) { // Do nothing if the user tries to select current cell if (rowIndex === this.currentRow && colIndex === this.currentCol) { - return; + return $.when(); } + wrap = wrap === undefined ? true : wrap; // Select the row then activate the widget in the correct cell var self = this; - this._selectRow(rowIndex).then(function () { + return this._selectRow(rowIndex).then(function () { var record = self.state.data[rowIndex]; - var fieldIndex = self._activateFieldWidget(record, colIndex - getNbButtonBefore(colIndex)); + var correctedIndex = colIndex - getNbButtonBefore(colIndex); + var fieldIndex = self._activateFieldWidget(record, correctedIndex, {inc: 1, wrap: wrap}); - // If no widget to activate in the row, unselect the row if (fieldIndex < 0) { - return self._unselectRow(); + return $.Deferred().reject(); } self.currentCol = fieldIndex + getNbButtonBefore(fieldIndex); @@ -543,7 +548,8 @@ ListRenderer.include({ // component up there, such as a form renderer. ev.stopPropagation(); if (this.currentCol + 1 < this.columns.length) { - this._selectCell(this.currentRow, this.currentCol + 1); + this._selectCell(this.currentRow, this.currentCol + 1, false) + .fail(this._moveToNextLine.bind(this)); } else { this._moveToNextLine(); } diff --git a/addons/web/static/tests/views/form_tests.js b/addons/web/static/tests/views/form_tests.js index 9295f0fa236..b6d416156aa 100644 --- a/addons/web/static/tests/views/form_tests.js +++ b/addons/web/static/tests/views/form_tests.js @@ -3075,6 +3075,35 @@ QUnit.module('Views', { form.destroy(); }); + QUnit.test('skip invisible fields when navigating with TAB', function (assert) { + assert.expect(1); + + this.data.partner.records[0].bar = true; + + var form = createView({ + View: FormView, + model: 'partner', + data: this.data, + arch: '
' + + '' + + '' + + '' + + '' + + '' + + '
', + res_id: 1, + }); + + form.$buttons.find('.o_form_button_edit').click(); + form.$('input[name="foo"]').focus(); + form.$('input[name="foo"]').trigger($.Event('keydown', {which: $.ui.keyCode.TAB})); + assert.strictEqual(form.$('input[name="int_field"]')[0], document.activeElement, + "int_field should be focused"); + + form.destroy(); + }); + QUnit.test('clicking on a stat button with a context', function (assert) { assert.expect(1); diff --git a/addons/web/static/tests/views/list_tests.js b/addons/web/static/tests/views/list_tests.js index ddfb985c3c1..50d5fbd7182 100644 --- a/addons/web/static/tests/views/list_tests.js +++ b/addons/web/static/tests/views/list_tests.js @@ -1784,6 +1784,112 @@ QUnit.module('Views', { list.destroy(); }); + QUnit.test('navigation with tab and readonly field', function (assert) { + // This test makes sure that if we have 2 cells in a row, the first in + // edit mode, and the second one readonly, then if we press TAB when the + // focus is on the first, then the focus skip the readonly cells and + // directly goes to the next line instead. + assert.expect(2); + + var list = createView({ + View: ListView, + model: 'foo', + data: this.data, + arch: '', + }); + + // click on first td and press TAB + list.$('td:contains(yop)').last().click(); + list.$('tr.o_selected_row input[name="foo"]').trigger({type: 'keydown', which: $.ui.keyCode.TAB}); + + assert.ok(list.$('tr.o_data_row:eq(1)').hasClass('o_selected_row'), + "2nd row should be selected"); + + // we do it again. This was broken because the this.currentRow variable + // was not properly set, and the second TAB could cause a crash. + list.$('tr.o_selected_row input[name="foo"]').trigger({type: 'keydown', which: $.ui.keyCode.TAB}); + assert.ok(list.$('tr.o_data_row:eq(2)').hasClass('o_selected_row'), + "3rd row should be selected"); + + list.destroy(); + }); + + QUnit.test('edition, then navigation with tab (with a readonly field)', function (assert) { + // This test makes sure that if we have 2 cells in a row, the first in + // edit mode, and the second one readonly, then if we edit and press TAB, + // (before debounce), the save operation is properly done (before + // selecting the next row) + assert.expect(4); + + var list = createView({ + View: ListView, + model: 'foo', + data: this.data, + arch: '', + mockRPC: function (route, args) { + if (args.method) { + assert.step(args.method); + } + return this._super.apply(this, arguments); + }, + fieldDebounce: 1, + }); + + // click on first td and press TAB + list.$('td:contains(yop)').click(); + list.$('tr.o_selected_row input[name="foo"]').val('new value').trigger('input'); + list.$('tr.o_selected_row input[name="foo"]').trigger({type: 'keydown', which: $.ui.keyCode.TAB}); + + assert.strictEqual(list.$('tbody tr:first td:contains(new value)').length, 1, + "should have the new value visible in dom"); + assert.verifySteps(["write", "read"]); + list.destroy(); + }); + + QUnit.test('skip invisible fields when navigating list view with TAB', function (assert) { + assert.expect(2); + + var list = createView({ + View: ListView, + model: 'foo', + data: this.data, + arch: '' + + '' + + '' + + '' + + '', + res_id: 1, + }); + + list.$('td:contains(gnap)').click(); + assert.strictEqual(list.$('input[name="foo"]')[0], document.activeElement, + "foo should be focused"); + list.$('input[name="foo"]').trigger($.Event('keydown', {which: $.ui.keyCode.TAB})); + assert.strictEqual(list.$('input[name="int_field"]')[0], document.activeElement, + "int_field should be focused"); + + list.destroy(); + }); + + QUnit.test('navigation: moving down with keydown', function (assert) { + assert.expect(2); + + var list = createView({ + View: ListView, + model: 'foo', + data: this.data, + arch: '', + }); + + list.$('td:contains(yop)').click(); + assert.ok(list.$('tr.o_data_row:eq(0)').hasClass('o_selected_row'), + "1st row should be selected"); + list.$('tr.o_selected_row input[name="foo"]').trigger({type: 'keydown', which: $.ui.keyCode.DOWN}); + assert.ok(list.$('tr.o_data_row:eq(1)').hasClass('o_selected_row'), + "2nd row should be selected"); + list.destroy(); + }); + }); });