Skip to content

Commit

Permalink
fix(overlays): tear down animations after dismiss (#28907)
Browse files Browse the repository at this point in the history
Issue number: resolves #28352

---------

<!-- 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. -->
Not all animations are getting properly destroyed after they run. This
means that styles can get stuck at the end value of their animation.

Specifically for this reproduction (as reported in the bug ticket), if
the modal animates from 1 to 0 opacity and then the modal gets reopened
with a different animation where the opacity should be 1 throughout
(i.e. the opacity isn't supposed to animate at all), the modal is
invisible because the opacity got stuck at 0 and never got reset to the
default value of 1.

This bug is probably causing some incorrect behavior on other edge cases
with overlays, but this is the only one I've identified.

### Reproduction steps
Note: you cannot reproduce this when using a modalController

1. Open a modal, e.g. [this
one](http://localhost:3333/src/components/modal/test/card-nav?ionic:mode=ios)
in `ios` mode on a screen wider than 768px
1. Close the modal
1. Open the same modal on a screen narrower than 768px
1. See that the modal does not appear

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

- Animations are properly destroyed after the animation completes
- The modal now appears as expected after following the reproduction
steps above

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

---------

Co-authored-by: Liam DeBeasi <[email protected]>
  • Loading branch information
mapsandapps and liamdebeasi authored Jan 31, 2024
1 parent d5dc901 commit 950fa40
Show file tree
Hide file tree
Showing 3 changed files with 51 additions and 5 deletions.
5 changes: 0 additions & 5 deletions core/src/components/modal/modal.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import { Style as StatusBarStyle, StatusBar } from '@utils/native/status-bar';
import {
GESTURE,
BACKDROP,
activeAnimations,
dismiss,
eventMethod,
prepareOverlay,
Expand Down Expand Up @@ -705,8 +704,6 @@ export class Modal implements ComponentInterface, OverlayInterface {
this.keyboardOpenCallback = undefined;
}

const enteringAnimation = activeAnimations.get(this) || [];

const dismissed = await dismiss<ModalDismissOptions>(
this,
data,
Expand All @@ -733,8 +730,6 @@ export class Modal implements ComponentInterface, OverlayInterface {
if (this.gesture) {
this.gesture.destroy();
}

enteringAnimation.forEach((ani) => ani.destroy());
}
this.currentBreakpoint = undefined;
this.animation = undefined;
Expand Down
45 changes: 45 additions & 0 deletions core/src/components/modal/test/animations/modal.e2e.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
import { expect } from '@playwright/test';
import { configs, test } from '@utils/test/playwright';

configs({ modes: ['ios'], directions: ['ltr'] }).forEach(({ config, title }) => {
test.describe(title('modal: animations'), () => {
test.beforeEach(async ({ page }) => {
await page.setContent(
`
<ion-modal is-open="true" trigger="open-modal"></ion-modal>
`,
config
);
});
test('card modal should clean up animations on dismiss', async ({ page }, testInfo) => {
testInfo.annotations.push({
type: 'issue',
description: 'https://github.com/ionic-team/ionic-framework/issues/28352',
});

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

const modal = page.locator('ion-modal');

const initialAnimations = await modal.evaluate((el: HTMLIonModalElement) => {
return el.shadowRoot!.getAnimations();
});

// While the modal is open, it should have animations
expect(initialAnimations.length).toBeGreaterThan(0);

await modal.evaluate((el: HTMLIonModalElement) => {
el.dismiss();
});

await ionModalDidDismiss.next();

const currentAnimations = await modal.evaluate((el: HTMLIonModalElement) => {
return el.shadowRoot!.getAnimations();
});

// Once the modal has finished closing, there should be no animations
expect(currentAnimations.length).toBe(0);
});
});
});
6 changes: 6 additions & 0 deletions core/src/utils/overlays.ts
Original file line number Diff line number Diff line change
Expand Up @@ -577,6 +577,12 @@ export const dismiss = async <OverlayDismissOptions>(
overlay.didDismiss.emit({ data, role });
overlay.didDismissShorthand?.emit({ data, role });

// Get a reference to all animations currently assigned to this overlay
// Then tear them down to return the overlay to its initial visual state
const animations = activeAnimations.get(overlay) || [];

animations.forEach((ani) => ani.destroy());

activeAnimations.delete(overlay);

/**
Expand Down

0 comments on commit 950fa40

Please sign in to comment.