fix(ngClass): remove classes already on the dom

[Decad]

classes with an expression that evaluates to false but are already on the dom do
not get removed.

[Decad]

Created a plnkr for the bug:

http://plnkr.co/edit/Y8QBY72cZ0yxaLcmxgwg

[mary-poppins]

I'm sorry, but I wasn't able to verify your Contributor License Agreement (CLA) signature. CLA signature is required for any code contributions to AngularJS.

Please sign our CLA and ensure that the CLA signature email address and the email address in this PR's commits match.

If you signed the CLA as a corporation, please let us know the company's name.

Thanks a bunch!

PS: If you signed the CLA in the past then most likely the email addresses don't match. Please sign the CLA again or update the email address in the commit of this PR.
PS2: If you are a Googler, please sign the CLA as well to simplify the CLA verification process.

[shahata]

I agree there is inconsistent behavior here. Here is a plunker that show the problem more clearly: http://plnkr.co/edit/qkpf51dEJaUGoWTEtgMi?p=preview (text only gets hidden after the second click when you might expect it to be hidden in the first place)

On the other hand, it might be reasonable that behavior is unexpected when using the same class name both in class and ng-class. @Decad do you have a use case where such usage makes sense?

[Decad]

The use case we came across this bug was when loading our Javascript with an async loader. We wanted to show a loading gif for a portion of the page that would be rendered by Angular once it had finished downloading. So we had the loading gif be visible via a class on load and hoped that once angular would have initialized it would remove the class hiding the loading gif and render that specific section of the page.

[shahata]

Specifically for your use case it would be easier to just use ng-show="false" or ng-if="false". Let's see what other ppl think about this issue. To me it looks like incorrect usage.

[jeffbcross]

It does seem inconsistent to me, but a valid use case would be helpful in determining the correct resolution.

[pkozlowski-opensource]

We seem to have a use-case now in #10912

[pkozlowski-opensource]

@Decad your patch seems to work but it would need some more work before it can be merged:

• currently there is code duplication with arrayClasses function
• I would prefer that we add a new test instead of modifying the existing ones

Would you be up to working on this PR some more?

[Decad]

Yeah I would be up for working on the PR a bit more. Could you clarify where the duplication of the arrayClasses is? I'm not sure I follow what you are saying.

[pkozlowski-opensource]

@Decad sorry for the late response, haven't noticed your comment. What I've meant that the code you've added in https://github.com/Decad/angular.js/commit/f7d1d5fb0aa89512cf8085f0ff378f80ac8152f9#diff-d0df966e85933ac85b0f511aa14ca126R69 is almost identical to the existing one in

angular.js/src/ng/directive/ngClass.js

Lines 98 to 113 in 46b8065

	function arrayClasses(classVal) {
	if (isArray(classVal)) {
	return classVal.join(' ').split(' ');
	} else if (isString(classVal)) {
	return classVal.split(' ');
	} else if (isObject(classVal)) {
	var classes = [];
	forEach(classVal, function(v, k) {
	if (v) {
	classes = classes.concat(k.split(' '));
	}
	});
	return classes;
	}
	return classVal;
	}

=> we should refactor things to avoid code duplication

[andreasvoigt]

👍

[nosch]

👍