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.

[matsko]

@grantyb great work for fixing this with ngShow/ngHide. Turns out that after looking more into the problem, $animate is causing the issue and the CSS class checking has to occur inside of it. So the solution has to fix it for all cases. The ngShow/ngHide PR you made only works for that one ($animate.addClass / removeClass was had this issue to begin with).

Here's the new PR:
#4612

Here's a working version of your demo with the new PR:
http://plnkr.co/edit/RSUdGE8uBFHXLbISVixD?p=preview

Sorry for closing your PR on this. Thank you for putting the work into it.

[grantyb]

Thanks Matias :-)

Interesting. My first thought was to check hasClass(), but that felt DOM-centric and implementation-specific. I was afraid I was falling into my old jQuery habits (storing "model" in DOM) :-) That's not a criticism; I think the area that you've amended is at the same level of abstraction as the fix you've added. It felt awkward to add class-level checks inside the ngShowHide.js file though.

I like how simple the general solution is now.

Thanks so much for your help and encouragement with this - I've really enjoyed the experience. It's my first contribution to OSS, and thanks to you and your kind and helpful attitude, I'm definitely going to do more!

Grant

On 24/10/2013, at 9:54 AM, Matias Niemelä notifications@github.com wrote:

@grantyb great work for fixing this with ngShow/ngHide. Turns out that after looking more into the problem, $animate is causing the issue and the CSS class checking has to occur inside of it. So the solution has to fix it for all cases. The ngShow/ngHide PR you made only works for that one ($animate.addClass / removeClass was had this issue to begin with).

Here's the new PR:
#4612

Here's a working version of your demo with the new PR:
http://plnkr.co/edit/RSUdGE8uBFHXLbISVixD?p=preview

Sorry for closing your PR on this. Thank you for putting the work into it.

—
Reply to this email directly or view it on GitHub.

[matsko]

@grantyb happy to hear that the fix is good. And thanks for the feedback about the OSS stuff. I'm happy to help. Keep contributing! You learn a lot when working on an open-source project.

[matsko]

Landed as 76b628b