From bf2866a27c3168b39edf0e251d9b1139fc5f42e6 Mon Sep 17 00:00:00 2001
From: Bruno Boi <boi@odoo.com>
Date: Fri, 27 Oct 2023 13:02:54 +0200
Subject: [PATCH] [FIX] web: trapfocus if first/last tabable changes

**Before this commit**
If a UI active element taker has its first or last
tabable element rendered conditionnaly, the focus trap mechanism will
not work properly.

**After this commit**
It works as intended.

Part-of: odoo/odoo#140885
---
 addons/web/static/src/core/ui/ui_service.js   | 15 ++++--
 .../web/static/tests/core/ui_service_tests.js | 49 ++++++++++++++++++-
 2 files changed, 58 insertions(+), 6 deletions(-)

diff --git a/addons/web/static/src/core/ui/ui_service.js b/addons/web/static/src/core/ui/ui_service.js
index c0ff1528b19..b6545bfaf5c 100644
--- a/addons/web/static/src/core/ui/ui_service.js
+++ b/addons/web/static/src/core/ui/ui_service.js
@@ -30,10 +30,16 @@ export function useActiveElement(refName) {
     const uiService = useService("ui");
     const owner = useRef(refName);
 
-    let lastTabableEl, firstTabableEl;
-
     function trapFocus(e) {
-        switch (getActiveHotkey(e)) {
+        const hotkey = getActiveHotkey(e);
+        if (!["tab", "shift+tab"].includes(hotkey)) {
+            return;
+        }
+        const el = e.currentTarget;
+        const tabableEls = getTabableElements(el);
+        const firstTabableEl = tabableEls[0] || el;
+        const lastTabableEl = tabableEls[tabableEls.length - 1] || el;
+        switch (hotkey) {
             case "tab":
                 if (document.activeElement === lastTabableEl) {
                     firstTabableEl.focus();
@@ -66,8 +72,7 @@ export function useActiveElement(refName) {
                      */
                     el.tabIndex = -1;
                 }
-                firstTabableEl = tabableEls[0] || el;
-                lastTabableEl = tabableEls[tabableEls.length - 1] || el;
+                const firstTabableEl = tabableEls[0] || el;
 
                 el.addEventListener("keydown", trapFocus);
 
diff --git a/addons/web/static/tests/core/ui_service_tests.js b/addons/web/static/tests/core/ui_service_tests.js
index d0ae28ff375..37e6d00b0f8 100644
--- a/addons/web/static/tests/core/ui_service_tests.js
+++ b/addons/web/static/tests/core/ui_service_tests.js
@@ -7,7 +7,7 @@ import { makeTestEnv } from "../helpers/mock_env";
 import { makeFakeLocalizationService } from "../helpers/mock_services";
 import { getFixture, mount, nextTick, triggerEvent } from "../helpers/utils";
 
-import { Component, xml } from "@odoo/owl";
+import { Component, useState, xml } from "@odoo/owl";
 const serviceRegistry = registry.category("services");
 
 let target;
@@ -231,3 +231,50 @@ QUnit.test("UI active element: trap focus - no focus element", async (assert) =>
     assert.strictEqual(event.defaultPrevented, true);
     assert.strictEqual(document.activeElement, target.querySelector("div[id=idActiveElement]"));
 });
+
+QUnit.test("UI active element: trap focus - first or last tabable changes", async (assert) => {
+    class MyComponent extends Component {
+        setup() {
+            this.show = useState({ a: true, c: false });
+            useActiveElement("delegatedRef");
+        }
+    }
+    MyComponent.template = xml`
+        <div>
+            <h1>My Component</h1>
+            <input type="text" name="outer"/>
+            <div id="idActiveElement" t-ref="delegatedRef">
+                <div>
+                    <input type="text" name="a" t-if="show.a"/>
+                    <input type="text" name="b"/>
+                    <input type="text" name="c" t-if="show.c"/>
+                </div>
+            </div>
+        </div>
+    `;
+
+    const env = await makeTestEnv({ ...baseConfig });
+    const comp = await mount(MyComponent, target, { env });
+
+    assert.strictEqual(document.activeElement, target.querySelector("input[name=a]"));
+    // Pressing 'Shift + Tab'
+    let event = await triggerEvent(document.activeElement, null, "keydown", {
+        key: "Tab",
+        shiftKey: true,
+    });
+    assert.strictEqual(event.defaultPrevented, true);
+    assert.strictEqual(document.activeElement, target.querySelector("input[name=b]"));
+
+    comp.show.a = false;
+    comp.show.c = true;
+    await nextTick();
+    assert.strictEqual(document.activeElement, target.querySelector("input[name=b]"));
+
+    // Pressing 'Shift + Tab'
+    event = await triggerEvent(document.activeElement, null, "keydown", {
+        key: "Tab",
+        shiftKey: true,
+    });
+    assert.strictEqual(event.defaultPrevented, true);
+    assert.strictEqual(document.activeElement, target.querySelector("input[name=c]"));
+});