javascript/ionic-framework
1
0
Fork 0
mirror of https://github.com/ionic-team/ionic-framework.git synced 2025-11-09 08:09:32 +08:00
Code Issues Packages Projects Releases Wiki Activity

fix(popover): viewport can be scrolled if no content present (#29215)

Browse Source 
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!
This commit is contained in:
Liam DeBeasi
2024-04-01 16:29:03 -04:00
committed by GitHub
parent 2053bc77a6
commit f08759c2b8
3 changed files with 119 additions and 2 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
core/src/components/popover/popover.scss
View File

@ -101,8 +101,6 @@
display: flex;
flex-direction: column;
overflow: hidden;
}
// Nested Popovers

92
core/src/components/popover/test/basic/popover.e2e.ts
View File

@ -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 }) => {