[FIX] web: properly compute popover position

- avoid updating default position list in place
- ensure computed position takes into account enough margin with border
- fallback to requested position instead of bottom

Part of task-2270731

closes odoo/odoo#58368

X-original-commit: 3fad17421518732db2085c3eec596600cfe34f03
Signed-off-by: Alexandre Kühn (aku) <aku@odoo.com>
Signed-off-by: Sébastien Theys (seb) <seb@odoo.com>

2 files changed, 41 insertions, 22 deletions

diff --git a/addons/web/static/src/js/core/popover.js b/addons/web/static/src/js/core/popover.js
index 28843d55d47..6210ce940b5 100644
--- a/addons/web/static/src/js/core/popover.js
+++ b/addons/web/static/src/js/core/popover.js
@@ -16,13 +16,13 @@ odoo.define('web.Popover', function () {
     class Popover extends Component {
         /**
          * @param {Object} props
-         * @param {String} props.position='bottom' 'top', 'bottom', 'left' or 'right'
+         * @param {String} [props.position='bottom']
          * @param {String} [props.title]
          */
         constructor() {
             super(...arguments);
             this.popoverRef = useRef('popover');
-            this.orderedPositions = ['top', 'right', 'bottom', 'left'];
+            this.orderedPositions = ['top', 'bottom', 'left', 'right'];
             this.state = useState({
                 displayed: false,
             });
@@ -99,7 +99,9 @@ odoo.define('web.Popover', function () {
                 return;
             }
 
-            const positionIndex = this.orderedPositions.indexOf(
+            // copy the default ordered position to avoid updating them in place
+            const possiblePositions = [...this.orderedPositions];
+            const positionIndex = possiblePositions.indexOf(
                 this.props.position
             );
 
@@ -110,9 +112,9 @@ odoo.define('web.Popover', function () {
 
             // check if the requested position fits the viewport; if not,
             // try all other positions and find one that does
-            const position = this.orderedPositions
+            const position = possiblePositions
                 .slice(positionIndex)
-                .concat(this.orderedPositions.slice(0, positionIndex))
+                .concat(possiblePositions.slice(0, positionIndex))
                 .map((pos) => positioningData[pos])
                 .find((pos) => {
                     this.popoverRef.el.style.top = `${pos.top}px`;
@@ -127,20 +129,19 @@ odoo.define('web.Popover', function () {
                     );
                 });
 
-            // remove all positioning classes
-            this.orderedPositions.forEach((pos) => {
+            // remove all existing positioning classes
+            possiblePositions.forEach((pos) => {
                 this.popoverRef.el.classList.remove(`o_popover--${pos}`);
             });
 
-            // apply the found position ('position' props or another position
-            // that fits the viewport) if it fits the viewport, otherwise
-            // fallback to 'bottom' if no position fits the viewport
             if (position) {
+                // apply the preferred found position that fits the viewport
                 this.popoverRef.el.classList.add(`o_popover--${position.name}`);
             } else {
-                this.popoverRef.el.style.top = `${positioningData.bottom.top}px`;
-                this.popoverRef.el.style.left = `${positioningData.bottom.left}px`;
-                this.popoverRef.el.classList.add(`o_popover--bottom`);
+                // use the given `position` props because no position fits
+                this.popoverRef.el.style.top = `${positioningData[this.props.position].top}px`;
+                this.popoverRef.el.style.left = `${positioningData[this.props.position].left}px`;
+                this.popoverRef.el.classList.add(`o_popover--${this.props.position}`);
             }
         }
 
@@ -233,14 +234,20 @@ odoo.define('web.Popover', function () {
         /**
          * Compute the expected positioning coordinates for each possible
          * positioning based on the target and popover sizes.
+         * In particular the popover must not overflow the viewport in any
+         * direction, it should actually stay at `margin` distance from the
+         * border to look good.
          *
          * @static
          * @param {HTMLElement} popoverElement The popover element
          * @param {HTMLElement} targetElement The target element, to which
          *  the popover will be visually 'bound'
+         * @param {integer} [margin=16] Minimal accepted margin from the border
+         *  of the viewport.
          * @returns {Object}
          */
-        static computePositioningData(popoverElement, targetElement) {
+        static computePositioningData(popoverElement, targetElement, margin = 16) {
+            // set target position, possible position
             const boundingRectangle = targetElement.getBoundingClientRect();
             const targetTop = boundingRectangle.top;
             const targetLeft = boundingRectangle.left;
@@ -248,25 +255,41 @@ odoo.define('web.Popover', function () {
             const targetWidth = targetElement.offsetWidth;
             const popoverHeight = popoverElement.offsetHeight;
             const popoverWidth = popoverElement.offsetWidth;
+            const windowWidth = window.innerWidth || document.documentElement.clientWidth;
+            const windowHeight = window.innerHeight || document.documentElement.clientHeight;
+            const leftOffsetForVertical = Math.max(
+                margin,
+                Math.min(
+                    Math.round(targetLeft - (popoverWidth - targetWidth) / 2),
+                    windowWidth - popoverWidth - margin,
+                ),
+            );
+            const topOffsetForHorizontal = Math.max(
+                margin,
+                Math.min(
+                    Math.round(targetTop - (popoverHeight - targetHeight) / 2),
+                    windowHeight - popoverHeight - margin,
+                ),
+            );
             return {
                 top: {
                     name: 'top',
                     top: Math.round(targetTop - popoverHeight),
-                    left: Math.round(targetLeft - (popoverWidth - targetWidth) / 2),
+                    left: leftOffsetForVertical,
                 },
                 right: {
                     name: 'right',
-                    top: Math.round(targetTop - (popoverHeight - targetHeight) / 2),
+                    top: topOffsetForHorizontal,
                     left: Math.round(targetLeft + targetWidth),
                 },
                 bottom: {
                     name: 'bottom',
                     top: Math.round(targetTop + targetHeight),
-                    left: Math.round(targetLeft - (popoverWidth - targetWidth) / 2),
+                    left: leftOffsetForVertical,
                 },
                 left: {
                     name: 'left',
-                    top: Math.round(targetTop - (popoverHeight - targetHeight) / 2),
+                    top: topOffsetForHorizontal,
                     left: Math.round(targetLeft - popoverWidth),
                 },
             };
diff --git a/addons/web/static/src/scss/popover.scss b/addons/web/static/src/scss/popover.scss
index aea72dcb446..500d097709e 100644
--- a/addons/web/static/src/scss/popover.scss
+++ b/addons/web/static/src/scss/popover.scss
@@ -73,7 +73,6 @@
     }
 
     &--top {
-        margin-top: -$arrow-size - $arrow-pad;
         animation: $animation-time slide-top;
 
         &::before,
@@ -94,7 +93,6 @@
     }
 
     &--right {
-        margin-left: $arrow-size + $arrow-pad;
         animation: $animation-time slide-right;
 
         &::before,
@@ -115,7 +113,6 @@
     }
 
     &--bottom {
-        margin-top: $arrow-size + $arrow-pad;
         animation: $animation-time slide-bottom;
 
         &::before,
@@ -136,7 +133,6 @@
     }
 
     &--left {
-        margin-left: -$arrow-size - $arrow-pad;
         animation: $animation-time slide-left;
 
         &::before,