Skip to content

Commit

Permalink
fix(popover): dynamic width popover is positioned correctly (#28072)
Browse files Browse the repository at this point in the history
Issue number: resolves #27190, resolves #24780

---------

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

Popovers with dynamic widths were not being positioned correctly
relative to the trigger element. This was happening because the child
component always had dimensions of 0 x 0. Ionic has logic built-in to
wait for the child components to be rendered, but this was not working
as intended for two reasons:

1. `this.usersElement` was referencing the popover element itself not
the user’s component. When calling `deepReady` on
https://github.com/ionic-team/ionic-framework/blob/01fc9b45116f7ad6ddc56c7fb1535dec798c2b3a/core/src/components/popover/popover.tsx#L477
we are waiting for the popover to be hydrated, not the child content.
The popover was already hydrated on page load, so this resolves
immediately. However, the child content that was just added to the DOM
has not yet been hydrated, so we aren’t waiting long enough.

This is happening because we return `BaseComponent `from
`attachComponent` which is a reference to the overlay:
https://github.com/ionic-team/ionic-framework/blob/01fc9b45116f7ad6ddc56c7fb1535dec798c2b3a/core/src/utils/framework-delegate.ts#L133

Other framework delegates return the actual child content:

- Core delegate with controller:
https://github.com/ionic-team/ionic-framework/blob/01fc9b45116f7ad6ddc56c7fb1535dec798c2b3a/core/src/utils/framework-delegate.ts#L35
(this is part of why the controller popover works but the inline popover
does not)
- React delegate:
https://github.com/ionic-team/ionic-framework/blob/01fc9b45116f7ad6ddc56c7fb1535dec798c2b3a/packages/react/src/framework-delegate.tsx#L31
- Vue delegate:
https://github.com/ionic-team/ionic-framework/blob/01fc9b45116f7ad6ddc56c7fb1535dec798c2b3a/packages/vue/src/framework-delegate.ts#L45

2. `attachComponent` is unable to return the correct element currently
because the child content has not been mounted yet in this scenario.
`ionMount` is emitted after `attachComponent` resolves:
https://github.com/ionic-team/ionic-framework/blob/01fc9b45116f7ad6ddc56c7fb1535dec798c2b3a/core/src/components/popover/popover.tsx#L466

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

- `ionMount` is emitted before `attachComponent` runs
- `attachComponent` now consistently returns the child view if present
in the DOM
- Added a test

## Does this introduce a breaking change?

- [ ] Yes
- [x] No

<!-- If this introduces a breaking change, please describe the impact
and migration path for existing applications below. -->


## 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.3.2-dev.11693321763.15a54694`

---------

Co-authored-by: ionitron <[email protected]>
  • Loading branch information
liamdebeasi and Ionitron authored Aug 31, 2023
1 parent cddefd1 commit 2a80eb6
Show file tree
Hide file tree
Showing 8 changed files with 143 additions and 5 deletions.
8 changes: 7 additions & 1 deletion core/src/components/modal/modal.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -447,10 +447,16 @@ export class Modal implements ComponentInterface, OverlayInterface {
this.currentBreakpoint = this.initialBreakpoint;

const { inline, delegate } = this.getDelegate(true);
this.usersElement = await attachComponent(delegate, el, this.component, ['ion-page'], this.componentProps, inline);

/**
* Emit ionMount so JS Frameworks have an opportunity
* to add the child component to the DOM. The child
* component will be assigned to this.usersElement below.
*/
this.ionMount.emit();

this.usersElement = await attachComponent(delegate, el, this.component, ['ion-page'], this.componentProps, inline);

/**
* When using the lazy loaded build of Stencil, we need to wait
* for every Stencil component instance to be ready before presenting
Expand Down
10 changes: 8 additions & 2 deletions core/src/components/popover/popover.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -449,6 +449,14 @@ export class Popover implements ComponentInterface, PopoverInterface {
const { el } = this;

const { inline, delegate } = this.getDelegate(true);

/**
* Emit ionMount so JS Frameworks have an opportunity
* to add the child component to the DOM. The child
* component will be assigned to this.usersElement below.
*/
this.ionMount.emit();

this.usersElement = await attachComponent(
delegate,
el,
Expand All @@ -463,8 +471,6 @@ export class Popover implements ComponentInterface, PopoverInterface {
}
this.configureDismissInteraction();

this.ionMount.emit();

/**
* When using the lazy loaded build of Stencil, we need to wait
* for every Stencil component instance to be ready before presenting
Expand Down
51 changes: 51 additions & 0 deletions core/src/components/popover/test/async/index.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
<!DOCTYPE html>
<html lang="en" dir="ltr">
<head>
<meta charset="UTF-8" />
<title>Popover - Async</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 type="module" src="../../../../../dist/ionic/ionic.esm.js"></script>
<style>
ion-popover {
--width: fit-content;
}

.wrapper {
display: flex;
justify-content: center;
align-items: end;

height: 100%;
}
</style>
</head>
<body>
<ion-content class="ion-padding">
<div class="wrapper">
<button id="button">Open Popover</button>
</div>

<ion-popover trigger="button"></ion-popover>
</ion-content>

<script>
const popover = document.querySelector('ion-popover');

popover.addEventListener('ionMount', () => {
popover.innerHTML = `
<div style="padding: 10px;">
<ion-list>
<ion-item>Item 1</ion-item>
</ion-list>
</div>
`;
});
</script>
</body>
</html>
54 changes: 54 additions & 0 deletions core/src/components/popover/test/async/popover.e2e.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
import { expect } from '@playwright/test';
import { configs, test } from '@utils/test/playwright';

/**
* This behavior does not vary across modes/directions
*/
configs({ modes: ['ios'], directions: ['ltr'] }).forEach(({ title, screenshot, config }) => {
test.describe(title('popover: alignment with async component'), async () => {
test('should align popover centered with button when component is added async', async ({ page }) => {
await page.goto('/src/components/popover/test/async', config);

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

const button = page.locator('#button');
await button.click();

await ionPopoverDidPresent.next();

await expect(page).toHaveScreenshot(screenshot(`popover-async`));
});
/**
* Framework delegate should fall back to returning the host
* component when no child content is passed otherwise
* the overlay will get stuck when trying to re-present.
*/
test('should open popover even if nothing was passed', async ({ page }) => {
await page.setContent(
`
<ion-popover></ion-popover>
`,
config
);

const popover = page.locator('ion-popover');
const ionPopoverDidPresent = await page.spyOnEvent('ionPopoverDidPresent');
const ionPopoverDidDismiss = await page.spyOnEvent('ionPopoverDidDismiss');

await popover.evaluate((el: HTMLIonPopoverElement) => el.present());

await ionPopoverDidPresent.next();
await expect(popover).toBeVisible();

await popover.evaluate((el: HTMLIonPopoverElement) => el.dismiss());

await ionPopoverDidDismiss.next();
await expect(popover).toBeHidden();

await popover.evaluate((el: HTMLIonPopoverElement) => el.present());

await ionPopoverDidPresent.next();
await expect(popover).toBeVisible();
});
});
});
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
25 changes: 23 additions & 2 deletions core/src/utils/framework-delegate.ts
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ export const CoreDelegate = () => {
cssClasses: string[] = []
) => {
BaseComponent = parentElement;
let ChildComponent;
/**
* If passing in a component via the `component` props
* we need to append it inside of our overlay component.
Expand Down Expand Up @@ -87,6 +88,8 @@ export const CoreDelegate = () => {
*/
BaseComponent.appendChild(el);

ChildComponent = el;

await new Promise((resolve) => componentOnReady(el, resolve));
} else if (
BaseComponent.children.length > 0 &&
Expand All @@ -96,7 +99,7 @@ export const CoreDelegate = () => {
* The delegate host wrapper el is only needed for modals and popovers
* because they allow the dev to provide custom content to the overlay.
*/
const root = BaseComponent.children[0] as HTMLElement;
const root = (ChildComponent = BaseComponent.children[0] as HTMLElement);
if (!root.classList.contains('ion-delegate-host')) {
/**
* If the root element is not a delegate host, it means
Expand All @@ -111,6 +114,13 @@ export const CoreDelegate = () => {
el.append(...BaseComponent.children);
// Append the new parent element to the original parent element.
BaseComponent.appendChild(el);

/**
* Update the ChildComponent to be the
* newly created div in the event that one
* does not already exist.
*/
ChildComponent = el;
}
}

Expand All @@ -130,7 +140,18 @@ export const CoreDelegate = () => {

app.appendChild(BaseComponent);

return BaseComponent;
/**
* We return the child component rather than the overlay
* reference itself since modal and popover will
* use this to wait for any Ionic components in the child view
* to be ready (i.e. componentOnReady) when using the
* lazy loaded component bundle.
*
* However, we fall back to returning BaseComponent
* in the event that a modal or popover is presented
* with no child content.
*/
return ChildComponent ?? BaseComponent;
};

const removeViewFromDom = () => {
Expand Down

0 comments on commit 2a80eb6

Please sign in to comment.