From 003609b8d268c19e91c190f5ee6db8dba17032f9 Mon Sep 17 00:00:00 2001 From: Bruno Boi Date: Tue, 31 Oct 2023 10:46:41 +0100 Subject: [PATCH] [IMP] web: soften the UI active element takeover **Good to know first** The UI active element takership system is directly bound to the focus trap mechanism. **Before this commit** If an element become the UI active element but has no tabable elements in its tree, we force its tabindex to the -1 value to make it programmatically focusable. That way we ensure the focus is effectively trapped. **After this commit** The tabindex is no more forced at all. Instead, if we detect that there are no tabable elements inside the UI active element candidate, it simply does not become the UI active element. In other words, this commit kind of weakens the focus trap. But this is perfectly fine for our use cases, i.e.: - dialogs always have at least one focusable button (close, ok...) - the website wysiwyg-adapter has a ton of focusable elements **Why ?** We need this change to permit other UI pieces to make use of the active element takeover mechanism, i.e. popovers. Part-of: odoo/odoo#140885 --- addons/web/static/src/core/ui/ui_service.js | 34 ++++++++----------- .../core/commands/command_service_tests.js | 2 +- .../core/hotkeys/hotkey_service_tests.js | 4 +-- .../web/static/tests/core/ui_service_tests.js | 30 +++++----------- 4 files changed, 27 insertions(+), 43 deletions(-) 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`