Skip to content

Commit

Permalink
fix(ui5-dialog): correctly restore body scrolling on ESC (#3696)
Browse files Browse the repository at this point in the history 
Body scrolling is now blocked only on first call of blockBodyScrolling and restored on the last call of unblockBodyScrolling.

Fixes #3690
  • Loading branch information
@dimovpetar
dimovpetar authored Aug 25, 2021
1 parent 2548935 commit 1c8e656
Show file tree
Hide file tree
Showing 4 changed files with 67 additions and 25 deletions.
21 changes: 16 additions & 5 deletions packages/main/src/Popup.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,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";
Expand Down Expand Up @@ -162,6 +162,8 @@ const createBlockingStyle = () => {

createBlockingStyle();

let bodyScrollingBlockers = 0;

/**
* @class
* <h3 class="comment-api-title">Overview</h3>
Expand Down Expand Up @@ -249,6 +251,12 @@ class Popup extends UI5Element {
* @protected
*/
static blockBodyScrolling() {
bodyScrollingBlockers++;

if (bodyScrollingBlockers !== 1) {
return;
}

if (window.pageYOffset > 0) {
document.body.style.top = `-${window.pageYOffset}px`;
}
Expand All @@ -260,6 +268,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 = "";
Expand Down Expand Up @@ -435,12 +449,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();
Expand Down
2 changes: 1 addition & 1 deletion packages/main/src/popup-utils/OpenedPopupsRegistry.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ const _keydownListener = event => {
}

if (isEscape(event)) {
openedRegistry.pop().instance.close(true);
openedRegistry[openedRegistry.length - 1].instance.close(true);
}
};

Expand Down
11 changes: 9 additions & 2 deletions packages/main/test/pages/Dialog.html
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down Expand Up @@ -322,7 +323,7 @@
<br>
<br>

<ui5-button id='popbtn'>Open Popover</ui5-button>
<ui5-button id="dangerDialogBtn">Open Dialog</ui5-button>



Expand Down Expand Up @@ -369,11 +370,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 () {
Expand Down Expand Up @@ -415,7 +422,7 @@

window["modals-open"].addEventListener("click", function (event) { pop.showAt(event.target) });

popbtn.addEventListener('click', function (event) {
dangerDialogBtn.addEventListener('click', function (event) {
danger.show();
});

Expand Down
58 changes: 41 additions & 17 deletions packages/main/test/specs/Dialog.spec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -300,29 +300,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");
});
});

0 comments on commit 1c8e656

Please sign in to comment.