From 7ef7d8022c391c8f62210999ec2debe276bd5429 Mon Sep 17 00:00:00 2001 From: Alexandar Mitsev Date: Mon, 7 Dec 2020 18:30:16 +0200 Subject: [PATCH 1/8] fix(ui5-dialog): apply initial focus after rendering Now the initial focus is applied also in cases when the dialog is opened before it is fully rendered. Fixes: #2537 --- packages/main/src/Popup.js | 27 +++++++++++++++++++++++---- 1 file changed, 23 insertions(+), 4 deletions(-) diff --git a/packages/main/src/Popup.js b/packages/main/src/Popup.js index 7a6052312953..93020fd75081 100644 --- a/packages/main/src/Popup.js +++ b/packages/main/src/Popup.js @@ -79,7 +79,15 @@ const metadata = { /** * @private */ - _disableInitialFocus: { + // _disableInitialFocus: { + // type: Boolean, + // }, + + /** + * True if the initial focus is already internally applied. + * @private + */ + _initialFocusApplied: { type: Boolean, }, @@ -325,9 +333,7 @@ class Popup extends UI5Element { this._focusedElementBeforeOpen = getFocusedElement(); this.show(); - if (!this._disableInitialFocus && !preventInitialFocus) { - this.applyInitialFocus(); - } + // this._preventInitialFocus = preventInitialFocus; this._addOpenedPopup(); @@ -335,6 +341,17 @@ class Popup extends UI5Element { this.fireEvent("after-open", {}, false, false); } + onAfterRendering() { + if (this.opened && + // !this._disableInitialFocus && + // !this._preventInitialFocus && + !this._initialFocusApplied) { + + this.applyInitialFocus(); + this._initialFocusApplied = true; + } + } + /** * Adds the popup to the "opened popups registry" * @protected @@ -373,6 +390,8 @@ class Popup extends UI5Element { this.resetFocus(); } + this._initialFocusApplied = false; + this.fireEvent("after-close", {}, false, false); } From fcdb89cc5e301217617e3c1e70c72cd820671fe4 Mon Sep 17 00:00:00 2001 From: Alexandar Mitsev Date: Mon, 7 Dec 2020 18:30:16 +0200 Subject: [PATCH 2/8] fix(ui5-dialog): apply initial focus after rendering Now the initial focus is applied also in cases when the dialog is opened before it is fully rendered. Fixes: #2537 --- packages/base/src/util/FocusableElements.js | 5 +++ packages/main/src/Popup.js | 38 +++++++++++++++------ packages/main/test/pages/Dialog.html | 16 +++++++++ 3 files changed, 49 insertions(+), 10 deletions(-) diff --git a/packages/base/src/util/FocusableElements.js b/packages/base/src/util/FocusableElements.js index c335a31553f4..6df5a1d7d046 100644 --- a/packages/base/src/util/FocusableElements.js +++ b/packages/base/src/util/FocusableElements.js @@ -38,6 +38,11 @@ const findFocusableElement = (container, forward) => { while (child) { const originalChild = child; + // @todo discuss + if (child.isUI5Element) { + return child; + } + child = child.isUI5Element ? child.getFocusDomRef() : child; if (!child) { return null; diff --git a/packages/main/src/Popup.js b/packages/main/src/Popup.js index 93020fd75081..bb309b522f0f 100644 --- a/packages/main/src/Popup.js +++ b/packages/main/src/Popup.js @@ -77,7 +77,7 @@ const metadata = { }, /** - * @private + * @protected */ // _disableInitialFocus: { // type: Boolean, @@ -91,6 +91,14 @@ const metadata = { type: Boolean, }, + /** + * True if the initial focus is already internally applied. + * @private + */ + _focusAppliedAfterOpen: { + type: Boolean, + }, + _blockLayerHidden: { type: Boolean, }, @@ -297,6 +305,23 @@ class Popup extends UI5Element { } } + /** + * @private + */ + _applyFocusAfterOpen() { + const shouldApply = !(this._focusAppliedAfterOpen || this._disableInitialFocus /* || this._preventInitialFocus */); + + if (this.isOpen() && shouldApply) { + + // @todo discuss + // setTimeout(() => { + this.applyInitialFocus(); + // }, 0); + + this._focusAppliedAfterOpen = true; + } + } + /** * Override this method to provide custom logic for the popup's open/closed state. Maps to the "opened" property by default. * @public @@ -342,14 +367,7 @@ class Popup extends UI5Element { } onAfterRendering() { - if (this.opened && - // !this._disableInitialFocus && - // !this._preventInitialFocus && - !this._initialFocusApplied) { - - this.applyInitialFocus(); - this._initialFocusApplied = true; - } + this._applyFocusAfterOpen(); } /** @@ -390,7 +408,7 @@ class Popup extends UI5Element { this.resetFocus(); } - this._initialFocusApplied = false; + this._focusAppliedAfterOpen = false; this.fireEvent("after-close", {}, false, false); } diff --git a/packages/main/test/pages/Dialog.html b/packages/main/test/pages/Dialog.html index 32f1c118cca5..6011b8a6fcfc 100644 --- a/packages/main/test/pages/Dialog.html +++ b/packages/main/test/pages/Dialog.html @@ -50,6 +50,9 @@

Open resizable dialog +
+
+ Open dialog which is created dynamically @@ -330,6 +333,19 @@ window["draggable-close"].addEventListener("click", function () { window["draggable-dialog"].close(); }); window["resizable-open"].addEventListener("click", function () { window["resizable-dialog"].open(); }); window["resizable-close"].addEventListener("click", function () { window["resizable-dialog"].close(); }); + + + window["dynamic-open"].addEventListener("click", function () { + let dialog = document.createElement("ui5-dialog"), + button = document.createElement("ui5-button"); + + button.appendChild(document.createTextNode("Ok")); + dialog.appendChild(button); + + document.body.appendChild(dialog); + + dialog.open(); + }); From f5e9a1ed049024dc6ad7870ddf52e4fbc0afc63a Mon Sep 17 00:00:00 2001 From: Alexandar Mitsev Date: Wed, 9 Dec 2020 19:54:28 +0200 Subject: [PATCH 3/8] await for focus dom ref --- packages/base/src/UI5Element.js | 10 ++++ packages/base/src/util/FocusableElements.js | 19 +++---- packages/main/src/Popup.js | 63 +++++---------------- packages/main/test/pages/Dialog.html | 3 + packages/main/test/specs/Dialog.spec.js | 16 ++++++ 5 files changed, 52 insertions(+), 59 deletions(-) diff --git a/packages/base/src/UI5Element.js b/packages/base/src/UI5Element.js index ef51a6cd1f21..9fb75c58e4e1 100644 --- a/packages/base/src/UI5Element.js +++ b/packages/base/src/UI5Element.js @@ -638,6 +638,16 @@ class UI5Element extends HTMLElement { } } + /** + * Waits for dom ref and then returns the DOM Element marked with "data-sap-focus-ref" inside the template. + * This is the element that will receive the focus by default. + * @public + */ + async getFocusDomRefAsync() { + await this._waitForDomRef(); + return this.getFocusDomRef(); + } + /** * Use this method in order to get a reference to an element in the shadow root of the web component or the static area item of the component * @public diff --git a/packages/base/src/util/FocusableElements.js b/packages/base/src/util/FocusableElements.js index 6df5a1d7d046..beb19b3d2e69 100644 --- a/packages/base/src/util/FocusableElements.js +++ b/packages/base/src/util/FocusableElements.js @@ -5,7 +5,7 @@ const isFocusTrap = el => { return el.hasAttribute("data-ui5-focus-trap"); }; -const getFirstFocusableElement = container => { +const getFirstFocusableElement = async container => { if (!container || isNodeHidden(container)) { return null; } @@ -13,7 +13,7 @@ const getFirstFocusableElement = container => { return findFocusableElement(container, true); }; -const getLastFocusableElement = container => { +const getLastFocusableElement = async container => { if (!container || isNodeHidden(container)) { return null; } @@ -21,7 +21,7 @@ const getLastFocusableElement = container => { return findFocusableElement(container, false); }; -const findFocusableElement = (container, forward) => { +const findFocusableElement = async (container, forward) => { let child; if (container.shadowRoot) { @@ -35,15 +35,12 @@ const findFocusableElement = (container, forward) => { let focusableDescendant; + /* eslint-disable no-await-in-loop */ + while (child) { const originalChild = child; - // @todo discuss - if (child.isUI5Element) { - return child; - } - - child = child.isUI5Element ? child.getFocusDomRef() : child; + child = child.isUI5Element ? await child.getFocusDomRefAsync() : child; if (!child) { return null; } @@ -53,7 +50,7 @@ const findFocusableElement = (container, forward) => { return (child && typeof child.focus === "function") ? child : null; } - focusableDescendant = findFocusableElement(child, forward); + focusableDescendant = await findFocusableElement(child, forward); if (focusableDescendant) { return (focusableDescendant && typeof focusableDescendant.focus === "function") ? focusableDescendant : null; } @@ -62,6 +59,8 @@ const findFocusableElement = (container, forward) => { child = forward ? originalChild.nextSibling : originalChild.previousSibling; } + /* eslint-enable no-await-in-loop */ + return null; }; diff --git a/packages/main/src/Popup.js b/packages/main/src/Popup.js index bb309b522f0f..aefec81fce8f 100644 --- a/packages/main/src/Popup.js +++ b/packages/main/src/Popup.js @@ -79,23 +79,7 @@ const metadata = { /** * @protected */ - // _disableInitialFocus: { - // type: Boolean, - // }, - - /** - * True if the initial focus is already internally applied. - * @private - */ - _initialFocusApplied: { - type: Boolean, - }, - - /** - * True if the initial focus is already internally applied. - * @private - */ - _focusAppliedAfterOpen: { + _disableInitialFocus: { type: Boolean, }, @@ -262,8 +246,8 @@ class Popup extends UI5Element { * Focus trapping * @private */ - forwardToFirst() { - const firstFocusable = getFirstFocusableElement(this); + async forwardToFirst() { + const firstFocusable = await getFirstFocusableElement(this); if (firstFocusable) { firstFocusable.focus(); @@ -274,8 +258,8 @@ class Popup extends UI5Element { * Focus trapping * @private */ - forwardToLast() { - const lastFocusable = getLastFocusableElement(this); + async forwardToLast() { + const lastFocusable = await getLastFocusableElement(this); if (lastFocusable) { lastFocusable.focus(); @@ -286,8 +270,8 @@ class Popup extends UI5Element { * Use this method to focus the element denoted by "initialFocus", if provided, or the first focusable element otherwise. * @protected */ - applyInitialFocus() { - this.applyFocus(); + async applyInitialFocus() { + await this.applyFocus(); } /** @@ -295,33 +279,18 @@ class Popup extends UI5Element { * or the first focusable element otherwise. * @public */ - applyFocus() { + async applyFocus() { + await this._waitForDomRef(); + const element = this.getRootNode().getElementById(this.initialFocus) || document.getElementById(this.initialFocus) - || getFirstFocusableElement(this); + || await getFirstFocusableElement(this); if (element) { element.focus(); } } - /** - * @private - */ - _applyFocusAfterOpen() { - const shouldApply = !(this._focusAppliedAfterOpen || this._disableInitialFocus /* || this._preventInitialFocus */); - - if (this.isOpen() && shouldApply) { - - // @todo discuss - // setTimeout(() => { - this.applyInitialFocus(); - // }, 0); - - this._focusAppliedAfterOpen = true; - } - } - /** * Override this method to provide custom logic for the popup's open/closed state. Maps to the "opened" property by default. * @public @@ -358,7 +327,9 @@ class Popup extends UI5Element { this._focusedElementBeforeOpen = getFocusedElement(); this.show(); - // this._preventInitialFocus = preventInitialFocus; + if (!this._disableInitialFocus && !preventInitialFocus) { + this.applyInitialFocus(); + } this._addOpenedPopup(); @@ -366,10 +337,6 @@ class Popup extends UI5Element { this.fireEvent("after-open", {}, false, false); } - onAfterRendering() { - this._applyFocusAfterOpen(); - } - /** * Adds the popup to the "opened popups registry" * @protected @@ -408,8 +375,6 @@ class Popup extends UI5Element { this.resetFocus(); } - this._focusAppliedAfterOpen = false; - this.fireEvent("after-close", {}, false, false); } diff --git a/packages/main/test/pages/Dialog.html b/packages/main/test/pages/Dialog.html index 6011b8a6fcfc..b8b99114fe46 100644 --- a/packages/main/test/pages/Dialog.html +++ b/packages/main/test/pages/Dialog.html @@ -339,7 +339,10 @@ let dialog = document.createElement("ui5-dialog"), button = document.createElement("ui5-button"); + button.setAttribute("id", "dynamic-dialog-button"); button.appendChild(document.createTextNode("Ok")); + + dialog.setAttribute("id", "dynamic-dialog"); dialog.appendChild(button); document.body.appendChild(dialog); diff --git a/packages/main/test/specs/Dialog.spec.js b/packages/main/test/specs/Dialog.spec.js index 7806d2706bd0..3b8770177170 100644 --- a/packages/main/test/specs/Dialog.spec.js +++ b/packages/main/test/specs/Dialog.spec.js @@ -113,6 +113,22 @@ describe("Dialog general interaction", () => { closeResizableDialogButton.click(); }); + + it("initial focus after dynamic dialog creation", () => { + const openDynamicDialog = browser.$("#dynamic-open"); + openDynamicDialog.click(); + + const dialog = browser.$("#dynamic-dialog"); + const button = dialog.shadow$("#dynamic-dialog-button"); + + dialog.open(); + + button.waitForExist(); + + assert.strictEqual(document.activeElement, button, "the active element is the first button"); + + dialog.close(); + }); }); From bebb075492de74743a9758f5b99466d19cc150ba Mon Sep 17 00:00:00 2001 From: Alexandar Mitsev Date: Thu, 10 Dec 2020 10:30:10 +0200 Subject: [PATCH 4/8] fixes in unit test --- packages/base/src/util/FocusableElements.js | 5 ++++- packages/main/test/specs/Dialog.spec.js | 7 ++++--- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/packages/base/src/util/FocusableElements.js b/packages/base/src/util/FocusableElements.js index beb19b3d2e69..d07c35a26eec 100644 --- a/packages/base/src/util/FocusableElements.js +++ b/packages/base/src/util/FocusableElements.js @@ -40,7 +40,10 @@ const findFocusableElement = async (container, forward) => { while (child) { const originalChild = child; - child = child.isUI5Element ? await child.getFocusDomRefAsync() : child; + if (child.isUI5Element) { + child = await child.getFocusDomRefAsync(); + } + if (!child) { return null; } diff --git a/packages/main/test/specs/Dialog.spec.js b/packages/main/test/specs/Dialog.spec.js index 3b8770177170..bb2e4c5daed1 100644 --- a/packages/main/test/specs/Dialog.spec.js +++ b/packages/main/test/specs/Dialog.spec.js @@ -119,12 +119,13 @@ describe("Dialog general interaction", () => { openDynamicDialog.click(); const dialog = browser.$("#dynamic-dialog"); - const button = dialog.shadow$("#dynamic-dialog-button"); - - dialog.open(); + dialog.waitForExist(); + const button = dialog.shadow$("#dynamic-dialog-button"); button.waitForExist(); + browser.pause(500); + assert.strictEqual(document.activeElement, button, "the active element is the first button"); dialog.close(); From af4ad3d38003a6403b00d8ae02558a01a03411e0 Mon Sep 17 00:00:00 2001 From: Alexandar Mitsev Date: Thu, 10 Dec 2020 12:11:33 +0200 Subject: [PATCH 5/8] fix in the test --- packages/main/test/pages/Dialog.html | 21 ++++++++++++--------- packages/main/test/specs/Dialog.spec.js | 19 ++++++++----------- 2 files changed, 20 insertions(+), 20 deletions(-) diff --git a/packages/main/test/pages/Dialog.html b/packages/main/test/pages/Dialog.html index b8b99114fe46..38d64bf3073c 100644 --- a/packages/main/test/pages/Dialog.html +++ b/packages/main/test/pages/Dialog.html @@ -336,19 +336,22 @@ window["dynamic-open"].addEventListener("click", function () { - let dialog = document.createElement("ui5-dialog"), - button = document.createElement("ui5-button"); + let dialog = document.createElement("ui5-dialog"), + button = document.createElement("ui5-button"); - button.setAttribute("id", "dynamic-dialog-button"); - button.appendChild(document.createTextNode("Ok")); + button.setAttribute("id", "dynamic-dialog-close-button"); + button.appendChild(document.createTextNode("Close")); + button.addEventListener("click", function () { + dialog.close(); + }); - dialog.setAttribute("id", "dynamic-dialog"); - dialog.appendChild(button); + dialog.setAttribute("id", "dynamic-dialog"); + dialog.appendChild(button); - document.body.appendChild(dialog); + document.body.appendChild(dialog); - dialog.open(); - }); + dialog.open(); + }); diff --git a/packages/main/test/specs/Dialog.spec.js b/packages/main/test/specs/Dialog.spec.js index bb2e4c5daed1..8bc7858b4c4f 100644 --- a/packages/main/test/specs/Dialog.spec.js +++ b/packages/main/test/specs/Dialog.spec.js @@ -114,22 +114,19 @@ describe("Dialog general interaction", () => { closeResizableDialogButton.click(); }); - it("initial focus after dynamic dialog creation", () => { - const openDynamicDialog = browser.$("#dynamic-open"); + it("initial focus after dynamic dialog creation", () => {​​ + const openDynamicDialog = browser.$("#dynamic-open"); openDynamicDialog.click(); - const dialog = browser.$("#dynamic-dialog"); - dialog.waitForExist(); + const closeButton = browser.$("#dynamic-dialog-close-button"); - const button = dialog.shadow$("#dynamic-dialog-button"); - button.waitForExist(); + browser.pause(500); - browser.pause(500); + const activeElement = $(browser.getActiveElement()); + assert.strictEqual(activeElement.getProperty("id"), closeButton.getProperty("id"), "the active element is the close button"); - assert.strictEqual(document.activeElement, button, "the active element is the first button"); - - dialog.close(); - }); + closeButton.click(); + }​​); }); From ea17476d7452313db4384d1e2f553675a9b2c58d Mon Sep 17 00:00:00 2001 From: Alexandar Mitsev Date: Thu, 10 Dec 2020 14:34:54 +0200 Subject: [PATCH 6/8] fix tabs in test and sample --- packages/main/test/pages/Dialog.html | 25 ++++++++++++------------- packages/main/test/specs/Dialog.spec.js | 14 +++++++------- 2 files changed, 19 insertions(+), 20 deletions(-) diff --git a/packages/main/test/pages/Dialog.html b/packages/main/test/pages/Dialog.html index 38d64bf3073c..c2cb24bec0f8 100644 --- a/packages/main/test/pages/Dialog.html +++ b/packages/main/test/pages/Dialog.html @@ -334,24 +334,23 @@ window["resizable-open"].addEventListener("click", function () { window["resizable-dialog"].open(); }); window["resizable-close"].addEventListener("click", function () { window["resizable-dialog"].close(); }); - window["dynamic-open"].addEventListener("click", function () { - let dialog = document.createElement("ui5-dialog"), - button = document.createElement("ui5-button"); + var dialog = document.createElement("ui5-dialog"), + button = document.createElement("ui5-button"); - button.setAttribute("id", "dynamic-dialog-close-button"); - button.appendChild(document.createTextNode("Close")); - button.addEventListener("click", function () { - dialog.close(); - }); + button.setAttribute("id", "dynamic-dialog-close-button"); + button.appendChild(document.createTextNode("Close")); + button.addEventListener("click", function () { + dialog.close(); + }); - dialog.setAttribute("id", "dynamic-dialog"); - dialog.appendChild(button); + dialog.setAttribute("id", "dynamic-dialog"); + dialog.appendChild(button); - document.body.appendChild(dialog); + document.body.appendChild(dialog); - dialog.open(); - }); + dialog.open(); + }); diff --git a/packages/main/test/specs/Dialog.spec.js b/packages/main/test/specs/Dialog.spec.js index 8bc7858b4c4f..bde7f033ee16 100644 --- a/packages/main/test/specs/Dialog.spec.js +++ b/packages/main/test/specs/Dialog.spec.js @@ -114,19 +114,19 @@ describe("Dialog general interaction", () => { closeResizableDialogButton.click(); }); - it("initial focus after dynamic dialog creation", () => {​​ - const openDynamicDialog = browser.$("#dynamic-open"); + it("initial focus after dynamic dialog creation", () => { + const openDynamicDialog = browser.$("#dynamic-open"); openDynamicDialog.click(); - const closeButton = browser.$("#dynamic-dialog-close-button"); + const closeButton = browser.$("#dynamic-dialog-close-button"); - browser.pause(500); + browser.pause(500); - const activeElement = $(browser.getActiveElement()); + const activeElement = $(browser.getActiveElement()); assert.strictEqual(activeElement.getProperty("id"), closeButton.getProperty("id"), "the active element is the close button"); - closeButton.click(); - }​​); + closeButton.click(); + }​​); }); From 3880357eef2e76132cbed574c908ffb7d7591929 Mon Sep 17 00:00:00 2001 From: Alexandar Mitsev Date: Thu, 10 Dec 2020 16:36:31 +0200 Subject: [PATCH 7/8] replace badly endcoded bracket symbol --- packages/main/test/specs/Dialog.spec.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/main/test/specs/Dialog.spec.js b/packages/main/test/specs/Dialog.spec.js index bde7f033ee16..6f6c15b35405 100644 --- a/packages/main/test/specs/Dialog.spec.js +++ b/packages/main/test/specs/Dialog.spec.js @@ -126,7 +126,7 @@ describe("Dialog general interaction", () => { assert.strictEqual(activeElement.getProperty("id"), closeButton.getProperty("id"), "the active element is the close button"); closeButton.click(); - }​​); + }); }); From 17932b420345ceaf5d378ce0a87ca54b3af70fb3 Mon Sep 17 00:00:00 2001 From: Alexandar Mitsev Date: Fri, 11 Dec 2020 13:32:01 +0200 Subject: [PATCH 8/8] return private visibility for _disableInitialFocus --- 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 aefec81fce8f..5931c30119df 100644 --- a/packages/main/src/Popup.js +++ b/packages/main/src/Popup.js @@ -77,7 +77,7 @@ const metadata = { }, /** - * @protected + * @private */ _disableInitialFocus: { type: Boolean,