Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

fix(ngMessages): ensure that multi-level transclusion works with ngMessagesInclude #11199

Closed
wants to merge 1 commit into from
Closed

fix(ngMessages): ensure that multi-level transclusion works with ngMessagesInclude #11199

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 14 additions & 20 deletions src/ngMessages/messages.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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('<div></div>').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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually, hang on. Previously, you were basically replacing the element with a comment placeholder. I think we should keep doing this, there's no good reason to keep it in the DOM (is there?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All fixed up now. And I added some comments to the unit test.

link: function($scope, element, attrs) {
var src = attrs.ngMessagesInclude || attrs.src;
$templateRequest(src).then(function(html) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

making this directive an element transclusion directive makes it unnecessary to add this manually (making it terminal would avoid worrying about compiling nested gunk)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we set terminal to true then will this run last if the priority is left unset?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

directives are evaluated in the deterministic order, but they'll stop when they reach the terminal one. similarly, child nodes of an element with a terminal directive won't be compiled (but there are no children, except the ones you load from the server and compile on your own.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I know how this works, but please answer my question if it will be handled last if a priority is not assigned in the directive definition.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, it will. the default priority is 0

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR has been updated with a new commit. I don't think we should set this to terminal since it is already working.

$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();
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 for removing all this though, it's more complicated than is really needed

};
});
}
};
}])
Expand Down
136 changes: 109 additions & 27 deletions test/ngMessages/messagesSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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() {
Expand All @@ -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() {
Expand All @@ -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('');


Expand All @@ -272,7 +276,7 @@ describe('ngMessages', function() {
};
});

expect(element.children().length).toBe(0);
expect(messageChildren(element).length).toBe(0);
expect(trim(element.text())).toEqual('');
});
});
Expand Down Expand Up @@ -346,29 +350,29 @@ 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() {
// remove the age!
$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() {
Expand All @@ -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]');
}
}));


Expand Down Expand Up @@ -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',
{'<div ng-messages-include="...">': '<div ng-messages="item">' +
'<div ng-messages-include="abc.html"></div>' +
'</div>',
'<ng-messages-include src="...">': '<ng-messages for="item">' +
'<ng-messages-include src="abc.html"></ng-messages-include>' +
'</ng-messages>'},
function(html) {
inject(function($compile, $rootScope, $templateCache) {
$templateCache.put('abc.html', '<div ng-message="a">A</div>' +
'<div ng-message="b">B</div>' +
'<div ng-message="c">C</div>');

html = '<div><div ng-repeat="item in items">' + html + '</div></div>';
$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',
{'<div ng-messages-include="...">': '<div ng-messages="data">' +
'<div ng-messages-include="abc.html"></div>' +
'</div>',
'<ng-messages-include src="...">': '<ng-messages for="data">' +
'<ng-messages-include src="abc.html"></ng-messages-include>' +
'</ng-messages>'},
function(html) {
inject(function($compile, $rootScope, $templateCache) {
$templateCache.put('abc.html', '<div></div>');

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',
{'<div ng-messages-include="...">': '<div ng-messages="data">' +
'<div ng-messages-include="abc.html"></div>' +
Expand All @@ -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() {
Expand All @@ -446,15 +528,15 @@ describe('ngMessages', function() {
};
});

expect(element.children().length).toBe(1);
expect(messageChildren(element).length).toBe(1);
expect(trim(element.text())).toEqual("C");
});
});

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, '<div>abc</div>');

expect($templateCache.get('tpl')).toBeUndefined();

Expand Down Expand Up @@ -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");
}));

Expand All @@ -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() {
Expand All @@ -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() {
Expand All @@ -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");
}));

Expand All @@ -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");
});
});
Expand All @@ -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");
}));

Expand All @@ -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");
}));
});
Expand Down