fix(ui5-popover): correctly position a popover if dynamically created (#2679)

Now when a popover is dynamically created it is shown only after its size can be determined.
This prevents its position to flicker on opening.

Fixes: #1755

Author: alexandar-mitsev

packages/main/src/Popover.js

@@ -1,4 +1,5 @@
 import Integer from "@ui5/webcomponents-base/dist/types/Integer.js";
+import ResizeHandler from "@ui5/webcomponents-base/dist/delegate/ResizeHandler.js";
 import Popup from "./Popup.js";
 import PopoverPlacementType from "./types/PopoverPlacementType.js";
 import PopoverVerticalAlign from "./types/PopoverVerticalAlign.js";
@@ -250,6 +251,12 @@ const metadata = {
  * @public
  */
 class Popover extends Popup {
+	constructor() {
+		super();
+
+		this._handleResize = this.handleResize.bind(this);
+	}
+
 	static get metadata() {
 		return metadata;
 	}
@@ -266,6 +273,14 @@ class Popover extends Popup {
 		return 10; // px
 	}
 
+	onEnterDOM() {
+		ResizeHandler.register(this, this._handleResize);
+	}
+
+	onExitDOM() {
+		ResizeHandler.deregister(this, this._handleResize);
+	}
+
 	isOpenerClicked(event) {
 		const target = event.target;
 		return target === this._opener || (target.getFocusDomRef && target.getFocusDomRef() === this._opener) || event.composedPath().indexOf(this._opener) > -1;
@@ -277,14 +292,15 @@ class Popover extends Popup {
 	 * @param {boolean} preventInitialFocus prevents applying the focus inside the popover
 	 * @public
 	 */
-	openBy(opener, preventInitialFocus = false) {
+	async openBy(opener, preventInitialFocus = false) {
 		if (!opener || this.opened) {
 			return;
 		}
 
 		this._opener = opener;
+		this._openerRect = opener.getBoundingClientRect();
 
-		super.open(preventInitialFocus);
+		await super.open(preventInitialFocus);
 	}
 
 	/**
@@ -332,21 +348,36 @@ class Popover extends Popup {
 			&& openerRect.right === 0;
 	}
 
+	handleResize() {
+		if (this.opened) {
+			this.reposition();
+		}
+	}
+
 	reposition() {
 		this.show();
 	}
 
 	show() {
 		let placement;
-		const popoverSize = this.popoverSize;
-		const openerRect = this._opener.getBoundingClientRect();
+		const popoverSize = this.getPopoverSize();
+
+		if (popoverSize.width === 0 || popoverSize.height === 0) {
+			// size can not be determined properly at this point, popover will be shown with the next reposition
+			return;
+		}
 
-		if (this.shouldCloseDueToNoOpener(openerRect) && this.isFocusWithin()) {
+		if (this.isOpen()) {
+			// update opener rect if it was changed during the popover being opened
+			this._openerRect = this._opener.getBoundingClientRect();
+		}
+
+		if (this.shouldCloseDueToNoOpener(this._openerRect) && this.isFocusWithin()) {
 			// reuse the old placement as the opener is not available,
 			// but keep the popover open as the focus is within
 			placement = this._oldPlacement;
 		} else {
-			placement = this.calcPlacement(openerRect, popoverSize);
+			placement = this.calcPlacement(this._openerRect, popoverSize);
 		}
 
 		const stretching = this.horizontalAlign === PopoverHorizontalAlign.Stretch;
@@ -379,7 +410,7 @@ class Popover extends Popup {
 		}
 	}
 
-	get popoverSize() {
+	getPopoverSize() {
 		let width,
 			height;
 		let rect = this.getBoundingClientRect();
@@ -391,17 +422,15 @@ class Popover extends Popup {
 			return { width, height };
 		}
 
-		this.style.visibility = "hidden";
 		this.style.display = "block";
+		this.style.top = "-10000px";
+		this.style.left = "-10000px";
 
 		rect = this.getBoundingClientRect();
 
 		width = rect.width;
 		height = rect.height;
 
-		this.hide();
-		this.style.visibility = "visible";
-
 		return { width, height };
 	}
 
@@ -595,6 +624,7 @@ class Popover extends Popup {
 		switch (this.horizontalAlign) {
 		case PopoverHorizontalAlign.Center:
 		case PopoverHorizontalAlign.Stretch:
+
 			left = targetRect.left - (popoverSize.width - targetRect.width) / 2;
 			break;
 		case PopoverHorizontalAlign.Left:

packages/main/src/Popup.js

@@ -1,3 +1,4 @@
+import { renderFinished } from "@ui5/webcomponents-base/dist/Render.js";
 import litRender from "@ui5/webcomponents-base/dist/renderer/LitRenderer.js";
 import { getRTL } from "@ui5/webcomponents-base/dist/config/RTL.js";
 import UI5Element from "@ui5/webcomponents-base/dist/UI5Element.js";
@@ -309,7 +310,7 @@ class Popup extends UI5Element {
 	 * @param {boolean} preventInitialFocus prevents applying the focus inside the popup
 	 * @public
 	 */
-	open(preventInitialFocus) {
+	async open(preventInitialFocus) {
 		const prevented = !this.fireEvent("before-open", {}, true, false);
 		if (prevented) {
 			return;
@@ -325,6 +326,7 @@ class Popup extends UI5Element {
 		this._zIndex = getNextZIndex();
 		this.style.zIndex = this._zIndex;
 		this._focusedElementBeforeOpen = getFocusedElement();
+
 		this.show();
 
 		if (!this._disableInitialFocus && !preventInitialFocus) {
@@ -334,6 +336,8 @@ class Popup extends UI5Element {
 		this._addOpenedPopup();
 
 		this.opened = true;
+
+		await renderFinished();
 		this.fireEvent("after-open", {}, false, false);
 	}
 

packages/main/test/pages/Popover.html

@@ -137,8 +137,8 @@
 
 
 	<ui5-multi-combobox>
-		<ui5-li>Hello</ui5-li>
-		<ui5-li>World</ui5-li>
+		<ui5-cb-item text="Hello"></ui5-cb-item>
+		<ui5-cb-item text="World"></ui5-cb-item>
 	</ui5-multi-combobox>
 
 	<br>
@@ -371,6 +371,19 @@
 	<br>
 	<br>
 
+	<ui5-button id="firstBtn">First button</ui5-button>
+	<ui5-button id="secondBtn">Second button</ui5-button>
+	<ui5-popover id="popNoFocusableContent" placement-type="Bottom">Sample text for the ui5-popover</ui5-popover>
+
+	<br>
+	<br>
+
+	<div style="height: 6rem; text-align: center;">
+		<ui5-button id="btnOpenDynamic" icon="overflow"></ui5-button>
+	</div>
+
+	<br>
+
 	<script>
 		btn.addEventListener("click", function (event) {
 			pop.openBy(btn);
@@ -451,6 +464,30 @@
 		btnOpenXRight.addEventListener("click", function (event) {
 			popXRight.openBy(targetOpener2);
 		});
+
+		firstBtn.addEventListener("click", function (event) {
+			popNoFocusableContent.innerHTML = "First button is clicked";
+			popNoFocusableContent.openBy(event.target);
+		});
+
+		secondBtn.addEventListener("click", function (event) {
+			popNoFocusableContent.innerHTML = "Second button is clicked";
+			popNoFocusableContent.openBy(event.target);
+		});
+
+		btnOpenDynamic.addEventListener("click", function () {
+			var popover = document.createElement("ui5-popover"),
+				button = document.createElement("ui5-button");
+
+			button.textContent = "Focusable element";
+			popover.appendChild(button);
+			popover.setAttribute("id", "dynamic-popover");
+			popover.setAttribute("placement-type", "Bottom");
+
+			document.body.appendChild(popover);
+
+			popover.openBy(btnOpenDynamic);
+		});
 	</script>
 </body>
 

packages/main/test/specs/Popover.spec.js

@@ -231,6 +231,43 @@ describe("Popover general interaction", () => {
 
 		assert.ok(ff.getProperty("focused"), "The first focusable element is focused.");
 	});
+
+	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");
+
+		browser.keys(["Shift", "Tab"]);
+
+		activeElementId = $(browser.getActiveElement()).getAttribute("id");
+
+		assert.ok(activeElementId, popoverId, "Popover remains focused");
+	});
+
+	it("tests that dynamically created popover is opened", () => {
+		browser.url("http://localhost:8080/test-resources/pages/Popover.html");
+
+		const btnOpenDynamic = $("#btnOpenDynamic");
+		btnOpenDynamic.click();
+		const popover = $('#dynamic-popover');
+
+		browser.waitUntil(
+			() => popover.getCSSProperty("top").parsed.value > 0 && popover.getCSSProperty("left").parsed.value > 0,
+			{
+				timeout: 500,
+				timeoutMsg: "popover was not opened after a timeout"
+			}
+		);
+
+		assert.ok(true, "popover is opened");
+	});
 });
 
 describe("Acc", () => {