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

feat($compile): explicitly request multi-element directive behaviour #5372

Closed
wants to merge 1 commit into from
Closed

feat($compile): explicitly request multi-element directive behaviour #5372

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
38 changes: 34 additions & 4 deletions src/ng/compile.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,13 @@
* The directive definition object provides instructions to the {@link ng.$compile
* compiler}. The attributes are:
*
* ### `multiElement`
* When this property is set to true, the HTML compiler will collect DOM nodes between
* nodes with the attributes `directive-name-start` and `directive-name-end`, and group them
* together as the directive elements. It is recomended that this feature be used on directives
* which are not strictly behavioural (such as {@link api/ng.directive:ngClick ngClick}), and which
* do not manipulate or replace child nodes (such as {@link api/ng.directive:ngInclude ngInclude}).
*
* #### `priority`
* When there are multiple directives defined on a single DOM element, sometimes it
* is necessary to specify the order in which the directives are applied. The `priority` is used
Expand Down Expand Up @@ -1047,10 +1054,12 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
}

var directiveNName = ngAttrName.replace(/(Start|End)$/, '');
if (ngAttrName === directiveNName + 'Start') {
attrStartName = name;
attrEndName = name.substr(0, name.length - 5) + 'end';
name = name.substr(0, name.length - 6);
if (directiveIsMultiElement(directiveNName)) {
if (ngAttrName === directiveNName + 'Start') {
attrStartName = name;
attrEndName = name.substr(0, name.length - 5) + 'end';
name = name.substr(0, name.length - 6);
}
}

nName = directiveNormalize(name.toLowerCase());
Expand Down Expand Up @@ -1663,6 +1672,27 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
}


/**
* looks up the directive and returns true if it is a multi-element directive,
* and therefore requires DOM nodes between -start and -end markers to be grouped
* together.
*
* @param {string} name name of the directive to look up.
* @returns true if directive was registered as multi-element.
*/
function directiveIsMultiElement(name) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't the logic be inversed here?
That is: a directive is considered multi-element if and only if all the directives registered with that name can handle multi-elements.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not totally sure how it should work, I mean ideally you'd only hand the collection of elements to ones with multiElement:true, and only hand the original element to the others. But that seems doable in a later patch, if someone needs that

if (hasDirectives.hasOwnProperty(name)) {
for(var directive, directives = $injector.get(name + Suffix),
i = 0, ii = directives.length; i<ii; i++) {
directive = directives[i];
if (directive.multiElement) {
return true;
}
}
}
return false;
}

/**
* When the element is replaced with HTML template then the new attributes
* on the template need to be merged with the existing attributes in the DOM.
Expand Down
1 change: 1 addition & 0 deletions src/ng/directive/ngIf.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@
*/
var ngIfDirective = ['$animate', function($animate) {
return {
multiElement: true,
transclude: 'element',
priority: 600,
terminal: true,
Expand Down
1 change: 1 addition & 0 deletions src/ng/directive/ngRepeat.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,7 @@ var ngRepeatDirective = ['$parse', '$animate', function($parse, $animate) {
var NG_REMOVED = '$$NG_REMOVED';
var ngRepeatMinErr = minErr('ngRepeat');
return {
multiElement: true,
transclude: 'element',
priority: 1000,
terminal: true,
Expand Down
22 changes: 14 additions & 8 deletions src/ng/directive/ngShowHide.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,10 +156,13 @@
</example>
*/
var ngShowDirective = ['$animate', function($animate) {
return function(scope, element, attr) {
scope.$watch(attr.ngShow, function ngShowWatchAction(value){
$animate[value ? 'removeClass' : 'addClass'](element, 'ng-hide');
});
return {
multiElement: true,
link: function(scope, element, attr) {
scope.$watch(attr.ngShow, function ngShowWatchAction(value){
$animate[value ? 'removeClass' : 'addClass'](element, 'ng-hide');
});
}
};
}];

Expand Down Expand Up @@ -307,9 +310,12 @@ var ngShowDirective = ['$animate', function($animate) {
</example>
*/
var ngHideDirective = ['$animate', function($animate) {
return function(scope, element, attr) {
scope.$watch(attr.ngHide, function ngHideWatchAction(value){
$animate[value ? 'addClass' : 'removeClass'](element, 'ng-hide');
});
return {
multiElement: true,
link: function(scope, element, attr) {
scope.$watch(attr.ngHide, function ngHideWatchAction(value){
$animate[value ? 'addClass' : 'removeClass'](element, 'ng-hide');
});
}
};
}];
2 changes: 2 additions & 0 deletions src/ng/directive/ngSwitch.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,7 @@ var ngSwitchWhenDirective = ngDirective({
transclude: 'element',
priority: 800,
require: '^ngSwitch',
multiElement: true,
link: function(scope, element, attrs, ctrl, $transclude) {
ctrl.cases['!' + attrs.ngSwitchWhen] = (ctrl.cases['!' + attrs.ngSwitchWhen] || []);
ctrl.cases['!' + attrs.ngSwitchWhen].push({ transclude: $transclude, element: element });
Expand All @@ -195,6 +196,7 @@ var ngSwitchDefaultDirective = ngDirective({
transclude: 'element',
priority: 800,
require: '^ngSwitch',
multiElement: true,
link: function(scope, element, attr, ctrl, $transclude) {
ctrl.cases['?'] = (ctrl.cases['?'] || []);
ctrl.cases['?'].push({ transclude: $transclude, element: element });
Expand Down
80 changes: 65 additions & 15 deletions test/ng/compileSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5487,6 +5487,40 @@ describe('$compile', function() {
expect(element.attr('dash-test4')).toBe('JamieMason');
}));

it('should keep attributes ending with -start single-element directives', function() {
module(function($compileProvider) {
$compileProvider.directive('dashStarter', function(log) {
return {
link: function(scope, element, attrs) {
log(attrs.onDashStart);
}
};
});
});
inject(function($compile, $rootScope, log) {
$compile('<span data-dash-starter data-on-dash-start="starter"></span>')($rootScope);
$rootScope.$digest();
expect(log).toEqual('starter');
});
});


it('should keep attributes ending with -end single-element directives', function() {
module(function($compileProvider) {
$compileProvider.directive('dashEnder', function(log) {
return {
link: function(scope, element, attrs) {
log(attrs.onDashEnd);
}
};
});
});
inject(function($compile, $rootScope, log) {
$compile('<span data-dash-ender data-on-dash-end="ender"></span>')($rootScope);
$rootScope.$digest();
expect(log).toEqual('ender');
});
});
});

});
Expand Down Expand Up @@ -5545,19 +5579,29 @@ describe('$compile', function() {
}));


it('should group on nested groups', inject(function($compile, $rootScope) {
$rootScope.show = false;
element = $compile(
'<div></div>' +
'<div ng-repeat-start="i in [1,2]">{{i}}A</div>' +
'<span ng-bind-start="\'.\'"></span>' +
'<span ng-bind-end></span>' +
'<div ng-repeat-end>{{i}}B;</div>' +
'<div></div>')($rootScope);
$rootScope.$digest();
element = jqLite(element[0].parentNode.childNodes); // reset because repeater is top level.
expect(element.text()).toEqual('1A..1B;2A..2B;');
}));
it('should group on nested groups', function() {
module(function($compileProvider) {
$compileProvider.directive("ngMultiBind", valueFn({
multiElement: true,
link: function(scope, element, attr) {
element.text(scope.$eval(attr.ngMultiBind));
}
}));
});
inject(function($compile, $rootScope) {
$rootScope.show = false;
element = $compile(
'<div></div>' +
'<div ng-repeat-start="i in [1,2]">{{i}}A</div>' +
'<span ng-multi-bind-start="\'.\'"></span>' +
'<span ng-multi-bind-end></span>' +
'<div ng-repeat-end>{{i}}B;</div>' +
'<div></div>')($rootScope);
$rootScope.$digest();
element = jqLite(element[0].parentNode.childNodes); // reset because repeater is top level.
expect(element.text()).toEqual('1A..1B;2A..2B;');
});
});


it('should group on nested groups of same directive', inject(function($compile, $rootScope) {
Expand Down Expand Up @@ -5749,6 +5793,7 @@ describe('$compile', function() {
module(function($compileProvider) {
$compileProvider.directive('foo', function() {
return {
multiElement: true
};
});
});
Expand All @@ -5766,12 +5811,16 @@ describe('$compile', function() {
it('should correctly collect ranges on multiple directives on a single element', function () {
module(function($compileProvider) {
$compileProvider.directive('emptyDirective', function() {
return function (scope, element) {
element.data('x', 'abc');
return {
multiElement: true,
link: function (scope, element) {
element.data('x', 'abc');
}
};
});
$compileProvider.directive('rangeDirective', function() {
return {
multiElement: true,
link: function (scope) {
scope.x = 'X';
scope.y = 'Y';
Expand Down Expand Up @@ -5799,6 +5848,7 @@ describe('$compile', function() {
module(function($compileProvider) {
$compileProvider.directive('foo', function() {
return {
multiElement: true
};
});
});
Expand Down
52 changes: 52 additions & 0 deletions test/ng/directive/ngSwitchSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,32 @@ describe('ngSwitch', function() {
}));


it('should show all elements between start and end markers that match the current value',
inject(function($rootScope, $compile) {
element = $compile(
'<ul ng-switch="select">' +
'<li ng-switch-when-start="1">A</li>' +
'<li>B</li>' +
'<li ng-switch-when-end>C</li>' +
'<li ng-switch-when-start="2">D</li>' +
'<li>E</li>' +
'<li ng-switch-when-end>F</li>' +
'</ul>')($rootScope);

$rootScope.$apply('select = "1"');
expect(element.find('li').length).toBe(3);
expect(element.find('li').eq(0).text()).toBe('A');
expect(element.find('li').eq(1).text()).toBe('B');
expect(element.find('li').eq(2).text()).toBe('C');

$rootScope.$apply('select = "2"');
expect(element.find('li').length).toBe(3);
expect(element.find('li').eq(0).text()).toBe('D');
expect(element.find('li').eq(1).text()).toBe('E');
expect(element.find('li').eq(2).text()).toBe('F');
}));


it('should switch on switch-when-default', inject(function($rootScope, $compile) {
element = $compile(
'<ng:switch on="select">' +
Expand All @@ -80,6 +106,32 @@ describe('ngSwitch', function() {
}));


it('should show all default elements between start and end markers when no match',
inject(function($rootScope, $compile) {
element = $compile(
'<ul ng-switch="select">' +
'<li ng-switch-when-start="1">A</li>' +
'<li>B</li>' +
'<li ng-switch-when-end>C</li>' +
'<li ng-switch-default-start>D</li>' +
'<li>E</li>' +
'<li ng-switch-default-end>F</li>' +
'</ul>')($rootScope);

$rootScope.$apply('select = "1"');
expect(element.find('li').length).toBe(3);
expect(element.find('li').eq(0).text()).toBe('A');
expect(element.find('li').eq(1).text()).toBe('B');
expect(element.find('li').eq(2).text()).toBe('C');

$rootScope.$apply('select = "2"');
expect(element.find('li').length).toBe(3);
expect(element.find('li').eq(0).text()).toBe('D');
expect(element.find('li').eq(1).text()).toBe('E');
expect(element.find('li').eq(2).text()).toBe('F');
}));


it('should show all switch-when-default', inject(function($rootScope, $compile) {
element = $compile(
'<ul ng-switch="select">' +
Expand Down