[FIX] web: rework draggable_hook scrolling

In Knowledge, the scrollParent for the `Y` axis of an embedded ungrouped Kanban
view is the article body, which contains the draggable `container` (which is
itself contained in the scrollParent for the `X` axis).

This commit introduce 2 fixes related to the `draggable_hook_builder`:
- In `draggable_hook_builder`, `updateRects` was modifying values of the
  `containerRect` from values of the `scrollParent`. Then, the `containerRect`
  was used in `handleEdgeScrolling` to compute a scroll value to apply on the
  `scrollParent`, and when updating the dragged element position, to compute the
  boundaries for the dragged element. This commit stores values for
  `scrollParent` in `scrollParentRect` alongside the modified `containerRect`.
  `scrollParentRect` will be used to compute the scroll value based only on the
  dimensions of the scrollParent, and the modified `containerRect` will be used
  to compute the boundaries.
- The `scrollParent` in the `X` axis can be different from the `scrollParent` in
  the `Y` axis. This commit stores both of them individually so that one is not
  neglected when higher in the DOM than the other.

Task-3291771

closes odoo/odoo#121089

Signed-off-by: Julien Mougenot (jum) <jum@odoo.com>

addons/web/static/src/core/utils/draggable_hook_builder.js

@@ -47,7 +47,8 @@ import { debounce, setRecurringAnimationFrame } from "@web/core/utils/timing";
  * @property {HTMLElement | null} [currentContainer=null]
  * @property {HTMLElement | null} [currentElement=null]
  * @property {DOMRect | null} [currentElementRect=null]
- * @property {HTMLElement | null} [scrollParent=null]
+ * @property {HTMLElement | null} [currentScrollParentX]
+ * @property {HTMLElement | null} [currentScrollParentY]
  * @property {boolean} [enabled=false]
  * @property {Position} [mouse={ x: 0, y: 0 }]
  * @property {Position} [offset={ x: 0, y: 0 }]
@@ -133,20 +134,44 @@ function getRect(el, options = {}) {
  *
  * If both of these assertions are true, it means that the element can effectively
  * be scrolled on at least one axis.
+ * @param {HTMLElement} el
+ * @returns {(HTMLElement | null)[]}
+ */
+function getScrollParents(el) {
+    return [getScrollParentX(el), getScrollParentY(el)];
+}
+
+/**
+ * @param {HTMLElement} el
+ * @returns {HTMLElement | null}
+ */
+function getScrollParentX(el) {
+    if (!el) {
+        return null;
+    }
+    if (el.scrollWidth > el.clientWidth) {
+        const overflow = getComputedStyle(el).getPropertyValue("overflow");
+        if (/\bauto\b|\bscroll\b/.test(overflow)) {
+            return el;
+        }
+    }
+    return getScrollParentX(el.parentElement);
+}
+/**
  * @param {HTMLElement} el
  * @returns {HTMLElement | null}
  */
-function getScrollParent(el) {
+function getScrollParentY(el) {
     if (!el) {
         return null;
     }
-    if (el.scrollWidth > el.clientWidth || el.scrollHeight > el.clientHeight) {
+    if (el.scrollHeight > el.clientHeight) {
         const overflow = getComputedStyle(el).getPropertyValue("overflow");
         if (/\bauto\b|\bscroll\b/.test(overflow)) {
             return el;
         }
     }
-    return getScrollParent(el.parentElement);
+    return getScrollParentY(el.parentElement);
 }
 
 /**
@@ -245,7 +270,9 @@ export function makeDraggableHook(hookParams = {}) {
                 state.dragging = true;
 
                 // Compute scrollable parent
-                ctx.scrollParent = getScrollParent(ctx.currentContainer);
+                [ctx.currentScrollParentX, ctx.currentScrollParentY] = getScrollParents(
+                    ctx.currentContainer
+                );
 
                 const [eRect] = updateRects();
 
@@ -280,7 +307,10 @@ export function makeDraggableHook(hookParams = {}) {
 
                 addStyle(document.body, bodyStyle);
 
-                if (ctx.scrollParent && ctx.edgeScrolling.enabled) {
+                if (
+                    (ctx.currentScrollParentX || ctx.currentScrollParentY) &&
+                    ctx.edgeScrolling.enabled
+                ) {
                     const cleanupFn = setRecurringAnimationFrame(handleEdgeScrolling);
                     cleanups.push(cleanupFn);
                 }
@@ -318,7 +348,8 @@ export function makeDraggableHook(hookParams = {}) {
                 ctx.currentElement = null;
                 ctx.currentElementRect = null;
                 ctx.origin = null;
-                ctx.scrollParent = null;
+                ctx.currentScrollParentX = null;
+                ctx.currentScrollParentY = null;
 
                 state.dragging = false;
             };
@@ -356,37 +387,37 @@ export function makeDraggableHook(hookParams = {}) {
              * the edge of the container.
              */
             const handleEdgeScrolling = (deltaTime) => {
-                const [eRect, cRect] = updateRects();
+                const [eRect, , xRect, yRect] = updateRects();
 
                 const { speed, threshold } = ctx.edgeScrolling;
                 const correctedSpeed = (speed / 16) * deltaTime;
-                const maxWidth = cRect.x + cRect.width;
-                const maxHeight = cRect.y + cRect.height;
 
                 const diff = {};
 
-                if (eRect.x - cRect.x < threshold) {
-                    diff.x = [eRect.x - cRect.x, -1];
-                } else if (maxWidth - eRect.x - eRect.width < threshold) {
-                    diff.x = [maxWidth - eRect.x - eRect.width, 1];
+                if (xRect) {
+                    const maxWidth = xRect.x + xRect.width;
+                    if (eRect.x - xRect.x < threshold) {
+                        diff.x = [eRect.x - xRect.x, -1];
+                    } else if (maxWidth - eRect.x - eRect.width < threshold) {
+                        diff.x = [maxWidth - eRect.x - eRect.width, 1];
+                    }
                 }
-                if (eRect.y - cRect.y < threshold) {
-                    diff.y = [eRect.y - cRect.y, -1];
-                } else if (maxHeight - eRect.y - eRect.height < threshold) {
-                    diff.y = [maxHeight - eRect.y - eRect.height, 1];
+                if (yRect) {
+                    const maxHeight = yRect.y + yRect.height;
+                    if (eRect.y - yRect.y < threshold) {
+                        diff.y = [eRect.y - yRect.y, -1];
+                    } else if (maxHeight - eRect.y - eRect.height < threshold) {
+                        diff.y = [maxHeight - eRect.y - eRect.height, 1];
+                    }
                 }
 
-                if (diff.x || diff.y) {
-                    const diffToScroll = ([delta, sign]) =>
-                        (1 - clamp(delta, 0, threshold) / threshold) * correctedSpeed * sign;
-                    const scrollParams = {};
-                    if (diff.x) {
-                        scrollParams.left = diffToScroll(diff.x);
-                    }
-                    if (diff.y) {
-                        scrollParams.top = diffToScroll(diff.y);
-                    }
-                    ctx.scrollParent.scrollBy(scrollParams);
+                const diffToScroll = ([delta, sign]) =>
+                    (1 - clamp(delta, 0, threshold) / threshold) * correctedSpeed * sign;
+                if (diff.y) {
+                    ctx.currentScrollParentY.scrollBy({ top: diffToScroll(diff.y) });
+                }
+                if (diff.x) {
+                    ctx.currentScrollParentX.scrollBy({ left: diffToScroll(diff.x) });
                 }
             };
 
@@ -507,19 +538,40 @@ export function makeDraggableHook(hookParams = {}) {
             const updateRects = () => {
                 // Container rect
                 const containerRect = getRect(ctx.currentContainer, { adjust: true });
-                if (ctx.scrollParent) {
-                    // Adjust container rect according to scrollparent
-                    const parentRect = getRect(ctx.scrollParent, { adjust: true });
-                    containerRect.x = Math.max(containerRect.x, parentRect.x);
-                    containerRect.y = Math.max(containerRect.y, parentRect.y);
-                    containerRect.width = Math.min(containerRect.width, parentRect.width);
-                    containerRect.height = Math.min(containerRect.height, parentRect.height);
+                let scrollParentXRect = null;
+                let scrollParentYRect = null;
+                if (ctx.edgeScrolling.enabled) {
+                    // Adjust container rect according to scrollParents
+                    if (ctx.currentScrollParentX) {
+                        scrollParentXRect = getRect(ctx.currentScrollParentX, { adjust: true });
+                        const right = Math.min(
+                            containerRect.right,
+                            scrollParentXRect.right
+                        );
+                        containerRect.x = Math.max(
+                            containerRect.x,
+                            scrollParentXRect.x
+                        );
+                        containerRect.width = right - containerRect.x;
+                    }
+                    if (ctx.currentScrollParentY) {
+                        scrollParentYRect = getRect(ctx.currentScrollParentY, { adjust: true });
+                        const bottom = Math.min(
+                            containerRect.bottom,
+                            scrollParentYRect.bottom
+                        );
+                        containerRect.y = Math.max(
+                            containerRect.y,
+                            scrollParentYRect.y
+                        );
+                        containerRect.height = bottom - containerRect.y;
+                    }
                 }
 
                 // Element rect
                 ctx.currentElementRect = getRect(ctx.currentElement);
 
-                return [ctx.currentElementRect, containerRect];
+                return [ctx.currentElementRect, containerRect, scrollParentXRect, scrollParentYRect];
             };
 
             // Component infos

addons/web/static/tests/core/utils/ui_tests.js

 /** @odoo-module **/
 
-import { dragAndDrop, getFixture, mount, nextTick } from "@web/../tests/helpers/utils";
+import {
+    drag,
+    dragAndDrop,
+    getFixture,
+    makeDeferred,
+    mount,
+    nextTick,
+    patchWithCleanup,
+    triggerHotkey
+} from "@web/../tests/helpers/utils";
+import { browser } from "@web/core/browser/browser";
 import { useSortable } from "@web/core/utils/sortable";
 
 import { Component, reactive, useRef, useState, xml } from "@odoo/owl";
@@ -198,6 +208,119 @@ QUnit.module("UI", ({ beforeEach }) => {
         assert.verifySteps(["start", "groupenter", "stop", "drop"]);
     });
 
+    QUnit.test("Sorting in groups with distinct per-axis scrolling", async (assert) => {
+        const nextAnimationFrame = async (timeDelta) => {
+            timeStamp += timeDelta;
+            animationFrameDef.resolve();
+            animationFrameDef = makeDeferred();
+            await Promise.resolve();
+        };
+
+        let animationFrameDef = makeDeferred();
+        let timeStamp = 0;
+        let handlers = new Set();
+
+        patchWithCleanup(browser, {
+            async requestAnimationFrame(handler) {
+                await animationFrameDef;
+                // Prevent setRecurringAnimationFrame from being recursive
+                // for better test control (only the first iteration/movement
+                // is needed to check that the scrolling works).
+                if (!handlers.has(handler)) {
+                    handler(timeStamp);
+                    handlers.add(handler);
+                }
+            },
+            performance: { now: () => timeStamp },
+        });
+
+        class List extends Component {
+            setup() {
+                useSortable({
+                    ref: useRef("root"),
+                    elements: ".item",
+                    groups: ".list",
+                    connectGroups: true,
+                    edgeScrolling: { speed: 16, threshold: 25 },
+                });
+            }
+        }
+
+        List.template = xml`
+            <div class="scroll_parent_y" style="max-width: 150px; max-height: 200px; overflow-y: scroll; overflow-x: hidden;">
+                <div class="spacer_before" style="min-height: 50px;"></div>
+                <div class="spacer_horizontal" style="min-height: 50px;"></div>
+                <div t-ref="root" class="root d-flex align-items-end" style="overflow-x: scroll;">
+                    <div class="d-flex">
+                        <div style="padding-left: 20px;"
+                            t-foreach="[1, 2, 3]" t-as="c" t-key="c" t-attf-class="list m-0 list{{ c }}">
+                            <div style="min-width: 50px; min-height: 50px; padding-top: 20px;"
+                                t-foreach="[1, 2, 3]" t-as="l" t-key="l" t-esc="'item' + l + '' + c" t-attf-class="item item{{ l + '' + c }}"/>
+                        </div>
+                    </div>
+                </div>
+                <div class="spacer_after" style="min-height: 150px;"></div>
+            </div>
+        `;
+        await mount(List, target);
+
+        assert.containsN(target, ".list", 3);
+        assert.containsN(target, ".item", 9);
+
+        const scrollParentX = target.querySelector(".root");
+        const scrollParentY = target.querySelector(".scroll_parent_y");
+        const assertScrolling = (top, left) => {
+            assert.strictEqual(scrollParentY.scrollTop, top);
+            assert.strictEqual(scrollParentX.scrollLeft, left);
+        }
+        const cancelDrag = async () => {
+            triggerHotkey("Escape");
+            await nextTick();
+            scrollParentY.scrollTop = 0;
+            scrollParentX.scrollLeft = 0;
+            await nextTick();
+            assert.containsNone(target, ".o_dragged");
+        }
+        assert.containsNone(target, ".o_dragged");
+
+        // Negative horizontal scrolling.
+        target.querySelector(".spacer_horizontal").scrollIntoView();
+        scrollParentX.scrollLeft = 16;
+        await nextTick();
+        assertScrolling(50, 16);
+        await drag(".item12", ".item11", "left");
+        await nextAnimationFrame(16);
+        assertScrolling(50, 0);
+        await cancelDrag();
+
+        // Positive horizontal scrolling.
+        target.querySelector(".spacer_horizontal").scrollIntoView();
+        await nextTick();
+        assertScrolling(50, 0);
+        await drag(".item11", ".item12", "right");
+        await nextAnimationFrame(16);
+        assertScrolling(50, 16);
+        await cancelDrag();
+
+        // Negative vertical scrolling.
+        target.querySelector(".root").scrollIntoView();
+        await nextTick();
+        assertScrolling(100, 0);
+        await drag(".item11", ".item11", "top");
+        await nextAnimationFrame(16);
+        assertScrolling(84, 0);
+        await cancelDrag();
+
+        // Positive vertical scrolling.
+        target.querySelector(".spacer_before").scrollIntoView();
+        await nextTick();
+        assertScrolling(0, 0);
+        await drag(".item21", ".item21", "bottom");
+        await nextAnimationFrame(16);
+        assertScrolling(16, 0);
+        await cancelDrag();
+    });
+
     QUnit.test("Dynamically disable sortable feature", async (assert) => {
         assert.expect(4);