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

$modal breaks forms b/c of transclusion #969

Closed
boneskull opened this issue Sep 9, 2013 · 68 comments
Closed

$modal breaks forms b/c of transclusion #969

boneskull opened this issue Sep 9, 2013 · 68 comments
Milestone

Comments

@boneskull
Copy link

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
Copy link

Chris,

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

@damrbaby
Copy link

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

@boneskull
Copy link
Author

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

@AGiorgetti
Copy link

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
Copy link

@AGiorgetti
Copy link

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
Copy link

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
Copy link

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

@boneskull
Copy link
Author

Thanks for taking a look.

@georgiosd
Copy link

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
Copy link

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
Copy link

@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
Copy link
Member

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
Copy link
Member

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
Copy link

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
Copy link

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 yakworks/angle-grinder@a3a1bc4#L4R28

@acornejo
Copy link

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
Copy link

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

BastianVoigt@ddc275a

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 '

With custom window class
' to have class 'additional'.

BastianVoigt added a commit to BastianVoigt/bootstrap that referenced this issue Sep 23, 2013
BastianVoigt added a commit to BastianVoigt/bootstrap that referenced this issue Sep 23, 2013
@BastianVoigt
Copy link

The fix does not work for me :-(

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

BastianVoigt added a commit to BastianVoigt/bootstrap that referenced this issue Sep 23, 2013
- delete directive modalWindow
- ngTransclude not needed any more
BastianVoigt added a commit to BastianVoigt/bootstrap that referenced this issue Sep 23, 2013
BastianVoigt added a commit to BastianVoigt/bootstrap that referenced this issue Sep 23, 2013
@boneskull
Copy link
Author

How about the workaround?

On Sep 23, 2013, at 5:30 AM, Bastian Voigt [email protected] 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.

@matt-way
Copy link

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
Copy link

@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
Copy link

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
Copy link

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

@kimardenmiller
Copy link

One approach that fixes this scope issue (and has helped us avoid other similar issues) is to link the model to a separate service which does not rely on the scope stack.

So in the template (in CoffeeScript):
image

Which links to a service:
image

@pgaertig
Copy link

Clever solution @matt-way. I improved it further:

<form name="$parent.form">

And it just works! I have got form entry in modal $scope and I don't have to modify my form markup inside. The only thing I don't like about this is the hacky name value.

@brianmarco
Copy link

Tx @pgaertig your tip worked great for me!

@plentz
Copy link

plentz commented Jun 10, 2014

@AGiorgetti you closed this referencing #972, and then @pkozlowski-opensource closed it referencing #981 which I don't think it's related to this one and this one should be re-open. Am I wrong? 😓

@AGiorgetti
Copy link

Hi, i didn't close anything, the reference at #972 was made because the problem reported there (a typeahead malfunction inside a modal) was related to the issues caused by the use of transclusion in the modal dialog. As of now I'm using a custom made version of the modal dialog which does not use transclusion at all, I'm just doing it manually playing with the templates (as I did in the past with the 'patched' version posted above at the time).

@pkozlowski-opensource
Copy link
Member

@plentz we've got a work-around in master 0b31e86, not released yet, so no need to re-open anything...

@plentz
Copy link

plentz commented Jun 10, 2014

@AGiorgetti thanks!

@pkozlowski-opensource coool! thanks a lot!

@kulicuu
Copy link

kulicuu commented Jul 10, 2014

boneskull thx for the brilliant workaround !

@LeleDev
Copy link

LeleDev commented Sep 16, 2014

I'm still having the same issue... 0b31e86 hasn't been released yet?

@LeleDev
Copy link

LeleDev commented Sep 16, 2014

Ok, I've compiled the master version with grunt and now it works. Hope that ui-boostrap 0.12 will be released soon!

@KKrisu
Copy link

KKrisu commented Nov 24, 2014

I just want to confirm - After upgrading ui-bootstrap to 0.12 it works correctly!

@devendher
Copy link

check it this plunker
might be it will helpful and i think it is complete solution that really what do you want http://plnkr.co/edit/ZM3UUt16P3cRainp0LK3?p=preview

@karianna karianna added this to the 0.12 milestone Apr 30, 2015
codedogfish pushed a commit to codedogfish/angular-ui-bootstrap that referenced this issue Sep 15, 2015
@solerman
Copy link

I was fighting over ng-model/ng-form myself this days. My problem was a bit different but also cased by the way scopes/controllers are handled. Basically I have a directive to watch from the parent form for changes and warn the user if he's about to leave the page without saving and that wasnt working if you only changed the modal's values. The workaround:

In the parent controller inject your form

   modalInstance = $modal.open({
        resolve: {
          containerForm: function () {
            return $scope.containerForm;
          }
        }
      });

And in the modal's template set the parent form before any control has the chance to initialize

    <ng-form name="$parent.modalForm">
        <any ng-init="$parent.modalForm.$$parentForm = containerForm"/>

@icfantv
Copy link
Contributor

icfantv commented Nov 28, 2015

@solerman, I should point out that using ng-init outside the very limited, documented areas is not correct.

Also, doing this with your form is not correct. You should be returning the model from the form in the modal as a result of the $promise from the modal and then, if needed, update your form's model with that data.

@solerman
Copy link

The problem is, with bindings the model is already updated in the model. To do what you say I would need to clone the (very complex) model, provide it to the modal and then hand-copy all the values back. For every modal.

@icfantv
Copy link
Contributor

icfantv commented Nov 30, 2015

@solerman, that sounds like an issue with design and UX rather than our library. do you need to send the entire model to the modal or can you just send the bit that the model intends to edit?

For the record, this is a very common paradigm - say, having a grid of data and then editing a grid item in a modal.

@solerman
Copy link

@icfantv the parent its a dashboard and the modal its just for a single dashboard item. There is a lot of stuff to configure in the item, I ain't sending the entire dashboard but just a single item. Think a bar chart, all the series, axis titles, colors, ranges, etc.

@icfantv
Copy link
Contributor

icfantv commented Nov 30, 2015

@solerman, without knowing exactly what you're using the modal to configure, it's hard to make a recommendation. It sounds like you're using the modal to configure chart options, but that's just a guess.

I can certainly appreciate not wanting to copy a "huge" model object. The only other options I can think of are:

  1. Don't copy the model, rather, send it live and use a cancel to restore the model, or
  2. Don't use a modal. Rather, use a physically separate details page. This would depend on the amount of data that needs to be edited.

I still think this boils down to a design and UX issue and not the library. We do get a lot of issues surrounding the use of parent forms wrapping "compartmentalized" forms (tabs, accordions, etc...). There are ways of working around our replace: true directives, including an example in our tabs demo - but in general, I'd argue that doing this is a somewhat bad approach. Part of that is due to how angular form validation was architected for 1.x. I don't know if or how this changes for ng2.

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

No branches or pull requests