ionic-team / ionic-framework

A powerful cross-platform UI toolkit for building native-quality iOS, Android, and Progressive Web Apps with HTML, CSS, and JavaScript.
https://ionicframework.com
MIT License
50.93k stars 13.52k forks source link

bug: dismiss callback from useIonModal is not called after first dismiss #28964

Open MattiaGeccheleAxa opened 7 months ago

MattiaGeccheleAxa commented 7 months ago

Prerequisites

Ionic Framework Version

v7.x

Current Behavior

When the canDismiss callback returns false the overlay layer is dismissed and the user can interact again with the modal. If the user tries to close it again the canDismiss callback callback is no longer called.

Screenshot 2024-02-02 at 17 27 57 Screenshot 2024-02-02 at 17 23 00

The Modal is opened in this way:

present({ canDismiss: canDismiss})

Expected Behavior

If the user does not wish to close the modal, canDismiss must return false. But the user should be allowed to close the modal at a later time.

Steps to Reproduce

  1. Open an IonModal using the hook useIonModal
  2. Provide a canDismiss function which returns resolve(false) if the user doesn't want to close the modal
  3. Close the modal using the dimissfunction
  4. Refuse to close the modal and the overlay layer will disappear
  5. Call the dismiss again
  6. Nothing happens

Code Reproduction URL

No response

Ionic Info

I have an error running it linked to the macOS: TypeError: undefined is not iterable (cannot read property Symbol(Symbol.iterator))

Additional Information

No response

ionitron-bot[bot] commented 7 months ago

Thanks for the issue! This issue has been labeled as needs reproduction. This label is added to issues that need a code reproduction.

Please reproduce this issue in an Ionic starter application and provide a way for us to access it (GitHub repo, StackBlitz, etc). Without a reliable code reproduction, it is unlikely we will be able to resolve the issue, leading to it being closed.

If you have already provided a code snippet and are seeing this message, it is likely that the code snippet was not enough for our team to reproduce the issue.

For a guide on how to create a good reproduction, see our Contributing Guide.

mattiaGec commented 7 months ago

Here the github repo https://github.com/mattiaGec/IonicModalTest

MattiaGeccheleAxa commented 7 months ago

https://github.com/ionic-team/ionic-framework/assets/121299562/bfeb6a94-341a-4f8c-aca8-2f3f71499681

sean-perkins commented 7 months ago

Hello @mattiaGec thank you for reporting this issue!

I can confirm the problematic behavior.

When the dismiss callback returned by useIonModal is called multiple times, it only attempts to dismiss the modal the first time it is called.

This is due to the implementation for the dismiss method in useOverlay: https://github.com/ionic-team/ionic-framework/blob/35ab6b4816bd627239de8d8b25ce0c86db8c74b4/packages/react/src/hooks/useOverlay.ts#L86-L90

The specific problem is this line of code: https://github.com/ionic-team/ionic-framework/blob/35ab6b4816bd627239de8d8b25ce0c86db8c74b4/packages/react/src/hooks/useOverlay.ts#L88 We should only be clearing the overlay reference if the overlay actually dismissed, by looking at the return value of overlayRef.current.dismiss. If the overlay did not dismiss, we should not clear the reference.

I will classify this as a bug and the team can prioritize it within the backlog. If anyone would like to submit a PR for this, I would gladly welcome the contribution!

This would be the proposed change that I would validate with testing:

const dismiss = useCallback(async (data?: any, role?: string) => {
  if (overlayRef.current) {
    const didDismiss = await overlayRef.current.dismiss(data, role);
    if (didDismiss) {
      overlayRef.current = undefined;
      containerElRef.current = undefined;
    }
  }
}, []);