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

Dismissing one of stacked modals dismisses keyboard/backdrop events for all modals #3515

Closed
kshitizsingh93 opened this issue Dec 14, 2019 · 5 comments · Fixed by #3532
Closed

Comments

@kshitizsingh93
Copy link

Bug description:

When using stacked modals to prevent dismissal of first level modal, dismissing the second level modal by keyboard/backdrop disables the keyboard/backdrop for first modal too.

Link to minimally-working StackBlitz that reproduces the issue:

https://stackblitz.com/edit/angular-dhs8oz

Versions of Angular, ng-bootstrap and Bootstrap:

Angular: 8.2.14

ng-bootstrap: 5.1.4

Bootstrap: 4.4.1

@masoudDaliriyan
Copy link

I guess this behavior because of the returned Promise in modalService.open don't resolve when dismissing modal, thus first opened modal can't be closed

@pkozlowski-opensource @maxokorokov
I don't know why in components/modal/modal-ref.ts the promise is resolved when the modal is closed and rejected when the modal is dismissed.

I changed to resolve the promise when is dismissing in components/modal/modal-ref.ts at _dismiss method and the bug going to fixed

I try to make PR for this issue

@kshitizsingh93
Copy link
Author

I think promise rejection is the intended way to dismiss a modal, so we can distinguish between a negative result and no result. The first opened modal can still be dismissed by calling activeModal.dismiss.

My guess was the keyboard and backdrop event listeners are detached before 'beforeDismiss' is called.

@benouat
Copy link
Member

benouat commented Dec 20, 2019

I don't think we do anything special about keyboard events in here. We rely on rxjs fromEvent, so we don't detach anything before beforeDismiss is called.... weird o_O

@kshitizsingh93
Copy link
Author

I tinkered a little bit more and discovered that you can dismiss the first modal by calling activeModal.dismiss() and dismiss the second modal by keyboard/backdrop any number of times before this bug appears, this only happens once you dismiss the first modal by keyboard/backdrop.

I haven't looked under the hood but shouldn't that point to slightly different implementation of the keyboard/backdrop events?

@eddietisma
Copy link

eddietisma commented Dec 25, 2019

I have this bug too, also with the scenario of a confirmation modal. Had to reverted to an earlier version.

I'm pretty sure it has to do with this change. More precisely with unsubscribing the dismissEvent using takeUntil - disregarding whether it returned true or not.

@maxokorokov maxokorokov added this to the 5.1.5 milestone Jan 7, 2020
maxokorokov added a commit to maxokorokov/ng-bootstrap that referenced this issue Jan 7, 2020
At the moment key/mouse handlers are removed when modal triggers dismiss event. The problem is that modal closure could be cancelled with `beforeDismiss` and modal will stay opened, but handlers will be removed.

This commit introduces `closed$` event triggered inside `onDestroy` to clean up handlers.

Fixes ng-bootstrap#3515
maxokorokov added a commit to maxokorokov/ng-bootstrap that referenced this issue Jan 7, 2020
At the moment key/mouse handlers are removed when modal triggers dismiss event. The problem is that modal closure could be cancelled with `beforeDismiss` and modal will stay opened, but handlers will be removed.

This commit introduces `closed$` event triggered inside `onDestroy` to clean up handlers.

Fixes ng-bootstrap#3515
maxokorokov added a commit that referenced this issue Jan 7, 2020
…3532)

At the moment key/mouse handlers are removed when modal triggers dismiss event. The problem is that modal closure could be cancelled with `beforeDismiss` and modal will stay opened, but handlers will be removed.

This commit introduces `closed$` event triggered inside `onDestroy` to clean up handlers.

Fixes #3515
maxokorokov added a commit that referenced this issue Jan 7, 2020
…3532)

At the moment key/mouse handlers are removed when modal triggers dismiss event. The problem is that modal closure could be cancelled with `beforeDismiss` and modal will stay opened, but handlers will be removed.

This commit introduces `closed$` event triggered inside `onDestroy` to clean up handlers.

Fixes #3515
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants