From d422302d98a64a5a11a8590c66a8102913c38df2 Mon Sep 17 00:00:00 2001 From: amandaesmith3 Date: Thu, 15 Feb 2024 12:24:23 -0600 Subject: [PATCH] add in progress fix with comments --- .../popover/animations/ios.enter.ts | 100 ++++++++++++++---- .../components/popover/animations/md.enter.ts | 12 ++- core/src/components/popover/popover.ios.scss | 3 + core/src/components/popover/popover.tsx | 15 +++ .../popover/test/adjustment/index.html | 93 ++++++++++++---- 5 files changed, 184 insertions(+), 39 deletions(-) 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 && }
this.dismiss() : undefined}> + {/** + * NOTE: One possibility is moving the arrow inside .popover-content and positioning the arrow relative + * to the content instead of to the whole screen. This has a few issues that we didn't have time to + * sort out: + * 1. The content hides overflow, so the arrow is no longer visible. However, setting overflow:visible + * also breaks the border radius for some reason. (Unsure why yet since border radius is set on the + * content el itself, so in theory overflow:hidden shouldn't be needed.) + * 2. Additional logic is needed to handle the arrow being above or below the content based on whether + * the popover has to flip due to being close to the bottom edge of the screen. calculateWindowAdjustment + * returns an addPopoverBottomClass var in the iOS enter animation that we may be able to use for this. + * (There are additional considerations for when popover's side prop is used to move the arrow, though.) + * 3. Additional logic is needed to handle the arrow moving horizontally to point at the trigger/event + * position. For example, if the popover is presented near the right edge of the screen, the arrow + * might be positioned more to the right instead of centered. + */} {enableArrow &&
}
diff --git a/core/src/components/popover/test/adjustment/index.html b/core/src/components/popover/test/adjustment/index.html index a918b29502..e8eb9adc23 100644 --- a/core/src/components/popover/test/adjustment/index.html +++ b/core/src/components/popover/test/adjustment/index.html @@ -16,13 +16,50 @@ import { popoverController } from '../../../../dist/ionic/index.esm.js'; window.popoverController = popoverController; + + + + +

Click everywhere to open the popover.

- Enable Safe Area + Options + + + + Enable Safe Area + + + Enable Popover Offset + + +
@@ -30,41 +67,61 @@