more comments

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,88 @@ 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.
+       *
+       * The clamp() function is available in all browsers we support as of Ionic v7, except for
+       * Firefox. The function was introduced in Firefox 75, which is conveniently the new minimum
+       * supported version in Ionic v8. As such, we'll probably want to just push the fix to v8,
+       * if v8 isn't already out by the time we get to this.
+       */
+      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 overlapping 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');
         }

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 should work great.
+   * 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(() => {

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 {

core/src/components/popover/popover.tsx

@@ -694,6 +694,21 @@ export class Popover implements ComponentInterface, PopoverInterface {
         {!parentPopover && <ion-backdrop tappable={this.backdropDismiss} visible={this.showBackdrop} part="backdrop" />}
 
         <div class="popover-wrapper ion-overlay-wrapper" onClick={dismissOnSelect ? () => 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 && <div class="popover-arrow" part="arrow"></div>}
           <div class="popover-content" part="content">
             <slot></slot>

core/src/components/popover/test/adjustment/index.html

@@ -16,27 +16,112 @@
       import { popoverController } from '../../../../dist/ionic/index.esm.js';
       window.popoverController = popoverController;
     </script>
+
+    <!-- <style>
+      .line {
+        position: absolute;
+        width: 100%;
+        height: 1px;
+        left: 0;
+      }
+
+      #min-line {
+        background-color: blue;
+      }
+
+      #preferred-line {
+        background-color: green;
+      }
+
+      #max-line {
+        background-color: red;
+      }
+    </style> -->
   </head>
   <body>
     <ion-app>
       <ion-content>
-        <p style="text-align: center">Click everywhere to open the popover.</p>
+        <!-- NOTE: Debugging elements to help visualize how the popover's position
+        is calculated. See comments near the bottom of ios.enter.ts for details. -->
+        <!-- <div id="min-line" class="line"></div>
+        <div id="preferred-line" class="line"></div>
+        <div id="max-line" class="line"></div> -->
+
+        <div style="text-align: center">
+          <p>Click everywhere to open the popover.</p>
+          <ion-button id="options-btn">Options</ion-button>
+          <ion-popover trigger="options-btn" class="options-popover">
+            <ion-list>
+              <ion-item>
+                <ion-checkbox id="safe-area-cb">Enable Safe Area</ion-checkbox>
+              </ion-item>
+              <ion-item>
+                <ion-checkbox id="offset-cb">Enable Popover Offset</ion-checkbox>
+              </ion-item>
+            </ion-list>
+          </ion-popover>
+        </div>
       </ion-content>
     </ion-app>
 
     <script>
       document.querySelector('ion-content').addEventListener('click', handleButtonClick);
+      document.querySelector('#safe-area-cb').addEventListener('ionChange', toggleSafeArea);
+      document.querySelector('#offset-cb').addEventListener('ionChange', toggleOffset);
 
       async function handleButtonClick(ev) {
+        const targetEl = ev.target.tagName;
+        if (targetEl === 'ION-CHECKBOX' || targetEl === 'ION-BUTTON') return;
+
         const popover = await popoverController.create({
           component: 'popover-example-page',
           event: ev,
-          reference: 'event',
+          reference: 'event'
         });
 
         popover.present();
       }
 
+      function toggleStyles(ev, styles, styleId) {
+        if(ev.detail.checked) {
+          document.head.insertAdjacentHTML('beforeend', `
+            <style id="${styleId}">
+              ${styles}
+            </style>
+          `);
+        } else {
+          const styles = document.querySelector(`#${styleId}`);
+          if(styles) styles.remove();
+        }
+      }
+
+      function toggleSafeArea(ev) {
+        toggleStyles(ev, `
+          html {
+            --ion-safe-area-top: 50px;
+            --ion-safe-area-right: 50px;
+            --ion-safe-area-bottom: 50px;
+            --ion-safe-area-left: 50px;
+          }
+
+          ion-content::part(background) {
+            box-shadow: inset 0 0 0 50px lightblue;
+          }
+        `, 'safe-area-styles');
+      }
+
+      /**
+       * NOTE: Make sure to also check any further fixes against a negative offset.
+       */
+      function toggleOffset(ev) {
+        toggleStyles(ev, `
+          ion-popover:not(.options-popover) {
+            --offset-x: 25px;
+            --offset-y: 25px;
+          }
+        `, 'offset-styles');
+      }
+
       customElements.define(
         'popover-example-page',
         class PopoverContent extends HTMLElement {