Skip to content

Commit

Permalink
[FIX] web: prevent traceback when pressing TAB in color_picker widget
Browse files Browse the repository at this point in the history
The color_picker widget, that can be seen for example in the track form
view for event tracks, had an issue: if the user clicked on it, then
pressed TAB, a traceback was displayed.

The problem comes from the fact that the color picker widget inherits
from FieldInput, but is not a fieldinput, so many expectations made by
the FieldInput code do not hold, such as the code run when handling
navigation (by TAB and such keypress). Because of that, the code in
_onNavigationMove crashed, because it expected an input.

Since this is a bug fix, I simply disabled the navigation in that case,
so no crash happens.  Sadly, this widget has still a big issue: it
clearly does not work as most users would expect: pressing TAB or arrows
should update the selection.  But this would be a more complicated
refactoring, for a bug which is clearly not critical, therefore this
commit implements the simple and safe solution.

Also, we disable the focus outline to minimize the wrong expectation.
Seeing them kind of implied that one could update the selection with the
keyboard.

Note that the widget color_picker was moved from another addon to web/, without
any tests nor documentation.

OPW 2467369

closes odoo#67174

Signed-off-by: Aaron Bohy (aab) <[email protected]>
  • Loading branch information
ged-odoo committed Mar 3, 2021
1 parent aa5eaf6 commit 580456b
Show file tree
Hide file tree
Showing 3 changed files with 38 additions and 0 deletions.
3 changes: 3 additions & 0 deletions addons/web/static/src/js/fields/basic_fields.js
Original file line number Diff line number Diff line change
Expand Up @@ -3666,6 +3666,9 @@ var FieldColorPicker = FieldInteger.extend({

}
},
_onNavigationMove() {
// disable navigation from FieldInput, to prevent a crash
}
});

return {
Expand Down
3 changes: 3 additions & 0 deletions addons/web/static/src/scss/color_picker.scss
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,9 @@
> li {
border: 2px solid white;
box-shadow: 0 0 0 1px gray('300');
> a:focus {
outline: none;
}
}
}
}
Expand Down
32 changes: 32 additions & 0 deletions addons/web/static/tests/fields/basic_fields_tests.js
Original file line number Diff line number Diff line change
Expand Up @@ -7615,6 +7615,38 @@ QUnit.module('basic_fields', {
form.destroy();
});

QUnit.module('FieldColorPicker');

QUnit.test('FieldColorPicker: can navigate away with TAB', async function (assert) {
assert.expect(1);

const form = await createView({
View: FormView,
model: 'partner',
data: this.data,
arch: `
<form string="Partners">
<field name="int_field" widget="color_picker"/>
<field name="foo" />
</form>`,
res_id: 1,
viewOptions: {
mode: 'edit',
},
});

form.$el.find('a.oe_kanban_color_1')[0].focus();

form.$el.find('a.oe_kanban_color_1').trigger($.Event('keydown', {
which: $.ui.keyCode.TAB,
keyCode: $.ui.keyCode.TAB,
}));
assert.strictEqual(document.activeElement, form.$el.find('input[name="foo"]')[0],
"foo field should be focused");
form.destroy();
});


QUnit.module('FieldBadge');

QUnit.test('FieldBadge component on a char field in list view', async function (assert) {
Expand Down

0 comments on commit 580456b

Please sign in to comment.