From a4bcbecff1eb767e69d46151a046c29287f5512d Mon Sep 17 00:00:00 2001
From: Mohammed Shekha <msh@openerp.com>
Date: Mon, 15 Oct 2018 12:09:34 +0000
Subject: [PATCH] [FIX] web: keyboard overlay with position `absolute`

Before this commit, when an element with access key has a CSS
position absolute , pressing the key 'ALT' to show the access
key overlay may break the position of the element.

This is due to altering the CSS position of the element which the
overlay is attached to: it becomes `relative`, so that the overlay
takes the whole size of the attached element.

In case of an absolute element, it should not become relative,
otherwise its position on the document may be completely different.
Since the overlay has position `absolute`, if its parent has also
position `absolute`, then keeping both with position `absolute` will
let the overlay take the size of the parent, without changing the
position of the parent.

Task-ID 1885445
---
 .../js/chrome/keyboard_navigation_mixin.js    | 16 ++++++--
 .../chrome/keyboard_navigation_mixin_tests.js | 41 ++++++++++++++++++-
 2 files changed, 52 insertions(+), 5 deletions(-)

diff --git a/addons/web/static/src/js/chrome/keyboard_navigation_mixin.js b/addons/web/static/src/js/chrome/keyboard_navigation_mixin.js
index dc65d2ea610..4913d5a6a80 100644
--- a/addons/web/static/src/js/chrome/keyboard_navigation_mixin.js
+++ b/addons/web/static/src/js/chrome/keyboard_navigation_mixin.js
@@ -45,13 +45,21 @@ odoo.define('web.KeyboardNavigationMixin', function (require) {
             var accesskeyElements = $(document).find('[accesskey]').filter(':visible');
             _.each(accesskeyElements, function (elem) {
                 var overlay = $(_.str.sprintf("<div class='o_web_accesskey_overlay'>%s</div>", $(elem).attr('accesskey').toUpperCase()));
+
+                var $overlayParent;
                 if (elem.tagName.toUpperCase() === "INPUT") {
-                    // special case for the search input that has an access key defined. We cannot set the overlay on the input itself, only on its parent
-                    overlay.appendTo($(elem).parent().css('position', 'relative'));
+                    // special case for the search input that has an access key
+                    // defined. We cannot set the overlay on the input itself,
+                    // only on its parent.
+                    $overlayParent = $(elem).parent();
+                } else {
+                    $overlayParent = $(elem);
                 }
-                else {
-                    overlay.appendTo($(elem).css('position', 'relative'));
+
+                if ($overlayParent.css('position') !== 'absolute') {
+                    $overlayParent.css('position', 'relative');
                 }
+                overlay.appendTo($overlayParent);
             });
         },
         /**
diff --git a/addons/web/static/tests/chrome/keyboard_navigation_mixin_tests.js b/addons/web/static/tests/chrome/keyboard_navigation_mixin_tests.js
index 38f13e7a117..e26451cd268 100644
--- a/addons/web/static/tests/chrome/keyboard_navigation_mixin_tests.js
+++ b/addons/web/static/tests/chrome/keyboard_navigation_mixin_tests.js
@@ -2,6 +2,7 @@ odoo.define('web.keyboard_navigation_mixin_tests', function (require) {
 "use strict";
 
 var KeyboardNavigationMixin = require('web.KeyboardNavigationMixin');
+var testUtils = require('web.test_utils');
 var Widget = require('web.Widget');
 
 QUnit.module('KeyboardNavigationMixin', function () {
@@ -16,7 +17,8 @@ QUnit.module('KeyboardNavigationMixin', function () {
                 return this._super.apply(this, arguments);
             },
         });
-        var w = new KeyboardWidget();
+        var parent = testUtils.createParent({});
+        var w = new KeyboardWidget(parent);
         w.appendTo($target);
 
         // minimum set of attribute to generate a native event that works with the mixin
@@ -26,6 +28,43 @@ QUnit.module('KeyboardNavigationMixin', function () {
         w.$el[0].dispatchEvent(e);
 
         assert.ok(w.$el.find('button[aria-keyshortcuts]')[0], 'the aria-keyshortcuts is set on the button');
+
+        parent.destroy();
+    });
+
+    QUnit.test('keep CSS position absolute for parent of overlay', function (assert) {
+        // If we change the CSS position of an 'absolute' element to 'relative',
+        // we may likely change its position on the document. Since the overlay
+        // CSS position is 'absolute', it will match the size and cover the
+        // parent with 'absolute' > 'absolute', without altering the position
+        // of the parent on the document.
+        assert.expect(1);
+        var $target = $('#qunit-fixture');
+        var $button;
+        var KeyboardWidget = Widget.extend(KeyboardNavigationMixin, {
+            start: function () {
+                $button = $('<button>').text('Click Me!').attr('accesskey', 'o');
+                // we need to define the accesskey because it will not be assigned on invisible buttons
+                this.$el.append($button);
+                return this._super.apply(this, arguments);
+            },
+        });
+        var parent = testUtils.createParent({});
+        var w = new KeyboardWidget(parent);
+        w.appendTo($target);
+
+        $button.css('position', 'absolute');
+
+        // minimum set of attribute to generate a native event that works with the mixin
+        var e = new Event("keydown");
+        e.key = '';
+        e.altKey = true;
+        w.$el[0].dispatchEvent(e);
+
+        assert.strictEqual($button.css('position'), 'absolute',
+            "should not have kept the CSS position of the button");
+
+        parent.destroy();
     });
 });
 });