Skip to content
This repository has been archived by the owner on May 29, 2019. It is now read-only.

Fix for animating the modal-backdrop #1267

Closed
wants to merge 1 commit into from
Closed

Fix for animating the modal-backdrop #1267

wants to merge 1 commit into from

Conversation

plainkeyman
Copy link
Contributor

When using CSS transitions, I tried to animate the modal-backdrop class to fade opacity. This worked, however when launching modal later, the "fade in" would already be applied. Setting animate to false would prevent this.

angular version: 1.2.0-rc.3

…ce. Changing this allowed it to animate for every modal
@pkozlowski-opensource
Copy link
Member

@plainkeyman thnx for taking time to open this PR but it can't be merged as - is. For all the fixes we require a unit tests that demonstrates the issue (a test should fail without a fix and pass when a fix is applied). If you need assistance with writing a test we would need, as a bare minimum, a plunker that illustrates an issue. One more thing - if this is only RC3-specific we won't fix it.

@plainkeyman
Copy link
Contributor Author

Thank you for looking at it - I'm trying to figure out how to test it. I don't see any tests for $modalStack. I think $modalStack might be difficult to test because it's holding onto state.

The problem seems to be because the $modalStack is reusing the backdropScope. Animate is set to true via the directive, however it never gets set to false after closing the last modal.

                if (backdropIndex() >= 0 && !backdropDomEl) {
                    backdropjqLiteEl = angular.element('<div modal-backdrop></div>');
                    backdropDomEl = $compile(backdropjqLiteEl)(backdropScope);  //****** scope still has animate == true
                    body.append(backdropDomEl);
                }

I think a better solution (than what I sent yesterday) would be to reset the backdropScope inside this code:

                //remove backdrop if no longer needed
                if (backdropIndex() == -1) {
                    backdropDomEl.remove();
                    backdropDomEl = undefined;
                    backdropScope = $rootScope.$new(true); // ****** reset scope
                }

@plainkeyman
Copy link
Contributor Author

I can look at making a modalStack.spec.js

@pkozlowski-opensource
Copy link
Member

@plainkeyman you can test on a highier level of the $modal service - just have a look at the existing tests. But I'm still not sure if I understand the underlying issue so a minimal plunk would help here.

@plainkeyman
Copy link
Contributor Author

Here's a plunker that shows the issue: http://plnkr.co/XHuoWH

Notice I've exaggerated the modal-backdrop fade in so you can see the issue. On first launch, it runs ok. Launch it again, and the animation doesn't fade anymore -- you have to reload the page.

If you uncomment the script to see my modified version - it will perform the fade each successive launch.

here is the change:

        //remove backdrop if no longer needed
        if (backdropIndex() == -1) {
          backdropDomEl.remove();
          backdropDomEl = undefined;

          /******* modified ******/
          backdropScope.$destroy();
          backdropScope = $rootScope.$new(true);
          /******* /modified ******/
        }

@pkozlowski-opensource
Copy link
Member

@plainkeyman sorry for the delay, I was finally able to have a closer look into this issue and you are completely right! Thnx for taking time to provide the fix and explain the underlying issue!

@plainkeyman
Copy link
Contributor Author

@pkozlowski-opensource actually, thank you for taking the time to look at it, and for making such an awesome library!

@ts46235
Copy link

ts46235 commented Mar 20, 2014

I am upgrading from 0.4.0 and now the modals always animate. Is there some way to turn that off so that they just appear and hide?

@ts46235
Copy link

ts46235 commented Mar 20, 2014

I think maybe what controlled that was dialogFade/backdropFade which don't appear as options anymore

@plainkeyman
Copy link
Contributor Author

I would say this fix was to animate the backdrop... not really the modal itself.

However both the backdrop as well as the modal itself can be controlled via css:

.modal.fade{-webkit-transition:none;}

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

Successfully merging this pull request may close these issues.

3 participants