ngShowDirective adds the ng-hide-remove class when the element is already shown

[grantyb]

ngShowDirective watches attr.ngShow, and adds ng-hide-remove class even when the attr.ngShow value "changes" from true to true (or from false to false).

Animations should only be applied if the actual value of ng-hide has changed:

The function on line 146 of https://github.com/angular/angular.js/blob/master/src/ng/directive/ngShowHide.js

var ngShowDirective = ['$animate', function($animate) {
  return function(scope, element, attr) {
    scope.$watch(attr.ngShow, function ngShowWatchAction(value){
      $animate[toBoolean(value) ? 'removeClass' : 'addClass'](element, 'ng-hide');
    });
  };
}];

should become

var ngShowDirective = ['$animate', function($animate) {
  return function(scope, element, attr) {
    scope.$watch(attr.ngShow, function ngShowWatchAction(value, oldValue){
      if ( toBoolean(value) !== toBoolean(oldValue) ) {
        $animate[toBoolean(value) ? 'removeClass' : 'addClass'](element, 'ng-hide');
      }
    });
  };
}];

and similar with ngHideDirective on line 298

[petebacondarwin]

I can't reproduce the problem here: http://plnkr.co/edit/Wqw0fSUR4DnerItVhsRx?p=preview
@grantyb - Can you modify the Plunker to demonstrate the issue?

[grantyb]

Thanks, @petebacondarwin. Great idea!

This reveals the issue, when you type in the new text field I added. http://plnkr.co/VQQUKN5rs8F0qGawmgdb

[grantyb]

P.S. I'd be happy and honoured to submit a pull request for this issue if you like :-)

[grantyb]

Hmm... actually, my naïve solution doesn't work - the ng-show directive wouldn't initially add the ng-hide class (since value and oldvalue are both "undefined" at the start, so the boolean value doesn't look like it's "changed". Needs more thought...

[matsko]

@grantyb how's this coming? I can make a quick PR towards the end of the week unless you're still on this.

[grantyb]

I'm a bit stuck, to tell you the truth.

My fix (above) only works for ngHideDirective, and that's only because of an accident - namely that if the initial toBoolean(oldValue) is false, this matches the fact that we haven't yet added or removed either of the ng-hide-add and ng-hide-remove classes; hence doing nothing is appropriate.

The ngShowDirective is more complicated. If value and oldValue are both initially "undefined" or falsy, we must take action to add ng-hide, even though it doesn't seem like the model has changed. Otherwise <span ng-show="undefinedValue"> will fail to be hidden at start up.

Is there a way to tell if this is the first time the $watch() function has been called? Then we could do this:

var ngShowDirective = ['$animate', function($animate) {
  return function(scope, element, attr) {
    scope.$watch(attr.ngShow, function ngShowWatchAction(value, oldValue){
      if ( isFirstTimeThisWatchHasFired || (toBoolean(value) !== toBoolean(oldValue)) ) {
        $animate[toBoolean(value) ? 'removeClass' : 'addClass'](element, 'ng-hide');
      }
    });
  };
}];

Any ideas?

[matsko]

You could try to define a variable outside of the watch and check if it is true. Also take a look at $compile to see if you setup the addClass/removeClass guard there: https://github.com/angular/angular.js/blob/master/src/ng/compile.js#L305

[grantyb]

Thanks @matsko!

I'm embarrassed that the solution is so obvious :-) I added the local variable, and it now works as I'd expect. I'll send a pull request ASAP. Not sure how to add tests for this feature though. Any advice?

[matsko]

A good place to start is with https://github.com/angular/angular.js/blob/master/test/ng/directive/ngShowHideSpec.js#L45. Just add another speak and try to use the mocking system that the two existing tests are using.

To run tests, do the following:

1. npm install
2. ./node_modules/.bin/bower install
3. grunt autotest

Then when everything is passing and your code is ready to go, try the following:
4. grunt test

After that change your commit message style to follow AngularJS' style: Take a look at some of the commits here: https://github.com/angular/angular.js/commits/master

[matsko]

@grantyb how's this coming along? Do you need me to help out in any way?

[grantyb]

Hi @matsko.

I've sent pull request #4479, and I think I've formatted it correctly now (my first ever pull request - very exciting stuff!)

I just had a brief holiday in Sydney, so haven't had a chance to write the tests. Also, I've never written unit tests in the framework used in Angular. I'd be keen to try, and from the looks of things, this is a fairly simple example to get started on.

I've been reading up on how to test code like this, and it's a great way to get deeper into how Angular works, so if there's no rush to get this fixed, I'd be happy to tackle the tests. I've got a huge stack of work to catch up on, though, so it won't happen this week. If you have the time and the inclination to write the tests, I'd be happy to learn from your example - I'll use the lesson to fix something else in the Angular code base :-)

[matsko]

@grantyb no problem. I can handle the tests. I'm trying to close just about all the animation PRs before 1.2 is released.