From c87c7778cbb6156ec7bde4ec8fdb0440267c4f37 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matias=20Niemel=C3=A4?= Date: Thu, 26 Feb 2015 19:46:49 -0500 Subject: [PATCH] fix(ngMessages): ensure that multi-level transclusion works with ngMessagesInclude ngRepeat and any other directives that alter the DOM structure using transclusion may cause ngMessagesInclude to behave in an unpredictable manner. This fix ensures that the element containing the ngMessagesInclude directive will stay in the DOM to avoid these issues. Closes #11196 --- src/ngMessages/messages.js | 34 ++++---- test/ngMessages/messagesSpec.js | 136 +++++++++++++++++++++++++------- 2 files changed, 123 insertions(+), 47 deletions(-) diff --git a/src/ngMessages/messages.js b/src/ngMessages/messages.js index 5fb585b98d58..53a1d2822fe9 100644 --- a/src/ngMessages/messages.js +++ b/src/ngMessages/messages.js @@ -500,27 +500,21 @@ angular.module('ngMessages', []) return { restrict: 'AE', - require: '^^ngMessages', - compile: function(element, attrs) { - var comment = jqLite($document[0].createComment(' ngMessagesInclude: ')); - element.after(comment); - - return function($scope, $element, attrs, ngMessagesCtrl) { - // we're removing this since we only need access to the newly - // created comment node as an anchor. - element.remove(); - - $templateRequest(attrs.ngMessagesInclude || attrs.src).then(function(html) { - var elements = jqLite('
').html(html).contents(); - var cursor = comment; - forEach(elements, function(elm) { - elm = jqLite(elm); - cursor.after(elm); - cursor = elm; - }); - $compile(elements)($scope); + require: '^^ngMessages', // we only require this for validation sake + link: function($scope, element, attrs) { + var src = attrs.ngMessagesInclude || attrs.src; + $templateRequest(src).then(function(html) { + $compile(html)($scope, function(contents) { + element.after(contents); + + // the anchor is placed for debugging purposes + var anchor = jqLite($document[0].createComment(' ngMessagesInclude: ' + src + ' ')); + element.after(anchor); + + // we don't want to pollute the DOM anymore by keeping an empty directive element + element.remove(); }); - }; + }); } }; }]) diff --git a/test/ngMessages/messagesSpec.js b/test/ngMessages/messagesSpec.js index 26215741c6bf..a678b406e609 100644 --- a/test/ngMessages/messagesSpec.js +++ b/test/ngMessages/messagesSpec.js @@ -4,6 +4,10 @@ describe('ngMessages', function() { beforeEach(inject.strictDi()); beforeEach(module('ngMessages')); + function messageChildren(element) { + return (element.length ? element[0] : element).querySelectorAll('[ng-message], [ng-message-exp]'); + } + function they(msg, vals, spec, focus) { forEach(vals, function(val, key) { var m = msg.replace('$prop', key); @@ -236,7 +240,7 @@ describe('ngMessages', function() { $rootScope.col = {}; }); - expect(element.children().length).toBe(0); + expect(messageChildren(element).length).toBe(0); expect(trim(element.text())).toEqual(''); $rootScope.$apply(function() { @@ -246,7 +250,7 @@ describe('ngMessages', function() { }; }); - expect(element.children().length).toBe(1); + expect(messageChildren(element).length).toBe(1); expect(trim(element.text())).toEqual('This message is blue'); $rootScope.$apply(function() { @@ -255,13 +259,13 @@ describe('ngMessages', function() { }; }); - expect(element.children().length).toBe(1); + expect(messageChildren(element).length).toBe(1); expect(trim(element.text())).toEqual('This message is red'); $rootScope.$apply(function() { $rootScope.col = null; }); - expect(element.children().length).toBe(0); + expect(messageChildren(element).length).toBe(0); expect(trim(element.text())).toEqual(''); @@ -272,7 +276,7 @@ describe('ngMessages', function() { }; }); - expect(element.children().length).toBe(0); + expect(messageChildren(element).length).toBe(0); expect(trim(element.text())).toEqual(''); }); }); @@ -346,21 +350,21 @@ describe('ngMessages', function() { ]; }); - expect(messageChildren().length).toBe(0); + expect(messageChildren(element).length).toBe(0); expect(trim(element.text())).toEqual(""); $rootScope.$apply(function() { $rootScope.col = { hair: true }; }); - expect(messageChildren().length).toBe(1); + expect(messageChildren(element).length).toBe(1); expect(trim(element.text())).toEqual("Your hair is too long"); $rootScope.$apply(function() { $rootScope.col = { age: true, hair: true}; }); - expect(messageChildren().length).toBe(1); + expect(messageChildren(element).length).toBe(1); expect(trim(element.text())).toEqual("Your age is incorrect"); $rootScope.$apply(function() { @@ -368,7 +372,7 @@ describe('ngMessages', function() { $rootScope.items.shift(); }); - expect(messageChildren().length).toBe(1); + expect(messageChildren(element).length).toBe(1); expect(trim(element.text())).toEqual("Your hair is too long"); $rootScope.$apply(function() { @@ -377,12 +381,8 @@ describe('ngMessages', function() { $rootScope.col.primary = true; }); - expect(messageChildren().length).toBe(1); + expect(messageChildren(element).length).toBe(1); expect(trim(element.text())).toEqual("Enter something"); - - function messageChildren() { - return element[0].querySelectorAll('[ng-message], [ng-message-exp]'); - } })); @@ -415,6 +415,88 @@ describe('ngMessages', function() { }); describe('when including templates', function() { + they('should work with a dynamic collection model which is managed by ngRepeat', + {'
': '
' + + '
' + + '
', + '': '' + + '' + + ''}, + function(html) { + inject(function($compile, $rootScope, $templateCache) { + $templateCache.put('abc.html', '
A
' + + '
B
' + + '
C
'); + + html = '
' + html + '
'; + $rootScope.items = [{},{},{}]; + + element = $compile(html)($rootScope); + $rootScope.$apply(function() { + $rootScope.items[0].a = true; + $rootScope.items[1].b = true; + $rootScope.items[2].c = true; + }); + + var elements = element[0].querySelectorAll('[ng-repeat]'); + + // all three collections should have atleast one error showing up + expect(messageChildren(element).length).toBe(3); + expect(messageChildren(elements[0]).length).toBe(1); + expect(messageChildren(elements[1]).length).toBe(1); + expect(messageChildren(elements[2]).length).toBe(1); + + // this is the standard order of the displayed error messages + expect(element.text().trim()).toBe('ABC'); + + $rootScope.$apply(function() { + $rootScope.items[0].a = false; + $rootScope.items[0].c = true; + + $rootScope.items[1].b = false; + + $rootScope.items[2].c = false; + $rootScope.items[2].a = true; + }); + + // with the 2nd item gone and the values changed + // we should see both 1 and 3 changed + expect(element.text().trim()).toBe('CA'); + + $rootScope.$apply(function() { + // add the value for the 2nd item back + $rootScope.items[1].b = true; + $rootScope.items.reverse(); + }); + + // when reversed we get back to our original value + expect(element.text().trim()).toBe('ABC'); + }); + }); + + they('should remove the $prop element and place a comment anchor node where it used to be', + {'
': '
' + + '
' + + '
', + '': '' + + '' + + ''}, + function(html) { + inject(function($compile, $rootScope, $templateCache) { + $templateCache.put('abc.html', '
'); + + element = $compile(html)($rootScope); + $rootScope.$digest(); + + var includeElement = element[0].querySelector('[ng-messages-include], ng-messages-include'); + expect(includeElement).toBeFalsy(); + + var comment = element[0].childNodes[0]; + expect(comment.nodeType).toBe(8); + expect(comment.nodeValue).toBe(' ngMessagesInclude: abc.html '); + }); + }); + they('should load a remote template using $prop', {'
': '
' + '
' + @@ -437,7 +519,7 @@ describe('ngMessages', function() { }; }); - expect(element.children().length).toBe(1); + expect(messageChildren(element).length).toBe(1); expect(trim(element.text())).toEqual("A"); $rootScope.$apply(function() { @@ -446,7 +528,7 @@ describe('ngMessages', function() { }; }); - expect(element.children().length).toBe(1); + expect(messageChildren(element).length).toBe(1); expect(trim(element.text())).toEqual("C"); }); }); @@ -454,7 +536,7 @@ describe('ngMessages', function() { it('should cache the template after download', inject(function($rootScope, $compile, $templateCache, $httpBackend) { - $httpBackend.expect('GET', 'tpl').respond(201, 'abc'); + $httpBackend.expect('GET', 'tpl').respond(201, '
abc
'); expect($templateCache.get('tpl')).toBeUndefined(); @@ -484,13 +566,13 @@ describe('ngMessages', function() { $rootScope.$digest(); - expect(element.children().length).toBe(1); + expect(messageChildren(element).length).toBe(1); expect(trim(element.text())).toEqual("Your value is that of failure"); $httpBackend.flush(); $rootScope.$digest(); - expect(element.children().length).toBe(1); + expect(messageChildren(element).length).toBe(1); expect(trim(element.text())).toEqual("You did not enter a value"); })); @@ -515,7 +597,7 @@ describe('ngMessages', function() { }; }); - expect(element.children().length).toBe(1); + expect(messageChildren(element).length).toBe(1); expect(trim(element.text())).toEqual("AAA"); $rootScope.$apply(function() { @@ -525,7 +607,7 @@ describe('ngMessages', function() { }; }); - expect(element.children().length).toBe(1); + expect(messageChildren(element).length).toBe(1); expect(trim(element.text())).toEqual("B"); $rootScope.$apply(function() { @@ -534,7 +616,7 @@ describe('ngMessages', function() { }; }); - expect(element.children().length).toBe(1); + expect(messageChildren(element).length).toBe(1); expect(trim(element.text())).toEqual("C"); })); @@ -560,7 +642,7 @@ describe('ngMessages', function() { }; }); - expect(element.children().length).toBe(2); + expect(messageChildren(element).length).toBe(2); expect(s(element.text())).toContain("13"); }); }); @@ -584,14 +666,14 @@ describe('ngMessages', function() { }; }); - expect(element.children().length).toBe(2); + expect(messageChildren(element).length).toBe(2); expect(s(element.text())).toEqual("XZ"); $rootScope.$apply(function() { $rootScope.data.y = {}; }); - expect(element.children().length).toBe(3); + expect(messageChildren(element).length).toBe(3); expect(s(element.text())).toEqual("XYZ"); })); @@ -616,14 +698,14 @@ describe('ngMessages', function() { }; }); - expect(element.children().length).toBe(2); + expect(messageChildren(element).length).toBe(2); expect(s(element.text())).toEqual("ZZZX"); $rootScope.$apply(function() { $rootScope.data.y = {}; }); - expect(element.children().length).toBe(3); + expect(messageChildren(element).length).toBe(3); expect(s(element.text())).toEqual("YYYZZZX"); })); });