Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(ui5-popover): fix "containing block" position issue #7870

Merged
merged 12 commits into from
Nov 29, 2023
+84 −11
Merged

fix(ui5-popover): fix "containing block" position issue #7870

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
86f1cad
refactor(ui5-popover): test stacking context fix
TeodorTaushanov Nov 16, 2023
3ab82f4
refactor(ui5-popover): fix lint error
TeodorTaushanov Nov 16, 2023
5869bb1
refactor(ui5-popover): change sample
TeodorTaushanov Nov 17, 2023
e0344b6
Merge remote-tracking branch 'origin/main' into popover_position
TeodorTaushanov Nov 20, 2023
ce4d858
fix(ui5-popover): fix containing block positioning issue
TeodorTaushanov Nov 21, 2023
5143c8c
Merge remote-tracking branch 'origin/main' into popover_position
TeodorTaushanov Nov 21, 2023
ff7d00d
fix(ui5-popover): document special cases
TeodorTaushanov Nov 21, 2023
7043235
fix(ui5-popover): improve documentation
TeodorTaushanov Nov 21, 2023
eb575e6
Merge remote-tracking branch 'origin/main' into popover_position
TeodorTaushanov Nov 22, 2023
dd460f3
fix(ui5-popover): improve documentation
TeodorTaushanov Nov 22, 2023
345bafe
Merge remote-tracking branch 'origin/main' into popover_position
TeodorTaushanov Nov 24, 2023
0e45bdc
fix(ui5-popover): address code comments
TeodorTaushanov Nov 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions packages/base/src/util/isElementContainingBlock.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
const isElementContainingBlock = (el: HTMLElement) => {
const computedStyle = getComputedStyle(el);

return ["size", "inline-size"].indexOf(computedStyle.containerType) > -1
|| ["transform", "perspective"].indexOf(computedStyle.willChange) > -1
|| ["layout", "paint", "strict", "content"].indexOf(computedStyle.contain) > -1
|| computedStyle.transform !== "none"
|| computedStyle.perspective !== "none"
|| computedStyle.backdropFilter !== "none";
};

export default isElementContainingBlock;
9 changes: 4 additions & 5 deletions packages/main/src/Dialog.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,11 +94,10 @@ const ICON_PER_STATE: Record<ValueStateWithIcon, string> = {
*
* <code>import "@ui5/webcomponents/dist/Dialog";</code>
*
* <b>Note:</b> We don't recommend nesting popup-like components (<code>ui5-dialog</code>, <code>ui5-popover</code>) inside <code>ui5-dialog</code>.
* Ideally you should create all popups on the same level inside your HTML page and just open them from one another, rather than nesting them.
*
* <b>Note:</b> We don't recommend nesting popup-like components (<code>ui5-dialog</code>, <code>ui5-popover</code>) inside other components containing z-index.
* This might break z-index management.
* <b>Note: </b> We recommend placing popup-like components (<code>ui5-dialog</code> and <code>ui5-popover</code>)
* outside any other components. Preferably the popup-like components should be placed in
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest (here and below):
Preferably, the popup-like components should be placed in an upper level HTML element,

* a top level HTML element. Otherwise, in some cases the parent elements can break
TeodorTaushanov marked this conversation as resolved.
Show resolved Hide resolved
* the position and/or z-index management of the popup-like components.
TeodorTaushanov marked this conversation as resolved.
Show resolved Hide resolved
*
* @constructor
* @author SAP SE
Expand Down
23 changes: 23 additions & 0 deletions packages/main/src/Popover.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import { isIOS } from "@ui5/webcomponents-base/dist/Device.js";
import DOMReference from "@ui5/webcomponents-base/dist/types/DOMReference.js";
import { getClosedPopupParent } from "@ui5/webcomponents-base/dist/util/PopupUtils.js";
import clamp from "@ui5/webcomponents-base/dist/util/clamp.js";
import isElementContainingBlock from "@ui5/webcomponents-base/dist/util/isElementContainingBlock.js";
import Popup from "./Popup.js";
import type { PopupBeforeCloseEventDetail as PopoverBeforeCloseEventDetail } from "./Popup.js";
import PopoverPlacementType from "./types/PopoverPlacementType.js";
Expand Down Expand Up @@ -79,6 +80,11 @@ type CalculatedPlacement = {
*
* <code>import "@ui5/webcomponents/dist/Popover.js";</code>
*
* <b>Note: </b> We recommend placing popup-like components (<code>ui5-dialog</code> and <code>ui5-popover</code>)
* outside any other components. Preferably the popup-like components should be placed in
* a top level HTML element. Otherwise, in some cases the parent elements can break
* the position and/or the z-index management of the popup-like components.
*
* @constructor
* @author SAP SE
* @alias sap.ui.webc.main.Popover
Expand Down Expand Up @@ -464,6 +470,9 @@ class Popover extends Popup {
this.arrowTranslateY = placement!.arrow.y;

top = this._adjustForIOSKeyboard(top);
const containingBlockClientRect = this._getContainingBlockClientRect();
left -= containingBlockClientRect.left;
top -= containingBlockClientRect.top;

Object.assign(this.style, {
top: `${top}px`,
Expand Down Expand Up @@ -492,6 +501,20 @@ class Popover extends Popup {
return top + (Number.parseInt(this.style.top || "0") - actualTop);
}

_getContainingBlockClientRect() {
TeodorTaushanov marked this conversation as resolved.
Show resolved Hide resolved
let parentNode = this.parentElement ? this.parentNode as HTMLElement : (this.parentNode as ShadowRoot).host as HTMLElement;
TeodorTaushanov marked this conversation as resolved.
Show resolved Hide resolved

while (parentNode) {
if (isElementContainingBlock(parentNode)) {
TeodorTaushanov marked this conversation as resolved.
Show resolved Hide resolved
return parentNode.getBoundingClientRect();
}

parentNode = parentNode.parentElement ? parentNode.parentNode as HTMLElement : (parentNode.parentNode as ShadowRoot).host as HTMLElement;
}

return { left: 0, top: 0 };
TeodorTaushanov marked this conversation as resolved.
Show resolved Hide resolved
}

getPopoverSize(): PopoverSize {
const rect = this.getBoundingClientRect(),
width = rect.width,
Expand Down
30 changes: 25 additions & 5 deletions packages/main/test/pages/Popover.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@

<body class="popover2auto">
<iframe id="clickThisIframe" src="//localhost:8080"></iframe>

<ui5-button id="btn">Click me !</ui5-button>

<ui5-popover id="pop" class="popover6auto" placement-type="Top" accessible-name="This popover is important">
Expand Down Expand Up @@ -120,6 +119,24 @@
<br>
<br>

<div class="containingBlock">
<div>
Containing Block Div
<pre>
container-type: inline-size;
position: relative;
z-index: 1;
</pre>
</div>
<ui5-button id="openPopoverInsideContainingBlockButton">Open Popover Inside a Containing Block</ui5-button>
<ui5-popover id="popoverInsideContainingBlock" placement-type="Bottom">
Popover<br>Inside<br>Containing<br>Block
</ui5-popover>
</div>

<br>
<br>

<ui5-button id="big-popover-button">
Open Big Popover
</ui5-button>
Expand Down Expand Up @@ -395,7 +412,7 @@
<ui5-button id="popoverFocusButton">Dialog Focus</ui5-button>
<ui5-popover id="popoverFocus">
<div slot="header">Header text</div>

<div slot="footer" class="dialog-footer">
<div style="flex: 1"></div>
<ui5-button id="closeButton" design="Emphasized">Close</ui5-button>
Expand All @@ -415,7 +432,7 @@ <h2>Horizontal Align</h2>
<div>
<ui5-checkbox id="rtlCb" text="rtl"></ui5-checkbox>
</div>

<div id="horizontalAlignContainer">
<div id="targetOpener" class="popover10auto">
<span>Target opener</span>
Expand All @@ -432,7 +449,6 @@ <h2>Horizontal Align</h2>
<span id="createAndRemoveResult" style="display: none;"></span>
<br>
<br>

<script>
btn.addEventListener("click", function (event) {
pop.showAt(btn);
Expand Down Expand Up @@ -626,7 +642,7 @@ <h2>Horizontal Align</h2>

createAndRemove.addEventListener("click", function () {
var pop = document.createElement("ui5-popover");

pop.setAttribute("open", true);
pop.setAttribute("opener", "createAndRemove");

Expand All @@ -650,6 +666,10 @@ <h2>Horizontal Align</h2>
})
});

openPopoverInsideContainingBlockButton.addEventListener("click", function () {
popoverInsideContainingBlock.showAt(openPopoverInsideContainingBlockButton);
});

</script>
</body>

Expand Down
7 changes: 7 additions & 0 deletions packages/main/test/pages/styles/Popover.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,3 +66,10 @@ ui5-date-picker,
height: 150px;
width: 300px;
}

.containingBlock {
border: 1px solid black;
container-type: inline-size;
position: relative;
z-index: 1;
TeodorTaushanov marked this conversation as resolved.
Show resolved Hide resolved
}
6 changes: 4 additions & 2 deletions packages/main/test/specs/Popover.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,8 @@ describe("Popover general interaction", () => {
const btnOpenPopover = await browser.$("#btnQuickViewCardOpener");
const btnMoveFocus = await browser.$("#btnMoveFocus");

await btnOpenPopover.scrollIntoView();

// assert - the opener is visible
assert.strictEqual(await btnOpenPopover.isDisplayedInViewport(), true,
"Opener is available.");
Expand Down Expand Up @@ -474,7 +476,7 @@ describe("Horizontal Alignment", () => {
await browser.$("#horizontalAlignBtn").click();
const popover = await browser.$("#popoverHorizontalAlign");
const opener = await browser.$("#targetOpener");

assert.ok(await isHorizontallyCentered(popover, opener), `Popover should be centered`);
});

Expand All @@ -492,7 +494,7 @@ describe("Horizontal Alignment", () => {
await browser.$("#horizontalAlignBtn").click();
const popover = await browser.$("#popoverHorizontalAlign");
const opener = await browser.$("#targetOpener");

assert.ok(await isHorizontallyRightAligned(popover, opener), `Popover should be right aligned`);
});

Expand Down