more comments

Issue number: resolves #23472

---------

<!-- Please do not submit updates to dependencies unless it fixes an
issue. -->

<!-- Please try to limit your pull request to one type (bugfix, feature,
etc). Submit multiple pull requests if needed. -->

## What is the current behavior?
<!-- Please describe the current behavior that you are modifying. -->

If multiple overlays are presented at the same time, none of them
receive `aria-hidden="true"`. This means that screen readers can read
contents from overlays behind the current one, which can be confusing
for users.

The original issue also reports router outlets getting `aria-hidden`
removed when any overlay is dismissed, not just the last one, but we've
since fixed that:
35ab6b4816/core/src/utils/overlays.ts (L573-L576)

## What is the new behavior?
<!-- Please describe the behavior or changes that are being added by
this PR. -->

All overlays besides the topmost one now receive `aria-hidden="true"`.
This means that screen readers will only announce the topmost overlay.

## Does this introduce a breaking change?

- [ ] Yes
- [x] No

<!--
  If this introduces a breaking change:
1. Describe the impact and migration path for existing applications
below.
  2. Update the BREAKING.md file with the breaking change.
3. Add "BREAKING CHANGE: [...]" to the commit description when merging.
See
https://github.com/ionic-team/ionic-framework/blob/main/.github/CONTRIBUTING.md#footer
for more information.
-->


## Other information

<!-- Any other information that is important to this PR such as
screenshots of how the component looks before and after the change. -->

core/package-lock.json

@@ -15,7 +15,7 @@
       },
       "devDependencies": {
         "@axe-core/playwright": "^4.8.4",
-        "@capacitor/core": "^5.6.0",
+        "@capacitor/core": "^5.7.0",
         "@capacitor/haptics": "^5.0.7",
         "@capacitor/keyboard": "^5.0.8",
         "@capacitor/status-bar": "^5.0.7",
@@ -634,9 +634,9 @@
       "dev": true
     },
     "node_modules/@capacitor/core": {
-      "version": "5.6.0",
-      "resolved": "https://registry.npmjs.org/@capacitor/core/-/core-5.6.0.tgz",
-      "integrity": "sha512-xJhCOUGPHw0QYDA3YH+CmL6qiV9DH4Ij3yPxSenymjrtLuXI197u9ddCZwGEwgVIkh9kGZBBKzsNkn89SZ2gdQ==",
+      "version": "5.7.0",
+      "resolved": "https://registry.npmjs.org/@capacitor/core/-/core-5.7.0.tgz",
+      "integrity": "sha512-wa9Fao+Axa1t2ZERMyQD9r0xyfglQyC4DHQKintzKaIqcRuVe9J31TmfD3IxROYi9LGpY4X8cq4m4bjb0W94Qg==",
       "dev": true,
       "dependencies": {
         "tslib": "^2.1.0"
@@ -11324,9 +11324,9 @@
       "dev": true
     },
     "@capacitor/core": {
-      "version": "5.6.0",
-      "resolved": "https://registry.npmjs.org/@capacitor/core/-/core-5.6.0.tgz",
-      "integrity": "sha512-xJhCOUGPHw0QYDA3YH+CmL6qiV9DH4Ij3yPxSenymjrtLuXI197u9ddCZwGEwgVIkh9kGZBBKzsNkn89SZ2gdQ==",
+      "version": "5.7.0",
+      "resolved": "https://registry.npmjs.org/@capacitor/core/-/core-5.7.0.tgz",
+      "integrity": "sha512-wa9Fao+Axa1t2ZERMyQD9r0xyfglQyC4DHQKintzKaIqcRuVe9J31TmfD3IxROYi9LGpY4X8cq4m4bjb0W94Qg==",
       "dev": true,
       "requires": {
         "tslib": "^2.1.0"

core/package.json

@@ -37,7 +37,7 @@
   },
   "devDependencies": {
     "@axe-core/playwright": "^4.8.4",
-    "@capacitor/core": "^5.6.0",
+    "@capacitor/core": "^5.7.0",
     "@capacitor/haptics": "^5.0.7",
     "@capacitor/keyboard": "^5.0.8",
     "@capacitor/status-bar": "^5.0.7",

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 {

core/src/utils/overlays.ts

@@ -491,6 +491,16 @@ export const present = async <OverlayPresentOptions>(
 
   setRootAriaHidden(true);
 
+  /**
+   * Hide all other overlays from screen readers so only this one
+   * can be read. Note that presenting an overlay always makes
+   * it the topmost one.
+   */
+  if (doc !== undefined) {
+    const presentedOverlays = getPresentedOverlays(doc);
+    presentedOverlays.forEach((o) => o.setAttribute('aria-hidden', 'true'));
+  }
+
   overlay.presented = true;
   overlay.willPresent.emit();
   overlay.willPresentShorthand?.emit();
@@ -528,6 +538,15 @@ export const present = async <OverlayPresentOptions>(
   if (overlay.keyboardClose && (document.activeElement === null || !overlay.el.contains(document.activeElement))) {
     overlay.el.focus();
   }
+
+  /**
+   * If this overlay was previously dismissed without being
+   * the topmost one (such as by manually calling dismiss()),
+   * it would still have aria-hidden on being presented again.
+   * Removing it here ensures the overlay is visible to screen
+   * readers.
+   */
+  overlay.el.removeAttribute('aria-hidden');
 };
 
 /**
@@ -625,6 +644,15 @@ export const dismiss = async <OverlayDismissOptions>(
   }
 
   overlay.el.remove();
+
+  /**
+   * If there are other overlays presented, unhide the new
+   * topmost one from screen readers.
+   */
+  if (doc !== undefined) {
+    getPresentedOverlay(doc)?.removeAttribute('aria-hidden');
+  }
+
   return true;
 };
 

core/src/utils/test/overlays/index.html

@@ -62,7 +62,7 @@
             </ion-toolbar>
           </ion-header>
           <ion-content class="ion-padding">
-            Modal Content
+            Modal ${id}
 
             <ion-item>
               <ion-input label="Text Input" class="modal-input modal-input-${id}"></ion-input>

core/src/utils/test/overlays/overlays.spec.ts

@@ -129,3 +129,68 @@ describe('setRootAriaHidden()', () => {
     expect(routerOutlet.hasAttribute('aria-hidden')).toEqual(false);
   });
 });
+
+describe('aria-hidden on individual overlays', () => {
+  it('should hide non-topmost overlays from screen readers', async () => {
+    const page = await newSpecPage({
+      components: [Modal],
+      html: `
+        <ion-modal id="one"></ion-modal>
+        <ion-modal id="two"></ion-modal>
+      `,
+    });
+
+    const modalOne = page.body.querySelector<HTMLIonModalElement>('ion-modal#one')!;
+    const modalTwo = page.body.querySelector<HTMLIonModalElement>('ion-modal#two')!;
+
+    await modalOne.present();
+    await modalTwo.present();
+
+    expect(modalOne.hasAttribute('aria-hidden')).toEqual(true);
+    expect(modalTwo.hasAttribute('aria-hidden')).toEqual(false);
+  });
+
+  it('should unhide new topmost overlay from screen readers when topmost is dismissed', async () => {
+    const page = await newSpecPage({
+      components: [Modal],
+      html: `
+        <ion-modal id="one"></ion-modal>
+        <ion-modal id="two"></ion-modal>
+      `,
+    });
+
+    const modalOne = page.body.querySelector<HTMLIonModalElement>('ion-modal#one')!;
+    const modalTwo = page.body.querySelector<HTMLIonModalElement>('ion-modal#two')!;
+
+    await modalOne.present();
+    await modalTwo.present();
+
+    // dismiss modalTwo so that modalOne becomes the new topmost overlay
+    await modalTwo.dismiss();
+
+    expect(modalOne.hasAttribute('aria-hidden')).toEqual(false);
+  });
+
+  it('should not keep overlays hidden from screen readers if presented after being dismissed while non-topmost', async () => {
+    const page = await newSpecPage({
+      components: [Modal],
+      html: `
+        <ion-modal id="one"></ion-modal>
+        <ion-modal id="two"></ion-modal>
+      `,
+    });
+
+    const modalOne = page.body.querySelector<HTMLIonModalElement>('ion-modal#one')!;
+    const modalTwo = page.body.querySelector<HTMLIonModalElement>('ion-modal#two')!;
+
+    await modalOne.present();
+    await modalTwo.present();
+
+    // modalOne is not the topmost overlay at this point and is hidden from screen readers
+    await modalOne.dismiss();
+
+    // modalOne will become the topmost overlay; ensure it isn't still hidden from screen readers
+    await modalOne.present();
+    expect(modalOne.hasAttribute('aria-hidden')).toEqual(false);
+  });
+});