angular-ui / bootstrap

PLEASE READ THE PROJECT STATUS BELOW. Native AngularJS (Angular) directives for Bootstrap. Smaller footprint (20kB gzipped), no 3rd party JS dependencies (jQuery, bootstrap JS) required. Please read the README.md file before submitting an issue!
http://angular-ui.github.io/bootstrap/
MIT License
14.28k stars 6.73k forks source link

$modal breaks forms b/c of transclusion #969

Closed boneskull closed 10 years ago

boneskull commented 11 years ago

Say I instantiate a modal:

$modal.open({
  controller: 'MyCtrl'
});

In my modal, I have a form:

<form name="myForm">...</form>

Because of transclusion, myForm is not available on MyCtrl's scope and I can't check for $valid, for example. A workaround I've found is to simply put an ng-controller="MyCtrl" within my modal template itself, and do this:

$modal.open({
  controller: function($scope, $modalInstance) {
    $scope.$modalInstance = $modalInstance;
  }
});

...and now I can get to the instance from within MyCtrl.

This is not ideal, but I'm not sure of what the fix should be. Instead of transclusion, would it be a good idea to actually insert an ngController directive within the template/modal/window.html template itself then say, ngTransclude within a child tag? Any better way to tackle this problem?

thanks Chris

georgiosd commented 11 years ago

Chris,

I do exactly the same - have forms in my modals but I'm checking $valid just fine. Can you do a plunker?

damrbaby commented 11 years ago

Just ran into the same issue here... wondering if it's a bug in angular 1.2.0-rc.2

boneskull commented 11 years ago

I'll try to plunk/fiddle this in the next few days.

AGiorgetti commented 11 years ago

Same issue here, I'm going to try create a spiked version of the new $modal that do not use ngTransclude. That's the cause of the creation of the 'unwanted' additional scope here. The scope that gets the validation injected is the one created by the transclusion, and it's a child of you modal controller's scope.

VaclavObornik commented 11 years ago

Same issue: http://plnkr.co/edit/TN37ED4E83XfsI4GrQhA?p=preview

AGiorgetti commented 11 years ago

OK, it seems I have two working patches for this: 1- I changed the way the scopes are created and nested (it uses two different scopes: one to manage the 'dialog specific' properties (animate, index, window-class) and a second tied to the controler of the modal content 2- this one just use one single scope (as previous implementation) and enrich it with an object (named $$modal) which contains the modal specific properties.

both of them do not use the ngTransclude directive in the template, they just insert the modal html content inside the template frame using angular DOM manipulation.

My bro is testing them, if they work well (and if I find how to post patches) I'll upload them. I don't like the idea of positing the big bunch of code right here.

AGiorgetti commented 11 years ago

This is the 'option 2' implementation: the main differences are in the 'modal-window' directive and in the '$modalStack.open' function. Things can be improved to use templates again for the modal frame.

Sorry for the big bunch of code, but I don't know how to submit a patch properly atm.

Update: fixed the code markdown

angular.module('ui.bootstrap.modal', [])

/**
 * A helper, internal data structure that acts as a map but also allows getting / removing
 * elements in the LIFO order
 */
  .factory('$$stackedMap', function () {
      return {
          createNew: function () {
              var stack = [];

              return {
                  add: function (key, value) {
                      stack.push({
                          key: key,
                          value: value
                      });
                  },
                  get: function (key) {
                      for (var i = 0; i < stack.length; i++) {
                          if (key == stack[i].key) {
                              return stack[i];
                          }
                      }
                  },
                  keys: function () {
                      var keys = [];
                      for (var i = 0; i < stack.length; i++) {
                          keys.push(stack[i].key);
                      }
                      return keys;
                  },
                  top: function () {
                      return stack[stack.length - 1];
                  },
                  remove: function (key) {
                      var idx = -1;
                      for (var i = 0; i < stack.length; i++) {
                          if (key == stack[i].key) {
                              idx = i;
                              break;
                          }
                      }
                      return stack.splice(idx, 1)[0];
                  },
                  removeTop: function () {
                      return stack.splice(stack.length - 1, 1)[0];
                  },
                  length: function () {
                      return stack.length;
                  }
              };
          }
      };
  })

/**
 * A helper directive for the $modal service. It creates a backdrop element.
 */
  .directive('modalBackdrop', ['$modalStack', '$timeout', function ($modalStack, $timeout) {
      return {
          restrict: 'EA',
          replace: true,
          templateUrl: 'template/modal/backdrop.html',
          link: function (scope, element, attrs) {

              //trigger CSS transitions
              $timeout(function () {
                  scope.animate = true;
              });

              scope.close = function (evt) {
                  var modal = $modalStack.getTop();
                  if (modal && modal.value.backdrop && modal.value.backdrop != 'static') {
                      evt.preventDefault();
                      evt.stopPropagation();
                      $modalStack.dismiss(modal.key, 'backdrop click');
                  }
              };
          }
      };
  }])

  .directive('modalWindow', ['$timeout', function ($timeout) {
      return {
          restrict: 'EA',
          //scope: {
          //  index: '@'
          //},
          replace: true,
          //transclude: true,
          //templateUrl: 'template/modal/window.html',
          link: function (scope, element, attrs) {
              scope.$$modal = {};
              scope.$$modal.index = attrs.index || 0;
              scope.$$modal.windowClass = attrs.windowClass || '';

              //trigger CSS transitions
              $timeout(function () {
                  scope.$$modal.animate = true;
              });
          }
      };
  }])

  .factory('$modalStack', ['$document', '$compile', '$rootScope', '$$stackedMap',
    function ($document, $compile, $rootScope, $$stackedMap) {

        var backdropjqLiteEl, backdropDomEl;
        var backdropScope = $rootScope.$new(true);
        var body = $document.find('body').eq(0);
        var openedWindows = $$stackedMap.createNew();
        var $modalStack = {};

        function backdropIndex() {
            var topBackdropIndex = -1;
            var opened = openedWindows.keys();
            for (var i = 0; i < opened.length; i++) {
                if (openedWindows.get(opened[i]).value.backdrop) {
                    topBackdropIndex = i;
                }
            }
            return topBackdropIndex;
        }

        $rootScope.$watch(backdropIndex, function (newBackdropIndex) {
            backdropScope.index = newBackdropIndex;
        });

        function removeModalWindow(modalInstance) {

            var modalWindow = openedWindows.get(modalInstance).value;

            //clean up the stack
            openedWindows.remove(modalInstance);

            //remove window DOM element
            modalWindow.modalDomEl.remove();

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

            //destroy scope
            modalWindow.modalScope.$destroy();
        }

        $document.bind('keydown', function (evt) {
            var modal;

            if (evt.which === 27) {
                modal = openedWindows.top();
                if (modal && modal.value.keyboard) {
                    $rootScope.$apply(function () {
                        $modalStack.dismiss(modal.key);
                    });
                }
            }
        });

        $modalStack.open = function (modalInstance, modal) {

            openedWindows.add(modalInstance, {
                deferred: modal.deferred,
                modalScope: modal.scope,
                backdrop: modal.backdrop,
                keyboard: modal.keyboard
            });

            var angularDomEl = angular.element("<div modal-window class=\"modal fade {{ $$modal.windowClass }}\" ng-class=\"{in: $$modal.animate}\" ng-style=\"{'z-index': 1050 + $$modal.index*10}\"></div>");
            angularDomEl.attr('window-class', modal.windowClass);
            angularDomEl.attr('index', openedWindows.length() - 1);
            angularDomEl.html(modal.content);
            var modalDomEl = $compile(angularDomEl)(modal.scope);

            openedWindows.top().value.modalDomEl = modalDomEl;
            body.append(modalDomEl);

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

        $modalStack.close = function (modalInstance, result) {
            var modal = openedWindows.get(modalInstance);
            if (modal) {
                modal.value.deferred.resolve(result);
                removeModalWindow(modalInstance);
            }
        };

        $modalStack.dismiss = function (modalInstance, reason) {
            var modalWindow = openedWindows.get(modalInstance).value;
            if (modalWindow) {
                modalWindow.deferred.reject(reason);
                removeModalWindow(modalInstance);
            }
        };

        $modalStack.getTop = function () {
            return openedWindows.top();
        };

        return $modalStack;
    }])

  .provider('$modal', function () {

      var $modalProvider = {
          options: {
              backdrop: true, //can be also false or 'static'
              keyboard: true
          },
          $get: ['$injector', '$rootScope', '$q', '$http', '$templateCache', '$controller', '$modalStack',
            function ($injector, $rootScope, $q, $http, $templateCache, $controller, $modalStack) {

                var $modal = {};

                function getTemplatePromise(options) {
                    return options.template ? $q.when(options.template) :
                      $http.get(options.templateUrl, { cache: $templateCache }).then(function (result) {
                          return result.data;
                      });
                }

                function getResolvePromises(resolves) {
                    var promisesArr = [];
                    angular.forEach(resolves, function (value, key) {
                        if (angular.isFunction(value) || angular.isArray(value)) {
                            promisesArr.push($q.when($injector.invoke(value)));
                        }
                    });
                    return promisesArr;
                }

                $modal.open = function (modalOptions) {

                    var modalResultDeferred = $q.defer();
                    var modalOpenedDeferred = $q.defer();

                    //prepare an instance of a modal to be injected into controllers and returned to a caller
                    var modalInstance = {
                        result: modalResultDeferred.promise,
                        opened: modalOpenedDeferred.promise,
                        close: function (result) {
                            $modalStack.close(modalInstance, result);
                        },
                        dismiss: function (reason) {
                            $modalStack.dismiss(modalInstance, reason);
                        }
                    };

                    //merge and clean up options
                    modalOptions = angular.extend({}, $modalProvider.options, modalOptions);
                    modalOptions.resolve = modalOptions.resolve || {};

                    //verify options
                    if (!modalOptions.template && !modalOptions.templateUrl) {
                        throw new Error('One of template or templateUrl options is required.');
                    }

                    var templateAndResolvePromise =
                      $q.all([getTemplatePromise(modalOptions)].concat(getResolvePromises(modalOptions.resolve)));

                    templateAndResolvePromise.then(function resolveSuccess(tplAndVars) {

                        var modalScope = (modalOptions.scope || $rootScope).$new();
                        modalScope.$close = modalInstance.close;
                        modalScope.$dismiss = modalInstance.dismiss;

                        var ctrlInstance, ctrlLocals = {};
                        var resolveIter = 1;

                        //controllers
                        if (modalOptions.controller) {
                            ctrlLocals.$scope = modalScope;
                            ctrlLocals.$modalInstance = modalInstance;
                            angular.forEach(modalOptions.resolve, function (value, key) {
                                ctrlLocals[key] = tplAndVars[resolveIter++];
                            });

                            ctrlInstance = $controller(modalOptions.controller, ctrlLocals);
                        }

                        $modalStack.open(modalInstance, {
                            scope: modalScope,
                            deferred: modalResultDeferred,
                            content: tplAndVars[0],
                            backdrop: modalOptions.backdrop,
                            keyboard: modalOptions.keyboard,
                            windowClass: modalOptions.windowClass
                        });

                    }, function resolveError(reason) {
                        modalResultDeferred.reject(reason);
                    });

                    templateAndResolvePromise.then(function () {
                        modalOpenedDeferred.resolve(true);
                    }, function () {
                        modalOpenedDeferred.reject(false);
                    });

                    return modalInstance;
                };

                return $modal;
            }]
      };

      return $modalProvider;
  });
tech-no-logical commented 11 years ago

I've hit this issue as well. trying AGiorgetti's version now, seems to work. thanks !

boneskull commented 11 years ago

Thanks for taking a look.

georgiosd commented 11 years ago

This is really strange. I have several models with ng-model bindings and they all work fine. With 0.6 and NG 1.2 RC1

tech-no-logical commented 11 years ago

this plunk demonstrates my problem :

http://plnkr.co/edit/A0Q8Nh?p=preview

notice how the ng-change doesn't see the 'correct' value when typing in the form-field. I've tried various configurations, with and without passing a $scope to the modal instance, I haven't gotton it to work

damrbaby commented 11 years ago

@georgiosd the ng-model bindings are working for me, but accessing the form by name from the scope (e.g. $scope.myForm) doesn't work.

ProLoser commented 11 years ago

You guys are aware that this ENTIRE bug is just because there's a superfluous childscope being created right?

In other words, if you had ng-model="directScopeProperty" it must be changed to ng-model="$parent.directScopeProperty". If you had ng-model="object.property" then it will continue working.

This is still a bug that should be addressed however, but again, it's just because there's 1 extra child scope being created for some reason.

@AGiorgetti I believe giant pastes like that aren't really helpful in comments.

pkozlowski-opensource commented 11 years ago

Thnx for this comment @ProLoser :-) I will update this issue with more explanations on what is going on and work-arround later today. And yes, planning on fixing it as I understand that it might be confusing. Just still pondering different options.

AGiorgetti commented 11 years ago

yes @ProLoser I know that, and I also said sorry in advance, I apologize again. Yes, that's the problem and it was spotted by @boneskull (the extra scope was created by the transclusion). The implementation I posted there get rid of the translusion and does not create the extra scope. It still need to be improved by using the template cache again for the modal frame (instead of inlining it as I did in the quick fix).

lucassus commented 11 years ago

I have similar issue. Inside a dialog template I have a form <form name="editForm" ...> but inside the controller $scope.editForm is undefined. Here is my example https://github.com/9ci/angle-grinder/commit/a3a1bc4483e01eb235d13f29709d2fd2604a8ddc#L4R28

acornejo commented 11 years ago

Is there anyone working on a fix? I'll admit that it is a bit dirty to inserting ng-style/ng-class styles directly into the dom, but @AGiorgetti code seems to do the trick.

PS: Thinking about it some more, I do think that modal-dialog and modal-content should not be inserted by transclusion in directives, but should be part of the user template (i.e. templateUrl parameter in modal). So this is extra points for that solution.

BastianVoigt commented 11 years ago

I found it hard to see what @AGiorgetti changed, so I made a fork.

https://github.com/batzee/bootstrap/commit/ddc275a35fb53f52baf870e28a715b02b4f70223

The change breaks some tests though:

Chrome 30.0 (Linux) $modal option by option custom window classes should support additional window class as string FAILED Expected '

' to have class 'additional'.

BastianVoigt commented 11 years ago

The fix does not work for me :-(

At least not the backport to 0.6. The modal is not displayed at all.

boneskull commented 11 years ago

How about the workaround?

On Sep 23, 2013, at 5:30 AM, Bastian Voigt notifications@github.com wrote:

The fix does not work for me :-(

At least not the backport to 0.6. The modal is not displayed at all.

— Reply to this email directly or view it on GitHub.

lucassus commented 11 years ago

@boneskull here's my dirty hack for the missing form https://github.com/9ci/angle-grinder/commit/78a96d6ca0ccfbe8d02113f6eb035aee2586ad62#L0L29

boneskull commented 11 years ago

That is very dirty indeed.

On Sep 24, 2013, at 12:52 PM, Łukasz Bandzarewicz notifications@github.com wrote:

@boneskull here's my dirty hack for the missing form 9ci/angle-grinder@78a96d6#L0L29

— Reply to this email directly or view it on GitHub.

BastianVoigt commented 11 years ago

@boneskull The last commit I posted here, b85c5cc, actually works.

boneskull commented 11 years ago

@BastianVoigt Does it animate?

BastianVoigt commented 11 years ago

@boneskull not sure what you mean by animate ?

boneskull commented 11 years ago

@BastianVoigt the modal; does it animate (fade/slide/whatever) when you open it

frapontillo commented 11 years ago

I am having this issue as well, are there any workarounds?

BastianVoigt commented 11 years ago

@boneskull sorry for the late reply. It does animate.

aozora commented 11 years ago

Hi, any news on this? I need it to be resolved because this is now a big issue for a almost production ready app!

georgiosd commented 11 years ago

Guys, I've been puzzled by this issue because I like I've been saying all along, I do the exact same thing and all my forms work fine.

I just realized that I use a custom button directive that will enable/disable the button depending on form validity.

Have you guys tried that? You'll need to require ^form and check the ngModelControllers for the form.

RyanElfman commented 10 years ago

I have a modal with a form and when I fill in the values inside the text boxes the form submits fine, its when I try to do validation on the required HTML attribute that it doesn't show.

http://plnkr.co/edit/5ShxH4?p=preview

Anybody found a work around for this?

Narretz commented 10 years ago

What's the status on this one? It breaks in funny ways when you have another not-isolated directive in the modal dom element. Sometimes stuff is accessible, sometimes it isn't. Couldn't we do some manual transclusion magic?

Narretz commented 10 years ago

Here's a plunkr with a possible solution:

http://plnkr.co/edit/ZjqZXLAwVjDL7nKIdkqe?p=preview

It uses manual transclusion, where the content of modal-window is transcluded into the parent scope of modal-window (ModalCtrl) So the scope of the content of modal-window is the same as the scope of the ModalController, while the modal-window directive is an isolate child of the ModalController. At least I guess that's how it works. note that the plunker works with Angular 1.2.3, not 1.0.8. However, not all tests pass currently in any version. The problem is that the tests don't see the manually transcluded content.

Mule52 commented 10 years ago

I am running into this same problem now and it's killing me. I am trying to follow the examples of launching modals that I find online, but all are similar and none of the examples talk about validation. They simply use the modal to open, save (close), and cancel (dismiss) - trivial. All of that works great. However getting to the form is not.

@boneskull, I am trying to following your workaround, but without success. I am not sure where to add the $modal.open().

I have a parent controller, the one that will have the $scope.openModal that contains a templateUrl, controller, resolve, etc. I then define a new controller, .controller('ModalInstanceCtrl', ['$scope', '$modalInstance', 'MyService', function ($scope, $modalInstance, MyService)... In this controller, I have the $scope.save and $scope.cancel which call $modalInstance.close and $modalInstance.dismiss. Before I call $modalInstance.close(), I would like to validate the form. I can't though, unless I go some roundabout way using a service.

Can you help me understand your workaround? Or do you have any more sample code illustrating the workaround?

Thanks for the help!

RBLU commented 10 years ago

Hi @Mule52 I used the following pattern as a workaround:

Now I have access to the form in my controller on the $scope as usual.

Mule52 commented 10 years ago

@RLBU, thank you for your reply! I will try your approach next.

In the meantime, I found this post, http://stackoverflow.com/questions/19312936/angularjs-modal-dialog-form-object-is-undefined-in-controller, and I was able to get my modal controller to access the form.

Your solution and the post I found might be similar. I will try yours. I've been fighting this for hours. Such relief.

Edit: I just got your approach to work. Thanks for the help!

pkozlowski-opensource commented 10 years ago

OK, looking over this thread I guess that everyone have figured out that all the issues are linked to the fact that $modal service is using transclusion for its windows (meaning: transcludes modal content into modal's window template: https://github.com/angular-ui/bootstrap/blob/master/template/modal/window.html).

I know that using transclusion is not ideal here as it creates a new scope. But all the alternatives are even worse:

Closing this issue as the real fix should come from AngularJS itself (via https://github.com/angular/angular.js/issues/5489) and other work-arrounds are just awful.

boneskull commented 10 years ago

Makes sense, thanks @pkozlowski-opensource

kimardenmiller commented 10 years ago

@pkozlowski-opensource , can you advise what the latest advice is .... still needs a work-around?

pkozlowski-opensource commented 10 years ago

@kimardenmiller work-around for? What is your particular use-case?

matt-way commented 10 years ago

For anyone still looking for a workaround for this it is quite simple. Just create an object in your $modal controllers scope (eg. $scope.container = {};).

Then define your form like this <form name="container.myform" novalidate>. You should now be easily able to access your form object from your modal controller $scope.container.myform.

kimardenmiller commented 10 years ago

Just trying to access new user inputs on the template form back into my modal controller to save back to my database.

What was so confusing is that my template finds all functions in the modal controller fine. I guess that's because the controller's scope is "above" the template's scope where the template can look up and see it, but the the controller can't look "below" itself to find the template's scope?

If I'm understanding it right, the work around is to set up an object in the modal template's assigned controller that finds the template form directly rather than going thru ng-model to get the form data to sync with the model? What happens when the Angular guys fix it? I guess this approach will work just as well as the official method, so our code can remain configured with this work-around without penalty?

Actually, it seems this method does two things, gives access to the form object, but also a model?:

image

So it seems the model name can just be duplicated in the form name + input field name to create a model that looks just like the ng-model it should be getting?"

image

image

Along with:

image

Can allow me to leave the rest of my controller unchanged?:

image

Seem all safe to do it like that?

/k

pkozlowski-opensource commented 10 years ago

@kimardenmiller yes, binding to object's attributes is the way to go. It will work even if AngularJS guys decide to fix transclusion issues in the framework.

kimardenmiller commented 10 years ago

Actually, now I can't duplicate what I did above.

See how I had the newProposal (above), which looks like a model on the scope?

But trying it again it's not present on the scope now: image

Only alone on it's own scope: image

How did I break that? Should I expect to see the form data on my modal controller scope, w/o the newProposalForm container like I had it for a moment?:

image

I'd like to configure it so I can address my model data as usual via newProposal and not have to change the rest of my controller to have to deal with the newProposalForm container.

pkozlowski-opensource commented 10 years ago

@kimardenmiller it is very hard to provide meaningful help without live code example. If you need further assistance please open a new issue and provide minimal reproduce scenario using http://plnkr.co/

kimardenmiller commented 10 years ago

I understood this thread to be about getting access to form model data that the two scopes problem made inaccessible. For the benefit of others trying this work-around, @matt-way 's approach works if one adds ".$modelValue" to the end of your reference.

So in the modal controller's beginning:

$scope.container = {}

and with a form name of:

%form( name='container.myForm'  )

one has access to a form's input value named formInputName like so:

$scope.container.myForm.formInputName.$modelValue

@pkozlowski-opensource , please let us know if I've made it more complicated than it has to be, or if the above are correct instructions.

Thanks.

matt-way commented 10 years ago

You don't have to use .$modelValue if you also set your ng-model references to the container. eg:

<input type="text" ng-model="container.textValue" name="textValue">

This way you access the form information at container.myForm.textValue, and the model value at container.textValue. Nice and easy.

This all comes back to the correct use of scope inheritance in angular. Have a read of this: http://jimhoskins.com/2012/12/14/nested-scopes-in-angularjs.html

kimardenmiller commented 10 years ago

@matt-way , thanks so much! I learned a ton in this exercise: So it's all about the read before write when child scopes set up objects and write to them. By setting an object on the parent scope with the same name as the model, the (child scope) template form uses the parent's object by that same name to hang the form on, and in so doing one is able to fake the model back to the parent scope. Very clever. The Hoskins link above explains it very well.

Thanks for the lesson! /kim

jeffgabhart commented 10 years ago

we could require people to systematically provide the whole modal window markup for each modal - this would be really bad as it would mean repeating code over and over again

Can someone help me understand why this is so much worse than the effort it takes to understand and workaround the scope issue?

NickBarton commented 10 years ago

Total cludge solution http://plnkr.co/edit/oiOuwSGcJDzkPBIdVURv?p=preview