diff --git a/core/src/components/popover/animations/ios.enter.ts b/core/src/components/popover/animations/ios.enter.ts
index aa4e056814..ca12b8bc17 100644
--- a/core/src/components/popover/animations/ios.enter.ts
+++ b/core/src/components/popover/animations/ios.enter.ts
@@ -53,7 +53,15 @@ export const iosEnterAnimation = (baseEl: HTMLElement, opts?: any): Animation =>
);
const padding = size === 'cover' ? 0 : POPOVER_IOS_BODY_PADDING;
- const margin = size === 'cover' ? 0 : 25;
+
+ /**
+ * NOTE: The original experience guessed at a safe area margin of 25
+ * for non-cover popovers. This was changed to always be 0 here for
+ * debugging purposes, so we can be sure any safe area adjustment we
+ * see is only from what we implement for this fix.
+ */
+ // const margin = size === 'cover' ? 0 : 25;
+ const margin = 0;
const {
originX,
@@ -61,8 +69,8 @@ export const iosEnterAnimation = (baseEl: HTMLElement, opts?: any): Animation =>
top,
left,
bottom,
- checkSafeAreaLeft,
- checkSafeAreaRight,
+ // checkSafeAreaLeft,
+ // checkSafeAreaRight,
arrowTop,
arrowLeft,
addPopoverBottomClass,
@@ -122,29 +130,83 @@ export const iosEnterAnimation = (baseEl: HTMLElement, opts?: any): Animation =>
contentEl.style.setProperty('bottom', `${bottom}px`);
}
- const safeAreaLeft = ' + var(--ion-safe-area-left, 0)';
- const safeAreaRight = ' - var(--ion-safe-area-right, 0)';
-
- let leftValue = `${left}px`;
-
- if (checkSafeAreaLeft) {
- leftValue = `${left}px${safeAreaLeft}`;
- }
- if (checkSafeAreaRight) {
- leftValue = `${left}px${safeAreaRight}`;
- }
-
- contentEl.style.setProperty('top', `calc(${top}px + var(--offset-y, 0))`);
- contentEl.style.setProperty('left', `calc(${leftValue} + var(--offset-x, 0))`);
+ /**
+ * NOTE: We account for safe area through pure CSS by clamping the popover position
+ * between the safe area bounds. This works except for the arrow position in certain
+ * circumstances. (See comments on arrow position below for what.) Breakdown of
+ * values used for the top position:
+ *
+ * - Min: calc(var(--ion-safe-area-top, 0px) + var(--offset-y, 0px) + ${arrowHeight}px)
+ * Top edge of safe area, plus custom y offset, plus a little gap to make room for the arrow.
+ * Note that arrowHeight resolves to 0px if the arrow is not displayed.
+ *
+ * - Preferred: calc(${top}px + var(--offset-y, 0px))
+ * Normal calculated value of the popover, including any custom offset. The value of top
+ * already accounts for all conditions aside from safe area, since it's what we were using
+ * before the fix.
+ *
+ * - Max: calc(100% - ${contentHeight + arrowHeight}px - var(--ion-safe-area-bottom) + var(--offset-y, 0px))
+ * Bottom edge of screen, minus the total height of the popover (content + arrow), minus
+ * the bottom safe area margin, including custom y offset.
+ *
+ * The left position is the same, but using x-axis values instead. We do this through
+ * pure CSS to avoid needing to call window.getComputedStyle() to figure out the computed
+ * safe area, which is very expensive and would introduce performance issues.
+ */
+ contentEl.style.setProperty('top', `clamp(calc(var(--ion-safe-area-top, 0px) + var(--offset-y, 0px) + ${arrowHeight}px), calc(${top}px + var(--offset-y, 0px)), calc(100% - ${contentHeight + arrowHeight}px - var(--ion-safe-area-bottom) + var(--offset-y, 0px)))`);
+ contentEl.style.setProperty('left', `clamp(calc(var(--ion-safe-area-left, 0px) + var(--offset-x, 0px)), calc(${left}px + var(--offset-x, 0px)), calc(100% - ${contentWidth}px - var(--ion-safe-area-right) + var(--offset-x, 0px)))`);
contentEl.style.setProperty('transform-origin', `${originY} ${originX}`);
if (arrowEl !== null) {
const didAdjustBounds = results.top !== top || results.left !== left;
const showArrow = shouldShowArrow(side, didAdjustBounds, ev, trigger);
+ /**
+ * NOTE: Currently the fix assumes a default value for the side prop, which always
+ * puts the arrow either above or below the popover. There are additional tweaks
+ * needed to handle the arrow being on the left or right side.
+ */
+
if (showArrow) {
- arrowEl.style.setProperty('top', `calc(${arrowTop}px + var(--offset-y, 0))`);
- arrowEl.style.setProperty('left', `calc(${arrowLeft}px + var(--offset-x, 0))`);
+ /**
+ * NOTE: Basically the same positioning logic as the popover content, but using the
+ * existing arrowTop and arrowLeft values instead. The hardcoded 5px in the left
+ * position was an early attempt at preventing the arrow from being flush with
+ * the left/right edge of the content, which causes it to look disconnected due
+ * to the content's border radius.
+ *
+ * The problem with this approach is that the popover content hits the max value
+ * sooner than the arrow, due to being taller. If you present a popover low enough
+ * on the screen to need safe area adjustment, but not so low that the popover flips
+ * to present above the trigger, the arrow will render inside the popover because
+ * the content has been adjusted for safe area but the arrow has not.
+ *
+ * In theory, the most straightforward way of fixing this would be to calculate the
+ * final rendered position of the popover and position the arrow relative to that.
+ * However, this would require using window.getComputedStyle(), which is exactly
+ * what we're trying to avoid.
+ */
+ arrowEl.style.setProperty('top', `clamp(calc(var(--ion-safe-area-top, 0px) + var(--offset-y, 0px)), calc(${arrowTop}px + var(--offset-y, 0px)), calc(100% - ${arrowHeight + contentHeight}px - var(--ion-safe-area-bottom) + var(--offset-y, 0px)))`);
+ arrowEl.style.setProperty('left', `clamp(calc(var(--ion-safe-area-left, 0px) + var(--offset-x, 0px) + 5px), calc(${arrowLeft}px + var(--offset-x, 0px)), calc(100% - ${arrowWidth}px - var(--ion-safe-area-right) + var(--offset-x, 0px) - 5px))`);
+
+ /**
+ * NOTE: An early attempt at positioning the arrow relative to the content.
+ * See comments in popover.tsx for details.
+ */
+ // arrowEl.style.setProperty('top', `-${arrowHeight}px`);
+ // arrowEl.style.setProperty('left', `calc(${contentWidth / 2}px - ${arrowWidth / 2}px)`);
+
+ /**
+ * NOTE: Some quick and dirty debugging code which will position some debug elements
+ * in the adjustment test template at the min, preferred, and max values for the
+ * popover content's position. This can help visualize how things are being calculated.
+ */
+ // const minLine = document.querySelector('#min-line') as HTMLElement;
+ // const preferredLine = document.querySelector('#preferred-line') as HTMLElement;
+ // const maxLine = document.querySelector('#max-line') as HTMLElement;
+ // minLine!.style.top = `calc(var(--ion-safe-area-top, 0px) + var(--offset-y, 0px))`;
+ // preferredLine!.style.top = `calc(${arrowTop}px + var(--offset-y, 0px))`;
+ // maxLine!.style.top = `calc(100% - ${arrowHeight + contentHeight}px - var(--ion-safe-area-bottom) + var(--offset-y, 0px))`;
} else {
arrowEl.style.setProperty('display', 'none');
}
diff --git a/core/src/components/popover/animations/md.enter.ts b/core/src/components/popover/animations/md.enter.ts
index e25f745cec..bdc7c9fbe8 100644
--- a/core/src/components/popover/animations/md.enter.ts
+++ b/core/src/components/popover/animations/md.enter.ts
@@ -78,11 +78,19 @@ export const mdEnterAnimation = (baseEl: HTMLElement, opts?: any): Animation =>
wrapperAnimation.addElement(root.querySelector('.popover-wrapper')!).duration(150).fromTo('opacity', 0.01, 1);
+ /**
+ * NOTE: See ios.enter.ts for details on what this is doing. For MD, this works perfectly.
+ * Technically, the logic for whether to flip the direction of the opening animation doesn't
+ * account for safe area, so there's a safe-area-margin-sized window where it flips in
+ * the wrong direction. However, because the margin is usually pretty small, this is barely
+ * noticeable in real world contexts. The popover is always positioned in a way that "feels"
+ * correct, and IMO this is an acceptable compromise.
+ */
contentAnimation
.addElement(contentEl)
.beforeStyles({
- top: `calc(${top}px + var(--offset-y, 0px))`,
- left: `calc(${left}px + var(--offset-x, 0px))`,
+ top: `clamp(calc(var(--ion-safe-area-top, 0) + var(--offset-y)), calc(${top}px + var(--offset-y, 0px)), calc(100% - ${contentHeight}px - var(--ion-safe-area-bottom)))`,
+ left: `clamp(calc(var(--ion-safe-area-left, 0) + var(--offset-x)), calc(${left}px + var(--offset-x, 0px)), calc(100% - ${contentWidth}px - var(--ion-safe-area-right)))`,
'transform-origin': `${originY} ${originX}`,
})
.beforeAddWrite(() => {
diff --git a/core/src/components/popover/popover.ios.scss b/core/src/components/popover/popover.ios.scss
index d8feb57b55..1897a634b4 100644
--- a/core/src/components/popover/popover.ios.scss
+++ b/core/src/components/popover/popover.ios.scss
@@ -42,6 +42,9 @@
* will allow the arrow to render above the backdrop.
*/
z-index: 11;
+
+ // NOTE: Useful for debugging purposes when the arrow overlaps the content.
+ // --background: pink;
}
.popover-arrow::after {
diff --git a/core/src/components/popover/popover.tsx b/core/src/components/popover/popover.tsx
index 46f922b834..1373727cde 100644
--- a/core/src/components/popover/popover.tsx
+++ b/core/src/components/popover/popover.tsx
@@ -694,6 +694,21 @@ export class Popover implements ComponentInterface, PopoverInterface {
{!parentPopover &&