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.

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

bug: progressEnd callback fires after user callbacks #28393

Closed
3 tasks done
liamdebeasi opened this issue Oct 20, 2023 · 2 comments
Closed
3 tasks done

bug: progressEnd callback fires after user callbacks #28393

liamdebeasi opened this issue Oct 20, 2023 · 2 comments
Labels
package: core @ionic/core package type: bug a confirmed bug report

Comments

@liamdebeasi
Copy link
Contributor

liamdebeasi commented Oct 20, 2023

Prerequisites

Ionic Framework Version

v6.x, v7.x

Current Behavior

The internal onFinish callback set at the end of a gesture in progressEnd can incorrectly fire after user onFinish callbacks fire: https://github.com/ionic-team/ionic-framework/blob/331c08aad542de158e53ed351705d4c396bb4e90/core/src/utils/animation/animation.ts#L830C4-L837

If a user chooses to update the keyframes in an onFinish callback this can cause the direction to be incorrect because we force the duration to either normal or reverse for progressEnd. This forced value allows us to play the animation to either the beginning or the end when a gesture ends (for example, the card modal can either be swiped to close or swipe and then bounce back to its opened state). We clear that forced value, but since the callback that does so runs after the user callback, it's possible to get a stale direction state.

Expected Behavior

I expect any internal value coercions to be reset when the animation ends before any user code runs.

Steps to Reproduce

  1. Open https://stackblitz.com/edit/angular-bux16w?file=src%2Fapp%2Fexample.component.ts
  2. Open the console.
  3. Drag the card along the track and then release it. The card will animate back to the beginning.
  4. Observe that when the user onFinish callback fires the direction is registered as "reverse" but it is registered as "normal" if run in a setTimeout.

The negative impact this has can be seen in the following reproduction:

  1. Open https://stackblitz.com/edit/angular-bux16w-ucfalw?file=src%2Fapp%2Fexample.component.ts
  2. Drag the card along the track and then release it. The card will animation back to the beginning and then update its keyframes to animate the background from green to red.
  3. Observe that when we call progressStart(true, 0) this jumps the animation back to the beginning. However, since the animation's direction is still forced to "reverse" the card shows red since that keyframe is technically the first keyframe now.
  4. Observe that calling progressStart again after a delay (see the setTimeout) the animation correctly jumps to the green keyframe since the coerced direction has been reset.
Screen.Recording.2023-10-20.at.4.24.30.PM.mov

Code Reproduction URL

No response

Ionic Info

N/A

Additional Information

No response

@liamdebeasi
Copy link
Contributor Author

Fixed in #28394.

liamdebeasi added a commit that referenced this issue Oct 23, 2023
Issue number: resolves #28393

---------

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

Our animation library's value coercion is not reset before developer
`onFinish` callbacks fire. This can lead to developers getting incorrect
state when querying for `getDuration` or `getDirection`

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

- Internal value coercion is reset before developer `onFinish` callbacks
fire.

## Does this introduce a breaking change?

- [ ] Yes
- [x] No

I'm putting this in a minor release to minimize risk. This is not a
breaking change, but there may be developers relying on this broken
behavior to implement a workaround.

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

Note: This change is needed for the toast swipe to dismiss feature
(FW-2004)
Copy link

ionitron-bot bot commented Nov 22, 2023

Thanks for the issue! This issue is being locked to prevent comments that are not relevant to the original issue. If this is still an issue with the latest version of Ionic, please create a new issue and ensure the template is fully filled out.

@ionitron-bot ionitron-bot bot locked and limited conversation to collaborators Nov 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
package: core @ionic/core package type: bug a confirmed bug report
Projects
None yet
Development

No branches or pull requests

1 participant