From f08759c2b8256ff66f8d1901bd8e0be4617db262 Mon Sep 17 00:00:00 2001
From: Liam DeBeasi <liamdebeasi@users.noreply.github.com>
Date: Mon, 1 Apr 2024 16:29:03 -0400
Subject: [PATCH] fix(popover): viewport can be scrolled if no content present
 (#29215)

Issue number: resolves #29211

---------

<!-- 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. -->

In https://github.com/ionic-team/ionic-framework/pull/28861 I fixed a
bug that caused `.popover-viewport` to have `overflow: hidden`. In
reality, this code should have always applied but due to an incorrect
selector it never did.

As it turns out in
https://github.com/ionic-team/ionic-framework/issues/29211, some
developers were relying on the broken behavior to build their
applications. In particular, developers were using `ion-popover` without
an `ion-content`. The linked change made it so that using popovers
without `ion-content` were not scrollable.

After talking with @mapsandapps we think it makes sense to officially
support this behavior. We support using [modals without
`ion-content`](https://ionicframework.com/docs/api/modal#custom-dialogs),
and we could not think of a reason to not support the same use case for
popover.

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

- If the `.popover-viewport` element has a child content then
`.popover-viewport `will not be scrollable.
- If the `.popover-viewport` element does not have a child content then
`.popover-viewport` will be scrollable.

I implemented this behavior using progressive enhancement via `:has`.
The [`:has` pseudo-class](https://caniuse.com/?search=%3Ahas) has
cross-browser support. Ionic v7 supports some versions of browsers that
do not have `:has` support. As a result, we fall back to the existing
behavior in this environment. Developers are able to get this behavior
on older browsers by explicitly setting `overflow: auto` on
`.popover-viewport`.

Fortunately, we will be dropping support for several of the older
browsers versions in Ionic v8, so the need to do the manual opt-in
should be less frequent as time goes on.

## 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. -->

Dev build: `7.8.2-dev.11711383079.118d48a5`

Testing:

1. Open https://codepen.io/liamdebeasi/pen/JjVJrZQ?editors=1100 (this
has a dev build installed)
2. Click each button to open a popover.
3. Verify that each popover can be scrolled.

I could not find a great way to automate this test, but let me know if
anyone has ideas!
---
 core/src/components/popover/popover.scss      |  2 -
 .../popover/test/basic/popover.e2e.ts         | 92 +++++++++++++++++++
 core/src/css/core.scss                        | 27 ++++++
 3 files changed, 119 insertions(+), 2 deletions(-)

diff --git a/core/src/components/popover/popover.scss b/core/src/components/popover/popover.scss
index 4181df6e69..ecd8acf2dc 100644
--- a/core/src/components/popover/popover.scss
+++ b/core/src/components/popover/popover.scss
@@ -101,8 +101,6 @@
   display: flex;
 
   flex-direction: column;
-
-  overflow: hidden;
 }
 
 // Nested Popovers
diff --git a/core/src/components/popover/test/basic/popover.e2e.ts b/core/src/components/popover/test/basic/popover.e2e.ts
index 81b76364f2..f05291ee2a 100644
--- a/core/src/components/popover/test/basic/popover.e2e.ts
+++ b/core/src/components/popover/test/basic/popover.e2e.ts
@@ -48,6 +48,98 @@ configs({ directions: ['ltr'] }).forEach(({ title, screenshot, config }) => {
  * Translucent popovers are only available on iOS
  */
 configs({ modes: ['ios'], directions: ['ltr'] }).forEach(({ title, screenshot, config }) => {
+  test.describe(title('popover: scrolling'), async () => {
+    test.beforeEach(({ skip }) => {
+      test.info().annotations.push({
+        type: 'issue',
+        description: 'https://github.com/ionic-team/ionic-framework/issues/29211',
+      });
+      // We are testing if Ionic sets overflow is set correctly on elements,
+      // so we do not need to test across browsers
+      skip.browser('webkit', 'Behavior does not vary across browsers');
+      skip.browser('firefox', 'Behavior does not vary across browsers');
+    });
+    test('should scroll to bottom without IonContent', async ({ page }) => {
+      await page.setContent(
+        `
+        <style>
+          ion-popover {
+            --height: 150px;
+          }
+        </style>
+        <ion-popover>
+          <p>Text</p>
+          <p>Text</p>
+          <p>Text</p>
+          <p>Text</p>
+          <p>Text</p>
+          <p>Text</p>
+          <p>Text</p>
+          <p>Text</p>
+          <p>Text</p>
+          <p>Text</p>
+        </ion-popover>
+      `,
+        config
+      );
+
+      const popover = page.locator('ion-popover');
+      const viewport = popover.locator('.popover-viewport');
+      const p = popover.locator('p');
+      const lastP = await p.last();
+
+      await popover.evaluate((el: HTMLIonPopoverElement) => el.present());
+
+      await expect(lastP).not.toBeInViewport();
+
+      // hover over viewport and scroll to bottom
+      await viewport.hover();
+      await page.mouse.wheel(0, 500);
+
+      await expect(lastP).toBeInViewport();
+    });
+    test('should scroll to bottom with IonContent', async ({ page }) => {
+      await page.setContent(
+        `
+        <style>
+          ion-popover {
+            --height: 150px;
+          }
+        </style>
+        <ion-popover>
+          <ion-content>
+            <p>Text</p>
+            <p>Text</p>
+            <p>Text</p>
+            <p>Text</p>
+            <p>Text</p>
+            <p>Text</p>
+            <p>Text</p>
+            <p>Text</p>
+            <p>Text</p>
+            <p>Text</p>
+          </ion-content>
+        </ion-popover>
+      `,
+        config
+      );
+
+      const popover = page.locator('ion-popover');
+      const content = popover.locator('ion-content');
+      const p = popover.locator('p');
+      const lastP = await p.last();
+
+      await popover.evaluate((el: HTMLIonPopoverElement) => el.present());
+
+      await expect(lastP).not.toBeInViewport();
+
+      // hover over viewport and scroll to bottom
+      await content.hover();
+      await page.mouse.wheel(0, 500);
+
+      await expect(lastP).toBeInViewport();
+    });
+  });
   test.describe(title('popover: translucent variants'), async () => {
     let popoverFixture!: PopoverFixture;
     test.beforeEach(async ({ page }) => {
diff --git a/core/src/css/core.scss b/core/src/css/core.scss
index 492b807447..afa23b0ada 100644
--- a/core/src/css/core.scss
+++ b/core/src/css/core.scss
@@ -375,3 +375,30 @@ ion-input input::-webkit-date-and-time-value {
   width: 320px;
   min-height: 320px;
 }
+
+/**
+ * If a popover has a child ion-content (or class equivalent) then the .popover-viewport element
+ * should not be scrollable to ensure the inner content does scroll. However, if the popover
+ * does not have a child ion-content (or class equivalent) then the .popover-viewport element
+ * should remain scrollable. This code exists globally because popover targets
+ * .popover-viewport using ::slotted which only supports simple selectors.
+ *
+ * Note that we do not need to account for .ion-content-scroll-host here because that
+ * class should always be placed within ion-content even if ion-content is not scrollable.
+ */
+.popover-viewport:has(> ion-content) {
+  overflow: hidden;
+}
+
+/**
+ * :has has cross-browser support, but it is still relatively new. As a result,
+ * we should fallback to the old behavior for environments that do not support :has.
+ * Developers can explicitly enable this behavior by setting overflow: visible
+ * on .popover-viewport if they know they are not going to use an ion-content.
+ * TODO FW-6106 Remove this
+ */
+@supports not selector(:has(> ion-content)) {
+  .popover-viewport {
+    overflow: hidden;
+  }
+}