pengyuthon/odoo_source
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

[IMP] web: soften the UI active element takeover

Browse Source 
**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
This commit is contained in:
Bruno Boi
2023-11-04 16:38:53 +00:00
parent bf2866a27c
commit 003609b8d2
4 changed files with 27 additions and 43 deletions
Download Patch File Download Diff File Expand all files Collapse all files
addons/web/static/src/core/ui/ui_service.js
+15 -19
View File
@@ -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]
);
}
addons/web/static/tests/core/commands/command_service_tests.js
+1 -1
View File
@@ -235,7 +235,7 @@ QUnit.test("global command with hotkey", async (assert) => {
useActiveElement("active");
}
}
MyComponent.template = xml`<div t-ref="active"></div>`;
MyComponent.template = xml`<div t-ref="active"><button/></div>`;
await mount(MyComponent, target, { env });
triggerHotkey("a");
addons/web/static/tests/core/hotkeys/hotkey_service_tests.js
+2 -2
View File
@@ -144,7 +144,7 @@ QUnit.test("[accesskey] attrs replaced by [data-hotkey], part 2", async (assert)
useActiveElement("bouh");
}
}
UIOwnershipTakerComponent.template = xml`<p class="owner" t-ref="bouh">bouh</p>`;
UIOwnershipTakerComponent.template = xml`<p class="owner" t-ref="bouh"><button/></p>`;
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`<p class="owner" t-ref="bouh">bouh</p>`;
UIOwnershipTakerComponent.template = xml`<p class="owner" t-ref="bouh"><button/></p>`;
class C extends Component {
setup() {
this.state = useState({ foo: false });
addons/web/static/tests/core/ui_service_tests.js
+9 -21
View File
@@ -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`
<div>
<h1>My Component</h1>
<div t-if="hasRef" id="owner" t-ref="delegatedRef"/>
<div t-if="hasRef" id="owner" t-ref="delegatedRef">
<input type="text"/>
</div>
</div>
`;
@@ -85,12 +87,15 @@ QUnit.test("a component can be the UI active element: with t-ref delegation", a
assert.deepEqual(ui.activeElement, document);
const comp = await mount(MyComponent, target, { env });
const input = target.querySelector("#owner input");
assert.deepEqual(ui.activeElement, document.getElementById("owner"));
assert.strictEqual(document.activeElement, input);
comp.hasRef = false;
comp.render();
await nextTick();
assert.deepEqual(ui.activeElement, document);
assert.strictEqual(document.activeElement, document.body);
});
QUnit.test("UI active element: trap focus", async (assert) => {
@@ -192,7 +197,7 @@ QUnit.test("UI active element: trap focus - default focus with autofocus", async
assert.strictEqual(event.defaultPrevented, false);
});
QUnit.test("UI active element: trap focus - no focus element", async (assert) => {
QUnit.test("do not become UI active element if no element to focus", async (assert) => {
class MyComponent extends Component {
setup() {
useActiveElement("delegatedRef");
@@ -212,24 +217,7 @@ QUnit.test("UI active element: trap focus - no focus element", async (assert) =>
const env = await makeTestEnv({ ...baseConfig });
await mount(MyComponent, target, { env });
assert.strictEqual(
document.activeElement,
target.querySelector("div[id=idActiveElement]"),
"when there is not other element, the focus is on the UI active element itself"
);
// Pressing 'Tab'
let event = await triggerEvent(document.activeElement, null, "keydown", { key: "Tab" });
assert.strictEqual(event.defaultPrevented, true);
assert.strictEqual(document.activeElement, target.querySelector("div[id=idActiveElement]"));
// 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("div[id=idActiveElement]"));
assert.strictEqual(env.services.ui.activeElement, document);
});
QUnit.test("UI active element: trap focus - first or last tabable changes", async (assert) => {