diff --git a/src/ngMessages/messages.js b/src/ngMessages/messages.js index bbe87bba372a..f79baa18cb71 100644 --- a/src/ngMessages/messages.js +++ b/src/ngMessages/messages.js @@ -410,6 +410,13 @@ angular.module('ngMessages', []) $scope.$watchCollection($attrs.ngMessages || $attrs['for'], ctrl.render); + // If the element is destroyed, proactively destroy all the currently visible messages + $element.on('$destroy', function() { + forEach(messages, function(item) { + item.message.detach(); + }); + }); + this.reRender = function() { if (!renderLater) { renderLater = true; @@ -444,6 +451,7 @@ angular.module('ngMessages', []) function findPreviousMessage(parent, comment) { var prevNode = comment; var parentLookup = []; + while (prevNode && prevNode !== parent) { var prevKey = prevNode.$$ngMessageNode; if (prevKey && prevKey.length) { @@ -455,8 +463,11 @@ angular.module('ngMessages', []) if (prevNode.childNodes.length && parentLookup.indexOf(prevNode) == -1) { parentLookup.push(prevNode); prevNode = prevNode.childNodes[prevNode.childNodes.length - 1]; + } else if (prevNode.previousSibling) { + prevNode = prevNode.previousSibling; } else { - prevNode = prevNode.previousSibling || prevNode.parentNode; + prevNode = prevNode.parentNode; + parentLookup.push(prevNode); } } } @@ -669,8 +680,8 @@ function ngMessageDirectiveFactory() { // when we are destroying the node later. var $$attachId = currentElement.$$attachId = ngMessagesCtrl.getAttachId(); - // in the event that the parent element is destroyed - // by any other structural directive then it's time + // in the event that the element or a parent element is destroyed + // by another structural directive then it's time // to deregister the message from the controller currentElement.on('$destroy', function() { if (currentElement && currentElement.$$attachId === $$attachId) { diff --git a/test/ngMessages/messagesSpec.js b/test/ngMessages/messagesSpec.js index 2a03c90075b2..5613844bf591 100644 --- a/test/ngMessages/messagesSpec.js +++ b/test/ngMessages/messagesSpec.js @@ -485,6 +485,126 @@ describe('ngMessages', function() { }); }); + describe('ngMessage nested nested inside elements', function() { + + it('should not crash or leak memory when the messages are transcluded, the first message is ' + + 'visible, and ngMessages is removed by ngIf', function() { + + module(function($compileProvider) { + $compileProvider.directive('messageWrap', function() { + return { + transclude: true, + scope: { + col: '=col' + }, + template: '
' + }; + }); + }); + + inject(function($rootScope, $compile) { + + element = $compile('
' + + '
A
' + + '
B
' + + '
')($rootScope); + + $rootScope.$apply(function() { + $rootScope.show = true; + $rootScope.col = { + a: true, + b: true + }; + }); + + expect(messageChildren(element).length).toBe(1); + expect(trim(element.text())).toEqual('A'); + + $rootScope.$apply('show = false'); + + expect(messageChildren(element).length).toBe(0); + }); + }); + + + it('should not crash when the first of two nested messages is removed', function() { + inject(function($rootScope, $compile) { + + element = $compile( + '
' + + '
' + + '
A
' + + '
B
' + + '
' + + '
' + )($rootScope); + + $rootScope.$apply(function() { + $rootScope.col = { + a: true, + b: false + }; + }); + + expect(messageChildren(element).length).toBe(1); + expect(trim(element.text())).toEqual('A'); + + var ctrl = element.controller('ngMessages'); + var deregisterSpy = spyOn(ctrl, 'deregister').and.callThrough(); + + var nodeA = element[0].querySelector('[ng-message="a"]'); + jqLite(nodeA).remove(); + $rootScope.$digest(); // The next digest triggers the error + + // Make sure removing the element triggers the deregistration in ngMessages + expect(trim(deregisterSpy.calls.mostRecent().args[0].nodeValue)).toBe('ngMessage: a'); + expect(messageChildren(element).length).toBe(0); + }); + }); + + + it('should not crash, but show deeply nested messages correctly after a message ' + + 'has been removed', function() { + inject(function($rootScope, $compile) { + + element = $compile( + '
' + + '
' + + '
A
' + + '
' + + '
B
' + + '
C
' + + '
' + + '
D
' + + '
' + + '
' + )($rootScope); + + $rootScope.$apply(function() { + $rootScope.col = { + a: true, + b: true + }; + }); + + expect(messageChildren(element).length).toBe(2); + expect(trim(element.text())).toEqual('AB'); + + var ctrl = element.controller('ngMessages'); + var deregisterSpy = spyOn(ctrl, 'deregister').and.callThrough(); + + var nodeB = element[0].querySelector('[ng-message="b"]'); + jqLite(nodeB).remove(); + $rootScope.$digest(); // The next digest triggers the error + + // Make sure removing the element triggers the deregistration in ngMessages + expect(trim(deregisterSpy.calls.mostRecent().args[0].nodeValue)).toBe('ngMessage: b'); + expect(messageChildren(element).length).toBe(1); + expect(trim(element.text())).toEqual('A'); + }); + }); + }); + describe('when including templates', function() { they('should work with a dynamic collection model which is managed by ngRepeat', {'
': '
' + @@ -691,6 +811,37 @@ describe('ngMessages', function() { expect(trim(element.text())).toEqual("C"); })); + + it('should properly detect a previous message, even if it was registered later', + inject(function($compile, $rootScope, $templateCache) { + $templateCache.put('include.html', '
A
'); + var html = + '
' + + '
' + + '
B
' + + '
C
' + + '
'; + + element = $compile(html)($rootScope); + $rootScope.$apply('items = {b: true, c: true}'); + + expect(element.text()).toBe('B'); + + var ctrl = element.controller('ngMessages'); + var deregisterSpy = spyOn(ctrl, 'deregister').and.callThrough(); + + var nodeB = element[0].querySelector('[ng-message="b"]'); + jqLite(nodeB).remove(); + + // Make sure removing the element triggers the deregistration in ngMessages + expect(trim(deregisterSpy.calls.mostRecent().args[0].nodeValue)).toBe('ngMessage: b'); + + $rootScope.$apply('items.a = true'); + + expect(element.text()).toBe('A'); + }) + ); + }); describe('when multiple', function() {