From c25cb7d48872fbb37b68f57c9c493ed8bb86a140 Mon Sep 17 00:00:00 2001 From: Igor Minar Date: Tue, 7 Aug 2012 17:23:16 -0700 Subject: [PATCH 1/4] refactor(formSpec): group preventDefault specs into a describe --- test/ng/directive/formSpec.js | 63 ++++++++++++++++++----------------- 1 file changed, 33 insertions(+), 30 deletions(-) diff --git a/test/ng/directive/formSpec.js b/test/ng/directive/formSpec.js index 8b87f266c2cf..fa047480594f 100644 --- a/test/ng/directive/formSpec.js +++ b/test/ng/directive/formSpec.js @@ -64,36 +64,6 @@ describe('form', function() { }); - it('should prevent form submission', function() { - var startingUrl = '' + window.location; - doc = jqLite('
'); - $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('')(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('
')(scope); expect(scope.myForm).toBeTruthy(); @@ -155,6 +125,39 @@ describe('form', function() { }); + describe('preventing default submission', function() { + + it('should prevent form submission', function() { + var startingUrl = '' + window.location; + doc = jqLite('
'); + $compile(doc)(scope); + + browserTrigger(doc.find('input')); + waitsFor( + function() { return true; }, + 'let browser breath, so that the form submission 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('')(scope); + doc.bind('submit', callback); + + browserTrigger(doc, 'submit'); + expect(callback).toHaveBeenCalledOnce(); + }); + }); + + describe('nested forms', function() { it('should chain nested forms', function() { From 5cec32492c52209ce11b38b8180f9bdb909d041b Mon Sep 17 00:00:00 2001 From: Igor Minar Date: Tue, 7 Aug 2012 22:03:02 -0700 Subject: [PATCH 2/4] test(form): fix broken preventDefault test the original test relied on incorrect assumptions about how jasmine async tests work (when setTimeout is triggered) and how browser reloads a page (the sequence of events) and thus the test passes even when the default is not prevented. this change fixes the test by registering an extra submit event handler that checks if the default was prevented. if the default was not prevented, the test will fail and the page will be reloaded causing the test runner to panic. --- test/ng/directive/formSpec.js | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/test/ng/directive/formSpec.js b/test/ng/directive/formSpec.js index fa047480594f..96855f52fc6d 100644 --- a/test/ng/directive/formSpec.js +++ b/test/ng/directive/formSpec.js @@ -128,17 +128,25 @@ describe('form', function() { describe('preventing default submission', function() { it('should prevent form submission', function() { - var startingUrl = '' + window.location; - doc = jqLite(''); + var nextTurn = false, + reloadPrevented; + + doc = jqLite(''); $compile(doc)(scope); + doc.bind('submit', function(e) { + reloadPrevented = e.defaultPrevented; + }); + browserTrigger(doc.find('input')); - waitsFor( - function() { return true; }, - 'let browser breath, so that the form submission can manifest itself', 10); + + // let the browser process all events (and potentially reload the page) + setTimeout(function() { nextTurn = true;}); + + waitsFor(function() { return nextTurn; }); runs(function() { - expect('' + window.location).toEqual(startingUrl); + expect(reloadPrevented).toBe(true); }); }); From 054d40f338f9000cddcf7f0513af37328b88ef41 Mon Sep 17 00:00:00 2001 From: Igor Minar Date: Tue, 7 Aug 2012 22:08:03 -0700 Subject: [PATCH 3/4] 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 --- src/ng/directive/form.js | 78 +++++++++++++++++--------- test/ng/directive/formSpec.js | 101 ++++++++++++++++++++++++++++++++-- 2 files changed, 146 insertions(+), 33 deletions(-) diff --git a/src/ng/directive/form.js b/src/ng/directive/form.js index cc229759b9cb..6b876e01be0e 100644 --- a/src/ng/directive/form.js +++ b/src/ng/directive/form.js @@ -227,38 +227,62 @@ function FormController(element, attrs) { */ -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); diff --git a/test/ng/directive/formSpec.js b/test/ng/directive/formSpec.js index 96855f52fc6d..51c6929fc84d 100644 --- a/test/ng/directive/formSpec.js +++ b/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('
'); + doc = jqLite('
' + + '' + + '
'); + + 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('
' + + '
' + + '' + + '
' + + '
'); + + 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('
')(scope); + doc = $compile('
')(scope); doc.bind('submit', callback); browserTrigger(doc, 'submit'); From c0d638a94b914edc76c5532c08a47ec4e60308d4 Mon Sep 17 00:00:00 2001 From: Igor Minar Date: Fri, 10 Aug 2012 10:24:28 -0700 Subject: [PATCH 4/4] test(jqLite): add missing test for $destroy event --- test/jqLiteSpec.js | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/test/jqLiteSpec.js b/test/jqLiteSpec.js index 406b7a5af4ae..1a56a3437692 100644 --- a/test/jqLiteSpec.js +++ b/test/jqLiteSpec.js @@ -2,6 +2,9 @@ describe('jqLite', function() { var scope, a, b, c; + + beforeEach(module(provideLog)); + beforeEach(function() { a = jqLite('
A
')[0]; b = jqLite('
B
')[0]; @@ -241,7 +244,7 @@ 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;';}); @@ -249,6 +252,18 @@ describe('jqLite', function() { expect(log).toEqual('destroy;'); }); + + it('should emit $destroy event if an element is removed via html()', inject(function(log) { + var element = jqLite('
x
'); + 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({});