angular / angular.js

AngularJS - HTML enhanced for web apps!
https://angularjs.org
MIT License
58.8k stars 27.48k forks source link

Hidden animated panels are visible on page load #12825

Open Morhpeuss opened 9 years ago

Morhpeuss commented 9 years ago

Hi,

I have hidden floating panel. HTML: <div class="modal animated left-modal " data-ng-hide="!showFormCreator" ng-cloak data-ng-controller="formCreatorController" data-ng-include="'/app/views/form-creator.html'"></div>

CSS:

.left-modal.ng-hide-add {      
     -webkit-animation:  fadeOutRight  0.5s ;   
    -moz-animation:   fadeOutRight 0.5s;
    -ms-animation:   fadeOutRight 0.5s;    
    animation:   fadeOutRight 0.5s;     
}

.left-modal.ng-hide-remove{
    -webkit-animation:  fadeInRight  .5s ;   
    -moz-animation:   fadeInRight .5s;
    -ms-animation:   fadeInRight .5s;    
    animation:   fadeInRight .5s;   
}

In angular 1.3.13 works all fine.

After update to 1.4.4 and now to 1.4.5 all hidden animated panels are visible on page load for few seconds.

After page load is all fine.

Thanks for help.

Josef

Narretz commented 9 years ago

Can you please post a demo via plnkr.co or similar that shows the problem?

Morhpeuss commented 9 years ago

Demo: http://plnkr.co/edit/xWkbSWkKu2fMfUhRdvsk . When the application starts, you will see the hidden animated panel.

gkalpak commented 8 years ago

A little more info:

This seems to have been introduced in 1.4.0-rc.0 (aka the big $animate refactoring). I suspect it is the last breaking change from here, namely, the addClass animation is cancelled while an enter animation is in progress.

What I haven't figured out yet, is what causes the enter animation (but it is happening as you can see here).

gkalpak commented 8 years ago

Aha...it's the ngInclude, of course.

So, I am tempted to say that this works as expected (in the sense that it's a documented breaking change). Still have to figure out the most appropriate work-around (suggestions welcome).

gkalpak commented 8 years ago

As is usually the case, moving the directive on a child element solves the issue. E.g.:

<!-- Change this: -->
<div ... ng-show="..." ng-include="...">Pane</div>

<!-- To that: -->
<div ... ng-show="..."><div ng-include="...">Pane></div></div>

Here is an updated plnkr.


So, like I said, I am inclined to calling this a works as expected. WDYT, @Narretz, @matsko ?

Narretz commented 8 years ago

Thanks for investigating @gkalpak! I don't think it's a cancellation of the addClass animation because the ngInclude does not have actual animation code. But I haven't looked at what happens yet.

gkalpak commented 8 years ago

@Narretz, I'm not sure what you mean by "the ngInclude does not have actual animation code". It does call $animate.enter() (which initiates a structural animation).

Narretz commented 8 years ago

That's right, it initiates an animation but when no styles are found the animation closes prematurely. Am 07.12.2015 21:33 schrieb "Georgios Kalpakas" notifications@github.com:

@Narretz https://github.com/Narretz, I'm not sure what you mean by "the ngInclude does not have actual animation code". It does call $animate.enter() (which initiates a structural animation).

— Reply to this email directly or view it on GitHub https://github.com/angular/angular.js/issues/12825#issuecomment-162651618 .

gkalpak commented 8 years ago

@Narretz, you are right; it's not enter cancelling addClass (they are merged actually). What makes the difference is that with ngInclude on the same element, the animations are enabled before ngShow tries to add the .ng-hide class for the first time (thus the animation).

Here's what is going on in both cases (with ngInclude on the same element and on a child element):

1. ngInclude on the same element as ngShow

<div ... ng-show="..." ng-include="...">...
  1. ngInclude transcludes the whole element (i.e. it removes it from the DOM and leaves a comment as a placeholder), before ngShow is compiled (since ngShow has a lower priority).
  2. ngInclude requests the template.
  3. By the time the template is back and the request promises are resolved, there have already been 2 $digests, thus animations are enabled (as per animateQueue.js#L113-L121).
  4. ngInclude creates a new element from the transcluded template element + the fetched HTML content, inserts it into the DOM and compiles/links it.
  5. ngShow on the newly created element, adds the .ng-hide class (using $animate.addClass()). Since animations are enabled, the user gets to see the element animating out of the view.

Compare this to:

2. ngInclude on a child element - ngShow on the parent

<div ... ng-show="..."><div ng-include="...">...
  1. ngInclude transcludes the child element (i.e. it removes it from the DOM and leaves a comment as a placeholder). Note that ngShow is not on the transcluded element, but on its parent.
  2. ngInclude requests the template.
  3. While the template is being fetched, ngShow's post-link function adds the .ng-hide class to the parent element (using $animate.addClass()).
  4. At this point, no request promises have been resolved yet, thus no two $digests have completed, which means animations are still disabled. With animations disabled, the .ng-hide class is added directly to the element's classes (without going through the ng-hide-animate/ng-hide-add/ng-hide-add-active hoops).
  5. By the time the template is back and ngInclude creates and inserts a new child element, the parent is already hidden (via .ng-hide).
  6. The user doesn't get to see the element animating out of the view.

That said, I still think this works as expected. One could think of several workarounds, but having the directives on different elements is the simplest imo.

It remains to be investigated, why it did work prior to v1.4.0-rc.0 :smiley: Still looking into it.

gkalpak commented 8 years ago

Here is why it used to work prior to v1.4.0-rc.0:

Class-based animations were "blocked" on elements currently performing a structural animation (see animate.js#L517-L525). So, not allowing class-based animations while a structural animation (like enter) was in progress, meant that the .ng-hide class was added immediately (no ng-hide-animate/add/add-active ceremony) - effectively hiding the element as soon as it was inserted into the DOM by ngInclude.

After all, allowing class-based animations along with structural ones is a feature (not a bug) :smiley:

gkalpak commented 8 years ago

If anyone's interested, here are two demos:

Narretz commented 8 years ago

Interesting. In this comment https://github.com/angular/angular.js/issues/11492#issuecomment-91605476, Matias said that animate would wait for the templates to download ...

Narretz commented 8 years ago

As a workaround, you can put your templates into the templateCache when your module runs: http://plnkr.co/edit/Q6o13RNXjdRtmw8LW67W?p=preview

There are also build tools that do this automatically for you, e.g. https://github.com/karlgoldstein/grunt-html2js

gkalpak commented 8 years ago

In this comment #11492 (comment), Matias said that animate would wait for the templates to download ...

Matias is right (obviously :smiley:). It (sort of) does. That's why there is no animation when ngShow is applied before downloading the template (thus no flicker), but there is an animation when ngShow is applied after downoading the template.

Narretz commented 8 years ago

The fact that it works (at least visually) when you add the template to the cache when the app runs, makes me think there is still a difference. Because template fetching is still async, even if the template is cached. It looks more like a timing issue.

Narretz commented 8 years ago

There's definitely something strange going on: ng-animate is adding these bogus classes on the initial hide / enter animation: ng-ng-hide-add ng-ng-hide-add-active. structural animations get this ng prefix added. So it looks like the class based animation is mistaken for a structural animation. Not sure if that's the cause of this problem, but it's definitely a bug.

Forked example with classList log: http://plnkr.co/edit/FDEAbvvjVJzZccuZWMwY?p=preview

gkalpak commented 8 years ago

Yes, I've noticed it. This happens because the (non-structural) addClass('ng-hide') animation gets merged into the (structural) enter animation (which is already in progress) and in some sense "inherits" the isStructural: true. Thus there are also the ng-prefixed classes added (along with the expected ng-hide-* classes).

It would be nice to fix that, but I don't think it is causing any issues.

Narretz commented 8 years ago

I've talked with @matsko about this and he said he would do some investigating. It's expected that the animations are merged, but the way they are merged doesn't look right.

The actual problem in the issue is caused by the way ngAnimate blocks animations on bootstrap. It waits two digest until animations are enabled. ngInclude calls $enter inside a watchActionFn, so it's happenind one digest later. We could delay animations further, but I'm not sure if that won't break some expectations. However, if you put it in the templateCache (best practice anyway) it doesn't animate, so it's probably more consistent to wait another digest.

gkalpak commented 8 years ago

The way $animate detects when the templates have been fetched and should enable animations is not 100% reliable. For example, because ngInclude calls $templateCache inside a watch-action callback and the src expression has to be evaluated first, $templateCache.totalPendingRequests is 0 when $animate checks, but not all (necessary) templates have been fetched.

That being said, I don't think it is reasonable to change how $animate detects "stability", because:

  1. It will break a lot of apps (and it will be very difficult for people to determine the source of the problem).
  2. I don't think there is a 100% reliable way to detect when the view is stable (and it heavily depends on the directives used and their arbitrary behaviors).

The current mechanism, tries to account for route and directive templates and it does it quite well.
It doesn't try to account for templates that directives might request asynchronously after evaluating an expression inside a $watch (which is what ngInclude does). Directives having that kind of behavior (and people using them) should be responsible for the side-effects of such behavior.

They can put them into wrappers, load templates into the $templateCache as a build step or whatever works for their usecase.

NeoGenet1c commented 8 years ago

Quick (& dirty) workaround is to add ng-hide class to the element.