From 7e9d1ac01ec0b271390a7377676995ed4332d61d Mon Sep 17 00:00:00 2001 From: Petar Dimov Date: Wed, 16 Dec 2020 11:56:48 +0200 Subject: [PATCH 1/4] fix(ui5-popup): correct focus when there is no focusable content When there is no focusable content, the focus should be on the root of the component. Fixes #2556 --- packages/main/src/Dialog.js | 11 ---------- packages/main/src/Popover.js | 11 ---------- packages/main/src/Popup.hbs | 2 ++ packages/main/src/Popup.js | 26 ++++++++++++++++++++--- packages/main/src/themes/PopupsCommon.css | 1 + packages/main/test/pages/Popover.html | 16 ++++++++++++++ packages/main/test/specs/Popover.spec.js | 17 +++++++++++++++ 7 files changed, 59 insertions(+), 25 deletions(-) diff --git a/packages/main/src/Dialog.js b/packages/main/src/Dialog.js index 4f7b17cd874c..101e0c760f20 100644 --- a/packages/main/src/Dialog.js +++ b/packages/main/src/Dialog.js @@ -203,17 +203,6 @@ class Dialog extends Popup { return "flex"; } - get classes() { - return { - root: { - "ui5-popup-root": true, - }, - content: { - "ui5-popup-content": true, - }, - }; - } - show() { super.show(); this._center(); diff --git a/packages/main/src/Popover.js b/packages/main/src/Popover.js index 12ec2757acf8..be710d65b72e 100644 --- a/packages/main/src/Popover.js +++ b/packages/main/src/Popover.js @@ -655,17 +655,6 @@ class Popover extends Popup { }; } - get classes() { - return { - root: { - "ui5-popup-root": true, - }, - content: { - "ui5-popup-content": true, - }, - }; - } - /** * Hook for descendants to hide header. */ diff --git a/packages/main/src/Popup.hbs b/packages/main/src/Popup.hbs index 7d7f1f49d88d..04e815b82292 100644 --- a/packages/main/src/Popup.hbs +++ b/packages/main/src/Popup.hbs @@ -6,6 +6,8 @@ aria-label="{{_ariaLabel}}" aria-labelledby="{{_ariaLabelledBy}}" dir="{{dir}}" + tabindex="-1" + @keydown={{_onRootFocusOut}} > diff --git a/packages/main/src/Popup.js b/packages/main/src/Popup.js index 5931c30119df..6f5c4b208d5d 100644 --- a/packages/main/src/Popup.js +++ b/packages/main/src/Popup.js @@ -7,6 +7,7 @@ import PopupTemplate from "./generated/templates/PopupTemplate.lit.js"; import PopupBlockLayer from "./generated/templates/PopupBlockLayerTemplate.lit.js"; import { getNextZIndex, getFocusedElement, isFocusedElementWithinNode } from "./popup-utils/PopupUtils.js"; import { addOpenedPopup, removeOpenedPopup } from "./popup-utils/OpenedPopupsRegistry.js"; +import { isTabPrevious } from "@ui5/webcomponents-base/dist/Keys.js"; // Styles import styles from "./generated/themes/Popup.css.js"; @@ -242,6 +243,12 @@ class Popup extends UI5Element { }); } + _onRootFocusOut(e) { + if (e.target === this._root && isTabPrevious(e)) { + e.preventDefault(); + } + } + /** * Focus trapping * @private @@ -251,6 +258,8 @@ class Popup extends UI5Element { if (firstFocusable) { firstFocusable.focus(); + } else { + this._root.focus(); } } @@ -263,6 +272,8 @@ class Popup extends UI5Element { if (lastFocusable) { lastFocusable.focus(); + } else { + this._root.focus(); } } @@ -284,7 +295,8 @@ class Popup extends UI5Element { const element = this.getRootNode().getElementById(this.initialFocus) || document.getElementById(this.initialFocus) - || await getFirstFocusableElement(this); + || await getFirstFocusableElement(this) + || this._root; // in case of no focusable content focus the root if (element) { element.focus(); @@ -468,6 +480,10 @@ class Popup extends UI5Element { return this.ariaLabel || undefined; } + get _root() { + return this.shadowRoot.querySelector(".ui5-popup-root"); + } + get dir() { return getRTL() ? "rtl" : "ltr"; } @@ -484,8 +500,12 @@ class Popup extends UI5Element { get classes() { return { - root: {}, - content: {}, + root: { + "ui5-popup-root": true, + }, + content: { + "ui5-popup-content": true, + }, }; } } diff --git a/packages/main/src/themes/PopupsCommon.css b/packages/main/src/themes/PopupsCommon.css index 091cbb06a80d..33a5ee9af43b 100644 --- a/packages/main/src/themes/PopupsCommon.css +++ b/packages/main/src/themes/PopupsCommon.css @@ -20,6 +20,7 @@ overflow: hidden; max-height: 94vh; max-width: 90vw; + outline: none; } @media screen and (-ms-high-contrast: active) { diff --git a/packages/main/test/pages/Popover.html b/packages/main/test/pages/Popover.html index a54fb1b8b138..6df3f5c34abb 100644 --- a/packages/main/test/pages/Popover.html +++ b/packages/main/test/pages/Popover.html @@ -371,6 +371,12 @@

+ First button + Second button + Sample text for the ui5-popover + +
+ diff --git a/packages/main/test/specs/Popover.spec.js b/packages/main/test/specs/Popover.spec.js index ce762f5cebb6..b98fa6a7c765 100644 --- a/packages/main/test/specs/Popover.spec.js +++ b/packages/main/test/specs/Popover.spec.js @@ -231,6 +231,23 @@ describe("Popover general interaction", () => { assert.ok(ff.getProperty("focused"), "The first focusable element is focused."); }); + + it("tests focus when there is no focusable content", () => { + const firstBtn = $("#firstBtn"); + const popoverId = "popNoFocusableContent"; + + firstBtn.click(); + + let activeElementId = $(browser.getActiveElement()).getAttribute("id"); + + assert.strictEqual(activeElementId, popoverId, "Popover is focused"); + + browser.keys(["Shift", "Tab"]); + + activeElementId = $(browser.getActiveElement()).getAttribute("id"); + + assert.ok(activeElementId, popoverId, "Popover remains focused"); + }); }); describe("Acc", () => { From a916238b49754f622993dc12aceb6016ad39e802 Mon Sep 17 00:00:00 2001 From: Petar Dimov Date: Wed, 16 Dec 2020 12:03:22 +0200 Subject: [PATCH 2/4] eslint --- packages/main/src/Popup.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/main/src/Popup.js b/packages/main/src/Popup.js index 6f5c4b208d5d..1ff8303b9bee 100644 --- a/packages/main/src/Popup.js +++ b/packages/main/src/Popup.js @@ -3,11 +3,11 @@ import { getRTL } from "@ui5/webcomponents-base/dist/config/RTL.js"; import UI5Element from "@ui5/webcomponents-base/dist/UI5Element.js"; import { getFirstFocusableElement, getLastFocusableElement } from "@ui5/webcomponents-base/dist/util/FocusableElements.js"; import createStyleInHead from "@ui5/webcomponents-base/dist/util/createStyleInHead.js"; +import { isTabPrevious } from "@ui5/webcomponents-base/dist/Keys.js"; import PopupTemplate from "./generated/templates/PopupTemplate.lit.js"; import PopupBlockLayer from "./generated/templates/PopupBlockLayerTemplate.lit.js"; import { getNextZIndex, getFocusedElement, isFocusedElementWithinNode } from "./popup-utils/PopupUtils.js"; import { addOpenedPopup, removeOpenedPopup } from "./popup-utils/OpenedPopupsRegistry.js"; -import { isTabPrevious } from "@ui5/webcomponents-base/dist/Keys.js"; // Styles import styles from "./generated/themes/Popup.css.js"; From 6f08c21fe50ffb59d1221359ced5d6adc7faf79f Mon Sep 17 00:00:00 2001 From: Petar Dimov Date: Wed, 16 Dec 2020 13:32:07 +0200 Subject: [PATCH 3/4] fix test --- packages/main/test/specs/Popover.spec.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/main/test/specs/Popover.spec.js b/packages/main/test/specs/Popover.spec.js index b98fa6a7c765..1a90dbb1d623 100644 --- a/packages/main/test/specs/Popover.spec.js +++ b/packages/main/test/specs/Popover.spec.js @@ -233,11 +233,13 @@ describe("Popover general interaction", () => { }); it("tests focus when there is no focusable content", () => { + browser.url("http://localhost:8080/test-resources/pages/Popover.html"); + const firstBtn = $("#firstBtn"); const popoverId = "popNoFocusableContent"; firstBtn.click(); - + let activeElementId = $(browser.getActiveElement()).getAttribute("id"); assert.strictEqual(activeElementId, popoverId, "Popover is focused"); From a5db06b5ade6d6974bf60fa515706ba16da252fa Mon Sep 17 00:00:00 2001 From: Petar Dimov Date: Thu, 7 Jan 2021 16:37:11 +0200 Subject: [PATCH 4/4] rename keydown handler --- packages/main/src/Popup.hbs | 2 +- packages/main/src/Popup.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/main/src/Popup.hbs b/packages/main/src/Popup.hbs index 04e815b82292..a03f3a45848f 100644 --- a/packages/main/src/Popup.hbs +++ b/packages/main/src/Popup.hbs @@ -7,7 +7,7 @@ aria-labelledby="{{_ariaLabelledBy}}" dir="{{dir}}" tabindex="-1" - @keydown={{_onRootFocusOut}} + @keydown={{_onkeydown}} > diff --git a/packages/main/src/Popup.js b/packages/main/src/Popup.js index 1ff8303b9bee..1cb5258c2871 100644 --- a/packages/main/src/Popup.js +++ b/packages/main/src/Popup.js @@ -243,7 +243,7 @@ class Popup extends UI5Element { }); } - _onRootFocusOut(e) { + _onkeydown(e) { if (e.target === this._root && isTabPrevious(e)) { e.preventDefault(); }