Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(popover): ensure popover does not go offscreen when adjusting top position #25350

Merged
merged 8 commits into from
Jun 9, 2022
Merged

fix(popover): ensure popover does not go offscreen when adjusting top position #25350

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
5ea24bc
test(popover): add failing test
liamdebeasi May 25, 2022
a3b4a72
fix(popover): ensure popover does not go offscreen when adjusting top…
liamdebeasi May 25, 2022
46a6b46
chore(): lint
liamdebeasi May 25, 2022
92ac0e4
chore(): clean up
liamdebeasi May 26, 2022
9234af6
chore(): improve test clarify
liamdebeasi May 26, 2022
af1f1aa
chore(): prettier
liamdebeasi May 26, 2022
015dadb
Merge branch 'main' into FW-1591
liamdebeasi Jun 1, 2022
c3a2258
Merge branch 'main' into FW-1591
liamdebeasi Jun 9, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 59 additions & 0 deletions core/src/components/popover/test/adjustment/index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
<!DOCTYPE html>
<html lang="en" dir="ltr">
<head>
<meta charset="UTF-8" />
<title>Popover - Adjustment</title>
<meta
name="viewport"
content="width=device-width, initial-scale=1.0, minimum-scale=1.0, maximum-scale=1.0, user-scalable=no, viewport-fit=cover"
/>
<link href="../../../../../css/ionic.bundle.css" rel="stylesheet" />
<link href="../../../../../scripts/testing/styles.css" rel="stylesheet" />
<script src="../../../../../scripts/testing/scripts.js"></script>
<script nomodule src="../../../../../dist/ionic/ionic.js"></script>
<script type="module" src="../../../../../dist/ionic/ionic.esm.js"></script>
<script type="module">
import { popoverController } from '../../../../dist/ionic/index.esm.js';
window.popoverController = popoverController;
</script>
</head>
<body>
<ion-app>
<ion-content>
<p style="text-align: center">Click everywhere to open the popover.</p>
</ion-content>
</ion-app>

<script>
document.querySelector('ion-content').addEventListener('click', handleButtonClick);

async function handleButtonClick(ev) {
const popover = await popoverController.create({
component: 'popover-example-page',
event: ev,
reference: 'event',
});

popover.present();
}

customElements.define(
'popover-example-page',
class PopoverContent extends HTMLElement {
connectedCallback() {
this.innerHTML = `
<ion-list>
<ion-list-header>Ionic</ion-list-header>
<ion-item button>Learn Ionic</ion-item>
<ion-item button>Documentation</ion-item>
<ion-item button>Showcase</ion-item>
<ion-item button>GitHub Repo</ion-item>
<ion-item button>Another Item</ion-item>
</ion-list>
`;
}
}
);
</script>
</body>
</html>
32 changes: 32 additions & 0 deletions core/src/components/popover/test/adjustment/popover.e2e.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
import { expect } from '@playwright/test';
import { test } from '@utils/test/playwright';

test.describe('popover: adjustment', async () => {
test('should not render the popover offscreen', async ({ page }) => {
await page.goto('/src/components/popover/test/adjustment');

/**
* We need to click in an area where
* there is not enough room to show the popover
* below the click coordinates but not enough
* room above the click coordinates that we
* can just move the popover to without it going
* offscreen.
*/
await page.setViewportSize({
width: 500,
height: 400,
});

const ionPopoverDidPresent = await page.spyOnEvent('ionPopoverDidPresent');

await page.mouse.click(300, 300);

await ionPopoverDidPresent.next();

const popoverContent = page.locator('ion-popover .popover-content');
const box = (await popoverContent.boundingBox())!;

expect(box.y > 0).toBe(true);
});
});
12 changes: 11 additions & 1 deletion core/src/components/popover/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -863,7 +863,17 @@ export const calculateWindowAdjustment = (
*/
if (triggerTop + triggerHeight + contentHeight > bodyHeight && (side === 'top' || side === 'bottom')) {
if (triggerTop - contentHeight > 0) {
top = triggerTop - contentHeight - triggerHeight - (arrowHeight - 1);
/**
* While we strive to align the popover with the trigger
* on smaller screens this is not always possible. As a result,
* we adjust the popover up so that it does not hang
* off the bottom of the screen. However, we do not want to move
* the popover up so much that it goes off the top of the screen.
*
* We chose 12 here so that the popover position looks a bit nicer as
* it is not right up against the edge of the screen.
*/
top = Math.max(12, triggerTop - contentHeight - triggerHeight - (arrowHeight - 1));
arrowTop = top + contentHeight;
originY = 'bottom';
addPopoverBottomClass = true;
Expand Down