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

fix($animate): improve detection and handling of invalid classNameFilter RegExp #14913

Closed
wants to merge 4 commits into from
Closed

fix($animate): improve detection and handling of invalid classNameFilter RegExp #14913

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8cdd7bd
fix($animate): improve detection on `ng-animate` in `classNameFilter`…
gkalpak Jul 15, 2016
5bf4ab0
fix($animate): reset `classNameFilter` to `null` when a disallowed Re…
gkalpak Jul 15, 2016
899c52e
fixup! fix($animate): improve detection on `ng-animate` in `className…
gkalpak Dec 19, 2016
7f0b72f
fixup! fix($animate): reset `classNameFilter` to `null` when a disall…
gkalpak Jan 25, 2017
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
8 changes: 8 additions & 0 deletions docs/content/error/$animate/nongcls.ngdoc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
@ngdoc error
@name $animate:nongcls
@fullName `ng-animate` class not allowed
@description

This error occurs, when trying to set `$animateProvider.classNameFilter()` to a RegExp containing
the reserved `ng-animate` class. Since `.ng-animate` will be added/removed by `$animate` itself,
using it as part of the `classNameFilter` RegExp is not allowed.
14 changes: 8 additions & 6 deletions src/ng/animate.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,7 @@ var $$CoreAnimateQueueProvider = /** @this */ function() {
*/
var $AnimateProvider = ['$provide', /** @this */ function($provide) {
var provider = this;
var classNameFilter = null;

this.$$registeredAnimations = Object.create(null);

Expand Down Expand Up @@ -247,15 +248,16 @@ var $AnimateProvider = ['$provide', /** @this */ function($provide) {
*/
this.classNameFilter = function(expression) {
if (arguments.length === 1) {
this.$$classNameFilter = (expression instanceof RegExp) ? expression : null;
if (this.$$classNameFilter) {
var reservedRegex = new RegExp('(\\s+|\\/)' + NG_ANIMATE_CLASSNAME + '(\\s+|\\/)');
if (reservedRegex.test(this.$$classNameFilter.toString())) {
throw $animateMinErr('nongcls','$animateProvider.classNameFilter(regex) prohibits accepting a regex value which matches/contains the "{0}" CSS class.', NG_ANIMATE_CLASSNAME);
classNameFilter = (expression instanceof RegExp) ? expression : null;
Copy link
Contributor

Choose a reason for hiding this comment

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

surely we should be throwing an error here, not silently ignoring invalid expressions??

Copy link
Member Author

Choose a reason for hiding this comment

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

This is not about ignoring invalid expressions (in the sense that they are of the wrong type).
This check is throwing when using ng-animate in your (valid) RegExp, since ng-animate is added by Angular itself.
Not sure why this is important tbh. (Off the top of my head, .ng-animate will be added after the classNameFilter() but I might be wrong.)

if (classNameFilter) {
var reservedRegex = new RegExp('[(\\s|\\/)]' + NG_ANIMATE_CLASSNAME + '[(\\s|\\/)]');
Copy link
Member Author

Choose a reason for hiding this comment

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

Btw, sneakiest change I've done to a RegExp ever 😈

if (reservedRegex.test(classNameFilter.toString())) {
classNameFilter = null;
throw $animateMinErr('nongcls', '$animateProvider.classNameFilter(regex) prohibits accepting a regex value which matches/contains the "{0}" CSS class.', NG_ANIMATE_CLASSNAME);
Copy link
Contributor

Choose a reason for hiding this comment

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

The error doesn't actually have an error page, can you add it?

}
}
}
return this.$$classNameFilter;
return classNameFilter;
};

this.$get = ['$$animateQueue', function($$animateQueue) {
Expand Down
50 changes: 31 additions & 19 deletions test/ngAnimate/animateSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,29 +255,41 @@ describe('animations', function() {
});
});

it('should throw a minErr if a regex value is used which partially contains or fully matches the `ng-animate` CSS class', function() {
it('should throw a minErr if a regex value is used which partially contains or fully matches the `ng-animate` CSS class',
module(function($animateProvider) {
assertError(/ng-animate/, true);
assertError(/first ng-animate last/, true);
assertError(/ng-animate-special/, false);
assertError(/first ng-animate-special last/, false);
assertError(/first ng-animate ng-animate-special last/, true);

function assertError(regex, bool) {
var expectation = expect(function() {
expect(setFilter(/ng-animate/)).toThrowMinErr('$animate', 'nongcls');
expect(setFilter(/first ng-animate last/)).toThrowMinErr('$animate', 'nongcls');
expect(setFilter(/first ng-animate ng-animate-special last/)).toThrowMinErr('$animate', 'nongcls');
expect(setFilter(/(ng-animate)/)).toThrowMinErr('$animate', 'nongcls');
expect(setFilter(/(foo|ng-animate|bar)/)).toThrowMinErr('$animate', 'nongcls');
expect(setFilter(/(foo|)ng-animate(|bar)/)).toThrowMinErr('$animate', 'nongcls');

expect(setFilter(/ng-animater/)).not.toThrow();
expect(setFilter(/my-ng-animate/)).not.toThrow();
expect(setFilter(/first ng-animater last/)).not.toThrow();
expect(setFilter(/first my-ng-animate last/)).not.toThrow();

function setFilter(regex) {
return function() {
$animateProvider.classNameFilter(regex);
});
};
}
})
);

var message = '$animateProvider.classNameFilter(regex) prohibits accepting a regex value which matches/contains the "ng-animate" CSS class.';
it('should clear the `classNameFilter` if a disallowed RegExp is passed',
module(function($animateProvider) {
var validRegex = /no-ng-animate/;
var invalidRegex = /no ng-animate/;

if (bool) {
expectation.toThrowMinErr('$animate', 'nongcls', message);
} else {
expectation.not.toThrowMinErr('$animate', 'nongcls', message);
}
}
});
});
$animateProvider.classNameFilter(validRegex);
expect($animateProvider.classNameFilter()).toEqual(validRegex);

// eslint-disable-next-line no-empty
try { $animateProvider.classNameFilter(invalidRegex); } catch (err) {}
expect($animateProvider.classNameFilter()).toBeNull();
})
);

it('should complete the leave DOM operation in case the classNameFilter fails', function() {
module(function($animateProvider) {
Expand Down