fix(ui5-dialog): correctly restore body scrolling on ESC (#3696)

dimovpetar · ilhan007 · commit 983af6aa38c3 · 2021-09-10T17:00:22.000+03:00
Body scrolling is now blocked only on first call of blockBodyScrolling and restored on the last call of unblockBodyScrolling. Fixes #3690
diff --git a/packages/main/src/Popup.js b/packages/main/src/Popup.js
@@ -9,7 +9,7 @@ import { isTabPrevious } from "@ui5/webcomponents-base/dist/Keys.js";
 import { getNextZIndex, getFocusedElement, isFocusedElementWithinNode } from "@ui5/webcomponents-base/dist/util/PopupUtils.js";
 import PopupTemplate from "./generated/templates/PopupTemplate.lit.js";
 import PopupBlockLayer from "./generated/templates/PopupBlockLayerTemplate.lit.js";
-import { getOpenedPopups, addOpenedPopup, removeOpenedPopup } from "./popup-utils/OpenedPopupsRegistry.js";
+import { addOpenedPopup, removeOpenedPopup } from "./popup-utils/OpenedPopupsRegistry.js";
 
 // Styles
 import styles from "./generated/themes/Popup.css.js";
@@ -150,6 +150,8 @@ const createBlockingStyle = () => {
 
 createBlockingStyle();
 
+let bodyScrollingBlockers = 0;
+
 /**
  * @class
  * <h3 class="comment-api-title">Overview</h3>
@@ -237,6 +239,12 @@ class Popup extends UI5Element {
 	 * @protected
 	 */
 	static blockBodyScrolling() {
+		bodyScrollingBlockers++;
+
+		if (bodyScrollingBlockers !== 1) {
+			return;
+		}
+
 		if (window.pageYOffset > 0) {
 			document.body.style.top = `-${window.pageYOffset}px`;
 		}
@@ -248,6 +256,12 @@ class Popup extends UI5Element {
 	 * @protected
 	 */
 	static unblockBodyScrolling() {
+		bodyScrollingBlockers--;
+
+		if (bodyScrollingBlockers !== 0) {
+			return;
+		}
+
 		document.body.classList.remove("ui5-popup-scroll-blocker");
 		window.scrollTo(0, -parseFloat(document.body.style.top));
 		document.body.style.top = "";
@@ -422,12 +436,9 @@ class Popup extends UI5Element {
 			return;
 		}
 
-		const openedPopups = getOpenedPopups();
 		if (this.isModal) {
 			this._blockLayerHidden = true;
-			if (openedPopups.length === 1) {
-				Popup.unblockBodyScrolling();
-			}
+			Popup.unblockBodyScrolling();
 		}
 
 		this.hide();
diff --git a/packages/main/src/popup-utils/OpenedPopupsRegistry.js b/packages/main/src/popup-utils/OpenedPopupsRegistry.js
@@ -35,7 +35,7 @@ const _keydownListener = event => {
 	}
 
 	if (isEscape(event)) {
-		openedRegistry.pop().instance.close(true);
+		openedRegistry[openedRegistry.length - 1].instance.close(true);
 	}
 };
 
diff --git a/packages/main/test/pages/Dialog.html b/packages/main/test/pages/Dialog.html
@@ -26,6 +26,7 @@
 
 <body>
 	<ui5-checkbox text="Compact size" id="cbCompact"></ui5-checkbox>
+	<ui5-checkbox text="Scrollable page" id="cbScrollable"></ui5-checkbox>
 	<br>
 	<ui5-button id="btnOpenDialog">Open Streched Dialog</ui5-button>
 	<br>
@@ -309,7 +310,7 @@
 		<br>
 		<br>
 
-		<ui5-button id='popbtn'>Open Popover</ui5-button>
+		<ui5-button id="dangerDialogBtn">Open Dialog</ui5-button>
 
 
 
@@ -356,11 +357,17 @@
 
 	<ui5-dialog id="empty-dialog">Empty</ui5-dialog>
 
+	<span id="scrollHelper" style="display: none; margin-top: 1000px;">SCroll Helper</span>
+
 	<script>
 		cbCompact.addEventListener("change", function () {
 			document.body.classList.toggle("ui5-content-density-compact", cbCompact.checked);
 		});
 
+		cbScrollable.addEventListener("change", function () {
+			scrollHelper.style.display = cbScrollable.checked ? "block" : "none";
+		});
+
 		let preventClosing = true;
 
 		btnOpenDialog.addEventListener("click", function () {
diff --git a/packages/main/test/specs/Dialog.spec.js b/packages/main/test/specs/Dialog.spec.js
@@ -167,29 +167,53 @@ describe("Acc", () => {
 	});
 });
 
-describe("Multiple dialogs page scroll", () => {
-		before(() => {
-			browser.url(`http://localhost:${PORT}/test-resources/pages/Dialog.html`);
-		});
+describe("Page scrolling", () => {
+	before(() => {
+		browser.url(`http://localhost:${PORT}/test-resources/pages/Dialog.html`);
+	});
 
-		it("tests multiple dialogs page scrolling", () => {
-			const preventButtonBefore = browser.$("#prevent");
+	it("tests that page scrolling is blocked and restored", () => {
+		browser.$("#cbScrollable").click();
+		const offsetHeightBefore = browser.$("body").getProperty("offsetHeight");
 
-			browser.setWindowSize(400, 400);
-			preventButtonBefore.scrollIntoView();
+		browser.$("#btnOpenDialog").click();
 
-			const offsetBefore = preventButtonBefore.getLocation('y');
+		assert.ok(browser.$("body").getProperty("offsetHeight") < offsetHeightBefore, "Body scrolling is blocked");
 
-			preventButtonBefore.click();
+		browser.$("#btnCloseDialog").click();
 
-			browser.keys("Escape");
-			const confirmButton = browser.$("#yes");
-			confirmButton.click();
+		assert.strictEqual(browser.$("body").getProperty("offsetHeight"), offsetHeightBefore, "Body scrolling is restored");
+		browser.$("#cbScrollable").click();
+	});
 
-			browser.setTimeout({ script: 5000 });
-    		const offsetAfter = preventButtonBefore.getLocation('y');
+	it("test page scrolling is restored after close with ESC", () => {
+		browser.$("#cbScrollable").click();
+		const offsetHeightBefore = browser.$("body").getProperty("offsetHeight");
 
-			assert.strictEqual(offsetBefore,  offsetAfter, "No vertical page scrolling when multiple dialogs are closed");
-		});
+		browser.$("#btnOpenDialog").click();
+		browser.keys("Escape");
+		assert.strictEqual(browser.$("body").getProperty("offsetHeight"), offsetHeightBefore, "Body scrolling is restored");
+
+		browser.$("#cbScrollable").click();
+	});
+
+	it("tests multiple dialogs page scrolling", () => {
+		const preventButtonBefore = browser.$("#prevent");
 
+		browser.setWindowSize(400, 400);
+		preventButtonBefore.scrollIntoView();
+
+		const offsetBefore = preventButtonBefore.getLocation('y');
+
+		preventButtonBefore.click();
+
+		browser.keys("Escape");
+		const confirmButton = browser.$("#yes");
+		confirmButton.click();
+
+		browser.setTimeout({ script: 5000 });
+		const offsetAfter = preventButtonBefore.getLocation('y');
+
+		assert.strictEqual(offsetBefore,  offsetAfter, "No vertical page scrolling when multiple dialogs are closed");
+	});
 });

Original file line number	Diff line number	Diff line change
`@@ -35,7 +35,7 @@ const _keydownListener = event => {`
`35`	`35`	`}`
`36`	`36`
`37`	`37`	`if (isEscape(event)) {`
`38`		`- openedRegistry.pop().instance.close(true);`
	`38`	`+ openedRegistry[openedRegistry.length - 1].instance.close(true);`
`39`	`39`	`}`
`40`	`40`	`};`
`41`	`41`