fix(form): prevent page reload when form destroyed

this fix ensures that we prevent the default action on form submission
(full page reload) even in cases when the form is being destroyed as
a result of the submit event handler (e.g. when route change is
triggered).

The fix is more complicated than I'd like it to be mainly because
we need to ensure that we don't create circular references between
js closures and dom elements via DOM event handlers that would then
result in a memory leak.

Also the differences between IE8, IE9 and normal browsers make testing
this ugly.

Closes #1238

Author: IgorMinar

src/ng/directive/form.js

@@ -227,38 +227,62 @@ function FormController(element, attrs) {
       </doc:scenario>
     </doc:example>
  */
-var formDirectiveDir = {
-  name: 'form',
-  restrict: 'E',
-  controller: FormController,
-  compile: function() {
-    return {
-      pre: function(scope, formElement, attr, controller) {
-        if (!attr.action) {
-          formElement.bind('submit', function(event) {
-            event.preventDefault();
-          });
-        }
+var formDirectiveFactory = function(isNgForm) {
+  return ['$timeout', function($timeout) {
+    var formDirective = {
+      name: 'form',
+      restrict: 'E',
+      controller: FormController,
+      compile: function() {
+        return {
+          pre: function(scope, formElement, attr, controller) {
+            if (!attr.action) {
+              // we can't use jq events because if a form is destroyed during submission the default
+              // action is not prevented. see #1238
+              //
+              // IE 9 is not affected because it doesn't fire a submit event and try to do a full
+              // page reload if the form was destroyed by submission of the form via a click handler
+              // on a button in the form. Looks like an IE9 specific bug.
+              var preventDefaultListener = function(event) {
+                event.preventDefault
+                  ? event.preventDefault()
+                  : event.returnValue = false; // IE
+              };
 
-        var parentFormCtrl = formElement.parent().controller('form'),
-            alias = attr.name || attr.ngForm;
+              addEventListenerFn(formElement[0], 'submit', preventDefaultListener);
+
+              // unregister the preventDefault listener so that we don't not leak memory but in a
+              // way that will achieve the prevention of the default action.
+              formElement.bind('$destroy', function() {
+                $timeout(function() {
+                  removeEventListenerFn(formElement[0], 'submit', preventDefaultListener);
+                }, 0, false);
+              });
+            }
+
+            var parentFormCtrl = formElement.parent().controller('form'),
+                alias = attr.name || attr.ngForm;
 
-        if (alias) {
-          scope[alias] = controller;
-        }
-        if (parentFormCtrl) {
-          formElement.bind('$destroy', function() {
-            parentFormCtrl.$removeControl(controller);
             if (alias) {
-              scope[alias] = undefined;
+              scope[alias] = controller;
             }
-            extend(controller, nullFormCtrl); //stop propagating child destruction handlers upwards
-          });
-        }
+            if (parentFormCtrl) {
+              formElement.bind('$destroy', function() {
+                parentFormCtrl.$removeControl(controller);
+                if (alias) {
+                  scope[alias] = undefined;
+                }
+                extend(controller, nullFormCtrl); //stop propagating child destruction handlers upwards
+              });
+            }
+          }
+        };
       }
     };
-  }
+
+    return isNgForm ? extend(copy(formDirective), {restrict: 'EAC'}) : formDirective;
+  }];
 };
 
-var formDirective = valueFn(formDirectiveDir);
-var ngFormDirective = valueFn(extend(copy(formDirectiveDir), {restrict: 'EAC'}));
+var formDirective = formDirectiveFactory();
+var ngFormDirective = formDirectiveFactory(true);

test/ng/directive/formSpec.js

@@ -129,14 +129,28 @@ describe('form', function() {
 
     it('should prevent form submission', function() {
       var nextTurn = false,
+          submitted = false,
           reloadPrevented;
 
-      doc = jqLite('<form><input type="submit" value="submit" ></form>');
+      doc = jqLite('<form ng-submit="submitMe()">' +
+                     '<input type="submit" value="submit">' +
+                   '</form>');
+
+      var assertPreventDefaultListener = function(e) {
+        reloadPrevented = e.defaultPrevented || (e.returnValue === false);
+      };
+
+      // native dom event listeners in IE8 fire in LIFO order so we have to register them
+      // there in different order than in other browsers
+      if (msie==8) addEventListenerFn(doc[0], 'submit', assertPreventDefaultListener);
+
       $compile(doc)(scope);
 
-      doc.bind('submit', function(e) {
-        reloadPrevented = e.defaultPrevented;
-      });
+      scope.submitMe = function() {
+        submitted = true;
+      }
+
+      if (msie!=8) addEventListenerFn(doc[0], 'submit', assertPreventDefaultListener);
 
       browserTrigger(doc.find('input'));
 
@@ -147,17 +161,92 @@ describe('form', function() {
 
       runs(function() {
         expect(reloadPrevented).toBe(true);
+        expect(submitted).toBe(true);
+
+        // prevent mem leak in test
+        removeEventListenerFn(doc[0], 'submit', assertPreventDefaultListener);
       });
     });
 
 
-    it('should not prevent form submission if action attribute present', function() {
+    it('should prevent the default when the form is destroyed by a submission via a click event',
+        inject(function($timeout) {
+      doc = jqLite('<div>' +
+                      '<form ng-submit="submitMe()">' +
+                        '<button ng-click="destroy()"></button>' +
+                      '</form>' +
+                    '</div>');
+
+      var form = doc.find('form'),
+          destroyed = false,
+          nextTurn = false,
+          submitted = false,
+          reloadPrevented;
+
+      scope.destroy = function() {
+        // yes, I know, scope methods should not do direct DOM manipulation, but I wanted to keep
+        // this test small. Imagine that the destroy action will cause a model change (e.g.
+        // $location change) that will cause some directive to destroy the dom (e.g. ngView+$route)
+        doc.html('');
+        destroyed = true;
+      }
+
+      scope.submitMe = function() {
+        submitted = true;
+      }
+
+      var assertPreventDefaultListener = function(e) {
+        reloadPrevented = e.defaultPrevented || (e.returnValue === false);
+      };
+
+      // native dom event listeners in IE8 fire in LIFO order so we have to register them
+      // there in different order than in other browsers
+      if (msie == 8) addEventListenerFn(form[0], 'submit', assertPreventDefaultListener);
+
+      $compile(doc)(scope);
+
+      if (msie != 8) addEventListenerFn(form[0], 'submit', assertPreventDefaultListener);
+
+      browserTrigger(doc.find('button'), 'click');
+
+      // let the browser process all events (and potentially reload the page)
+      setTimeout(function() { nextTurn = true;}, 100);
+
+      waitsFor(function() { return nextTurn; });
+
+
+      // I can't get IE8 to automatically trigger submit in this test, in production it does it
+      // properly
+      if (msie == 8) browserTrigger(form, 'submit');
+
+      runs(function() {
+        expect(doc.html()).toBe('');
+        expect(destroyed).toBe(true);
+        expect(submitted).toBe(false); // this is known corner-case that is not currently handled
+                                       // the issue is that the submit listener is destroyed before
+                                       // the event propagates there. we can fix this if we see
+                                       // the issue in the wild, I'm not going to bother to do it
+                                       // now. (i)
+
+        // IE9 is special and it doesn't fire submit event when form was destroyed
+        if (msie != 9) {
+          expect(reloadPrevented).toBe(true);
+          $timeout.flush();
+        }
+
+        // prevent mem leak in test
+        removeEventListenerFn(form[0], 'submit', assertPreventDefaultListener);
+      });
+    }));
+
+
+    it('should NOT prevent form submission if action attribute present', function() {
       var callback = jasmine.createSpy('submit').andCallFake(function(event) {
         expect(event.isDefaultPrevented()).toBe(false);
         event.preventDefault();
       });
 
-      doc = $compile('<form name="x" action="some.py" />')(scope);
+      doc = $compile('<form action="some.py"></form>')(scope);
       doc.bind('submit', callback);
 
       browserTrigger(doc, 'submit');