Prevent default bug

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/jqLiteSpec.js

@@ -2,6 +2,9 @@
 describe('jqLite', function() {
   var scope, a, b, c;
 
+
+  beforeEach(module(provideLog));
+
   beforeEach(function() {
     a = jqLite('<div>A</div>')[0];
     b = jqLite('<div>B</div>')[0];
@@ -241,14 +244,26 @@ describe('jqLite', function() {
       expect(jqLite(c).data('prop')).toBeUndefined();
     });
 
-    it('should call $destroy function if element removed', function() {
+    it('should emit $destroy event if element removed via remove()', function() {
       var log = '';
       var element = jqLite(a);
       element.bind('$destroy', function() {log+= 'destroy;';});
       element.remove();
       expect(log).toEqual('destroy;');
     });
 
+
+    it('should emit $destroy event if an element is removed via html()', inject(function(log) {
+      var element = jqLite('<div><span>x</span></div>');
+      element.find('span').bind('$destroy', log.fn('destroyed'));
+
+      element.html('');
+
+      expect(element.html()).toBe('');
+      expect(log).toEqual('destroyed');
+    }));
+
+
     it('should retrieve all data if called without params', function() {
       var element = jqLite(a);
       expect(element.data()).toEqual({});

test/ng/directive/formSpec.js

@@ -64,36 +64,6 @@ describe('form', function() {
   });
 
 
-  it('should prevent form submission', function() {
-    var startingUrl = '' + window.location;
-    doc = jqLite('<form name="myForm"><input type="submit" value="submit" />');
-    $compile(doc)(scope);
-
-    browserTrigger(doc.find('input'));
-    waitsFor(
-        function() { return true; },
-        'let browser breath, so that the form submision can manifest itself', 10);
-
-    runs(function() {
-      expect('' + window.location).toEqual(startingUrl);
-    });
-  });
-
-
-  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.bind('submit', callback);
-
-    browserTrigger(doc, 'submit');
-    expect(callback).toHaveBeenCalledOnce();
-  });
-
-
   it('should publish form to scope when name attr is defined', function() {
     doc = $compile('<form name="myForm"></form>')(scope);
     expect(scope.myForm).toBeTruthy();
@@ -155,6 +125,136 @@ describe('form', function() {
   });
 
 
+  describe('preventing default submission', function() {
+
+    it('should prevent form submission', function() {
+      var nextTurn = false,
+          submitted = false,
+          reloadPrevented;
+
+      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);
+
+      scope.submitMe = function() {
+        submitted = true;
+      }
+
+      if (msie!=8) addEventListenerFn(doc[0], 'submit', assertPreventDefaultListener);
+
+      browserTrigger(doc.find('input'));
+
+      // let the browser process all events (and potentially reload the page)
+      setTimeout(function() { nextTurn = true;});
+
+      waitsFor(function() { return nextTurn; });
+
+      runs(function() {
+        expect(reloadPrevented).toBe(true);
+        expect(submitted).toBe(true);
+
+        // prevent mem leak in test
+        removeEventListenerFn(doc[0], 'submit', assertPreventDefaultListener);
+      });
+    });
+
+
+    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 action="some.py"></form>')(scope);
+      doc.bind('submit', callback);
+
+      browserTrigger(doc, 'submit');
+      expect(callback).toHaveBeenCalledOnce();
+    });
+  });
+
+
   describe('nested forms', function() {
 
     it('should chain nested forms', function() {