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

Popover: close(), then open() doesn't work when animation is enabled #3918

Open
Skalman opened this issue Dec 22, 2020 · 3 comments
Open

Popover: close(), then open() doesn't work when animation is enabled #3918

Skalman opened this issue Dec 22, 2020 · 3 comments

Comments

@Skalman
Copy link

Skalman commented Dec 22, 2020

Bug description:

If animation is enabled, popover.close() and then popover.open() fails to keep the popover open.

popover.close();
popover.open(); // the popover should stay open here - but it stays closed

Link to minimally-working StackBlitz that reproduces the issue:

https://stackblitz.com/edit/angular-q3hazv?file=src%2Fapp%2Fpopover-triggers.html

Versions of Angular, ng-bootstrap and Bootstrap:

Angular: 10.0.14

ng-bootstrap: 8.0.0

Bootstrap: 4.5.0

Discussion

Why this is a problem

It's a regression from previous version.

You can't call .open() twice with different contexts - you need the .close() in between.

Mitigation

Turn off animation

@maxokorokov
Copy link
Member

I wouldn't say it's a regression as there were no animations before ;)

With animations you have to decide what to to with already running transition on the element in case of imperative calls like .open() and .close().

It looks to me it boils down to this code we have in popup.ts (so not only popover):

open() {
    ngbRunTransition(..., {runningTransition: 'continue'});
}

close() {
    ngbRunTransition(..., {runningTransition: 'stop'});
}

Looks like it was a choice to do it this way:

  • .open() does nothing if there is a transition running, otherwise runs a new opening transition
  • .close() stops currently running transition on an element and runs a new closing transition

It's a good point about calling .open(ctx) with different contexts.
But what would you expect to happen if you open with ctx2 while in the middle of animating opening with ctx1?

Bootstrap, for example, ignores any calls while opening/closing transitions are running.

I can't recollect why it was done like this at the first place, @fbasso, do you have any comments on this?

@fbasso
Copy link
Member

fbasso commented Jan 12, 2021

I think this decision has been taken because of a UX design:

  • A user can click outside a popup while opening: it will stop the opening animation for the closing one
  • A user close the popup: there is no action on his side to be done to reopen the popup during the closing animation. The same behavior as an alert for example.

I don't know if it was the right assumption to do, but as far as I remember, it was the reason.

@Skalman
Copy link
Author

Skalman commented Feb 13, 2021

(Sorry for the long delay)

@maxokorokov:

But what would you expect to happen if you open with ctx2 while in the middle of animating opening with ctx1?

I'd expect ctx1 to no longer be part of any popover. If you need to abruptly close ctx1 to do this, that would also be okay.
And then I'd expect ctx2 to be used.

(I'd call it a regression: closing and opening in succession worked previously by default, but no longer works without changing settings. Not that the categorization is important 😄)

@fbasso:

I think this decision has been taken because of a UX design:

I'm not sure I understand. You don't mention anything about opening, which apparently is the operation that has the issue.

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

No branches or pull requests

3 participants