Skip to content

Commit 1754496

Browse files
dimovpetardimovpetar
authored
fix(ui5-popup): body styles are no longer modified (#4813)
Popups add styles to the body element to prevent scrolling, which can disrupt the layout of the page. Now the scrolling is prevented by adding overflow: hidden style to the html element. Fixes #4347
1 parent 71946fc commit 1754496

File tree

5 files changed

+64
-41
lines changed

5 files changed

+64
-41
lines changed

‎packages/main/src/Popup.js

Lines changed: 15 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -188,7 +188,7 @@ const createBlockingStyle = () => {
188188

189189
createBlockingStyle();
190190

191-
const bodyScrollingBlockers = new Set();
191+
const pageScrollingBlockers = new Set();
192192

193193
/**
194194
* @class
@@ -200,7 +200,7 @@ const bodyScrollingBlockers = new Set();
200200
*
201201
* 1. The Popup class handles modality:
202202
* - The "isModal" getter can be overridden by derivatives to provide their own conditions when they are modal or not
203-
* - Derivatives may call the "blockBodyScrolling" and "unblockBodyScrolling" static methods to temporarily remove scrollbars on the body
203+
* - Derivatives may call the "blockPageScrolling" and "unblockPageScrolling" static methods to temporarily remove scrollbars on the html element
204204
* - Derivatives may call the "open" and "close" methods which handle focus, manage the popup registry and for modal popups, manage the blocking layer
205205
*
206206
* 2. Provides blocking layer (relevant for modal popups only):
@@ -264,7 +264,7 @@ class Popup extends UI5Element {
264264

265265
onExitDOM() {
266266
if (this.isOpen()) {
267-
Popup.unblockBodyScrolling(this);
267+
Popup.unblockPageScrolling(this);
268268
this._removeOpenedPopup();
269269
}
270270

@@ -287,36 +287,31 @@ class Popup extends UI5Element {
287287
}
288288

289289
/**
290-
* Temporarily removes scrollbars from the body
290+
* Temporarily removes scrollbars from the html element
291291
* @protected
292292
*/
293-
static blockBodyScrolling(popup) {
294-
bodyScrollingBlockers.add(popup);
293+
static blockPageScrolling(popup) {
294+
pageScrollingBlockers.add(popup);
295295

296-
if (bodyScrollingBlockers.size !== 1) {
296+
if (pageScrollingBlockers.size !== 1) {
297297
return;
298298
}
299299

300-
if (window.pageYOffset > 0) {
301-
document.body.style.top = `-${window.pageYOffset}px`;
302-
}
303-
document.body.classList.add("ui5-popup-scroll-blocker");
300+
document.documentElement.classList.add("ui5-popup-scroll-blocker");
304301
}
305302

306303
/**
307-
* Restores scrollbars on the body, if needed
304+
* Restores scrollbars on the html element, if needed
308305
* @protected
309306
*/
310-
static unblockBodyScrolling(popup) {
311-
bodyScrollingBlockers.delete(popup);
307+
static unblockPageScrolling(popup) {
308+
pageScrollingBlockers.delete(popup);
312309

313-
if (bodyScrollingBlockers.size !== 0) {
310+
if (pageScrollingBlockers.size !== 0) {
314311
return;
315312
}
316313

317-
document.body.classList.remove("ui5-popup-scroll-blocker");
318-
window.scrollTo(0, -parseFloat(document.body.style.top));
319-
document.body.style.top = "";
314+
document.documentElement.classList.remove("ui5-popup-scroll-blocker");
320315
}
321316

322317
_scroll(e) {
@@ -446,7 +441,7 @@ class Popup extends UI5Element {
446441
// create static area item ref for block layer
447442
this.getStaticAreaItemDomRef();
448443
this._blockLayerHidden = false;
449-
Popup.blockBodyScrolling(this);
444+
Popup.blockPageScrolling(this);
450445
}
451446

452447
this._zIndex = getNextZIndex();
@@ -492,7 +487,7 @@ class Popup extends UI5Element {
492487

493488
if (this.isModal) {
494489
this._blockLayerHidden = true;
495-
Popup.unblockBodyScrolling(this);
490+
Popup.unblockPageScrolling(this);
496491
}
497492

498493
this.hide();

‎packages/main/src/themes/PopupGlobal.css

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,3 @@
11
.ui5-popup-scroll-blocker {
2-
width: 100%;
3-
height: 100%;
4-
position: fixed;
5-
overflow: hidden;
2+
overflow: hidden;
63
}

‎packages/main/test/pages/Dialog.html

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -457,14 +457,18 @@
457457
<span>Hello World!</span>
458458
</ui5-dialog>
459459

460-
<span id="scrollHelper" class="dialog9auto">SCroll Helper</span>
460+
<div id="scrollHelper" class="dialog9scrollHelper">
461+
<span>Scroll Helper</span>
462+
<ui5-button id="scrolledBtn" class="scrolledBtn">open</ui5-button>
463+
</div>
461464

462465
<script>
463-
cbCompact.addEventListener("change", function () {
466+
cbCompact.addEventListener("ui5-change", function () {
464467
document.body.classList.toggle("ui5-content-density-compact", cbCompact.checked);
465468
});
466469

467-
cbScrollable.addEventListener("change", function () {
470+
cbScrollable.addEventListener("ui5-change", function () {
471+
console.error("change")
468472
scrollHelper.style.display = cbScrollable.checked ? "block" : "none";
469473
});
470474

@@ -582,6 +586,10 @@
582586
window["dialog-acc-name-ref"].show();
583587
});
584588

589+
window["scrolledBtn"].addEventListener("click", function () {
590+
window["dialog"].show();
591+
});
592+
585593
</script>
586594
</body>
587595

‎packages/main/test/pages/styles/Dialog.css

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,9 +38,15 @@
3838
width: 300px
3939
}
4040

41-
.dialog9auto {
41+
.dialog9scrollHelper {
4242
display: none;
4343
margin-top: 1000px;
44+
width: 3000px;
45+
}
46+
47+
.dialog9scrollHelper .scrolledBtn {
48+
margin-left: 1500px;
49+
margin-bottom: 200px;
4450
}
4551

4652
/* Remove the responsive paddings for the content area of the Dialog */

‎packages/main/test/specs/Dialog.spec.js

Lines changed: 30 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -334,40 +334,57 @@ describe("Page scrolling", () => {
334334
});
335335

336336
it("tests that page scrolling is blocked and restored", async () => {
337-
await browser.$("#cbScrollable").click();
338-
const offsetHeightBefore = await browser.$("body").getProperty("offsetHeight");
339-
340337
await browser.$("#btnOpenDialog").click();
338+
let pageOverflow = await browser.execute("return window.getComputedStyle(document.documentElement).overflow;");
341339

342-
assert.isBelow(await browser.$("body").getProperty("offsetHeight"), offsetHeightBefore, "Body scrolling is blocked");
340+
assert.strictEqual(pageOverflow, "hidden", "Page scrolling is blocked");
343341

344342
await browser.$("#btnCloseDialog").click();
343+
pageOverflow = await browser.execute("return window.getComputedStyle(document.documentElement).overflow;");
345344

346-
assert.strictEqual(await browser.$("body").getProperty("offsetHeight"), offsetHeightBefore, "Body scrolling is restored");
347-
await browser.$("#cbScrollable").click();
345+
assert.strictEqual(pageOverflow, "visible", "Page scrolling is restored");
348346
});
349347

350-
it("tests that page scrolling is blocked and restored after multiple show() of same dialog", async () => {
348+
it("tests that page scrolling position is preserved", async () => {
349+
// scroll position might change slightly when the scrollbars hide and then appear again
350+
const SCROLLBAR_DELTA = 20;
351351
await browser.$("#cbScrollable").click();
352-
const offsetHeightBefore = await browser.$("body").getProperty("offsetHeight");
352+
const scrolledButton = await $("#scrolledBtn");
353+
await scrolledButton.scrollIntoView();
354+
const scrollLeftBefore = await browser.$("html").getProperty("scrollLeft");
355+
const scrollTopBefore = await browser.$("html").getProperty("scrollTop");
356+
await scrolledButton.click();
357+
358+
assert.strictEqual(await browser.$("html").getProperty("scrollLeft"), scrollLeftBefore, "Horizontal page scroll position is preserved");
359+
assert.approximately(await browser.$("html").getProperty("scrollTop"), scrollTopBefore, SCROLLBAR_DELTA, "Vertical page scroll position is preserved");
360+
361+
await browser.keys("Escape");
353362

363+
assert.strictEqual(await browser.$("html").getProperty("scrollLeft"), scrollLeftBefore, "Horizontal page scroll position is preserved");
364+
assert.approximately(await browser.$("html").getProperty("scrollTop"), scrollTopBefore, SCROLLBAR_DELTA, "Vertical page scroll position is preserved");
365+
366+
await browser.$("#cbScrollable").click();
367+
});
368+
369+
it("tests that page scrolling is blocked and restored after multiple show() of same dialog", async () => {
354370
await browser.$("#multiple-show").click();
371+
let pageOverflow = await browser.execute("return window.getComputedStyle(document.documentElement).overflow;");
355372

356-
assert.isBelow(await browser.$("body").getProperty("offsetHeight"), offsetHeightBefore, "Body scrolling is blocked");
373+
assert.strictEqual(pageOverflow, "hidden", "Page scrolling is blocked");
357374

358375
await browser.$("#btnCloseDialog").click();
376+
pageOverflow = await browser.execute("return window.getComputedStyle(document.documentElement).overflow;");
359377

360-
assert.strictEqual(await browser.$("body").getProperty("offsetHeight"), offsetHeightBefore, "Body scrolling is restored");
361-
await browser.$("#cbScrollable").click();
378+
assert.strictEqual(pageOverflow, "visible", "Page scrolling is restored");
362379
});
363380

364381
it("test page scrolling is restored after close with ESC", async () => {
365382
await browser.$("#cbScrollable").click();
366-
const offsetHeightBefore = await browser.$("body").getProperty("offsetHeight");
383+
const scrollHeightBefore = await browser.$("html").getProperty("scrollHeight");
367384

368385
await browser.$("#btnOpenDialog").click();
369386
await browser.keys("Escape");
370-
assert.strictEqual(await browser.$("body").getProperty("offsetHeight"), offsetHeightBefore, "Body scrolling is restored");
387+
assert.strictEqual(await browser.$("html").getProperty("scrollHeight"), scrollHeightBefore, "Body scrolling is restored");
371388

372389
await browser.$("#cbScrollable").click();
373390
});

0 commit comments

Comments
 (0)