Skip to content
This repository was archived by the owner on Sep 5, 2024. It is now read-only.

Conversation

crisbeto
Copy link
Member

The menu backdrop wasn't being cleaned up, if the menu got destroyed while it was animating.

Fixes #8727.

@crisbeto crisbeto added the needs: review This PR is waiting on review from the team label Jun 14, 2016
@crisbeto crisbeto added in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs needs: review This PR is waiting on review from the team and removed needs: review This PR is waiting on review from the team in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs labels Jun 14, 2016
@crisbeto
Copy link
Member Author

@devversion Could you give me some feedback on this since you were dealing with the interim elements?

@ThomasBurleson ThomasBurleson added this to the 1.1.0 milestone Jun 15, 2016
@@ -60,13 +60,19 @@ function MenuProvider($$interimElementProvider) {
$animate.enter(options.backdrop, $document[0].body);
}

// Handles cases where the menu was destroyed, while it was animating.
var destroyListener = scope.$on('$destroy', hideBackdrop);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is quite unnormal to have a destroy listener inside of the interims onShow method.

@devversion
Copy link
Member

@crisbeto Having a destroy listener here is not the correct solution.

Once the interim Element will be destroyed, the onRemove event will be called as well.
Also with the option options.$destroy (you don't need it for this case)

At #8727, an error will be thrown inside of a promise, which leads to a cancellation of the onRemove method.

The error will be thrown, because the cleanupInteraction method is not defined before the animation completes.

The fix would be

    function onRemove(scope, element, opts) {
      opts.cleanupInteraction && opts.cleanupInteraction();

@crisbeto crisbeto added needs: work and removed needs: review This PR is waiting on review from the team labels Jun 15, 2016
@crisbeto
Copy link
Member Author

Thanks, I'll give it another shot. If I remember correctly, the onRemove wasn't firing yesterday.

@devversion
Copy link
Member

Sounds good! Yeah it will be triggered, I guess you checked the execution after the Runtime Exception.

The menu backdrop wasn't being cleaned up, if the menu got destroyed while it was animating.

Fixes angular#8727.
@crisbeto crisbeto force-pushed the 8727/menu-backdrop branch from 38e884a to dc2c553 Compare June 15, 2016 17:35
@crisbeto crisbeto added needs: review This PR is waiting on review from the team and removed needs: work labels Jun 15, 2016
@crisbeto
Copy link
Member Author

That null check for the cleanupInteraction worked fine for the codepen from #8727 @devversion, but it didn't help when destroying the scope programmatically in the unit test. I also had to call the remove method in the directive's destroy listener.

@devversion
Copy link
Member

devversion commented Jun 15, 2016

@crisbeto Yeah, you're actually using the fix, because you're triggering the interimElements destroy function, which will come to the cleanupInteraction fix.

This is definitely the perfect solution. LGTM!

@ThomasBurleson ThomasBurleson added pr: merge ready This PR is ready for a caretaker to review and removed needs: review This PR is waiting on review from the team labels Jun 18, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
pr: merge ready This PR is ready for a caretaker to review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants