Skip to content
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
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
79 changes: 41 additions & 38 deletions core/src/components/popover/popover.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -27,12 +27,7 @@ import { iosEnterAnimation } from './animations/ios.enter';
import { iosLeaveAnimation } from './animations/ios.leave';
import { mdEnterAnimation } from './animations/md.enter';
import { mdLeaveAnimation } from './animations/md.leave';
import {
configureDismissInteraction,
configureKeyboardInteraction,
configureTriggerInteraction,
waitOneFrame,
} from './utils';
import { configureDismissInteraction, configureKeyboardInteraction, configureTriggerInteraction } from './utils';

/**
* @virtualProp {"ios" | "md"} mode - The mode determines which platform styles to use.
Expand Down Expand Up @@ -464,40 +459,48 @@ export class Popover implements ComponentInterface, PopoverInterface {
}
this.configureDismissInteraction();

// TODO: FW-2773: Apply this to only the lazy build.
/**
* ionMount only needs to be emitted if the popover is inline.
*/
this.ionMount.emit();
/**
* Wait one raf before presenting the popover.
* This allows the lazy build enough time to
* calculate the popover dimensions for the animation.
*/
await waitOneFrame();

this.currentTransition = present(this, 'popoverEnter', iosEnterAnimation, mdEnterAnimation, {
event: event || this.event,
size: this.size,
trigger: this.triggerEl,
reference: this.reference,
side: this.side,
align: this.alignment,
});

await this.currentTransition;

this.currentTransition = undefined;

/**
* If popover is nested and was
* presented using the "Right" arrow key,
* we need to move focus to the first
* descendant inside of the popover.
*/
if (this.focusDescendantOnPresent) {
focusFirstDescendant(this.el, this.el);
}
return new Promise((resolve) => {
/**
* Wait two request animation frame loops before presenting the popover.
* This allows the framework implementations enough time to mount
* the popover contents, so the bounding box is set when the popover
* transition starts.
*
* On Angular and React, a single raf is enough time, but for Vue
* we need to wait two rafs. As a result we are using two rafs for
* all frameworks to ensure the popover is presented correctly.
*/
raf(() => {
raf(async () => {
Comment on lines +475 to +476
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to wait 2 frames just in Ionic Vue?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Short answer, I'm not sure. I tested with a single request animation frame delay and compared against each and only Vue did not render the inner contents in time for the transition. Bumping to two consistently rendered correctly. I'm not familiar to the internals of how Vue handles rendering.

My intention with this bug fix is looking at a target for a patch release, but realizing we should be able to rip most/all of this away in a minor or major release, if we change either or both:

  1. The animation library to support asynchronous behavior.
  2. Add support to overlays to accurately track when content is mounted before running the transition.

this.currentTransition = present(this, 'popoverEnter', iosEnterAnimation, mdEnterAnimation, {
event: event || this.event,
size: this.size,
trigger: this.triggerEl,
reference: this.reference,
side: this.side,
align: this.alignment,
});

await this.currentTransition;

this.currentTransition = undefined;

/**
* If popover is nested and was
* presented using the "Right" arrow key,
* we need to move focus to the first
* descendant inside of the popover.
*/
if (this.focusDescendantOnPresent) {
focusFirstDescendant(this.el, this.el);
}

resolve();
});
});
});
}

/**
Expand Down
4 changes: 0 additions & 4 deletions core/src/components/popover/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -928,7 +928,3 @@ export const shouldShowArrow = (side: PositionSide, didAdjustBounds = false, ev?

return true;
};

export const waitOneFrame = () => {
return new Promise<void>((resolve) => raf(() => resolve()));
};
11 changes: 11 additions & 0 deletions packages/react/src/components/createInlineOverlayComponent.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,17 @@ export const createInlineOverlayComponent = <PropType, ElementType>(
componentDidMount() {
this.componentDidUpdate(this.props);

/**
* Mount the inner component when the
* overlay is about to open.
*
* For ion-popover, this is when `ionMount` is emitted.
* For other overlays, this is when `willPresent` is emitted.
*/
this.ref.current?.addEventListener('ionMount', () => {
this.setState({ isOpen: true });
});

/**
* Mount the inner component
* when overlay is about to open.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -83,4 +83,4 @@ const PopoverComponent: React.FC = () => {
);
};

export default PopoverComponent;
export default PopoverComponent;
1 change: 1 addition & 0 deletions packages/vue/src/vue-component-lib/overlays.ts
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,7 @@ export const defineOverlayContainer = <Props extends object>(name: string, defin
const elementRef = ref();

onMounted(() => {
elementRef.value.addEventListener('ion-mount', () => isOpen.value = true);
elementRef.value.addEventListener('will-present', () => isOpen.value = true);
elementRef.value.addEventListener('did-dismiss', () => isOpen.value = false);
});
Expand Down