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

ngClass[Odd/Even] overhaul #15228

Closed
wants to merge 15 commits into from
Closed

ngClass[Odd/Even] overhaul #15228

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
8822006
refactor(ngClass): remove unnecessary dependency on `$animate`
gkalpak Oct 7, 2016
0409589
refactor(ngClass): remove redundant `$observe`r
gkalpak Oct 7, 2016
2705085
test(ngClass): add some tests about one-time bindings and objects ins…
gkalpak Oct 7, 2016
0ba4be6
fix(ngClassOdd/Even): keep track of classes even if odd/even do not m…
gkalpak Oct 7, 2016
89268af
perf(ngClass): only access the element's `data` once
gkalpak Oct 7, 2016
2871a11
refactor(ngClass): simplify conditions
gkalpak Oct 7, 2016
a7e69f7
refactor(ngClass): move helper functions outside the closure
gkalpak Oct 7, 2016
37cec7a
refactor(ngClass): exit `arrayDifference()` early if an input is empty
gkalpak Oct 7, 2016
5708424
perf(ngClass): avoid deep-watching (if possible) and unnecessary copies
gkalpak Oct 7, 2016
739f723
fixup! refactor(ngClass): remove unnecessary dependency on `$animate`
gkalpak Oct 8, 2016
181c51b
fixup! refactor(ngClass): remove redundant `$observe`r
gkalpak Oct 8, 2016
16e666b
fixup! test(ngClass): add some tests about one-time bindings and obje…
gkalpak Oct 8, 2016
dfe4307
squash! fix(ngClassOdd/Even): keep track of classes even if odd/even …
gkalpak Oct 8, 2016
f766698
fixup! perf(ngClass): avoid deep-watching (if possible) and unnecessa…
gkalpak Oct 8, 2016
a9742f9
fixup! refactor(ngClass): simplify conditions
gkalpak Oct 10, 2016
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
23 changes: 8 additions & 15 deletions src/ng/directive/ngClass.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,25 +8,19 @@

function classDirective(name, selector) {
name = 'ngClass' + name;
return ['$animate', function($animate) {

return [function() {
return {
restrict: 'AC',
link: function(scope, element, attr) {
var oldVal;

scope.$watch(attr[name], ngClassWatchAction, true);

attr.$observe('class', function(value) {
ngClassWatchAction(scope.$eval(attr[name]));
});


if (name !== 'ngClass') {
scope.$watch('$index', function($index, old$index) {
/* eslint-disable no-bitwise */
var mod = $index & 1;
if (mod !== (old$index & 1)) {
var classes = arrayClasses(scope.$eval(attr[name]));
var classes = arrayClasses(oldVal);
if (mod === selector) {
addClasses(classes);
} else {
Expand All @@ -37,6 +31,8 @@ function classDirective(name, selector) {
});
}

scope.$watch(attr[name], ngClassWatchAction, true);

function addClasses(classes) {
var newClasses = digestClassCounts(classes, 1);
attr.$addClass(newClasses);
Expand Down Expand Up @@ -69,12 +65,9 @@ function classDirective(name, selector) {
var toRemove = arrayDifference(oldClasses, newClasses);
toAdd = digestClassCounts(toAdd, 1);
toRemove = digestClassCounts(toRemove, -1);
if (toAdd && toAdd.length) {
$animate.addClass(element, toAdd);
}
if (toRemove && toRemove.length) {
$animate.removeClass(element, toRemove);
}

attr.$addClass(toAdd);
attr.$removeClass(toRemove);
Copy link
Contributor

Choose a reason for hiding this comment

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

I never knew about these helpers!!

}

function ngClassWatchAction(newVal) {
Expand Down
242 changes: 196 additions & 46 deletions test/ng/directive/ngClassSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
'use strict';

describe('ngClass', function() {
fdescribe('ngClass', function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Naught naughty

Copy link
Member Author

Choose a reason for hiding this comment

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

I already have a fixup commit at the end 😛

var element;

beforeEach(module(function($compileProvider) {
Expand Down Expand Up @@ -244,31 +244,42 @@ describe('ngClass', function() {
}));


it('should allow ngClassOdd/Even on the same element with overlapping classes', inject(function($rootScope, $compile, $animate) {
var className;

element = $compile('<ul><li ng-repeat="i in [0,1,2]" ng-class-odd="\'same odd\'" ng-class-even="\'same even\'"></li><ul>')($rootScope);
it('should allow ngClassOdd/Even on the same element with overlapping classes',
inject(function($compile, $rootScope) {
element = $compile(
'<ul>' +
'<li ng-repeat="i in [0,1,2]" ' +
'ng-class-odd="\'same odd\'" ' +
'ng-class-even="\'same even\'">' +
'</li>' +
'<ul>')($rootScope);
$rootScope.$digest();
var e1 = jqLite(element[0].childNodes[1]);
var e2 = jqLite(element[0].childNodes[5]);
expect(e1.hasClass('same')).toBeTruthy();
expect(e1.hasClass('odd')).toBeTruthy();
expect(e2.hasClass('same')).toBeTruthy();
expect(e2.hasClass('odd')).toBeTruthy();

var e1 = element.children().eq(0);
var e2 = element.children().eq(1);
var e3 = element.children().eq(2);

expect(e1).toHaveClass('same');
expect(e1).toHaveClass('odd');
expect(e1).not.toHaveClass('even');
expect(e2).toHaveClass('same');
expect(e2).not.toHaveClass('odd');
expect(e2).toHaveClass('even');
expect(e3).toHaveClass('same');
expect(e3).toHaveClass('odd');
expect(e3).not.toHaveClass('even');
})
);

it('should allow ngClass with overlapping classes', inject(function($rootScope, $compile, $animate) {
it('should allow ngClass with overlapping classes', inject(function($rootScope, $compile) {
element = $compile('<div ng-class="{\'same yes\': test, \'same no\': !test}"></div>')($rootScope);
$rootScope.$digest();

expect(element).toHaveClass('same');
expect(element).not.toHaveClass('yes');
expect(element).toHaveClass('no');

$rootScope.$apply(function() {
$rootScope.test = true;
});
$rootScope.$apply('test = true');

expect(element).toHaveClass('same');
expect(element).toHaveClass('yes');
Expand Down Expand Up @@ -300,37 +311,79 @@ describe('ngClass', function() {
}));


it('should reapply ngClass when interpolated class attribute changes', inject(function($rootScope, $compile) {
element = $compile('<div class="one {{cls}} three" ng-class="{four: four}"></div>')($rootScope);

$rootScope.$apply(function() {
$rootScope.cls = 'two';
$rootScope.four = true;
});
expect(element).toHaveClass('one');
expect(element).toHaveClass('two'); // interpolated
expect(element).toHaveClass('three');
expect(element).toHaveClass('four');

$rootScope.$apply(function() {
$rootScope.cls = 'too';
});
expect(element).toHaveClass('one');
expect(element).toHaveClass('too'); // interpolated
expect(element).toHaveClass('three');
expect(element).toHaveClass('four'); // should still be there
expect(element.hasClass('two')).toBeFalsy();

$rootScope.$apply(function() {
$rootScope.cls = 'to';
});
expect(element).toHaveClass('one');
expect(element).toHaveClass('to'); // interpolated
expect(element).toHaveClass('three');
expect(element).toHaveClass('four'); // should still be there
expect(element.hasClass('two')).toBeFalsy();
expect(element.hasClass('too')).toBeFalsy();
}));
it('should reapply ngClass when interpolated class attribute changes',
inject(function($compile, $rootScope) {
element = $compile(
'<div>' +
'<div class="one {{two}} three" ng-class="{five: five}"></div>' +
'<div class="one {{two}} three {{four}}" ng-class="{five: five}"></div>' +
'</div>')($rootScope);
var e1 = element.children().eq(0);
var e2 = element.children().eq(1);

$rootScope.$apply('two = "two"; five = true');

expect(e1).toHaveClass('one');
expect(e1).toHaveClass('two');
expect(e1).toHaveClass('three');
expect(e1).not.toHaveClass('four');
expect(e1).toHaveClass('five');
expect(e2).toHaveClass('one');
expect(e2).toHaveClass('two');
expect(e2).toHaveClass('three');
expect(e2).not.toHaveClass('four');
expect(e2).toHaveClass('five');

$rootScope.$apply('two = "too"');
Copy link
Contributor

Choose a reason for hiding this comment

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

although this is very clever "two" and "too" are very similar and actually make the test harder to follow.

Copy link
Member Author

Choose a reason for hiding this comment

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

The cleverness credits go to the original test 😃
I can change it.


expect(e1).toHaveClass('one');
expect(e1).not.toHaveClass('two');
expect(e1).toHaveClass('too');
expect(e1).toHaveClass('three');
expect(e1).not.toHaveClass('four');
expect(e1).toHaveClass('five');
expect(e2).toHaveClass('one');
expect(e2).not.toHaveClass('two');
expect(e2).toHaveClass('too');
expect(e2).toHaveClass('three');
expect(e2).not.toHaveClass('four');
expect(e2).toHaveClass('five');

$rootScope.$apply('two = "to"; four = "four"');

expect(e1).toHaveClass('one');
expect(e1).not.toHaveClass('two');
expect(e1).not.toHaveClass('too');
expect(e1).toHaveClass('to');
expect(e1).toHaveClass('three');
expect(e1).not.toHaveClass('four');
expect(e1).toHaveClass('five');
expect(e2).toHaveClass('one');
expect(e2).not.toHaveClass('two');
expect(e2).not.toHaveClass('too');
expect(e2).toHaveClass('to');
expect(e2).toHaveClass('three');
expect(e2).toHaveClass('four');
expect(e2).toHaveClass('five');

$rootScope.$apply('five = false');

expect(e1).toHaveClass('one');
expect(e1).not.toHaveClass('two');
expect(e1).not.toHaveClass('too');
expect(e1).toHaveClass('to');
expect(e1).toHaveClass('three');
expect(e1).not.toHaveClass('four');
expect(e1).not.toHaveClass('five');
expect(e2).toHaveClass('one');
expect(e2).not.toHaveClass('two');
expect(e2).not.toHaveClass('too');
expect(e2).toHaveClass('to');
expect(e2).toHaveClass('three');
expect(e2).toHaveClass('four');
expect(e2).not.toHaveClass('five');
})
);


it('should not mess up class value due to observing an interpolated class attribute', inject(function($rootScope, $compile) {
Expand Down Expand Up @@ -409,6 +462,47 @@ describe('ngClass', function() {
expect(e2.hasClass('odd')).toBeFalsy();
}));


it('should keep track of old classes even if odd/even does not match `$index` atm',
Copy link
Contributor

Choose a reason for hiding this comment

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

This description makes hard to understand what is actually being tested.

Are you saying that ngClass is not removing old classes when both the $index and the expression value changes?

Copy link
Member Author

Choose a reason for hiding this comment

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

It might be because I was trying to solve it differently at first. Here is what was happening before this change:

  1. Assume ng-class-odd="foo", $index = 0, foo = 'foo'. The element has className === 'foo'.
  2. If the $index and foo change simultaneously, say $index = 1, foo = 'bar', then:
    1. The attr[name] watch kicks in first and it checks if scope.$index & 1 === selector (i.e. it uses the current value of $index to determine if it needs to update the classes). It decides it doesn't need to do anything, so at this point the element still has className === 'foo'.
    2. The $index watch kicks in. Since it determines that $index & 1 !== old$index & 1, it decides to remove the classes. But it tries to scope.$eval(attr[name]) which now evaluates to 'bar'. So the element will still have className === 'foo'.

Basically, the problem is that both watch-actions check against the current value of each other to determine what to do, which fails when both change in the same digest.
I.e. the attr[name] watch uses scope.$index (while it should use the previous index) and the $index watch uses scope.$eval(attr[name]) (while it should use the previous value - the "old classes").

The commit fxes it by:

  1. Moving the $index watch before the attr[name] watch.
  2. Using oldVal from the $index watch to add/remove.

This way, when the $index & 1 changes, the $index watch will correctly add/remove the classes and then the attr[name] watch will update the classes (i.e. add the new ones) if necessary.

I hope this makes sense 😃

How about:

It should add/remove the correct classes when the expression and $index change simultaneously

inject(function($compile, $rootScope) {
element = $compile(
'<div>' +
'<div ng-class-odd="foo"></div>' +
'<div ng-class-even="foo"></div>' +
'</div>')($rootScope);
var odd = element.children().eq(0);
var even = element.children().eq(1);

$rootScope.$apply('$index = 0; foo = "class1"');

expect(odd).toHaveClass('class1');
expect(odd).not.toHaveClass('class2');
expect(even).not.toHaveClass('class1');
expect(even).not.toHaveClass('class2');

$rootScope.$apply('$index = 1; foo = "class2"');

expect(odd).not.toHaveClass('class1');
expect(odd).not.toHaveClass('class2');
expect(even).not.toHaveClass('class1');
expect(even).toHaveClass('class2');

$rootScope.$apply('foo = "class1"');

expect(odd).not.toHaveClass('class1');
expect(odd).not.toHaveClass('class2');
expect(even).toHaveClass('class1');
expect(even).not.toHaveClass('class2');

$rootScope.$apply('$index = 2');

expect(odd).toHaveClass('class1');
expect(odd).not.toHaveClass('class2');
expect(even).not.toHaveClass('class1');
expect(even).not.toHaveClass('class2');
})
);

it('should support mixed array/object variable with a mutating object',
inject(function($rootScope, $compile) {
element = $compile('<div ng-class="classVar"></div>')($rootScope);
Expand All @@ -424,6 +518,62 @@ describe('ngClass', function() {
})
);

it('should do value stabilization as expected when one-time binding',
inject(function($rootScope, $compile) {
element = $compile('<div ng-class="::className"></div>')($rootScope);
$rootScope.$digest();

$rootScope.className = 'foo';
$rootScope.$digest();
expect(element).toHaveClass('foo');

$rootScope.className = 'bar';
$rootScope.$digest();
expect(element).toHaveClass('foo');
})
);

it('should remove the watcher when static array one-time binding',
inject(function($rootScope, $compile) {
element = $compile('<div ng-class="::[className]"></div>')($rootScope);
$rootScope.$digest();

$rootScope.$apply('className = "foo"');
expect(element).toHaveClass('foo');

$rootScope.$apply('className = "bar"');
expect(element).toHaveClass('foo');
expect(element).not.toHaveClass('bar');
})
);

it('should do value remove the watcher when static map one-time binding',
Copy link
Contributor

Choose a reason for hiding this comment

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

the test description here seems like a hybrid of the two tests above :-)

inject(function($rootScope, $compile) {
element = $compile('<div ng-class="::{foo: fooPresent}"></div>')($rootScope);
$rootScope.$digest();

$rootScope.fooPresent = true;
$rootScope.$digest();
expect(element).toHaveClass('foo');

$rootScope.fooPresent = false;
$rootScope.$digest();
expect(element).toHaveClass('foo');
})
);

it('should track changes of mutating object inside an array',
inject(function($rootScope, $compile) {
$rootScope.classVar = [{orange: true}];
element = $compile('<div ng-class="classVar"></div>')($rootScope);
$rootScope.$digest();
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it worth adding an expectation that it should have the class "orange" here?


$rootScope.classVar[0].orange = false;
$rootScope.$digest();

expect(element).not.toHaveClass('orange');
})
);
});

describe('ngClass animations', function() {
Expand Down