From 551f894b03f3a954c6a9712e196e9301e70dbda4 Mon Sep 17 00:00:00 2001 From: Bruno Boi Date: Thu, 12 May 2022 11:42:29 +0000 Subject: [PATCH] [FIX] web: conflicting accesskey/data-hotkey attrs Issue: Sometime an element with an [accesskey] attribute could be triggered through the standard mechanism (accesskeys) instead of the hotkey service mechanism (elements with [data-hotkey] attributes). Steps to reproduce: - open a record on a form view with a pager - open some dialog (without using the keyboard) - use ALT+N => the pager behind the dialog switch to the next record This comes from the browser default mechanism for elements with an [accesskey] attribute. The hotkey service should have taken over on this standard behavior but it did not because of the dialog which became the UI active element. NB: this occurs only on a certain set of browser/OS combination. See the following table https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/accesskey#try_it Before this commit: The hotkey service would take over the accesskey standard behavior ONLY inside the UI active element. After this commit: The hotkey service will always take over the accesskey standard behavior, disregarding the UI active element. closes odoo/odoo#91261 X-original-commit: 582963a6a18ea3a2e8d3e050af4978067bfe21e1 Signed-off-by: Bruno Boi Signed-off-by: Aaron Bohy (aab) --- .../static/src/core/hotkeys/hotkey_service.js | 17 +-- .../core/hotkeys/hotkey_service_tests.js | 100 +++++++++++++++++- 2 files changed, 108 insertions(+), 9 deletions(-) diff --git a/addons/web/static/src/core/hotkeys/hotkey_service.js b/addons/web/static/src/core/hotkeys/hotkey_service.js index 185efa47315..77b13e41f63 100644 --- a/addons/web/static/src/core/hotkeys/hotkey_service.js +++ b/addons/web/static/src/core/hotkeys/hotkey_service.js @@ -70,14 +70,15 @@ export const hotkeyService = { return; } - // FIXME : this is a temporary hack. It replaces all [accesskey] attrs by [data-hotkey] on all elements. - const elementsWithoutDataHotkey = getVisibleElements( - activeElement, - "[accesskey]:not([data-hotkey])" - ); - for (const el of elementsWithoutDataHotkey) { - el.dataset.hotkey = el.accessKey; - el.removeAttribute("accesskey"); + // Replace all [accesskey] attrs by [data-hotkey] on all elements. + // This is needed to take over on the default accesskey behavior + // and also to avoid any conflict with it. + const elementsWithAccessKey = document.querySelectorAll("[accesskey]"); + for (const el of elementsWithAccessKey) { + if (el instanceof HTMLElement) { + el.dataset.hotkey = el.accessKey; + el.removeAttribute("accesskey"); + } } // Special case: open hotkey overlays 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 7562df48d8a..20a73185089 100644 --- a/addons/web/static/tests/core/hotkeys/hotkey_service_tests.js +++ b/addons/web/static/tests/core/hotkeys/hotkey_service_tests.js @@ -15,7 +15,7 @@ import { triggerHotkey, } from "../../helpers/utils"; -const { Component, xml } = owl; +const { Component, useState, xml } = owl; const serviceRegistry = registry.category("services"); let env; @@ -52,6 +52,104 @@ QUnit.test("register / unregister", async (assert) => { assert.verifySteps([key]); }); +QUnit.test("[accesskey] attrs replaced by [data-hotkey]", async (assert) => { + const div = document.createElement("div"); + div.className = "foo"; + div.accessKey = "a"; + div.onclick = () => assert.step("click"); + div.textContent = "foo"; + target.appendChild(div); + + // div must only have [accesskey] attribute + assert.containsOnce(target, ".foo"); + assert.containsOnce(target, ".foo[accesskey]"); + assert.containsNone(target, ".foo[data-hotkey]"); + + // press any hotkey, i.e. the left arrow + triggerHotkey("arrowleft"); + await nextTick(); + + // div should now only have [data-hotkey] attribute + assert.containsOnce(target, ".foo"); + assert.containsOnce(target, ".foo[data-hotkey]"); + assert.containsNone(target, ".foo[accesskey]"); + + // try to press the related hotkey, just to make sure it works + assert.verifySteps([]); + triggerHotkey("a", true); + await nextTick(); + assert.verifySteps(["click"]); +}); + +QUnit.test("[accesskey] attrs replaced by [data-hotkey], part 2", async (assert) => { + class UIOwnershipTakerComponent extends Component { + setup() { + useActiveElement("bouh"); + } + } + UIOwnershipTakerComponent.template = xml`

bouh

`; + class MyComponent extends Component { + setup() { + this.state = useState({ foo: true }); + this.step = assert.step.bind(assert); + } + } + MyComponent.components = { UIOwnershipTakerComponent }; + MyComponent.template = xml` +
+ +
foo
+
+ `; + const comp = await mount(MyComponent, target, { env }); + + // UIOwnershipTakerComponent should be there and it should be the ui active element + assert.containsOnce(target, "main .owner"); + assert.strictEqual(target.querySelector("main .owner"), env.services.ui.activeElement); + + // div must only have [accesskey] attribute + assert.containsOnce(target, "main div"); + assert.containsOnce(target, "main div[accesskey]"); + assert.containsNone(target, "main div[data-hotkey]"); + + // press any hotkey, i.e. the left arrow + triggerHotkey("arrowleft"); + await nextTick(); + + // div should now only have [data-hotkey] attribute + assert.containsOnce(target, "main div"); + assert.containsOnce(target, "main div[data-hotkey]"); + assert.containsNone(target, "main div[accesskey]"); + + // try to press the related hotkey, it should not work as the ui active element is different + assert.notEqual( + env.services.ui.getActiveElementOf(target.querySelector("main div")), + env.services.ui.activeElement + ); + assert.verifySteps([]); + triggerHotkey("a", true); + await nextTick(); + assert.verifySteps([]); + + // remove the UIOwnershipTakerComponent + comp.state.foo = false; + await nextTick(); + assert.strictEqual( + env.services.ui.getActiveElementOf(target.querySelector("main div")), + env.services.ui.activeElement + ); + + assert.containsNone(target, "main .owner"); + assert.containsOnce(target, "main div"); + assert.containsOnce(target, "main div[data-hotkey]"); + assert.containsNone(target, "main div[accesskey]"); + + assert.verifySteps([]); + triggerHotkey("a", true); + await nextTick(); + assert.verifySteps(["click"]); +}); + QUnit.test("data-hotkey", async (assert) => { assert.expect(2);