From e343f5f5f95607b91a61fed167a6f11f0eeaeaa8 Mon Sep 17 00:00:00 2001 From: Vincent Schippefilt Date: Mon, 3 Sep 2018 16:20:06 +0200 Subject: [PATCH] [IMP] web: x2many adds a new line by tabbing on a not dirty line Before this change, when the focus was on the first field on a x2many field, and the user pressed TAB, the line would be discarded and the focus moved to the next field (at the level of the x2many) in the form. This was an issue because if the first field was required, the focus could not be moved out of the field with TAB so the user had to discard the line manually (with ESC) before moving to the next field This change works with 2 changes: 1. detecting if a line is dirty (has been modified by the user) and at the end of the line, decide to either - discard the line and move the next field, if the line is not dirty - commit the line and add a new line, if the line is dirty 2. allow users to use TAB through required field inside x2many so they can easilly go to the end of the line. Task id : 1872248 --- .../web/static/src/js/fields/basic_fields.js | 5 +- .../static/src/js/views/basic/basic_model.js | 3 + .../src/js/views/list/list_controller.js | 1 + .../js/views/list/list_editable_renderer.js | 39 ++++++++---- .../tests/fields/relational_fields_tests.js | 7 ++- addons/web/static/tests/views/list_tests.js | 61 ++++++++++++++++--- 6 files changed, 93 insertions(+), 23 deletions(-) diff --git a/addons/web/static/src/js/fields/basic_fields.js b/addons/web/static/src/js/fields/basic_fields.js index 08b6d47f0f5..41885837353 100644 --- a/addons/web/static/src/js/fields/basic_fields.js +++ b/addons/web/static/src/js/fields/basic_fields.js @@ -334,7 +334,8 @@ var InputField = DebouncedField.extend({ } if (ev.data.direction ==='next' && this.attrs.modifiersValue && - this.attrs.modifiersValue.required) { + this.attrs.modifiersValue.required && + this.viewType !== 'list') { if (!this.$input.val()){ this.setInvalidClass(); ev.stopPropagation(); @@ -388,7 +389,7 @@ var NumericField = InputField.extend({ * * Note: We have to overwrite this method to set the input's type to number if * option setted into the field. - * + * * @override * @private */ diff --git a/addons/web/static/src/js/views/basic/basic_model.js b/addons/web/static/src/js/views/basic/basic_model.js index fbb4b0b122c..9dd7c4d1141 100644 --- a/addons/web/static/src/js/views/basic/basic_model.js +++ b/addons/web/static/src/js/views/basic/basic_model.js @@ -594,6 +594,7 @@ var BasicModel = AbstractModel.extend({ getDomain: element.getDomain, getFieldNames: element.getFieldNames, id: element.id, + isDirty: element.isDirty, limit: element.limit, model: element.model, offset: element.offset, @@ -648,6 +649,7 @@ var BasicModel = AbstractModel.extend({ getFieldNames: element.getFieldNames, groupedBy: element.groupedBy, id: element.id, + isDirty: element.isDirty, isOpen: element.isOpen, limit: element.limit, model: element.model, @@ -3490,6 +3492,7 @@ var BasicModel = AbstractModel.extend({ dataPoint.getContext = this._getContext.bind(this, dataPoint); dataPoint.getDomain = this._getDomain.bind(this, dataPoint); dataPoint.getFieldNames = this._getFieldNames.bind(this, dataPoint); + dataPoint.isDirty = this.isDirty.bind(this, dataPoint.id); this.localData[dataPoint.id] = dataPoint; diff --git a/addons/web/static/src/js/views/list/list_controller.js b/addons/web/static/src/js/views/list/list_controller.js index 40736b1b508..3691903c079 100644 --- a/addons/web/static/src/js/views/list/list_controller.js +++ b/addons/web/static/src/js/views/list/list_controller.js @@ -27,6 +27,7 @@ var ListController = BasicController.extend({ selection_changed: '_onSelectionChanged', toggle_column_order: '_onToggleColumnOrder', toggle_group: '_onToggleGroup', + navigation_move: '_onNavigationMove', }), /** * @constructor 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 9cfd6f0cd61..c423ddd3f02 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 @@ -813,6 +813,10 @@ ListRenderer.include({ _onFooterClick: function () { this.unselectRow(); }, + /** + * @param {KeyDownEvent} e + * @private + */ _onKeyDownAddRecord: function(e) { switch(e.keyCode) { case $.ui.keyCode.ENTER: @@ -823,25 +827,25 @@ ListRenderer.include({ } }, /** - * It will returns the first visible widget that is editable + * It will returns the last visible widget that is editable * * @private - * @returns {Class} Widget returns first widget + * @returns {Class} Widget returns last widget */ - _getFirstWidget: function () { + _getLastWidget: function () { var record = this.state.data[this.currentRow]; var recordWidgets = this.allFieldWidgets[record.id]; - var firstWidget = _.find(recordWidgets, function (widget) { - var isFirst = + var lastWidget = _.chain(recordWidgets).filter(function (widget) { + var isLast = widget.$el.is(':visible') && ( widget.$('input').length > 0 || widget.tagName === 'input' || widget.$('textarea').length > 0 || widget.tagName === 'textarea' ) && !widget.$el.hasClass('o_readonly_modifier'); - return isFirst; - }); - return firstWidget; + return isLast; + }).last().value(); + return lastWidget; }, /** @@ -872,11 +876,22 @@ ListRenderer.include({ break; case 'next': // When navigating with the keyboard, we want to get out of the list editable if the - // first field is left empty. + // entire line is left unmodified and we are on the next line. var column = this.columns[this.currentFieldIndex]; - var firstWidget = this._getFirstWidget(); - if (column.attrs.name === firstWidget.name && !firstWidget.$input.val()) { - this.trigger_up('activate_next_widget'); + var lastWidget = this._getLastWidget(); + if (column.attrs.name === lastWidget.name) { + if (this.currentRow + 1 < this.state.data.length) { + this._selectCell(this.currentRow+1, 0, {wrap:false}) + .fail(this._moveToNextLine.bind(this)); + } else { + var currentRowData = this.state.data[this.currentRow]; + if (currentRowData.isDirty(currentRowData.id)) { + this._moveToNextLine(); + } + else { + this.trigger_up('activate_next_widget'); + } + } } else { if (this.currentFieldIndex + 1 < this.columns.length) { this._selectCell(this.currentRow, this.currentFieldIndex + 1, {wrap: false}) diff --git a/addons/web/static/tests/fields/relational_fields_tests.js b/addons/web/static/tests/fields/relational_fields_tests.js index 8809d219e49..63ac20a0a32 100644 --- a/addons/web/static/tests/fields/relational_fields_tests.js +++ b/addons/web/static/tests/fields/relational_fields_tests.js @@ -13493,7 +13493,8 @@ QUnit.module('relational_fields', { form.destroy(); }); - QUnit.test('when Navigating to a many to one with tabs, not filling in the first field and hitting tab, we should not add a first line but navigate to the next control', function (assert) { + QUnit.test('when Navigating to a many2one with tabs, not filling any field and hitting tab,' + + ' we should not add a first line but navigate to the next control', function (assert) { assert.expect(3); this.data.partner.records[0].turtles = []; @@ -13536,6 +13537,10 @@ QUnit.module('relational_fields', { keyCode: $.ui.keyCode.TAB, })); + // skips the first field of the one2many + $(document.activeElement).trigger($.Event('keydown', {which: $.ui.keyCode.TAB})); + + // skips the second (and last) field of the one2many $(document.activeElement).trigger($.Event('keydown', {which: $.ui.keyCode.TAB})); assert.strictEqual(assert.strictEqual(form.$el.find('input[name="foo"]')[0], diff --git a/addons/web/static/tests/views/list_tests.js b/addons/web/static/tests/views/list_tests.js index fedb878a669..cb1007c26c8 100644 --- a/addons/web/static/tests/views/list_tests.js +++ b/addons/web/static/tests/views/list_tests.js @@ -2352,7 +2352,7 @@ QUnit.module('Views', { }); QUnit.test('pressing tab on last cell of editable list view', function (assert) { - assert.expect(7); + assert.expect(9); var list = createView({ View: ListView, @@ -2369,6 +2369,10 @@ QUnit.module('Views', { assert.strictEqual(document.activeElement.name, "foo", "focus should be on an input with name = foo"); + //it will not create a new line unless a modification is made + document.activeElement.value = "blip-changed"; + $(document.activeElement).trigger({type: 'change'}); + list.$('tr.o_selected_row input[name="foo"]').trigger({type: 'keydown', which: 9}); // tab assert.strictEqual(document.activeElement.name, "int_field", "focus should be on an input with name = int_field"); @@ -2380,7 +2384,10 @@ QUnit.module('Views', { assert.strictEqual(document.activeElement.name, "foo", "focus should be on an input with name = foo"); - assert.verifySteps(['/web/dataset/search_read', '/web/dataset/call_kw/foo/default_get']); + assert.verifySteps(['/web/dataset/search_read', + '/web/dataset/call_kw/foo/write', + '/web/dataset/call_kw/foo/read', + '/web/dataset/call_kw/foo/default_get']); list.destroy(); }); @@ -2499,7 +2506,7 @@ QUnit.module('Views', { list.destroy(); }); - QUnit.test('navigation with tab and readonly field', function (assert) { + QUnit.test('navigation with tab and readonly field (no modification)', 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 @@ -2515,6 +2522,43 @@ QUnit.module('Views', { // 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('navigation with tab and readonly field (with modification)', 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 skips 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(); + + //modity the cell content + document.activeElement.value = "blip-changed"; + $(document.activeElement).trigger({type: 'change'}); + 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'), @@ -2578,6 +2622,7 @@ QUnit.module('Views', { '' + '' + '' + + '' + '', res_id: 1, viewOptions: { @@ -2597,10 +2642,10 @@ QUnit.module('Views', { assert.ok(form.$('.o_field_widget[name=o2m] .o_data_row:nth(1)').hasClass('o_selected_row'), "second row should be in edition"); - // Press 'Tab' -> should go back to first line as the create action isn't available + // Press 'Tab' -> should get out of the one to many and go to the next field of the form form.$('.o_field_widget[name=o2m] .o_selected_row input').trigger({type: 'keydown', which: 9}); - assert.ok(form.$('.o_field_widget[name=o2m] .o_data_row:first').hasClass('o_selected_row'), - "first row should be in edition"); + assert.strictEqual(document.activeElement, form.$('input[name="foo"]')[0], + "the next field should be selected"); form.destroy(); }); @@ -2623,12 +2668,12 @@ QUnit.module('Views', { } return this._super.apply(this, arguments); }, - fieldDebounce: 1 + 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"]').val('new value').trigger('change'); 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,