diff --git a/addons/web/static/src/core/ui/ui_service.js b/addons/web/static/src/core/ui/ui_service.js index b6545bfaf5c..e25e03de119 100644 --- a/addons/web/static/src/core/ui/ui_service.js +++ b/addons/web/static/src/core/ui/ui_service.js @@ -12,14 +12,18 @@ import { EventBus, reactive, useEffect, useRef } from "@odoo/owl"; export const SIZES = { XS: 0, VSM: 1, SM: 2, MD: 3, LG: 4, XL: 5, XXL: 6 }; +function getFirstAndLastTabableElements(el) { + const tabableEls = getTabableElements(el); + return [tabableEls[0], tabableEls[tabableEls.length - 1]]; +} + /** * This hook will set the UI active element - * when the caller component will mount/unmount. + * when the caller component will mount/patch and + * only if the t-reffed element has some tabable elements. * * The caller component could pass a `t-ref` value of its template * to delegate the UI active element to another element than itself. - * In that case, it is mandatory that the referenced element is fixed and - * not dynamically attached in/detached from the DOM (e.g. with t-if directive). * * @param {string} refName */ @@ -28,7 +32,7 @@ export function useActiveElement(refName) { throw new Error("refName not given to useActiveElement"); } const uiService = useService("ui"); - const owner = useRef(refName); + const ref = useRef(refName); function trapFocus(e) { const hotkey = getActiveHotkey(e); @@ -36,9 +40,7 @@ export function useActiveElement(refName) { return; } const el = e.currentTarget; - const tabableEls = getTabableElements(el); - const firstTabableEl = tabableEls[0] || el; - const lastTabableEl = tabableEls[tabableEls.length - 1] || el; + const [firstTabableEl, lastTabableEl] = getFirstAndLastTabableElements(el); switch (hotkey) { case "tab": if (document.activeElement === lastTabableEl) { @@ -60,19 +62,13 @@ export function useActiveElement(refName) { useEffect( (el) => { if (el) { + const [firstTabableEl] = getFirstAndLastTabableElements(el); + if (!firstTabableEl) { + // no tabable elements: no need to trap focus nor become the UI active element + return; + } const oldActiveElement = document.activeElement; uiService.activateElement(el); - const tabableEls = getTabableElements(el); - if (tabableEls.length === 0 && el.tabIndex < 0) { - /** - * It's possible that the active element is not a focusable element, - * adding tabindex="-1" will allow the element to be focusable. - * Note that, even if the default of tabIndex is -1, for the element to be - * focusable it should be explicitly set. - */ - el.tabIndex = -1; - } - const firstTabableEl = tabableEls[0] || el; el.addEventListener("keydown", trapFocus); @@ -100,7 +96,7 @@ export function useActiveElement(refName) { }; } }, - () => [owner.el] + () => [ref.el] ); } diff --git a/addons/web/static/tests/core/commands/command_service_tests.js b/addons/web/static/tests/core/commands/command_service_tests.js index ac704a79c77..8753e9326f5 100644 --- a/addons/web/static/tests/core/commands/command_service_tests.js +++ b/addons/web/static/tests/core/commands/command_service_tests.js @@ -235,7 +235,7 @@ QUnit.test("global command with hotkey", async (assert) => { useActiveElement("active"); } } - MyComponent.template = xml`
`; + MyComponent.template = xml``; await mount(MyComponent, target, { env }); triggerHotkey("a"); diff --git a/addons/web/static/tests/core/hotkeys/hotkey_service_tests.js b/addons/web/static/tests/core/hotkeys/hotkey_service_tests.js index 8c423bfbad1..7c1137a8bfc 100644 --- a/addons/web/static/tests/core/hotkeys/hotkey_service_tests.js +++ b/addons/web/static/tests/core/hotkeys/hotkey_service_tests.js @@ -144,7 +144,7 @@ QUnit.test("[accesskey] attrs replaced by [data-hotkey], part 2", async (assert) useActiveElement("bouh"); } } - UIOwnershipTakerComponent.template = xml`bouh
`; + UIOwnershipTakerComponent.template = xml``; class MyComponent extends Component { setup() { this.state = useState({ foo: true }); @@ -1015,7 +1015,7 @@ QUnit.test("operating area and UI active element", async (assert) => { useActiveElement("bouh"); } } - UIOwnershipTakerComponent.template = xml`bouh
`; + UIOwnershipTakerComponent.template = xml``; class C extends Component { setup() { this.state = useState({ foo: false }); diff --git a/addons/web/static/tests/core/ui_service_tests.js b/addons/web/static/tests/core/ui_service_tests.js index 37e6d00b0f8..d352ff201f6 100644 --- a/addons/web/static/tests/core/ui_service_tests.js +++ b/addons/web/static/tests/core/ui_service_tests.js @@ -66,7 +66,7 @@ QUnit.test("use block and unblock several times to block ui with ui service", as assert.strictEqual(blockUI, null, "ui should not be blocked"); }); -QUnit.test("a component can be the UI active element: with t-ref delegation", async (assert) => { +QUnit.test("a component can be the UI active element: simple usage", async (assert) => { class MyComponent extends Component { setup() { useActiveElement("delegatedRef"); @@ -76,7 +76,9 @@ QUnit.test("a component can be the UI active element: with t-ref delegation", a MyComponent.template = xml`